Re: [PATCH rdma-next v4 1/7] RDMA/restrack: Add general infrastructure to track RDMA resources

[Bart Van Assche <Bart.VanAssche-Sjgp3cTcYWE@public.gmane.org>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4618 bytes --]

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‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±­ÙšŠ{ayº\x1dʇڙë,j\a­¢f£¢·hš‹»öì\x17/oSc¾™Ú³9˜uÀ¦æå‰È&jw¨®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿïêäz¹Þ–Šàþf£¢·hšˆ§~ˆmš