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:
signature.asc
Description: PGP signature
