-----"Leon Romanovsky" <leon@xxxxxxxxxx> wrote: -----
>To: "Bernard Metzler" <bmt@xxxxxxxxxxxxxx>
>From: "Leon Romanovsky" <leon@xxxxxxxxxx>
>Date: 04/28/2019 01:07PM
>Cc: linux-rdma@xxxxxxxxxxxxxxx, "Bernard Metzler"
><bmt@xxxxxxxxxxxxxxxxxxx>
>Subject: Re: [PATCH v8 02/12] SIW main include file
>
>On Fri, Apr 26, 2019 at 03:18:42PM +0200, Bernard Metzler wrote:
>> From: Bernard Metzler <bmt@xxxxxxxxxxxxxxxxxxx>
>>
>> Signed-off-by: Bernard Metzler <bmt@xxxxxxxxxxxxxx>
>> ---
>> drivers/infiniband/sw/siw/siw.h | 733
>++++++++++++++++++++++++++++++++
>> 1 file changed, 733 insertions(+)
>> create mode 100644 drivers/infiniband/sw/siw/siw.h
>>
>> diff --git a/drivers/infiniband/sw/siw/siw.h
>b/drivers/infiniband/sw/siw/siw.h
>> new file mode 100644
>> index 000000000000..9a3c2abbd858
>> --- /dev/null
>> +++ b/drivers/infiniband/sw/siw/siw.h
>> @@ -0,0 +1,733 @@
>> +/* SPDX-License-Identifier: GPL-2.0 or BSD-3-Clause */
>> +
>> +/* Authors: Bernard Metzler <bmt@xxxxxxxxxxxxxx> */
>> +/* Copyright (c) 2008-2019, IBM Corporation */
>> +
>> +#ifndef _SIW_H
>> +#define _SIW_H
>> +
>> +#include <linux/idr.h>
>> +#include <rdma/ib_verbs.h>
>> +#include <linux/socket.h>
>> +#include <linux/skbuff.h>
>> +#include <linux/in.h>
>> +#include <linux/fs.h>
>> +#include <linux/netdevice.h>
>> +#include <crypto/hash.h>
>> +#include <linux/resource.h> /* MLOCK_LIMIT */
>> +#include <linux/module.h>
>> +#include <linux/version.h>
>> +#include <linux/llist.h>
>> +#include <linux/mm.h>
>> +#include <linux/sched/signal.h>
>> +
>> +#include <rdma/siw_user.h>
>> +#include "iwarp.h"
>> +
>> +/* driver debugging enabled */
>> +#define DEBUG
>
>I clearly remember that we asked to remove this.
Absolutely. Sorry, it sneaked in again since I did some
debugging. Will remove...
>
>> + spinlock_t lock;
>> +
>> + /* object management */
>> + struct idr qp_idr;
>> + struct idr mem_idr;
>
>Why IDR and not XArray?
Memory access keys and QP IDs are generated as random
numbers, since both are exposed to the application.
Since XArray is not designed for sparsely distributed
id ranges, I am still in favor of IDR for these two
resources.
>
>> + /* active objects statistics */
>
>refcount_t please
>
>> + atomic_t num_qp;
>> + atomic_t num_cq;
>> + atomic_t num_pd;
>> + atomic_t num_mr;
>> + atomic_t num_srq;
>> + atomic_t num_cep;
>> + atomic_t num_ctx;
>> +
>
These counters are only used to limit the amount of
resources allocated to their max values per device.
Since there is no equivalent for atomic_inc_return()
for refcounters I'd suggest to stay with atomic_t:
refcount_inc(&sdev->num_mr);
if (refcount_read(&sdev->num_mr) > SIW_MAX_MR) {
siw_dbg_pd(pd, "too many mr's\n");
rv = -ENOMEM;
goto err_out;
}
vs.
if (atomic_inc_return(&sdev->num_mr) > SIW_MAX_MR) {
siw_dbg_pd(pd, "too many mr's\n");
rv = -ENOMEM;
goto err_out;
}
><...>
>
>> +/*
>> + * Generic memory representation for registered siw memory.
>> + * Memory lookup always via higher 24 bit of STag (STag index).
>> + * Object relates to memory window if embedded mr pointer is valid
>> + */
>> +struct siw_mem {
>> + struct siw_device *sdev;
>> + struct kref ref;
>
><...>
>
>> +struct siw_qp {
>> + struct ib_qp base_qp;
>> + struct siw_device *sdev;
>> + struct kref ref;
>
>I wonder if kref is needed in driver code.
>
><...>
Memory and QP objects are, while generally maintained
by the RDMA midlayer, also guarded against deallocation
while still in use by the driver. The code tries to avoid
taking resource locks for operations like in flight RDMA reads
on a memory object. So it makes use of the release function
in kref_put().
>
>> +/* Varia */
>
>????
>
right, will remove useless comment.
>> +extern void siw_cq_flush(struct siw_cq *cq);
>> +extern void siw_sq_flush(struct siw_qp *qp);
>> +extern void siw_rq_flush(struct siw_qp *qp);
>> +extern int siw_reap_cqe(struct siw_cq *cq, struct ib_wc *wc);
>> +extern void siw_print_hdr(union iwarp_hdr *hdr, int qp_id, char
>*string);
>> +#endif
>> --
>> 2.17.2
>>
>
>
