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

Jiri Pirko
2026-04-16T13:34:11Z

Thu, Apr 16, 2026 at 02:10:00PM +0200, mrgolin@amazon.com wrote:
>On Mon, Apr 13, 2026 at 08:22:05PM +0200, Jiri Pirko wrote:
>> >> >While I like the idea of standardizing the way we pass buffer
>> >> >information to the kernel, the list thing looks like over generalization
>> >> >to me, especially after Leon's refactoring of CQ creation. Maybe we can
>> >> >add buffer as a new attribute type that can be used for multiple
>> >> >parameters in a command, and have a helper with the code below that
>> >> >takes an attribute id and returns a umem object, letting each handler
>> >> >store it. This would also make it easier for drivers to pass their
>> >> >private buffers using this infrastructure.
>> >>
>> >> Currently we have set of attrs (4) to pass CQ umem. I tried to make this
>> >> very smooth for all possible uverbs, passing single attr of array of
>> >> structs describing a buffer. Uverb attr api knows how to work with
>> >> arrays, all clicks.
>> >>
>> >> Drivers can easily pass their specific buffers over this list too. I
>> >> didn't implement it as there was no need, but the idea is to have index>X
>> >> for driver specific indexes.
>> >
>> >Why do we need to invent a new way instead of just adding another
>> >argument in a command, that consists of all the info needed to pass a
>> >buffer? Also how can this work for objects that have only private umem?
>>
>> You can put the buf array attr to any uverb, some may not have
>> "standard" indexes.
>>
>Not sure I fully follow your idea here, can you elaborate on how you
>plan to reserve index>X range in an enum used as index into dynamic
>array?

#define UVERBS_BUF_DRIVER_BASE 1024

and then:

struct ib_uverbs_buffer_desc bufs[] = {
{
.fd = cq_dmabuf_fd,
.type = IB_UVERBS_BUFFER_TYPE_DMABUF,
.index = UVERBS_BUF_CQ_BUF, /* 0 */
.addr = 0,
.length = cq_buf_size,
},
{
.fd = dbr_dmabuf_fd,
.type = IB_UVERBS_BUFFER_TYPE_DMABUF,
.index = UVERBS_BUF_CQ_DBR, /* 1 */
.addr = dbr_offset,
.length = 8,
},
{
.fd = uar_dmabuf_fd,
.type = IB_UVERBS_BUFFER_TYPE_DMABUF,
->>>>>>>>> .index = MLX5_BUF_CQ_UAR, /* 1024 */
.addr = 0,
.length = 4096,
},
};

>
>> >
>> >> What's the benefit of passing per-uverb attrs with a struct? Perhaps I'm
>> >> missing something.
>> >
>> >Mostly simplification by untying two unrelated things:
>> >1) way of passing args to kernel
>> >2) object lifetime management
>>
>> Could you be more specific please?
>>
>>
>> >
>> >And also significantly reducing the amount of code changes required to
>> >achieve this.
>>
>> Significantly? I'm not sure I follow, but I guess that is related to my
>> previous question. I'm not sure I understand what you have exacly in
>> mind. Regarding UAPI, I think I understand, but regarding kernel
>> internals, I don't :(
>>
>
>I imagine the changes for getting umem using the new mechanism in
>create CQ command are about:
>
>Define a new optional buffer attribute:
>
>- UVERBS_ATTR_UHW());
>+ UVERBS_ATTR_UHW(),
>+ UVERBS_ATTR_BUFFER(UVERBS_ATTR_CREATE_CQ_BUFFER,
>+ UA_OPTIONAL));

Okay, that may be doable. I'm just curious about the in-kernel
management of umems. With list, it aligns. With your suggested approach
we would have to iterate over these attrs and assemble in-kernel list to
manage umems.

>
>Get umem from the new attribute if available or fallback to existing
>attributes:
>
>- umem = uverbs_create_cq_get_umem(ib_dev, attrs);
>- if (IS_ERR(umem)) {
>- ret = PTR_ERR(umem);
>- goto err_event_file;
>- }
>+ if (uverbs_attr_is_valid(attrs, UVERBS_ATTR_CREATE_CQ_BUFFER)) {
>+ ret = uverbs_get_umem(&umem, attrs, UVERBS_ATTR_CREATE_CQ_BUFFER);
>+ if (ret)
>+ goto err_event_file;
>+ } else {
>+ umem = uverbs_create_cq_get_umem(ib_dev, attrs);
>+ if (IS_ERR(umem)) {
>+ ret = PTR_ERR(umem);
>+ goto err_event_file;
>+ }
>+ }
>
>Drivers don't need to change.

uverbs_create_cq_get_umem only works with legacy attrs. Not that
interesting. How do you propose to handle other umems, when uverb
supports multiple umems (like + DRB umem for create CQ)? I'm
particularly interested in consumption validation and life-cycle
management (that is a bit trickier for create QP).

>
>
>> >> >> + * Optional array of struct ib_uverbs_buffer_desc describing memory regions
>> >> >> + * backed by dma-buf or user virtual address. Can be added to any method
>> >> >> + * that needs external buffer support.
>> >> >> + * Each entry carries an index field selecting the per-command buffer slot.
>> >> >> + * Use ib_umem_list_create() to map them and ib_umem_list_load() to access.
>> >> >> + */
>> >> >> +#define UVERBS_ATTR_BUFFERS() \
>> >> >> + UVERBS_ATTR_PTR_IN(UVERBS_ATTR_BUFFERS, \
>> >> >> + UVERBS_ATTR_MIN_SIZE( \
>> >> >> + sizeof(struct ib_uverbs_buffer_desc)), \
>> >> >> + UA_OPTIONAL, \
>> >> >> + UA_ALLOC_AND_COPY)
>> >> >> +
>> >> >> /* =================================================
>> >> >> * Parsing infrastructure
>> >> >> * =================================================
>> >> >> diff --git a/include/uapi/rdma/ib_user_ioctl_cmds.h b/include/uapi/rdma/ib_user_ioctl_cmds.h
>> >> >> index 72041c1b0ea5..10aa6568abf1 100644
>> >> >> --- a/include/uapi/rdma/ib_user_ioctl_cmds.h
>> >> >> +++ b/include/uapi/rdma/ib_user_ioctl_cmds.h
>> >> >> @@ -64,6 +64,7 @@ enum {
>> >> >> UVERBS_ATTR_UHW_IN = UVERBS_ID_DRIVER_NS,
>> >> >> UVERBS_ATTR_UHW_OUT,
>> >> >> UVERBS_ID_DRIVER_NS_WITH_UHW,
>> >> >> + UVERBS_ATTR_BUFFERS,
>
>I don't think you can add anything here as it overlaps with driver
>specific attributes. I suggest defining per command attr id and passing
>it by caller into ib_umem_list_create.
>
>> >> >> };
>> >> >>
>> >> >> enum uverbs_methods_device {
>> >> >> diff --git a/include/uapi/rdma/ib_user_ioctl_verbs.h b/include/uapi/rdma/ib_user_ioctl_verbs.h
>> >> >> index 90c5cd8e7753..41ed9f75b4de 100644
>> >> >> --- a/include/uapi/rdma/ib_user_ioctl_verbs.h
>> >> >> +++ b/include/uapi/rdma/ib_user_ioctl_verbs.h
>> >> >> @@ -273,4 +273,31 @@ 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,
>> >> >> +};
>> >> >> +
>> >> >> +/*
>> >> >> + * 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;
>> >> >> +};
>> >> >> +
>> >> >> #endif
>> >> >> --
>> >> >> 2.53.0
>> >> >>