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:
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.
> > > +/**
> > > + * 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.
Thanks,
Bart.��.n��������+%������w��{.n�����{���fk��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
