[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-rdma
Subject:    Re: [PATCH for-next v1 2/4] RDMA/uverbs: uobj_get_obj_read should return the ib_uobject
From:       Shamir Rabinovitch <shamir.rabinovitch () oracle ! com>
Date:       2019-04-30 20:46:02
Message-ID: 20190430204602.GC30695 () srabinov-laptop
[Download RAW message or body]

On Tue, Apr 30, 2019 at 02:45:02PM -0400, Jason Gunthorpe wrote:
> On Tue., Apr. 30, 2019, 2:43 p.m. Shamir Rabinovitch, <
> shamir.rabinovitch@oracle.com> wrote:
> 
> > On Tue, Apr 30, 2019 at 02:54:08PM -0300, Jason Gunthorpe wrote:
> > > On Tue, Apr 30, 2019 at 05:23:22PM +0300, Shamir Rabinovitch wrote:
> > > > future patch will remove the ib_uobject pointer from the ib_x
> > > > objects. the uobj_get_obj_read and uobj_put_obj_read macros
> > > > were constructed with the ability to reach the ib_uobject from
> > > > ib_x in mind. this need to change now.
> > > >
> > > > Signed-off-by: Shamir Rabinovitch <shamir.rabinovitch@oracle.com>
> > > >  drivers/infiniband/core/uverbs_cmd.c | 165 +++++++++++++++++++++------
> > > >  include/rdma/uverbs_std_types.h      |   8 +-
> > > >  2 files changed, 137 insertions(+), 36 deletions(-)
> > > >
> > > > diff --git a/drivers/infiniband/core/uverbs_cmd.c
> > b/drivers/infiniband/core/uverbs_cmd.c
> > > > index 76ac113d1da5..93363c41e77e 100644
> > > > +++ b/drivers/infiniband/core/uverbs_cmd.c
> > > > @@ -37,6 +37,7 @@
> > > >  #include <linux/fs.h>
> > > >  #include <linux/slab.h>
> > > >  #include <linux/sched.h>
> > > > +#include <linux/list.h>
> > > >
> > > >  #include <linux/uaccess.h>
> > > >
> > > > @@ -700,6 +701,7 @@ static int ib_uverbs_reg_mr(struct
> > uverbs_attr_bundle *attrs)
> > > >     struct ib_uverbs_reg_mr      cmd;
> > > >     struct ib_uverbs_reg_mr_resp resp;
> > > >     struct ib_uobject           *uobj;
> > > > +   struct ib_uobject           *pduobj;
> > > >     struct ib_pd                *pd;
> > > >     struct ib_mr                *mr;
> > > >     int                          ret;
> > > > @@ -720,7 +722,8 @@ static int ib_uverbs_reg_mr(struct
> > uverbs_attr_bundle *attrs)
> > > >     if (IS_ERR(uobj))
> > > >             return PTR_ERR(uobj);
> > > >
> > > > -   pd = uobj_get_obj_read(pd, UVERBS_OBJECT_PD, cmd.pd_handle, attrs);
> > > > +   pd = uobj_get_obj_read(pd, UVERBS_OBJECT_PD, cmd.pd_handle, attrs,
> > > > +                          pduobj);
> > >
> > > This should be &pduobj in all places so it reads sensibly..
> >
> > OK. Will change the macro.
> >
> > >
> > > > @@ -2009,6 +2034,12 @@ static int ib_uverbs_post_send(struct
> > uverbs_attr_bundle *attrs)
> > > >     const struct ib_sge __user *sgls;
> > > >     const void __user *wqes;
> > > >     struct uverbs_req_iter iter;
> > > > +   struct uobj_list_item {
> > > > +           struct list_head list;
> > > > +           struct ib_uobject *uobj;
> > > > +   };
> > > > +   struct uobj_list_item *item, *tmp;
> > > > +   LIST_HEAD(ud_uobj_list);
> > >
> > > I'd rather not add this for AH's if we don't plan to drop the uobject
> > > pointer right away.. Same for the other place making a big logic
> > > change
> >
> > What's the concern? We drop the uobject in (almost) same line of code
> > where the current code do that. The uobjects are not held beyond the
> > function. Can you elaborate?
> >
> 
> I don't like adding a memory allocation.
> 
> Jason

Is it possible to drop the AH uobject ref in this case right after we
figure the ib_ah ? I thought no.. 

(1) I can move the uobject pointer to the ib_ud_wr if it make more sense
since the ud wr tie the AH uobject to the ib_ah in this case. This has
some benefit for object sharing. 

(2) Or leave the uobject pointer in the ib_ah and just use it in this case while
still modifying the macro for the sake of the rest of the code. This leave us with 
same situation with regard for object sharing.

As for WQ, (1) is not option AFAK. Only (2). 



[prev in list] [next in list] [prev in thread] [next in thread]