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

Jiri Pirko
2026-04-22T11:33:13Z

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.

>
>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