On Thu, Jan 31, 2019 at 07:37:29PM +0200, Leon Romanovsky wrote:
> On Thu, Jan 31, 2019 at 10:27:51AM -0700, Jason Gunthorpe wrote:
> > On Thu, Jan 31, 2019 at 11:48:06AM +0200, Leon Romanovsky wrote:
> > > > > diff --git a/include/rdma/restrack.h b/include/rdma/restrack.h
> > > > > index 92d11c35221a..c81e43d043cf 100644
> > > > > +++ b/include/rdma/restrack.h
> > > > > @@ -161,4 +161,26 @@ struct xarray *rdma_dev_to_xa(struct ib_device *dev,
> > > > > enum rdma_restrack_type type);
> > > > > void rdma_rt_read_lock(struct ib_device *dev, enum rdma_restrack_type type);
> > > > > void rdma_rt_read_unlock(struct ib_device *dev, enum rdma_restrack_type type);
> > > > > +
> > > > > +/**
> > > > > + * rdma_res_to_id() - Unique ID as seen by restrack
> > > > > + * @res: resrouce to operate
> > > > > + *
> > > > > + * Return: unique ID
> > > > > + */
> > > > > +static inline u32 rdma_res_to_id(struct rdma_restrack_entry *res)
> > > > > +{
> > > > > + return res->id;
> > > > > +}
> > > > > +/**
> > > > > + * rdma_rt_set_type() - Set restrack entry type,
> > > > > + * prior to call rdma_restrack_add()
> > > > > + * @res: resrouce to operate
> > > > > + * @type: Actual type
> > > > > + */
> > > > > +static inline void rdma_rt_set_type(struct rdma_restrack_entry *res,
> > > > > + enum rdma_restrack_type type)
> > > > > +{
> > > > > + res->type = type;
> > > > > +}
> > > >
> > > > This seems a bit obfuscating, generally kernel code tries to avoid
> > > > this sort of trivial accessor style, I thought
> > >
> > > It is not really correct, kernel code distinguishes between driver code
> > > and core code. Obfuscation is not allowed in drivers, but fine for core
> > > core which needs to hide implementation from their users.
> >
> > Do you ever intend to change these implementations?
>
> I have some ideas how to remove those types completely, but it won't be
> for now and it maybe won't work at all.
Well then do this when you get that part done
> > > Such difference is due to need to refactor. You will need to refactor
> > > core code in one place, but changes in drivers in many places.
> >
> > Generally I see this as being defered until a need to change actually
> > arises, then you switch the open coded struct access to function and
> > then rework the acessors
> >
> > This sort of wrapper pattern, as well as the
> > rdma_rt_read_lock/rdma_rt_read_unlock wrappering don't seem like good
> > kernel design.. This isn't Java.
>
> I don't like such pattern too, but it removes some complexity from the
> users. Care to send patch to convert xa_lock() back to spinlock?
There is *always* some counter example to everything in the
kernel. Maybe xa is justified, I don't know.
Doesn't really seem well justified here..
Jason
