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: 5860 bytes --]

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?

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

> +
> +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?

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

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

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

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š