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

[Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 7014 bytes --]

On Tue, Jan 16, 2018 at 09:33:18PM +0000, Bart Van Assche wrote:
> On Mon, 2018-01-15 at 17:12 +0200, Leon Romanovsky wrote:
> > +int rdma_restrack_init(struct rdma_restrack_root *res)
> > +{
> > +	int i = 0;
> > +
> > +	for (; i < _RDMA_RESTRACK_MAX; i++) {
> > +		refcount_set(&res->cnt[i], 1);
> > +		INIT_LIST_HEAD_RCU(&res->list[i]);
> > +		init_rwsem(&res->rwsem[i]);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +void rdma_restrack_clean(struct rdma_restrack_root *res)
> > +{
> > +	int i = 0;
> > +
> > +	for (; i < _RDMA_RESTRACK_MAX; i++) {
> > +		WARN_ON_ONCE(!refcount_dec_and_test(&res->cnt[i]));
> > +		WARN_ON_ONCE(!list_empty(&res->list[i]));
> > +	}
> > +}
>
> Is it really useful to set res->cnt to 1 in rdma_restrack_init() and to decrement
> it in rdma_restrack_clean()? Why not to set res->cnt to 0 in the initialization
> function?

I'm using refcount_dec() in rdma_restrack_del() and hitting 0 will cause
warning from refcount code. I feel that the simple refcount_dec() more
easy to read than refcount_dec_and_test()

>
> > +
> > +static bool is_restrack_valid(enum rdma_restrack_obj type)
> > +{
> > +	return !(type >= _RDMA_RESTRACK_MAX);
> > +}
>
> Whether or not an enum is signed depends on the compiler. So 'type' should be cast
> to an unsigned type before being compared against _RDMA_RESTRACK_MAX. Additionally,
> why does the name _RDMA_RESTRACK_MAX start with a single underscore? I'm not aware
> of any other constant in the IB stack of which the name starts with an underscore.
>

We can remove this function and I can use double underscores to mark
that it is not needed to use outside of restrack code.

> > +
> > +int rdma_restrack_count(struct rdma_restrack_root *res,
> > +			enum rdma_restrack_obj type)
> > +{
> > +	if (!is_restrack_valid(type))
> > +		return 0;
> > +
> > +	/*
> > +	 * The counter was initialized to 1 at the beginning.
> > +	 */
> > +	return refcount_read(&res->cnt[type]) - 1;
> > +}
> > +EXPORT_SYMBOL(rdma_restrack_count);
>
> Why are invalid resource tracking IDs ignored silently instead of e.g. triggering
> a kernel warning?

I'll remove the is_restrack_valid() check, in current implementation it
is always valid.

>
> > +void rdma_restrack_add(struct rdma_restrack_entry *res,
> > +		       enum rdma_restrack_obj type, const char *comm)
> > +{
> > +	struct ib_device *dev;
> > +	struct ib_pd *pd;
> > +	struct ib_cq *cq;
> > +	struct ib_qp *qp;
> > +
> > +	if (!is_restrack_valid(type))
> > +		return;
> > +
> > +	switch (type) {
> > +	case RDMA_RESTRACK_PD:
> > +		pd = container_of(res, struct ib_pd, res);
> > +		dev = pd->device;
> > +		break;
> > +	case RDMA_RESTRACK_CQ:
> > +		cq = container_of(res, struct ib_cq, res);
> > +		dev = cq->device;
> > +		break;
> > +	case RDMA_RESTRACK_QP:
> > +		qp = container_of(res, struct ib_qp, res);
> > +		dev = qp->device;
> > +		break;
> > +	default:
> > +		/* unreachable */
> > +		return;
> > +	}
>
> Please do not add unreachable default clauses but instead leave the default clause
> out such that the compiler can detect missing case labels.
>
> > @@ -1527,9 +1528,10 @@ struct ib_pd {
> >  	u32			unsafe_global_rkey;
> >
> >  	/*
> > -	 * Implementation details of the RDMA core, don't use in drivers:
> > +	 * Implementation details of the RDMA core, don't use in the drivers
>
> The above change changes a grammatically correct sentence into a grammatically
> incorrect one.
>
> > +	/*
> > +	 * Internal to RDMA/core, don't use in the drivers
> > +	 */
> > +	struct rdma_restrack_entry res;
>
> Does a single-line comment have to be formatted as a block comment? Additionally,
> please leave out "the".
>
> > +/**
> > + * 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.

>
> > +/**
> > + * 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.

> > +/**
> > + * 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'

Thanks

>
> Thanks,
>
> Bart.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]