On Sun, Jan 14, 2018 at 02:26:13PM -0700, Jason Gunthorpe wrote:
> On Fri, Jan 12, 2018 at 08:19:24AM +0200, Leon Romanovsky wrote:
> > On Thu, Jan 11, 2018 at 12:55:35PM -0700, Jason Gunthorpe wrote:
> > > On Thu, Jan 11, 2018 at 07:47:27PM +0000, Bart Van Assche wrote:
> > > > On Thu, 2018-01-11 at 17:31 +0200, Leon Romanovsky wrote:
> > > > > +struct rdma_restrack_entry {
> > > > > + struct list_head list;
> > > > > +
> > > > > + /*
> > > > > + * The entries are filled during rdma_restrack_add,
> > > > > + * can be attempted to be free during rdma_restrack_del.
> > > > > + *
> > > > > + * As an example for that, see mlx5 QPs with type MLX5_IB_QPT_HW_GSI
> > > > > + */
> > > > > + bool valid;
> > > > > +
> > > > > + /*
> > > > > + * Sleepabale RCU to protect object data.
> > > > > + */
> > > > > + struct srcu_struct srcu;
> > > > > +
> > > > > + struct task_struct *task;
> > > > > + char *task_comm;
> > > > > +};
> > > >
> > > > Please use the kernel-doc syntax to document the meaning of the names of the
> > > > fields in this structure. It is e.g. nontrivial to guess what "task_comm" stands
> > > > for. The presense of a task_struct pointer is also nontrivial.
> > >
> > > And task_comm doesn't seem like a great name anymore, and should be
> > > const char *, right ?
> >
> > No problem, what will be great "name" now? "const char *name"?
>
> I haven't looked closely yet, but is is the kernel name for the object
> right? So kern_name? And same remark about the netlink attribute name
> as well.
No problem
>
> I'm also uncertain that _ib_create_qp is a good idea, and I really
> dislike the _ for a public API.
>
> Do you propose to add a kernel name to every kernel object we add
> restrack to?
>
> It feels simpler to give a name to alloc_pd and use that name as the
> kernel name for every object created under it? Or is there a reason to
> have a per object name?
Not really.
>
> Jason
>
Attachment:
signature.asc
Description: PGP signature
