Re: [PATCH rdma-next v3 03/17] RDMA/core: Introduce generic buffer descriptor infrastructure for umem

[Jiri Pirko <jiri@resnulli.us>]

Wed, May 06, 2026 at 03:37:57PM +0200, jgg@ziepe.ca wrote:
>On Mon, May 04, 2026 at 03:57:17PM +0200, Jiri Pirko wrote:
>
>> +/**
>> + * ib_umem_get - Canonical on-demand umem getter.
>> + * @device:        IB device.
>> + * @udata:         uverbs udata bundle (may be NULL).
>> + * @attr_id:       per-command UMEM attribute id; consulted if @udata is set.
>> + * @legacy_filler: optional command-specific legacy attr filler.
>> + *                 invoked if @udata is set.
>> + * @va_fallback:   if true, build a VA-typed desc with @addr.
>> + * @addr:          user VA, used if @va_fallback is true.
>> + * @size:          driver-required minimum length.
>> + * @access:        IB access flags forwarded to ib_umem_get_desc().
>> + *
>> + * Return: valid umem on success, ERR_PTR(...) on error, NULL
>> + * if no source produced a buffer (only possible when @va_fallback is false).
>> + */
>> +struct ib_umem *ib_umem_get(struct ib_device *device, struct ib_udata *udata,
>> +			    u16 attr_id,
>> +			    ib_umem_buf_desc_filler_t legacy_filler,
>> +			    bool va_fallback, u64 addr, size_t size, int access)
>
>I didn't try to look at what the drivers actually do, but I'm slightly
>surprised not to see an addr_size here? Is it the case the drivers
>don't have have a uhw->size to go along with their uhw->va?

"size_t size". What am I missing?

>
>I guess mrs always use mr->len and the cq/qps are doing something like
>uhw->ncqes*SIZE_CQE?
>
>Did you find any counter example?
>
>> +		ret = legacy_filler(attrs, &legacy_desc);
>> +		if (!ret) {
>> +			if (have_desc) {
>> +				ibdev_err(device,
>> +					  "UMEM attr (id=%u) and legacy attrs are mutually exclusive\n",
>> +					  attr_id);
>> +				return ERR_PTR(-EINVAL);
>
>We must never print on system calls, it just gives a way for unpriv
>userspace to fill the dmesg.

Sure. Will fix.


>
>> --- a/include/uapi/rdma/ib_user_ioctl_verbs.h
>> +++ b/include/uapi/rdma/ib_user_ioctl_verbs.h
>> @@ -273,4 +273,27 @@ struct ib_uverbs_gid_entry {
>>  	__u32 netdev_ifindex; /* It is 0 if there is no netdev associated with it */
>>  };
>>  
>> +enum ib_uverbs_buffer_type {
>> +	IB_UVERBS_BUFFER_TYPE_DMABUF,
>> +	IB_UVERBS_BUFFER_TYPE_VA,
>> +};
>
>I've learned it helps backporters to add the =0, =1

Why do we care about backporters? I mean, the mainline code is what we
care of, and for that, enum default values are well defined and enough.
What am I missing?

>
>Jason