Re: [PATCH rdma-next v2 01/15] RDMA/core: Introduce generic buffer descriptor infrastructure for umem

Jiri Pirko
2026-04-22T14:06:07Z

Wed, Apr 22, 2026 at 01:33:06PM +0200, jiri@resnulli.us wrote:
>Tue, Apr 21, 2026 at 03:46:35PM +0200, jgg@ziepe.ca wrote:
>>On Sat, Apr 11, 2026 at 04:49:01PM +0200, Jiri Pirko wrote:
>>> @@ -332,3 +333,250 @@ int ib_umem_copy_from(void *dst, struct ib_umem *umem, size_t offset,
>>
>>> +struct ib_umem_list *ib_umem_list_create(struct ib_device *device,
>>> + const struct uverbs_attr_bundle *attrs,
>>> + unsigned int slot_max)
>>> +{
>>> + const struct ib_uverbs_buffer_desc *descs;
>>> + struct ib_umem_dmabuf *umem_dmabuf;
>>> + struct ib_umem_list *list;
>>> + struct ib_umem *umem;
>>> + unsigned int count;
>>> + int num_descs;
>>> + int err;
>>> + int i;
>>> +
>>> + if (WARN_ON_ONCE(slot_max >= BITS_PER_LONG))
>>> + return ERR_PTR(-EINVAL);
>>> + count = slot_max + 1;
>>> +
>>> + num_descs = uverbs_attr_ptr_get_array_size(
>>> + (struct uverbs_attr_bundle *)attrs, UVERBS_ATTR_BUFFERS,
>>> + sizeof(*descs));
>>
>>uverbs_attr_ptr_get_array_size() should get a const on the parameter,
>>seems to have been missed originally
>
>Okay.
>
>
>>
>>> +/*
>>> + * Describes a single buffer backed by dma-buf or user virtual address.
>>> + * Passed as an array via UVERBS_ATTR_BUFFERS. Each uverb command that
>>> + * accepts this attribute defines its own per-command buffer slot enum.
>>> + * The index field selects the buffer slot this descriptor maps to.
>>> + *
>>> + * @fd: dma-buf file descriptor (valid for IB_UVERBS_BUFFER_TYPE_DMABUF)
>>> + * @type: buffer type from enum ib_uverbs_buffer_type
>>> + * @index: per-command buffer slot index
>>> + * @reserved: must be zero
>>> + * @addr: offset within dma-buf, or user virtual address for VA
>>> + * @length: buffer length in bytes
>>> + */
>>> +struct ib_uverbs_buffer_desc {
>>> + __s32 fd;
>>> + __u32 type;
>>> + __u32 index;
>>> + __u32 reserved;
>>> + __aligned_u64 addr;
>>> + __aligned_u64 length;
>>> +};
>>
>>This seems like a good idea, we should have done it earlier :\
>
>Yeah :/
>
>
>>
>>Arguably if you do this then the first issue of being more flexible
>>with umems is addressed, so a uverbs_attr_ptr_get_umem() looks much
>>more feasible.
>>
>>Just brain storming, but if we let the driver pass in its uhw
>>information inot a getter:
>>
>> struct ib_umem *uverbs_attr_get_umem(struct
>> uverbs_attr_bundle *attrs, u16 idx,
>> u64 uhw_umem_base, u64 umem_len);
>>
>> dbr_umem = uverbs_attr_get_umem(attrs,
>> MLX5_IB_ATTR_QP_DBR, uhw->base, uhw->len);
>>
>>Then if the new attribute is provided the uhw is ignored, otherwise a
>>ib_uverbs_buffer_desc is created from the udata parameters instead.
>>
>>Drivers use the normal attr indexes to define their many umems for
>>something complicated lik QP.
>
>Won't this go backwards? I mean, I was under impression that we want to
>move the umem creation to core. What you suggest is the driver initiates
>the umem creation. I personally think that it is nicer the way you
>suggest, since the core is the owner and responsible for cleanup and
>umems are created upon need.
>
>One think. How about the consumption checking? I remember that for my
>previous attempt on uverb umems you asked to check if each attr was
>processed or not and in case it was not, yell out at the user.

Well, I think I can still track consumption per loaded attr. I'm on it.

>
>
>>
>>For the lifecycle.. This series adds a
>> + cq->umem_list = umem_list;
>>
>>So it is not a big leap to imagine a linked list in the object that is
>>appended by the umem create function. Pass the list head into the umem
>>allocator, free the whole linked list in the core code.
>
>Yeah, that would be okay.
>
>>
>>This has some appeal because it is an easier conversion of all the
>>drivers, instead of re-threading their flows to accept a pre-created
>>umem they just have to be updated to call the new function in all the
>>places they are currently getting umems.
>>
>>You'd probably have a further helper for cq that could extract the
>>existing common cq attrs to a ib_uverbs_buffer_desc:
>>
>> cq_umem = uverbs_attr_get_cq_umem(attrs, cq, uhw->base, uhw->len);
>>
>>Probably similar for mr and a common mr attribute.
>
>Got it.
>
>
>>
>>This will be easier to put revocable and dynamic ops into the scheme,
>>they can be passed as arugments to the get function instead of some
>>complicated thing in the central ops structure.
>>
>>Jason