On Thu, Jan 18, 2018 at 03:32:03AM +0000, Bart Van Assche wrote:
> On Wed, 2018-01-17 at 07:47 +0200, Leon Romanovsky wrote:
> > On Tue, Jan 16, 2018 at 09:33:18PM +0000, Bart Van Assche wrote:
> > > > +/**
> > > > + * enum rdma_restrack_obj - HW objects to track
> > > > + */
> > > > +enum rdma_restrack_obj {
> > > > + /**
> > > > + * @RDMA_RESTRACK_PD: Protection domain (PD)
> > > > + */
> > > > + RDMA_RESTRACK_PD,
> > > > + /**
> > > > + * @RDMA_RESTRACK_CQ: Completion queue (CQ)
> > > > + */
> > > > + RDMA_RESTRACK_CQ,
> > > > + /**
> > > > + * @RDMA_RESTRACK_QP: Queue pair (QP)
> > > > + */
> > > > + RDMA_RESTRACK_QP,
> > > > + /* private: counts number of elements, always last */
> > > > + _RDMA_RESTRACK_MAX
> > > > +};
> > >
> > > This looks really ugly to me. Please use kernel-doc syntax to document the RDMA
> > > resource types.
> >
> > I used kerne-doc and _RDMA_RESTRACK_MAX was an exception, it is not supposed to be
> > used outside of restrack code.
>
> Hello Leon,
>
> Please have a look at the following example from Documentation/kernel-doc-nano-HOWTO.txt:
>
Cite from that document:
1 NOTE: this document is outdated and will eventually be removed. See
2 Documentation/doc-guide/ for current information.
I followed guidelines from Documentation/doc-guide/kernel-doc.rst.
> kernel-doc for structs, unions, enums, and typedefs
> ---------------------------------------------------
>
> [ ... ]
>
> /**
> * struct my_struct - short description
> * @a: first member
> * @b: second member
> *
> * Longer description
> */
> struct my_struct {
> int a;
> int b;
> /* private: internal use only */
> int c;
> };
>
> > > > +/**
> > > > + * struct rdma_restrack_root - main resource tracking management
> > > > + * entity, per-device
> > > > + */
> > > > +struct rdma_restrack_root {
> > > > + /**
> > > > + * @cnt: global counter to avoid the need to count number
> > > > + * of elements in the object's list.
> > > > + *
> > > > + * It can be different from the list_count, because we are
> > > > + * not taking lock during counter increment and don't
> > > > + * synchronize the RCU.
> > > > + */
> > > > + refcount_t cnt[_RDMA_RESTRACK_MAX];
> > > > + /**
> > > > + * @list: linked list of all entries per-object
> > > > + */
> > > > + struct list_head list[_RDMA_RESTRACK_MAX];
> > > > + /* private: Internal read/write lock.
> > > > + * It is needed to protect the add/delete list operations.
> > > > + */
> > > > + struct rw_semaphore rwsem[_RDMA_RESTRACK_MAX];
> > > > +};
> > >
> > > The above looks wrong to me. Please change the above into an array of data structures
> > > instead of a data structure that is full of arrays of identical size.
> >
> > It is a matter for taste.
>
> No, it is not. The above layout makes it impossible for the CPU prefetcher to
> be effective if _RDMA_RESTRACK_MAX would be large. Additionally, the above
> layout makes it impossible to pass a pointer to the (cnt, list, rwsem) triplet
> from one function to another.
Thanks for the explanation, I'm changing it now.
>
> > > > +/**
> > > > + * struct rdma_restrack_entry - metadata per-entry
> > > > + */
> > > > +struct rdma_restrack_entry {
> > > > + /**
> > > > + * @list: linked list between entries
> > > > + */
> > > > + struct list_head list;
> > > > + /**
> > > > + * @valid: validity indicator
> > > > + *
> > > > + * 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;
> > > > + /**
> > > > + * @srcu: sleepable RCU to protect object data.
> > > > + */
> > > > + struct srcu_struct srcu;
> > > > + /**
> > > > + * @task: owner of resource tracking entity
> > > > + *
> > > > + * There are two types of entities: created by user and created
> > > > + * by kernel.
> > > > + *
> > > > + * This is relevant for the entities created by users.
> > > > + * For the entieies created by kernel, this pointer will be NULL.
> > > > + */
> > > > + struct task_struct *task;
> > > > + /**
> > > > + * @kern_name: name of owner for the kernel created entities.
> > > > + */
> > > > + const char *kern_name;
> > > > +};
> > >
> > > Again, please use the kernel-doc syntax to document structure members. Additionally,
> > > please fix the spelling of "entieies".
> >
> > It was formatted according to kernel-doc checker:
> > ➜ linux-rdma git:(rn/restrack-v5) ./scripts/kernel-doc include/rdma/restrack.h |grep warnin
> > include/rdma/restrack.h:59: warning: Enum value '_RDMA_RESTRACK_MAX' not described in enum 'rdma_restrack_obj'
>
> Again, please have a look at Documentation/kernel-doc-nano-HOWTO.txt. That
> document shows that members of data structures should be documented above the
> "struct" keyword instead of inside the structure definition.
Please read Documentation/doc-guide/kernel-doc.rst - "In-line member
documentation comments" section.
Regarding other comments, I'm working to address them in new patchset.
Thanks for the review.
>
> Thanks,
>
> Bart.
Attachment:
signature.asc
Description: PGP signature
