Re: [PATCH rdma-next 1/1] RDMA/mana_ib: indicate that inline data is not supported

[Leon Romanovsky]

On Tue, Jul 16, 2024 at 02:55:26PM +0000, Konstantin Taranov wrote:
> > > > > Set max_inline_data to zero during RC QP creation.
> > > > >
> > > > > Fixes: fdefb9184962 ("RDMA/mana_ib: Implement uapi to create and
> > > > > destroy RC QP")
> > > > > Signed-off-by: Konstantin Taranov <kotaranov@xxxxxxxxxxxxx>
> > > > > ---
> > > > >  drivers/infiniband/hw/mana/qp.c | 2 ++
> > > > >  1 file changed, 2 insertions(+)
> > > > >
> > > > > diff --git a/drivers/infiniband/hw/mana/qp.c
> > > > > b/drivers/infiniband/hw/mana/qp.c index 73d67c853b6f..d9f24a763e72
> > > > > 100644
> > > > > --- a/drivers/infiniband/hw/mana/qp.c
> > > > > +++ b/drivers/infiniband/hw/mana/qp.c
> > > > > @@ -426,6 +426,8 @@ static int mana_ib_create_rc_qp(struct ib_qp
> > > > > *ibqp,
> > > > struct ib_pd *ibpd,
> > > > >  	u64 flags = 0;
> > > > >  	u32 doorbell;
> > > > >
> > > > > +	/* inline data is not supported */
> > > > > +	attr->cap.max_inline_data = 0;
> > > >
> > > > Can you please point to me to the flow where attr is not zeroed before?
> > > >
> > >
> > > Sorry, I do not understand the question. I cannot point to something that is
> > not in the code.
> > >
> > > It is to support the case when user asks for x bytes inlined when it
> > > creates a QP, and we respond with actual allowed inline data for the
> > > created QP. (as defined in: "The function ibv_create_qp() will update
> > > the qp_init_attr->cap struct with the actual QP values of the QP that
> > > was created;")
> > >
> > > The kernel logic is inside "static int create_qp(struct uverbs_attr_bundle
> > *attrs, struct ib_uverbs_ex_create_qp *cmd)"
> > > where we do the following:
> > > attr.cap.max_inline_data = cmd->max_inline_data; qp =
> > > ib_create_qp_user(..,&attr,..);
> > 
> > Awesome, ib_create_qp_user() is called exactly in two places, and in both
> > cases I see this line "struct ib_qp_init_attr attr = {}; "
> > 
> > It means that attr is zeroed.
> > 
> 
> I think there is some misunderstanding.
> So, the attr is zeroed at init. Then it is filled with values from the user request.
> With
> 	attr.cap.max_send_wr     = cmd->max_send_wr;
> 	attr.cap.max_recv_wr     = cmd->max_recv_wr;
> 	attr.cap.max_send_sge    = cmd->max_send_sge;
> 	attr.cap.max_recv_sge    = cmd->max_recv_sge;
> 	attr.cap.max_inline_data = cmd->max_inline_data;
> or with:
> 	set_caps(&attr, &cap, true);
> 
> So at this moment there is a value from the user in the attr (it is not 0).
> ib_create_qp_user is called with the attr.
> 
> The drivers are allowed to tune the cap values during QP create.
> So I want to set it to 0 as some other provides (see pvrdma_create_qp,
> rvt_create_qp, set_kernel_sq_size from mlx4).
> 
> After the driver call has happened, the modified cap value are copied to temp variable:
> 	resp.base.max_recv_sge    = attr.cap.max_recv_sge;
> 	resp.base.max_send_sge    = attr.cap.max_send_sge;
> 	resp.base.max_recv_wr     = attr.cap.max_recv_wr;
> 	resp.base.max_send_wr     = attr.cap.max_send_wr;
> 	resp.base.max_inline_data = attr.cap.max_inline_data;
> or with:
> 	set_caps(&attr, &cap, false);
> And then copied to the user with:
> 	uverbs_response(attrs, &resp, sizeof(resp));
> or with
> 	uverbs_copy_to_struct_or_zero(attrs,
> 					UVERBS_ATTR_CREATE_QP_RESP_CAP, &cap,
> 					sizeof(cap));
> 
> Do you agree that my patch solves a problem in mana_ib (there is no problem in core)?
> Or do you think that I am trying to solve non-existent problem?

Yes, you are. If user asked for specific functionality (max_inline_data != 0) and
your device doesn't support it, you should return an error.

pvrdma, mlx4 and rvt are not good examples, they should return an error as well, but because of being
legacy code, we won't change them.

Thanks

> 
> Thanks
> 
> > Thanks
> > 
> > > resp.base.max_inline_data = attr.cap.max_inline_data;
> > >
> > > So, my change makes sure that the response will have 0 and not the
> > > value the user asked, as we do not support inlining. So without the
> > > fix, the user who was asking for inlining was falsely seeing that we support
> > it (example of such an application is rdma_server from librdmacm).
> > >
> > > Thanks
> > >
> > > > Thanks
> > > >
> > > > >  	if (!udata || udata->inlen < sizeof(ucmd))
> > > > >  		return -EINVAL;
> > > > >
> > > > > --
> > > > > 2.43.0
> > > > >