Re: [PATCH for-next 1/2] RDMA/uverbs: Add a common way to create CQ with umem

["Margolin, Michael" <mrgolin@amazon.com>]

On 7/4/2025 9:14 PM, Jason Gunthorpe wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On Tue, Jul 01, 2025 at 11:15:44PM +0000, Michael Margolin wrote:
>> Add ioctl command attributes and a common handling for the option to
>> create CQs with memory buffers passed from userspace. When required
>> attributes are supplied, create umem for driver use and store it in CQ
>> context.
>> The extension enables creation of CQs on top of preallocated CPU
>> virtual or device memory buffers, by supplying VA or dmabuf fd, in a
>> common way. At first stage the command fails for any driver that didn't
>> explicitly state its support by setting a flag in the ops struct.
>>
>> Signed-off-by: Michael Margolin <mrgolin@amazon.com>
>> ---
>>   drivers/infiniband/core/device.c              |  3 +
>>   drivers/infiniband/core/uverbs_std_types_cq.c | 82 ++++++++++++++++++-
>>   include/rdma/ib_verbs.h                       |  6 ++
>>   include/uapi/rdma/ib_user_ioctl_cmds.h        |  4 +
>>   4 files changed, 91 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
>> index 468ed6bd4722..8b0ce0ec15dd 100644
>> --- a/drivers/infiniband/core/device.c
>> +++ b/drivers/infiniband/core/device.c
>> @@ -2667,6 +2667,9 @@ void ib_set_device_ops(struct ib_device *dev, const struct ib_device_ops *ops)
>>        dev_ops->uverbs_no_driver_id_binding |=
>>                ops->uverbs_no_driver_id_binding;
>>
>> +     dev_ops->uverbs_support_cq_with_umem |=
>> +             ops->uverbs_support_cq_with_umem;
> This seems to have turned out quite nice!
>
> I might just suggest a tweak to streamline this flow
>
> Add:
>
> +       int (*create_cq_umem)(struct ib_cq *cq,
> +                             const struct ib_cq_init_attr *attr,
> +                             struct ib_umem *umem,
> +                             struct uverbs_attr_bundle *attrs);
>
>
> Instead of the uverbs_support_cq_with_umem
>
> Then the core code would check the two ops and if only umem is present
> it will insist on the new attributes, if it is not present it will
> refuse them otherwise it will call the correct op.
>
> In the driver the create_cq would obtain the umem from the private
> attrs and then call create_cq_umem() in the same way
>
> So it becomes quite easy to just reorganize the drivers to support
> this.
>
> Jason

Generally I would say that adding a new op for an additional optional 
param isn't scalable but I implemented your suggestion and I do think it 
makes things more structured for drivers as well as clarifying umem 
ownership.

Sending v2 series.

Michael