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