Skip to contentSkip to conversation

RDMA resource tracking

PATCH Superseded Leon Romanovsky 3 participants 20 messages Jan 15, 2018 – Jan 18, 2018 11 messages not in the archive

Subsystems (7)

v4 of 7 Superseded Latest is v7 →

lore
Share & tools
lore
#0 lore raw

v4 cover RDMA resource tracking

Changelog:
v3->4:
* Added Steve ROB tags.
* Updated restrack.h to be compatible with kernel-doc format
* Fixed bug when not all QPs (more than one netlink page) were returned by netlink interface
* Removed "MAX" values from object summary. In followup patches, I will eturn object summary per-namespace and not for init PID namespace only. Max values can be seen with ibv_devinfo tool.
* Renamed "comm" to be kern_name.
* Fix spelling errors.

I didn't change implementation of setting names in objects, because I want to do it after Steve's implementation of RESTRACK_OBJ_CMA, to be sure that it works for both of us. Steve's implementation is almost ready and he is waiting for this code to be accepted.

v2->v3:
* Added support of PID namespaces.
* Rewrote rdma_restraack_add function to ensure that it won't appear in lists before it is valid.
* Replace pid/task name caching logic to use task_struct instead.
* Removed report of name of task's for user space objects. Users are expected to read it through /proc/PID/comm.
v1->v2:
* Rebased on latest rdma/for-next
* Replaced mutex lock which protect linked lists to be RW semaphore. It has no impact on current implementation, because there is only one reader (nldev) and it is serialized. However better to be prepared for multiple readers from the beginning.
* Added reinitialization next QP entry logic to ensure that it exists and didn't vanish during fill_req_qp() work.
v0->v1:
* Dropped RFC
* Separated to thre series, one for for-rc, and two for-next.

-------
The original goal of this series was to allow ability to view connection (QP) information about running processes, however I used this opportunity and created common infrastructure to track and report various resources. The report part is implemented in netlink (nldev), but smart ULPs can now create advanced usage models based on device utilization.

The current implementation relies on one lock per-object per-device, so creation/destroying of various objects (CQ, PD, e.t.c) on various or the same devices doesn't interfere each with another.

The data protection is performed with SRCU and its reader-writer model ensures that resource won't be destroyed till readers will finish their work.

Possible future work will include:
* Reducing number of locks in RDMA, because of SRCU.
* Converting CMA to be based completely on resource tracking.
* Addition of other objects and extending current to give full and detailed state of the RDMA kernel stack.
* Replacing synchronize_srcu with call_srcu to make destroy flow non-blocking.
* Provide reliable device reset flow, preserving resource creation ordering.

Thanks
---------------------------------------

Leon Romanovsky (7):
RDMA/restrack: Add general infrastructure to track RDMA resources RDMA/core: Add helper function to create named QPs
RDMA: Annotate create QP callers
RDMA/core: Add resource tracking for create and destroy CQs RDMA/core: Add resource tracking for create and destroy PDs RDMA/nldev: Provide global resource utilization RDMA/nldev: Provide detailed QP information

 drivers/infiniband/core/Makefile           |   2 +-
 drivers/infiniband/core/cma.c              |   1 +
 drivers/infiniband/core/core_priv.h        |  20 ++
 drivers/infiniband/core/cq.c               |   3 +
 drivers/infiniband/core/device.c           |   7 +
 drivers/infiniband/core/mad.c              |   1 +
 drivers/infiniband/core/nldev.c            | 391 +++++++++++++++++++++++++++++
 drivers/infiniband/core/restrack.c         | 182 ++++++++++++++
 drivers/infiniband/core/uverbs_cmd.c       |   5 +-
 drivers/infiniband/core/uverbs_std_types.c |   2 +
 drivers/infiniband/core/verbs.c            |   8 +-
 drivers/infiniband/hw/mlx4/mad.c           |   1 +
 drivers/infiniband/hw/mlx4/qp.c            |   1 +
 drivers/infiniband/hw/mlx5/gsi.c           |   2 +
 drivers/infiniband/ulp/ipoib/ipoib_cm.c    |   4 +-
 drivers/infiniband/ulp/ipoib/ipoib_verbs.c |   1 +
 drivers/infiniband/ulp/srp/ib_srp.c        |   1 +
 drivers/infiniband/ulp/srpt/ib_srpt.c      |   1 +
 include/rdma/ib_verbs.h                    |  23 +-
 include/rdma/restrack.h                    | 197 +++++++++++++++
 include/uapi/rdma/rdma_netlink.h           |  55 ++++
 net/smc/smc_ib.c                           |   1 +
 22 files changed, 902 insertions(+), 7 deletions(-)
 create mode 100644 drivers/infiniband/core/restrack.c
 create mode 100644 include/rdma/restrack.h

--
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20180115151255.30167-1-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org

#2 lore raw

PATCH v4 1/7 RDMA/restrack: Add general infrastructure to track RDMA resources

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

The RDMA subsystem has very strict set of objects to work on it, but it completely lacks tracking facilities and no visibility of resource utilization.

The following patch adds such infrastructure to keep track of RDMA resources to help with debugging of user space applications. The primary user of this infrastructure is RDMA nldev netlink (following patches), but it is not limited too.

At this stage, the main three objects (PD, CQ and QP) are added, and more will be added later.

There are four new functions in use by RDMA/core:

 * rdma_restrack_init(...)   - initializes restrack database
 * rdma_restrack_clean(...)  - cleans restrack database
 * rdma_restrack_add(...)    - adds object to be tracked
 * rdma_restrack_del(...)    - removes object from tracking

3 functions and one iterator visible to kernel users:
* rdma_restrack_count(...) - returns number of allocated objects of specific type
* rdma_restrack_lock(...) - Lock primitive to protect access to list of resources
* rdma_restrack_unlock(...)- Unlock primitive to protect access to list of resources
* for_each_res_safe(...) - iterates over all relevant objects in
the restrack database.

---
 drivers/infiniband/core/Makefile    |   2 +-
 drivers/infiniband/core/core_priv.h |   1 +
 drivers/infiniband/core/device.c    |   7 ++
 drivers/infiniband/core/restrack.c  | 182 +++++++++++++++++++++++++++++++++
 include/rdma/ib_verbs.h             |  17 +++-
 include/rdma/restrack.h             | 197 ++++++++++++++++++++++++++++++++++++
 6 files changed, 404 insertions(+), 2 deletions(-)
 create mode 100644 drivers/infiniband/core/restrack.c
 create mode 100644 include/rdma/restrack.h
drivers/infiniband/core/Makefile +1 -1
index 504b926552c6..f69833db0a32 100644
@@ -12,7 +12,7 @@ ib_core-y :=			packer.o ud_header.o verbs.o cq.o rw.o sysfs.o \
 				device.o fmr_pool.o cache.o netlink.o \
 				roce_gid_mgmt.o mr_pool.o addr.o sa_query.o \
 				multicast.o mad.o smi.o agent.o mad_rmpp.o \
-				security.o nldev.o
+				security.o nldev.o restrack.o
 
 ib_core-$(CONFIG_INFINIBAND_USER_MEM) += umem.o
 ib_core-$(CONFIG_INFINIBAND_ON_DEMAND_PAGING) += umem_odp.o
drivers/infiniband/core/core_priv.h +1 -0
index aef9aa0ac0e6..2b1372da708a 100644
@@ -40,6 +40,7 @@
 #include <rdma/ib_verbs.h>
 #include <rdma/opa_addr.h>
 #include <rdma/ib_mad.h>
+#include <rdma/restrack.h>
 #include "mad_priv.h"
 
 /* Total number of ports combined across all struct ib_devices's */
drivers/infiniband/core/device.c +7 -0
index 2826e06311a5..c3e389f8c99a 100644
@@ -263,6 +263,11 @@ struct ib_device *ib_alloc_device(size_t size)
 	if (!device)
 		return NULL;
 
+	if (rdma_restrack_init(&device->res)) {
+		kfree(device);
+		return NULL;
+	}
+
 	device->dev.class = &ib_class;
 	device_initialize(&device->dev);
 
@@ -596,6 +601,8 @@ void ib_unregister_device(struct ib_device *device)
 	}
 	up_read(&lists_rwsem);
 
+	rdma_restrack_clean(&device->res);
+
 	ib_device_unregister_rdmacg(device);
 	ib_device_unregister_sysfs(device);
 
drivers/infiniband/core/restrack.c new file +182 -0

182 lines not on this page · raw message

include/rdma/ib_verbs.h +16 -1
index f25c03687ee9..271265cd7c1a 100644
@@ -63,6 +63,7 @@
 #include <linux/uaccess.h>
 #include <linux/cgroup_rdma.h>
 #include <uapi/rdma/ib_user_verbs.h>
+#include <rdma/restrack.h>
 
 #define IB_FW_VERSION_NAME_MAX	ETHTOOL_FWVERS_LEN
 
@@ -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
 	 */
 	struct ib_mr	       *__internal_mr;
+	struct rdma_restrack_entry res;
 };
 
 struct ib_xrcd {
@@ -1570,6 +1572,10 @@ struct ib_cq {
 		struct irq_poll		iop;
 		struct work_struct	work;
 	};
+	/*
+	 * Internal to RDMA/core, don't use in the drivers
+	 */
+	struct rdma_restrack_entry res;
 };
 
 struct ib_srq {
@@ -1746,6 +1752,11 @@ struct ib_qp {
 	struct ib_rwq_ind_table *rwq_ind_tbl;
 	struct ib_qp_security  *qp_sec;
 	u8			port;
+
+	/*
+	 * Internal to RDMA/core, don't use in the drivers
+	 */
+	struct rdma_restrack_entry     res;
 };
 
 struct ib_mr {
@@ -2352,6 +2363,10 @@ struct ib_device {
 #endif
 
 	u32                          index;
+	/*
+	 * Implementation details of the RDMA core, don't use in the drivers
+	 */
+	struct rdma_restrack_root     res;
 
 	/**
 	 * The following mandatory functions are used only at device
include/rdma/restrack.h new file +197 -0

197 lines not on this page · raw message

Signature · 6 lines
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20180115151255.30167-2-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org

#4 lore raw

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

Quoted 267 lines · Leon Romanovsky

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

The RDMA subsystem has very strict set of objects to work on it, but it completely lacks tracking facilities and no visibility of resource utilization.

The following patch adds such infrastructure to keep track of RDMA resources to help with debugging of user space applications. The primary user of this infrastructure is RDMA nldev netlink (following patches), but it is not limited too.

At this stage, the main three objects (PD, CQ and QP) are added, and more will be added later.

There are four new functions in use by RDMA/core:

 * rdma_restrack_init(...)   - initializes restrack database
 * rdma_restrack_clean(...)  - cleans restrack database
 * rdma_restrack_add(...)    - adds object to be tracked
 * rdma_restrack_del(...)    - removes object from tracking

3 functions and one iterator visible to kernel users:
* rdma_restrack_count(...) - returns number of allocated objects of specific type
* rdma_restrack_lock(...) - Lock primitive to protect access to list of resources
* rdma_restrack_unlock(...)- Unlock primitive to protect access to list of resources
* for_each_res_safe(...) - iterates over all relevant objects in
the restrack database.

---
 drivers/infiniband/core/Makefile    |   2 +-
 drivers/infiniband/core/core_priv.h |   1 +
 drivers/infiniband/core/device.c    |   7 ++
 drivers/infiniband/core/restrack.c  | 182 +++++++++++++++++++++++++++++++++
 include/rdma/ib_verbs.h             |  17 +++-
 include/rdma/restrack.h             | 197 ++++++++++++++++++++++++++++++++++++
 6 files changed, 404 insertions(+), 2 deletions(-)
 create mode 100644 drivers/infiniband/core/restrack.c
 create mode 100644 include/rdma/restrack.h
drivers/infiniband/core/Makefile +1 -1
index 504b926552c6..f69833db0a32 100644
@@ -12,7 +12,7 @@ ib_core-y :=			packer.o ud_header.o verbs.o cq.o rw.o sysfs.o \
 				device.o fmr_pool.o cache.o netlink.o \
 				roce_gid_mgmt.o mr_pool.o addr.o sa_query.o \
 				multicast.o mad.o smi.o agent.o mad_rmpp.o \
-				security.o nldev.o
+				security.o nldev.o restrack.o
 
 ib_core-$(CONFIG_INFINIBAND_USER_MEM) += umem.o
 ib_core-$(CONFIG_INFINIBAND_ON_DEMAND_PAGING) += umem_odp.o
drivers/infiniband/core/core_priv.h +1 -0
index aef9aa0ac0e6..2b1372da708a 100644
@@ -40,6 +40,7 @@
 #include <rdma/ib_verbs.h>
 #include <rdma/opa_addr.h>
 #include <rdma/ib_mad.h>
+#include <rdma/restrack.h>
 #include "mad_priv.h"
 
 /* Total number of ports combined across all struct ib_devices's */
drivers/infiniband/core/device.c +7 -0
index 2826e06311a5..c3e389f8c99a 100644
@@ -263,6 +263,11 @@ struct ib_device *ib_alloc_device(size_t size)
 	if (!device)
 		return NULL;
 
+	if (rdma_restrack_init(&device->res)) {
+		kfree(device);
+		return NULL;
+	}
+
 	device->dev.class = &ib_class;
 	device_initialize(&device->dev);
 
@@ -596,6 +601,8 @@ void ib_unregister_device(struct ib_device *device)
 	}
 	up_read(&lists_rwsem);
 
+	rdma_restrack_clean(&device->res);
+
 	ib_device_unregister_rdmacg(device);
 	ib_device_unregister_sysfs(device);
 
drivers/infiniband/core/restrack.c new file +167 -0
new file mode 100644
index 000000000000..879b79ea31da
@@ -0,0 +1,182 @@
+/*
+ * Copyright (c) 2017 Mellanox Technologies. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ * 3. Neither the names of the copyright holders nor the names of its
+ *    contributors may be used to endorse or promote products derived from
+ *    this software without specific prior written permission.
+ *
+ * Alternatively, this software may be distributed under the terms of the
+ * GNU General Public License ("GPL") version 2 as published by the Free
+ * Software Foundation.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include <rdma/ib_verbs.h>
+#include <rdma/restrack.h>
+#include <linux/rculist.h>
+#include <linux/sched/task.h>
+
+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]));
+	}
+}
+
+static bool is_restrack_valid(enum rdma_restrack_obj type)
+{
+	return !(type >= _RDMA_RESTRACK_MAX);
+}
+
+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);
+
+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;
+	}
+
+	if (init_srcu_struct(&res->srcu))
+		/*
+		 * We are not returning error, because there is nothing
+		 * we can do it in such case, it is already too late to
+		 * crash the driver just of failure in resource tracking.
+		 *
+		 * Simply leave this resource as not valid.
+		 */
+		return;
+
+	if (!comm || !strlen(comm)) {
+		res->kern_name = NULL;
+		get_task_struct(current);
+		res->task = current;
+	} else {
+		res->task = NULL;
+		res->kern_name = kstrdup_const(comm, GFP_KERNEL);
+		if (!res->kern_name)
+			goto out;
+	}
+
+	refcount_inc(&dev->res.cnt[type]);
+
+	down_write(&dev->res.rwsem[type]);
+	list_add(&res->list, &dev->res.list[type]);
+	res->valid = true;
+	up_write(&dev->res.rwsem[type]);
+	return;
+
+out:
+	res->valid = false;
+	cleanup_srcu_struct(&res->srcu);
+}
+EXPORT_SYMBOL(rdma_restrack_add);
+
+void rdma_restrack_del(struct rdma_restrack_entry *res,
+		       enum rdma_restrack_obj type)
+{
+	struct ib_device *dev;
+	struct ib_pd *pd;
+	struct ib_cq *cq;
+	struct ib_qp *qp;
+
+	if (!is_restrack_valid(type) || !res->valid)
+		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:

drivers/infiniband/core/restrack.c line 177

Quoted 10 lines
drivers/infiniband/core/restrack.c new file +10 -0
+		/* unreachable */
+		return;
+	}
+
+	refcount_dec(&dev->res.cnt[type]);
+	down_write(&dev->res.rwsem[type]);
+	list_del(&res->list);
+	res->valid = false;
+	kfree_const(res->kern_name);
+	put_task_struct(res->task);

There is an error here, it should be
if(res->task)
put_task_struct(res->task);

Resend?

Thanks

#5 lore raw

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

In reply to a message not in the archive

T24gTW9uLCAyMDE4LTAxLTE1IGF0IDE3OjEyICswMjAwLCBMZW9uIFJvbWFub3Zza3kgd3JvdGU6 DQo+ICtpbnQgcmRtYV9yZXN0cmFja19pbml0KHN0cnVjdCByZG1hX3Jlc3RyYWNrX3Jvb3QgKnJl cykNCj4gK3sNCj4gKwlpbnQgaSA9IDA7DQo+ICsNCj4gKwlmb3IgKDsgaSA8IF9SRE1BX1JFU1RS QUNLX01BWDsgaSsrKSB7DQo+ICsJCXJlZmNvdW50X3NldCgmcmVzLT5jbnRbaV0sIDEpOw0KPiAr CQlJTklUX0xJU1RfSEVBRF9SQ1UoJnJlcy0+bGlzdFtpXSk7DQo+ICsJCWluaXRfcndzZW0oJnJl cy0+cndzZW1baV0pOw0KPiArCX0NCj4gKw0KPiArCXJldHVybiAwOw0KPiArfQ0KPiArDQo+ICt2 b2lkIHJkbWFfcmVzdHJhY2tfY2xlYW4oc3RydWN0IHJkbWFfcmVzdHJhY2tfcm9vdCAqcmVzKQ0K PiArew0KPiArCWludCBpID0gMDsNCj4gKw0KPiArCWZvciAoOyBpIDwgX1JETUFfUkVTVFJBQ0tf TUFYOyBpKyspIHsNCj4gKwkJV0FSTl9PTl9PTkNFKCFyZWZjb3VudF9kZWNfYW5kX3Rlc3QoJnJl cy0+Y250W2ldKSk7DQo+ICsJCVdBUk5fT05fT05DRSghbGlzdF9lbXB0eSgmcmVzLT5saXN0W2ld KSk7DQo+ICsJfQ0KPiArfQ0KDQpJcyBpdCByZWFsbHkgdXNlZnVsIHRvIHNldCByZXMtPmNudCB0 byAxIGluIHJkbWFfcmVzdHJhY2tfaW5pdCgpIGFuZCB0byBkZWNyZW1lbnQNCml0IGluIHJkbWFf cmVzdHJhY2tfY2xlYW4oKT8gV2h5IG5vdCB0byBzZXQgcmVzLT5jbnQgdG8gMCBpbiB0aGUgaW5p dGlhbGl6YXRpb24NCmZ1bmN0aW9uPw0KDQo+ICsNCj4gK3N0YXRpYyBib29sIGlzX3Jlc3RyYWNr X3ZhbGlkKGVudW0gcmRtYV9yZXN0cmFja19vYmogdHlwZSkNCj4gK3sNCj4gKwlyZXR1cm4gISh0 eXBlID49IF9SRE1BX1JFU1RSQUNLX01BWCk7DQo+ICt9DQoNCldoZXRoZXIgb3Igbm90IGFuIGVu dW0gaXMgc2lnbmVkIGRlcGVuZHMgb24gdGhlIGNvbXBpbGVyLiBTbyAndHlwZScgc2hvdWxkIGJl IGNhc3QNCnRvIGFuIHVuc2lnbmVkIHR5cGUgYmVmb3JlIGJlaW5nIGNvbXBhcmVkIGFnYWluc3Qg X1JETUFfUkVTVFJBQ0tfTUFYLiBBZGRpdGlvbmFsbHksDQp3aHkgZG9lcyB0aGUgbmFtZSBfUkRN QV9SRVNUUkFDS19NQVggc3RhcnQgd2l0aCBhIHNpbmdsZSB1bmRlcnNjb3JlPyBJJ20gbm90IGF3 YXJlDQpvZiBhbnkgb3RoZXIgY29uc3RhbnQgaW4gdGhlIElCIHN0YWNrIG9mIHdoaWNoIHRoZSBu YW1lIHN0YXJ0cyB3aXRoIGFuIHVuZGVyc2NvcmUuDQoNCj4gKw0KPiAraW50IHJkbWFfcmVzdHJh Y2tfY291bnQoc3RydWN0IHJkbWFfcmVzdHJhY2tfcm9vdCAqcmVzLA0KPiArCQkJZW51bSByZG1h X3Jlc3RyYWNrX29iaiB0eXBlKQ0KPiArew0KPiArCWlmICghaXNfcmVzdHJhY2tfdmFsaWQodHlw ZSkpDQo+ICsJCXJldHVybiAwOw0KPiArDQo+ICsJLyoNCj4gKwkgKiBUaGUgY291bnRlciB3YXMg aW5pdGlhbGl6ZWQgdG8gMSBhdCB0aGUgYmVnaW5uaW5nLg0KPiArCSAqLw0KPiArCXJldHVybiBy ZWZjb3VudF9yZWFkKCZyZXMtPmNudFt0eXBlXSkgLSAxOw0KPiArfQ0KPiArRVhQT1JUX1NZTUJP TChyZG1hX3Jlc3RyYWNrX2NvdW50KTsNCg0KV2h5IGFyZSBpbnZhbGlkIHJlc291cmNlIHRyYWNr aW5nIElEcyBpZ25vcmVkIHNpbGVudGx5IGluc3RlYWQgb2YgZS5nLiB0cmlnZ2VyaW5nDQphIGtl cm5lbCB3YXJuaW5nPw0KDQo+ICt2b2lkIHJkbWFfcmVzdHJhY2tfYWRkKHN0cnVjdCByZG1hX3Jl c3RyYWNrX2VudHJ5ICpyZXMsDQo+ICsJCSAgICAgICBlbnVtIHJkbWFfcmVzdHJhY2tfb2JqIHR5 cGUsIGNvbnN0IGNoYXIgKmNvbW0pDQo+ICt7DQo+ICsJc3RydWN0IGliX2RldmljZSAqZGV2Ow0K PiArCXN0cnVjdCBpYl9wZCAqcGQ7DQo+ICsJc3RydWN0IGliX2NxICpjcTsNCj4gKwlzdHJ1Y3Qg aWJfcXAgKnFwOw0KPiArDQo+ICsJaWYgKCFpc19yZXN0cmFja192YWxpZCh0eXBlKSkNCj4gKwkJ cmV0dXJuOw0KPiArDQo+ICsJc3dpdGNoICh0eXBlKSB7DQo+ICsJY2FzZSBSRE1BX1JFU1RSQUNL X1BEOg0KPiArCQlwZCA9IGNvbnRhaW5lcl9vZihyZXMsIHN0cnVjdCBpYl9wZCwgcmVzKTsNCj4g KwkJZGV2ID0gcGQtPmRldmljZTsNCj4gKwkJYnJlYWs7DQo+ICsJY2FzZSBSRE1BX1JFU1RSQUNL X0NROg0KPiArCQljcSA9IGNvbnRhaW5lcl9vZihyZXMsIHN0cnVjdCBpYl9jcSwgcmVzKTsNCj4g KwkJZGV2ID0gY3EtPmRldmljZTsNCj4gKwkJYnJlYWs7DQo+ICsJY2FzZSBSRE1BX1JFU1RSQUNL X1FQOg0KPiArCQlxcCA9IGNvbnRhaW5lcl9vZihyZXMsIHN0cnVjdCBpYl9xcCwgcmVzKTsNCj4g KwkJZGV2ID0gcXAtPmRldmljZTsNCj4gKwkJYnJlYWs7DQo+ICsJZGVmYXVsdDoNCj4gKwkJLyog dW5yZWFjaGFibGUgKi8NCj4gKwkJcmV0dXJuOw0KPiArCX0NCg0KUGxlYXNlIGRvIG5vdCBhZGQg dW5yZWFjaGFibGUgZGVmYXVsdCBjbGF1c2VzIGJ1dCBpbnN0ZWFkIGxlYXZlIHRoZSBkZWZhdWx0 IGNsYXVzZQ0Kb3V0IHN1Y2ggdGhhdCB0aGUgY29tcGlsZXIgY2FuIGRldGVjdCBtaXNzaW5nIGNh c2UgbGFiZWxzLg0KDQo+IEBAIC0xNTI3LDkgKzE1MjgsMTAgQEAgc3RydWN0IGliX3BkIHsNCj4g IAl1MzIJCQl1bnNhZmVfZ2xvYmFsX3JrZXk7DQo+ICANCj4gIAkvKg0KPiAtCSAqIEltcGxlbWVu dGF0aW9uIGRldGFpbHMgb2YgdGhlIFJETUEgY29yZSwgZG9uJ3QgdXNlIGluIGRyaXZlcnM6DQo+ ICsJICogSW1wbGVtZW50YXRpb24gZGV0YWlscyBvZiB0aGUgUkRNQSBjb3JlLCBkb24ndCB1c2Ug aW4gdGhlIGRyaXZlcnMNCg0KVGhlIGFib3ZlIGNoYW5nZSBjaGFuZ2VzIGEgZ3JhbW1hdGljYWxs eSBjb3JyZWN0IHNlbnRlbmNlIGludG8gYSBncmFtbWF0aWNhbGx5DQppbmNvcnJlY3Qgb25lLg0K DQo+ICsJLyoNCj4gKwkgKiBJbnRlcm5hbCB0byBSRE1BL2NvcmUsIGRvbid0IHVzZSBpbiB0aGUg ZHJpdmVycw0KPiArCSAqLw0KPiArCXN0cnVjdCByZG1hX3Jlc3RyYWNrX2VudHJ5IHJlczsNCg0K RG9lcyBhIHNpbmdsZS1saW5lIGNvbW1lbnQgaGF2ZSB0byBiZSBmb3JtYXR0ZWQgYXMgYSBibG9j ayBjb21tZW50PyBBZGRpdGlvbmFsbHksDQpwbGVhc2UgbGVhdmUgb3V0ICJ0aGUiLg0KDQo+ICsv KioNCj4gKyAqIGVudW0gcmRtYV9yZXN0cmFja19vYmogLSBIVyBvYmplY3RzIHRvIHRyYWNrDQo+ ICsgKi8NCj4gK2VudW0gcmRtYV9yZXN0cmFja19vYmogew0KPiArCS8qKg0KPiArCSAqIEBSRE1B X1JFU1RSQUNLX1BEOiBQcm90ZWN0aW9uIGRvbWFpbiAoUEQpDQo+ICsJICovDQo+ICsJUkRNQV9S RVNUUkFDS19QRCwNCj4gKwkvKioNCj4gKwkgKiBAUkRNQV9SRVNUUkFDS19DUTogQ29tcGxldGlv biBxdWV1ZSAoQ1EpDQo+ICsJICovDQo+ICsJUkRNQV9SRVNUUkFDS19DUSwNCj4gKwkvKioNCj4g KwkgKiBAUkRNQV9SRVNUUkFDS19RUDogUXVldWUgcGFpciAoUVApDQo+ICsJICovDQo+ICsJUkRN QV9SRVNUUkFDS19RUCwNCj4gKwkvKiBwcml2YXRlOiBjb3VudHMgbnVtYmVyIG9mIGVsZW1lbnRz LCBhbHdheXMgbGFzdCAqLw0KPiArCV9SRE1BX1JFU1RSQUNLX01BWA0KPiArfTsNCg0KVGhpcyBs b29rcyByZWFsbHkgdWdseSB0byBtZS4gUGxlYXNlIHVzZSBrZXJuZWwtZG9jIHN5bnRheCB0byBk b2N1bWVudCB0aGUgUkRNQQ0KcmVzb3VyY2UgdHlwZXMuDQoNCj4gKy8qKg0KPiArICogc3RydWN0 IHJkbWFfcmVzdHJhY2tfcm9vdCAtIG1haW4gcmVzb3VyY2UgdHJhY2tpbmcgbWFuYWdlbWVudA0K PiArICogZW50aXR5LCBwZXItZGV2aWNlDQo+ICsgKi8NCj4gK3N0cnVjdCByZG1hX3Jlc3RyYWNr X3Jvb3Qgew0KPiArCS8qKg0KPiArCSAqIEBjbnQ6IGdsb2JhbCBjb3VudGVyIHRvIGF2b2lkIHRo ZSBuZWVkIHRvIGNvdW50IG51bWJlcg0KPiArCSAqIG9mIGVsZW1lbnRzIGluIHRoZSBvYmplY3Qn cyBsaXN0Lg0KPiArCSAqDQo+ICsJICogSXQgY2FuIGJlIGRpZmZlcmVudCBmcm9tIHRoZSBsaXN0 X2NvdW50LCBiZWNhdXNlIHdlIGFyZQ0KPiArCSAqIG5vdCB0YWtpbmcgbG9jayBkdXJpbmcgY291 bnRlciBpbmNyZW1lbnQgYW5kIGRvbid0DQo+ICsJICogc3luY2hyb25pemUgdGhlIFJDVS4NCj4g KwkgKi8NCj4gKwlyZWZjb3VudF90CQljbnRbX1JETUFfUkVTVFJBQ0tfTUFYXTsNCj4gKwkvKioN Cj4gKwkgKiBAbGlzdDogbGlua2VkIGxpc3Qgb2YgYWxsIGVudHJpZXMgcGVyLW9iamVjdA0KPiAr CSAqLw0KPiArCXN0cnVjdCBsaXN0X2hlYWQJbGlzdFtfUkRNQV9SRVNUUkFDS19NQVhdOw0KPiAr CS8qIHByaXZhdGU6IEludGVybmFsIHJlYWQvd3JpdGUgbG9jay4NCj4gKwkgKiBJdCBpcyBuZWVk ZWQgdG8gcHJvdGVjdCB0aGUgYWRkL2RlbGV0ZSBsaXN0IG9wZXJhdGlvbnMuDQo+ICsJICovDQo+ ICsJc3RydWN0IHJ3X3NlbWFwaG9yZQlyd3NlbVtfUkRNQV9SRVNUUkFDS19NQVhdOw0KPiArfTsN Cg0KVGhlIGFib3ZlIGxvb2tzIHdyb25nIHRvIG1lLiBQbGVhc2UgY2hhbmdlIHRoZSBhYm92ZSBp bnRvIGFuIGFycmF5IG9mIGRhdGEgc3RydWN0dXJlcw0KaW5zdGVhZCBvZiBhIGRhdGEgc3RydWN0 dXJlIHRoYXQgaXMgZnVsbCBvZiBhcnJheXMgb2YgaWRlbnRpY2FsIHNpemUuDQoNCj4gKy8qKg0K PiArICogc3RydWN0IHJkbWFfcmVzdHJhY2tfZW50cnkgLSBtZXRhZGF0YSBwZXItZW50cnkNCj4g KyAqLw0KPiArc3RydWN0IHJkbWFfcmVzdHJhY2tfZW50cnkgew0KPiArCS8qKg0KPiArCSAqIEBs aXN0OiBsaW5rZWQgbGlzdCBiZXR3ZWVuIGVudHJpZXMNCj4gKwkgKi8NCj4gKwlzdHJ1Y3QgbGlz dF9oZWFkCWxpc3Q7DQo+ICsJLyoqDQo+ICsJICogQHZhbGlkOiB2YWxpZGl0eSBpbmRpY2F0b3IN Cj4gKwkgKg0KPiArCSAqIFRoZSBlbnRyaWVzIGFyZSBmaWxsZWQgZHVyaW5nIHJkbWFfcmVzdHJh Y2tfYWRkLA0KPiArCSAqIGNhbiBiZSBhdHRlbXB0ZWQgdG8gYmUgZnJlZSBkdXJpbmcgcmRtYV9y ZXN0cmFja19kZWwuDQo+ICsJICoNCj4gKwkgKiBBcyBhbiBleGFtcGxlIGZvciB0aGF0LCBzZWUg bWx4NSBRUHMgd2l0aCB0eXBlIE1MWDVfSUJfUVBUX0hXX0dTSQ0KPiArCSAqLw0KPiArCWJvb2wJ CQl2YWxpZDsNCj4gKwkvKioNCj4gKwkgKiBAc3JjdTogc2xlZXBhYmxlIFJDVSB0byBwcm90ZWN0 IG9iamVjdCBkYXRhLg0KPiArCSAqLw0KPiArCXN0cnVjdCBzcmN1X3N0cnVjdAlzcmN1Ow0KPiAr CS8qKg0KPiArCSAqIEB0YXNrOiBvd25lciBvZiByZXNvdXJjZSB0cmFja2luZyBlbnRpdHkNCj4g KwkgKg0KPiArCSAqIFRoZXJlIGFyZSB0d28gdHlwZXMgb2YgZW50aXRpZXM6IGNyZWF0ZWQgYnkg dXNlciBhbmQgY3JlYXRlZA0KPiArCSAqIGJ5IGtlcm5lbC4NCj4gKwkgKg0KPiArCSAqIFRoaXMg aXMgcmVsZXZhbnQgZm9yIHRoZSBlbnRpdGllcyBjcmVhdGVkIGJ5IHVzZXJzLg0KPiArCSAqIEZv ciB0aGUgZW50aWVpZXMgY3JlYXRlZCBieSBrZXJuZWwsIHRoaXMgcG9pbnRlciB3aWxsIGJlIE5V TEwuDQo+ICsJICovDQo+ICsJc3RydWN0IHRhc2tfc3RydWN0CSp0YXNrOw0KPiArCS8qKg0KPiAr CSAqIEBrZXJuX25hbWU6IG5hbWUgb2Ygb3duZXIgZm9yIHRoZSBrZXJuZWwgY3JlYXRlZCBlbnRp dGllcy4NCj4gKwkgKi8NCj4gKwljb25zdCBjaGFyCQkqa2Vybl9uYW1lOw0KPiArfTsNCg0KQWdh aW4sIHBsZWFzZSB1c2UgdGhlIGtlcm5lbC1kb2Mgc3ludGF4IHRvIGRvY3VtZW50IHN0cnVjdHVy ZSBtZW1iZXJzLiBBZGRpdGlvbmFsbHksDQpwbGVhc2UgZml4IHRoZSBzcGVsbGluZyBvZiAiZW50 aWVpZXMiLg0KDQpUaGFua3MsDQoNCkJhcnQu
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

#7 lore raw

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

Quoted 19 lines · Bart Van Assche
Quoted 18 lines · Leon Romanovsky
drivers/infiniband/core/restrack.c new file +18 -0
+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++) {
Quoted 8 lines

drivers/infiniband/core/restrack.c line 59

Quoted 4 lines
drivers/infiniband/core/restrack.c new file +4 -0
+		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()

Quoted 12 lines

drivers/infiniband/core/restrack.c line 64

Quoted 6 lines
drivers/infiniband/core/restrack.c new file +5 -0
+
+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.

Quoted 8 lines
Quoted 8 lines
drivers/infiniband/core/restrack.c new file +8 -0
+
+int rdma_restrack_count(struct rdma_restrack_root *res,
+			enum rdma_restrack_obj type)
+{
+	if (!is_restrack_valid(type))
+		return 0;
+
+	/*
Quoted 8 lines

drivers/infiniband/core/restrack.c line 77

Quoted 5 lines
drivers/infiniband/core/restrack.c new file +5 -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.

Quoted 65 lines
Quoted 19 lines
drivers/infiniband/core/restrack.c new file +18 -0
+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);

drivers/infiniband/core/restrack.c line 106

Quoted 10 lines
drivers/infiniband/core/restrack.c new file +10 -0
+		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.

include/rdma/ib_verbs.h struct ib_pd { · line 1531

Quoted 6 lines
include/rdma/ib_verbs.h +1 -1
@@ -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.

include/rdma/ib_verbs.h struct ib_cq { · line 1578

Quoted 4 lines
include/rdma/ib_verbs.h +4 -0
+	/*
+	 * 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".

Quoted 14 lines
include/rdma/restrack.h new file +14 -0
+/**
+ * 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)
Quoted 8 lines

include/rdma/restrack.h line 59

Quoted 5 lines
include/rdma/restrack.h new file +5 -0
+	 */
+	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.

Quoted 19 lines
Quoted 19 lines
include/rdma/restrack.h new file +18 -0
+/**
+ * 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];
Quoted 9 lines

include/rdma/restrack.h line 83

Quoted 5 lines
include/rdma/restrack.h new file +5 -0
+	/* 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.

Quoted 31 lines
Quoted 31 lines
include/rdma/restrack.h new file +31 -0
+/**
+ * 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;
Quoted 8 lines

include/rdma/restrack.h line 120

Quoted 5 lines
include/rdma/restrack.h new file +5 -0
+	/**
+	 * @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

Quoted 4 lines

Thanks,

Bart.

A message that is not in the archive (sent off-list or not yet received): 20180117054721.GE13639-U/[email protected]

#9 lore raw

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

T24gV2VkLCAyMDE4LTAxLTE3IGF0IDA3OjQ3ICswMjAwLCBMZW9uIFJvbWFub3Zza3kgd3JvdGU6 DQo+IE9uIFR1ZSwgSmFuIDE2LCAyMDE4IGF0IDA5OjMzOjE4UE0gKzAwMDAsIEJhcnQgVmFuIEFz c2NoZSB3cm90ZToNCj4gPiA+ICsvKioNCj4gPiA+ICsgKiBlbnVtIHJkbWFfcmVzdHJhY2tfb2Jq IC0gSFcgb2JqZWN0cyB0byB0cmFjaw0KPiA+ID4gKyAqLw0KPiA+ID4gK2VudW0gcmRtYV9yZXN0 cmFja19vYmogew0KPiA+ID4gKwkvKioNCj4gPiA+ICsJICogQFJETUFfUkVTVFJBQ0tfUEQ6IFBy b3RlY3Rpb24gZG9tYWluIChQRCkNCj4gPiA+ICsJICovDQo+ID4gPiArCVJETUFfUkVTVFJBQ0tf UEQsDQo+ID4gPiArCS8qKg0KPiA+ID4gKwkgKiBAUkRNQV9SRVNUUkFDS19DUTogQ29tcGxldGlv biBxdWV1ZSAoQ1EpDQo+ID4gPiArCSAqLw0KPiA+ID4gKwlSRE1BX1JFU1RSQUNLX0NRLA0KPiA+ ID4gKwkvKioNCj4gPiA+ICsJICogQFJETUFfUkVTVFJBQ0tfUVA6IFF1ZXVlIHBhaXIgKFFQKQ0K PiA+ID4gKwkgKi8NCj4gPiA+ICsJUkRNQV9SRVNUUkFDS19RUCwNCj4gPiA+ICsJLyogcHJpdmF0 ZTogY291bnRzIG51bWJlciBvZiBlbGVtZW50cywgYWx3YXlzIGxhc3QgKi8NCj4gPiA+ICsJX1JE TUFfUkVTVFJBQ0tfTUFYDQo+ID4gPiArfTsNCj4gPiANCj4gPiBUaGlzIGxvb2tzIHJlYWxseSB1 Z2x5IHRvIG1lLiBQbGVhc2UgdXNlIGtlcm5lbC1kb2Mgc3ludGF4IHRvIGRvY3VtZW50IHRoZSBS RE1BDQo+ID4gcmVzb3VyY2UgdHlwZXMuDQo+IA0KPiBJIHVzZWQga2VybmUtZG9jIGFuZCBfUkRN QV9SRVNUUkFDS19NQVggd2FzIGFuIGV4Y2VwdGlvbiwgaXQgaXMgbm90IHN1cHBvc2VkIHRvIGJl DQo+IHVzZWQgb3V0c2lkZSBvZiByZXN0cmFjayBjb2RlLg0KDQpIZWxsbyBMZW9uLA0KDQpQbGVh c2UgaGF2ZSBhIGxvb2sgYXQgdGhlIGZvbGxvd2luZyBleGFtcGxlIGZyb20gRG9jdW1lbnRhdGlv bi9rZXJuZWwtZG9jLW5hbm8tSE9XVE8udHh0Og0KDQprZXJuZWwtZG9jIGZvciBzdHJ1Y3RzLCB1 bmlvbnMsIGVudW1zLCBhbmQgdHlwZWRlZnMNCi0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t LS0tLS0tLS0tLS0tLS0tLS0tLS0tLQ0KDQpbIC4uLiBdDQoNCi8qKg0KICogc3RydWN0IG15X3N0 cnVjdCAtIHNob3J0IGRlc2NyaXB0aW9uDQogKiBAYTogZmlyc3QgbWVtYmVyDQogKiBAYjogc2Vj b25kIG1lbWJlcg0KICoNCiAqIExvbmdlciBkZXNjcmlwdGlvbg0KICovDQpzdHJ1Y3QgbXlfc3Ry dWN0IHsNCiAgICBpbnQgYTsNCiAgICBpbnQgYjsNCi8qIHByaXZhdGU6IGludGVybmFsIHVzZSBv bmx5ICovDQogICAgaW50IGM7DQp9Ow0KDQo+ID4gPiArLyoqDQo+ID4gPiArICogc3RydWN0IHJk bWFfcmVzdHJhY2tfcm9vdCAtIG1haW4gcmVzb3VyY2UgdHJhY2tpbmcgbWFuYWdlbWVudA0KPiA+ ID4gKyAqIGVudGl0eSwgcGVyLWRldmljZQ0KPiA+ID4gKyAqLw0KPiA+ID4gK3N0cnVjdCByZG1h X3Jlc3RyYWNrX3Jvb3Qgew0KPiA+ID4gKwkvKioNCj4gPiA+ICsJICogQGNudDogZ2xvYmFsIGNv dW50ZXIgdG8gYXZvaWQgdGhlIG5lZWQgdG8gY291bnQgbnVtYmVyDQo+ID4gPiArCSAqIG9mIGVs ZW1lbnRzIGluIHRoZSBvYmplY3QncyBsaXN0Lg0KPiA+ID4gKwkgKg0KPiA+ID4gKwkgKiBJdCBj YW4gYmUgZGlmZmVyZW50IGZyb20gdGhlIGxpc3RfY291bnQsIGJlY2F1c2Ugd2UgYXJlDQo+ID4g PiArCSAqIG5vdCB0YWtpbmcgbG9jayBkdXJpbmcgY291bnRlciBpbmNyZW1lbnQgYW5kIGRvbid0 DQo+ID4gPiArCSAqIHN5bmNocm9uaXplIHRoZSBSQ1UuDQo+ID4gPiArCSAqLw0KPiA+ID4gKwly ZWZjb3VudF90CQljbnRbX1JETUFfUkVTVFJBQ0tfTUFYXTsNCj4gPiA+ICsJLyoqDQo+ID4gPiAr CSAqIEBsaXN0OiBsaW5rZWQgbGlzdCBvZiBhbGwgZW50cmllcyBwZXItb2JqZWN0DQo+ID4gPiAr CSAqLw0KPiA+ID4gKwlzdHJ1Y3QgbGlzdF9oZWFkCWxpc3RbX1JETUFfUkVTVFJBQ0tfTUFYXTsN Cj4gPiA+ICsJLyogcHJpdmF0ZTogSW50ZXJuYWwgcmVhZC93cml0ZSBsb2NrLg0KPiA+ID4gKwkg KiBJdCBpcyBuZWVkZWQgdG8gcHJvdGVjdCB0aGUgYWRkL2RlbGV0ZSBsaXN0IG9wZXJhdGlvbnMu DQo+ID4gPiArCSAqLw0KPiA+ID4gKwlzdHJ1Y3Qgcndfc2VtYXBob3JlCXJ3c2VtW19SRE1BX1JF U1RSQUNLX01BWF07DQo+ID4gPiArfTsNCj4gPiANCj4gPiBUaGUgYWJvdmUgbG9va3Mgd3Jvbmcg dG8gbWUuIFBsZWFzZSBjaGFuZ2UgdGhlIGFib3ZlIGludG8gYW4gYXJyYXkgb2YgZGF0YSBzdHJ1 Y3R1cmVzDQo+ID4gaW5zdGVhZCBvZiBhIGRhdGEgc3RydWN0dXJlIHRoYXQgaXMgZnVsbCBvZiBh cnJheXMgb2YgaWRlbnRpY2FsIHNpemUuDQo+IA0KPiBJdCBpcyBhIG1hdHRlciBmb3IgdGFzdGUu DQoNCk5vLCBpdCBpcyBub3QuIFRoZSBhYm92ZSBsYXlvdXQgbWFrZXMgaXQgaW1wb3NzaWJsZSBm b3IgdGhlIENQVSBwcmVmZXRjaGVyIHRvDQpiZSBlZmZlY3RpdmUgaWYgX1JETUFfUkVTVFJBQ0tf TUFYIHdvdWxkIGJlIGxhcmdlLiBBZGRpdGlvbmFsbHksIHRoZSBhYm92ZQ0KbGF5b3V0IG1ha2Vz IGl0IGltcG9zc2libGUgdG8gcGFzcyBhIHBvaW50ZXIgdG8gdGhlIChjbnQsIGxpc3QsIHJ3c2Vt KSB0cmlwbGV0DQpmcm9tIG9uZSBmdW5jdGlvbiB0byBhbm90aGVyLg0KDQo+ID4gPiArLyoqDQo+ ID4gPiArICogc3RydWN0IHJkbWFfcmVzdHJhY2tfZW50cnkgLSBtZXRhZGF0YSBwZXItZW50cnkN Cj4gPiA+ICsgKi8NCj4gPiA+ICtzdHJ1Y3QgcmRtYV9yZXN0cmFja19lbnRyeSB7DQo+ID4gPiAr CS8qKg0KPiA+ID4gKwkgKiBAbGlzdDogbGlua2VkIGxpc3QgYmV0d2VlbiBlbnRyaWVzDQo+ID4g PiArCSAqLw0KPiA+ID4gKwlzdHJ1Y3QgbGlzdF9oZWFkCWxpc3Q7DQo+ID4gPiArCS8qKg0KPiA+ ID4gKwkgKiBAdmFsaWQ6IHZhbGlkaXR5IGluZGljYXRvcg0KPiA+ID4gKwkgKg0KPiA+ID4gKwkg KiBUaGUgZW50cmllcyBhcmUgZmlsbGVkIGR1cmluZyByZG1hX3Jlc3RyYWNrX2FkZCwNCj4gPiA+ ICsJICogY2FuIGJlIGF0dGVtcHRlZCB0byBiZSBmcmVlIGR1cmluZyByZG1hX3Jlc3RyYWNrX2Rl bC4NCj4gPiA+ICsJICoNCj4gPiA+ICsJICogQXMgYW4gZXhhbXBsZSBmb3IgdGhhdCwgc2VlIG1s eDUgUVBzIHdpdGggdHlwZSBNTFg1X0lCX1FQVF9IV19HU0kNCj4gPiA+ICsJICovDQo+ID4gPiAr CWJvb2wJCQl2YWxpZDsNCj4gPiA+ICsJLyoqDQo+ID4gPiArCSAqIEBzcmN1OiBzbGVlcGFibGUg UkNVIHRvIHByb3RlY3Qgb2JqZWN0IGRhdGEuDQo+ID4gPiArCSAqLw0KPiA+ID4gKwlzdHJ1Y3Qg c3JjdV9zdHJ1Y3QJc3JjdTsNCj4gPiA+ICsJLyoqDQo+ID4gPiArCSAqIEB0YXNrOiBvd25lciBv ZiByZXNvdXJjZSB0cmFja2luZyBlbnRpdHkNCj4gPiA+ICsJICoNCj4gPiA+ICsJICogVGhlcmUg YXJlIHR3byB0eXBlcyBvZiBlbnRpdGllczogY3JlYXRlZCBieSB1c2VyIGFuZCBjcmVhdGVkDQo+ ID4gPiArCSAqIGJ5IGtlcm5lbC4NCj4gPiA+ICsJICoNCj4gPiA+ICsJICogVGhpcyBpcyByZWxl dmFudCBmb3IgdGhlIGVudGl0aWVzIGNyZWF0ZWQgYnkgdXNlcnMuDQo+ID4gPiArCSAqIEZvciB0 aGUgZW50aWVpZXMgY3JlYXRlZCBieSBrZXJuZWwsIHRoaXMgcG9pbnRlciB3aWxsIGJlIE5VTEwu DQo+ID4gPiArCSAqLw0KPiA+ID4gKwlzdHJ1Y3QgdGFza19zdHJ1Y3QJKnRhc2s7DQo+ID4gPiAr CS8qKg0KPiA+ID4gKwkgKiBAa2Vybl9uYW1lOiBuYW1lIG9mIG93bmVyIGZvciB0aGUga2VybmVs IGNyZWF0ZWQgZW50aXRpZXMuDQo+ID4gPiArCSAqLw0KPiA+ID4gKwljb25zdCBjaGFyCQkqa2Vy bl9uYW1lOw0KPiA+ID4gK307DQo+ID4gDQo+ID4gQWdhaW4sIHBsZWFzZSB1c2UgdGhlIGtlcm5l bC1kb2Mgc3ludGF4IHRvIGRvY3VtZW50IHN0cnVjdHVyZSBtZW1iZXJzLiBBZGRpdGlvbmFsbHks DQo+ID4gcGxlYXNlIGZpeCB0aGUgc3BlbGxpbmcgb2YgImVudGllaWVzIi4NCj4gDQo+IEl0IHdh cyBmb3JtYXR0ZWQgYWNjb3JkaW5nIHRvIGtlcm5lbC1kb2MgY2hlY2tlcjoNCj4g4p6cICBsaW51 eC1yZG1hIGdpdDoocm4vcmVzdHJhY2stdjUpIC4vc2NyaXB0cy9rZXJuZWwtZG9jIGluY2x1ZGUv cmRtYS9yZXN0cmFjay5oIHxncmVwIHdhcm5pbg0KPiBpbmNsdWRlL3JkbWEvcmVzdHJhY2suaDo1 OTogd2FybmluZzogRW51bSB2YWx1ZSAnX1JETUFfUkVTVFJBQ0tfTUFYJyBub3QgZGVzY3JpYmVk IGluIGVudW0gJ3JkbWFfcmVzdHJhY2tfb2JqJw0KDQpBZ2FpbiwgcGxlYXNlIGhhdmUgYSBsb29r IGF0IERvY3VtZW50YXRpb24va2VybmVsLWRvYy1uYW5vLUhPV1RPLnR4dC4gVGhhdA0KZG9jdW1l bnQgc2hvd3MgdGhhdCBtZW1iZXJzIG9mIGRhdGEgc3RydWN0dXJlcyBzaG91bGQgYmUgZG9jdW1l bnRlZCBhYm92ZSB0aGUNCiJzdHJ1Y3QiIGtleXdvcmQgaW5zdGVhZCBvZiBpbnNpZGUgdGhlIHN0 cnVjdHVyZSBkZWZpbml0aW9uLg0KDQpUaGFua3MsDQoNCkJhcnQu
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

#11 lore raw

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

Quoted 25 lines · Bart Van Assche
Quoted 24 lines · Leon Romanovsky
Quoted 22 lines · Bart Van Assche
Quoted 9 lines
(quoted diff) +9 -0
+/**
+ * enum rdma_restrack_obj - HW objects to track
+ */
+enum rdma_restrack_obj {
+	/**
+	 * @RDMA_RESTRACK_PD: Protection domain (PD)
+	 */
+	RDMA_RESTRACK_PD,
+	/**

Review

Quoted 10 lines
(quoted diff) +10 -0
+	 * @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.

Quoted 7 lines
Quoted 2 lines

I used kerne-doc and _RDMA_RESTRACK_MAX was an exception, it is not supposed to be used outside of restrack code.

Hello Leon,

Please have a look at the following example from Documentation/kernel-doc-nano-HOWTO.txt:

Cite from that document:
1 NOTE: this document is outdated and will eventually be removed. See 2 Documentation/doc-guide/ for current information.

I followed guidelines from Documentation/doc-guide/kernel-doc.rst.

Quoted 46 lines

kernel-doc for structs, unions, enums, and typedefs
---------------------------------------------------

[ ... ]

/**
* struct my_struct - short description
* @a: first member
* @b: second member
*
* Longer description
*/

struct my_struct {
    int a;
    int b;
/* private: internal use only */
    int c;
};
Quoted 27 lines
Quoted 26 lines
Quoted 13 lines
(quoted diff) +13 -0
+/**
+ * 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.
+	 */

Review

Quoted 10 lines
(quoted diff) +10 -0
+	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.

Quoted 6 lines
Quoted 1 line

It is a matter for taste.

No, it is not. The above layout makes it impossible for the CPU prefetcher to be effective if _RDMA_RESTRACK_MAX would be large. Additionally, the above layout makes it impossible to pass a pointer to the (cnt, list, rwsem) triplet from one function to another.

Thanks for the explanation, I'm changing it now.

Quoted 41 lines
Quoted 41 lines
Quoted 40 lines
Quoted 27 lines
(quoted diff) +26 -0
+/**
+ * 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.

Review

Quoted 10 lines
(quoted diff) +10 -0
+	 *
+	 * 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".

Quoted 7 lines
Quoted 3 lines

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'

Again, please have a look at Documentation/kernel-doc-nano-HOWTO.txt. That document shows that members of data structures should be documented above the "struct" keyword instead of inside the structure definition.

Please read Documentation/doc-guide/kernel-doc.rst - "In-line member documentation comments" section.

Regarding other comments, I'm working to address them in new patchset.

Thanks for the review.

Quoted 4 lines

Thanks,

Bart.

A message that is not in the archive (sent off-list or not yet received): 20180118052020.GS13639-U/[email protected]

#13 lore raw

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

T24gVGh1LCAyMDE4LTAxLTE4IGF0IDA3OjIwICswMjAwLCBMZW9uIFJvbWFub3Zza3kgd3JvdGU6 DQo+IFBsZWFzZSByZWFkIERvY3VtZW50YXRpb24vZG9jLWd1aWRlL2tlcm5lbC1kb2MucnN0IC0g IkluLWxpbmUgbWVtYmVyDQo+IGRvY3VtZW50YXRpb24gY29tbWVudHMiIHNlY3Rpb24uDQoNCkEg cXVvdGUgZnJvbSB0aGF0IGRvY3VtZW50Og0KDQo8cXVvdGU+DQpTdHJ1Y3R1cmUsIHVuaW9uLCBh bmQgZW51bWVyYXRpb24gZG9jdW1lbnRhdGlvbg0KLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t LS0tLS0tLS0tLS0tLS0tLS0tLS0NCg0KVGhlIGdlbmVyYWwgZm9ybWF0IG9mIGEgc3RydWN0LCB1 bmlvbiwgYW5kIGVudW0ga2VybmVsLWRvYyBjb21tZW50IGlzOjoNCg0KICAvKioNCiAgICogc3Ry dWN0IHN0cnVjdF9uYW1lIC0gQnJpZWYgZGVzY3JpcHRpb24uDQogICAqIEBtZW1iZXJfbmFtZTog RGVzY3JpcHRpb24gb2YgbWVtYmVyIG1lbWJlcl9uYW1lLg0KICAgKg0KICAgKiBEZXNjcmlwdGlv biBvZiB0aGUgc3RydWN0dXJlLg0KICAgKi8NCjwvcXVvdGU+DQoNClNpbmNlIHRoZSBhYm92ZSBz eW50YXggaXMgc2hvd24gZmlyc3QgSSB0aGluayBpdCdzIHRoZSBwcmVmZXJyZWQgc3ludGF4Lg0K DQpCYXJ0Lg==
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

#15 lore raw

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

Quoted 20 lines · Bart Van Assche
Quoted 2 lines · Leon Romanovsky

Please read Documentation/doc-guide/kernel-doc.rst - "In-line member documentation comments" section.

A quote from that document:

<quote>
Structure, union, and enumeration documentation
-----------------------------------------------

The general format of a struct, union, and enum kernel-doc comment is::

/**
* struct struct_name - Brief description.
* @member_name: Description of member member_name. *
* Description of the structure.
*/
</quote>

Quoted 1 line

Since the above syntax is shown first I think it's the preferred syntax.

I like to have documentation in-place near their actual usage/declaration, so as long as one of the options won't be marked as mandatory, I'll leave my implementation.

Thanks

Quoted 2 lines

Bart.

#16 lore raw

PATCH v4 2/7 RDMA/core: Add helper function to create named QPs

In reply to a message not in the archive

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

The QPs in the RDMA stack can be created by kernel or users space, but the owner is not visible to users after that.

The added helper keeps track of newly created QP together with the name of the owner. In case of kernel, the caller to create_qp is supposed to update QP's attribute with the name. For user space callers no change is needed and the name will be taken from the process name.

This helper sets qp->device field for all QP types including XRC_TGT, which RDMA/core didn't do before.

---
 drivers/infiniband/core/core_priv.h | 19 +++++++++++++++++++
 include/rdma/ib_verbs.h             |  6 ++++++
 2 files changed, 25 insertions(+)
drivers/infiniband/core/core_priv.h +19 -0
index 2b1372da708a..3f1442bd5fdc 100644
@@ -301,4 +301,23 @@ struct ib_device *ib_device_get_by_index(u32 ifindex);
 /* RDMA device netlink */
 void nldev_init(void);
 void nldev_exit(void);
+
+static inline struct ib_qp *_ib_create_qp(struct ib_device *dev,
+					  struct ib_pd *pd,
+					  struct ib_qp_init_attr *attr,
+					  struct ib_udata *udata)
+{
+	struct ib_qp *qp;
+
+	qp = dev->create_qp(pd, attr, udata);
+	if (!IS_ERR(qp)) {
+		qp->device = dev;
+		if (attr->qp_type < IB_QPT_MAX)
+			rdma_restrack_add(&qp->res,
+					  RDMA_RESTRACK_QP,
+					  attr->comm);
+	}
+
+	return qp;
+}
 #endif /* _CORE_PRIV_H */
include/rdma/ib_verbs.h +6 -0
index 271265cd7c1a..41d4f59cac5e 100644
@@ -1141,6 +1141,12 @@ struct ib_qp_init_attr {
 	u8			port_num;
 	struct ib_rwq_ind_table *rwq_ind_tbl;
 	u32			source_qpn;
+
+	/*
+	 * Name of entity which created this QP, empty string means that
+	 * it will be taken automatically from task_struct.
+	 */
+	char comm[TASK_COMM_LEN];
 };
 
 struct ib_qp_open_attr {
Signature · 6 lines
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20180115151255.30167-3-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org

#18 lore raw

Re: [PATCH rdma-next v4 2/7] RDMA/core: Add helper function to create named QPs

T24gTW9uLCAyMDE4LTAxLTE1IGF0IDE3OjEyICswMjAwLCBMZW9uIFJvbWFub3Zza3kgd3JvdGU6 DQo+IC0tLSBhL2luY2x1ZGUvcmRtYS9pYl92ZXJicy5oDQo+ICsrKyBiL2luY2x1ZGUvcmRtYS9p Yl92ZXJicy5oDQo+IEBAIC0xMTQxLDYgKzExNDEsMTIgQEAgc3RydWN0IGliX3FwX2luaXRfYXR0 ciB7DQo+ICAJdTgJCQlwb3J0X251bTsNCj4gIAlzdHJ1Y3QgaWJfcndxX2luZF90YWJsZSAqcndx X2luZF90Ymw7DQo+ICAJdTMyCQkJc291cmNlX3FwbjsNCj4gKw0KPiArCS8qDQo+ICsJICogTmFt ZSBvZiBlbnRpdHkgd2hpY2ggY3JlYXRlZCB0aGlzIFFQLCBlbXB0eSBzdHJpbmcgbWVhbnMgdGhh dA0KPiArCSAqIGl0IHdpbGwgYmUgdGFrZW4gYXV0b21hdGljYWxseSBmcm9tIHRhc2tfc3RydWN0 Lg0KPiArCSAqLw0KPiArCWNoYXIgY29tbVtUQVNLX0NPTU1fTEVOXTsNCj4gIH07DQoNCldoeSBp cyBjb21tW10gYW4gYXJyYXk/IEZvciBxdWV1ZSBwYWlycyBjcmVhdGVkIGZyb20gdXNlciBzcGFj ZSwgYXJlIHRoZXJlIGFueQ0KcXVldWUgcGFpcnMgdGhhdCBjYW4gbGl2ZSBsb25nZXIgdGhhbiB0 aGUgdGFzayB0aGF0IGNyZWF0ZWQgdGhlbT8gSWYgbm90LCBkb2VzDQp0aGF0IG1lYW4gdGhhdCBp dCBpcyBzYWZlIHRvIGNoYW5nZSBjb21tIGludG8gYSBjb25zdCBjaGFyIHBvaW50ZXI/DQoNCkFk ZGl0aW9uYWxseSwgcGxlYXNlIHVzZSB0aGUga2VybmVsLWRvYyBzeW50YXggdG8gZG9jdW1lbnQg c3RydWN0dXJlIG1lbWJlcnMuDQoNClRoYW5rcywNCg0KQmFydC4=
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

#20 lore raw

Re: [PATCH rdma-next v4 2/7] RDMA/core: Add helper function to create named QPs

Quoted 10 lines · Bart Van Assche
Quoted 9 lines · Leon Romanovsky
include/rdma/ib_verbs.h +3 -0
@@ -1141,6 +1141,12 @@ struct ib_qp_init_attr {
 	u8			port_num;
 	struct ib_rwq_ind_table *rwq_ind_tbl;
 	u32			source_qpn;
+
+	/*
+	 * Name of entity which created this QP, empty string means that
Quoted 8 lines

include/rdma/ib_verbs.h struct ib_qp_init_attr { · line 1150

Quoted 4 lines
include/rdma/ib_verbs.h +3 -0
+	 * it will be taken automatically from task_struct.
+	 */
+	char comm[TASK_COMM_LEN];
 };

Why is comm[] an array? For queue pairs created from user space, are there any queue pairs that can live longer than the task that created them? If not, does that mean that it is safe to change comm into a const char pointer?

As I wrote in cover letter, this code will be refactored and moved to PD once Steve's implementation of CMA resource tracking will be accepted in upstream.

Quoted 2 lines

Additionally, please use the kernel-doc syntax to document structure members.

Kernel-doc checker doesn't work with patches and ib_verbs.h full of kernel-doc warning.

Quoted 4 lines

Thanks,

Bart.

A message that is not in the archive (sent off-list or not yet received): 20180117055942.GF13639-U/[email protected]

#22 lore raw

Re: [PATCH rdma-next v4 2/7] RDMA/core: Add helper function to create named QPs

T24gV2VkLCAyMDE4LTAxLTE3IGF0IDA3OjU5ICswMjAwLCBMZW9uIFJvbWFub3Zza3kgd3JvdGU6 DQo+IE9uIFR1ZSwgSmFuIDE2LCAyMDE4IGF0IDA5OjM1OjQyUE0gKzAwMDAsIEJhcnQgVmFuIEFz c2NoZSB3cm90ZToNCj4gPiBBZGRpdGlvbmFsbHksIHBsZWFzZSB1c2UgdGhlIGtlcm5lbC1kb2Mg c3ludGF4IHRvIGRvY3VtZW50IHN0cnVjdHVyZSBtZW1iZXJzLg0KPiANCj4gS2VybmVsLWRvYyBj aGVja2VyIGRvZXNuJ3Qgd29yayB3aXRoIHBhdGNoZXMgYW5kIGliX3ZlcmJzLmggZnVsbCBvZg0K PiBrZXJuZWwtZG9jIHdhcm5pbmcuDQoNCkkgY2FuIHJldmlldyB0aGUga2VybmVsLWRvYyBzeW50 YXggZm9yIHlvdSBpZiB0aGUga2VybmVsLWRvYyBjaGVja2luZyBzY3JpcHQNCnRyaWdnZXJzIHRv byBtYW55IHdhcm5pbmdzLg0KDQpCYXJ0Lg==
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#23 lore raw

PATCH v4 3/7 RDMA: Annotate create QP callers

In reply to a message not in the archive

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

Update all callers to provide owner name through QP attribute structure and connect create_qp with helper which supports resource tracking.

---
 drivers/infiniband/core/cma.c              | 1 +
 drivers/infiniband/core/mad.c              | 1 +
 drivers/infiniband/core/uverbs_cmd.c       | 3 +--
 drivers/infiniband/core/verbs.c            | 4 ++--
 drivers/infiniband/hw/mlx4/mad.c           | 1 +
 drivers/infiniband/hw/mlx4/qp.c            | 1 +
 drivers/infiniband/hw/mlx5/gsi.c           | 2 ++
 drivers/infiniband/ulp/ipoib/ipoib_cm.c    | 4 +++-
 drivers/infiniband/ulp/ipoib/ipoib_verbs.c | 1 +
 drivers/infiniband/ulp/srp/ib_srp.c        | 1 +
 drivers/infiniband/ulp/srpt/ib_srpt.c      | 1 +
 net/smc/smc_ib.c                           | 1 +
 12 files changed, 16 insertions(+), 5 deletions(-)
drivers/infiniband/core/cma.c +1 -0
index 30d1c32a816f..3810716ea65e 100644
@@ -858,6 +858,7 @@ int rdma_create_qp(struct rdma_cm_id *id, struct ib_pd *pd,
 		return -EINVAL;
 
 	qp_init_attr->port_num = id->port_num;
+	strncpy(qp_init_attr->comm, "rdma-cm", TASK_COMM_LEN);
 	qp = ib_create_qp(pd, qp_init_attr);
 	if (IS_ERR(qp))
 		return PTR_ERR(qp);
drivers/infiniband/core/mad.c +1 -0
index c50596f7f98a..f73551fc5a02 100644
@@ -3103,6 +3103,7 @@ static int create_mad_qp(struct ib_mad_qp_info *qp_info,
 	qp_init_attr.port_num = qp_info->port_priv->port_num;
 	qp_init_attr.qp_context = qp_info;
 	qp_init_attr.event_handler = qp_event_handler;
+	strncpy(qp_init_attr.comm, "rdma-mad", TASK_COMM_LEN);
 	qp_info->qp = ib_create_qp(qp_info->port_priv->pd, &qp_init_attr);
 	if (IS_ERR(qp_info->qp)) {
 		dev_err(&qp_info->port_priv->device->dev,
drivers/infiniband/core/uverbs_cmd.c +1 -2
index c216d98bb816..62b3c5d71cce 100644
@@ -1514,7 +1514,7 @@ static int create_qp(struct ib_uverbs_file *file,
 	if (cmd->qp_type == IB_QPT_XRC_TGT)
 		qp = ib_create_qp(pd, &attr);
 	else
-		qp = device->create_qp(pd, &attr, uhw);
+		qp = _ib_create_qp(device, pd, &attr, uhw);
 
 	if (IS_ERR(qp)) {
 		ret = PTR_ERR(qp);
@@ -1527,7 +1527,6 @@ static int create_qp(struct ib_uverbs_file *file,
 			goto err_cb;
 
 		qp->real_qp	  = qp;
-		qp->device	  = device;
 		qp->pd		  = pd;
 		qp->send_cq	  = attr.send_cq;
 		qp->recv_cq	  = attr.recv_cq;
drivers/infiniband/core/verbs.c +2 -2
index 15dd26cab5d8..8522639cd315 100644
@@ -882,7 +882,7 @@ struct ib_qp *ib_create_qp(struct ib_pd *pd,
 	if (qp_init_attr->cap.max_rdma_ctxs)
 		rdma_rw_init_qp(device, qp_init_attr);
 
-	qp = device->create_qp(pd, qp_init_attr, NULL);
+	qp = _ib_create_qp(device, pd, qp_init_attr, NULL);
 	if (IS_ERR(qp))
 		return qp;
 
@@ -892,7 +892,6 @@ struct ib_qp *ib_create_qp(struct ib_pd *pd,
 		return ERR_PTR(ret);
 	}
 
-	qp->device     = device;
 	qp->real_qp    = qp;
 	qp->uobject    = NULL;
 	qp->qp_type    = qp_init_attr->qp_type;
@@ -1520,6 +1519,7 @@ int ib_destroy_qp(struct ib_qp *qp)
 	if (!qp->uobject)
 		rdma_rw_cleanup_mrs(qp);
 
+	rdma_restrack_del(&qp->res, RDMA_RESTRACK_QP);
 	ret = qp->device->destroy_qp(qp);
 	if (!ret) {
 		if (pd)
drivers/infiniband/hw/mlx4/mad.c +1 -0
index 0793a21d76f4..f816df420fb9 100644
@@ -1834,6 +1834,7 @@ static int create_pv_sqp(struct mlx4_ib_demux_pv_ctx *ctx,
 	qp_init_attr.init_attr.port_num = ctx->port;
 	qp_init_attr.init_attr.qp_context = ctx;
 	qp_init_attr.init_attr.event_handler = pv_qp_event_handler;
+	strncpy(qp_init_attr.init_attr.comm, "mlx4-sriov", TASK_COMM_LEN);
 	tun_qp->qp = ib_create_qp(ctx->pd, &qp_init_attr.init_attr);
 	if (IS_ERR(tun_qp->qp)) {
 		ret = PTR_ERR(tun_qp->qp);
drivers/infiniband/hw/mlx4/qp.c +1 -0
index f045491f2c14..ebdfac3eb673 100644
@@ -1689,6 +1689,7 @@ struct ib_qp *mlx4_ib_create_qp(struct ib_pd *pd,
 		if (is_eth &&
 		    dev->dev->caps.flags2 & MLX4_DEV_CAP_FLAG2_ROCE_V1_V2) {
 			init_attr->create_flags |= MLX4_IB_QP_CREATE_ROCE_V2_GSI;
+			strncpy(init_attr->comm, "mlx4-gsi", TASK_COMM_LEN);
 			sqp->roce_v2_gsi = ib_create_qp(pd, init_attr);
 
 			if (IS_ERR(sqp->roce_v2_gsi)) {
drivers/infiniband/hw/mlx5/gsi.c +2 -0
index 79e6309460dc..b1b177d1a0dd 100644
@@ -184,6 +184,7 @@ struct ib_qp *mlx5_ib_gsi_create_qp(struct ib_pd *pd,
 		hw_init_attr.cap.max_send_sge = 0;
 		hw_init_attr.cap.max_inline_data = 0;
 	}
+	strncpy(hw_init_attr.comm, "mlx5-gsi", TASK_COMM_LEN);
 	gsi->rx_qp = ib_create_qp(pd, &hw_init_attr);
 	if (IS_ERR(gsi->rx_qp)) {
 		mlx5_ib_warn(dev, "unable to create hardware GSI QP. error %ld\n",
@@ -264,6 +265,7 @@ static struct ib_qp *create_gsi_ud_qp(struct mlx5_ib_gsi_qp *gsi)
 		.sq_sig_type = gsi->sq_sig_type,
 		.qp_type = IB_QPT_UD,
 		.create_flags = mlx5_ib_create_qp_sqpn_qp1(),
+		.comm = "mlx5-gsi",
 	};
 
 	return ib_create_qp(pd, &init_attr);
drivers/infiniband/ulp/ipoib/ipoib_cm.c +3 -1
index dfbb8fdda5f6..d165b3a25340 100644
@@ -263,6 +263,7 @@ static struct ib_qp *ipoib_cm_create_rx_qp(struct net_device *dev,
 		.sq_sig_type = IB_SIGNAL_ALL_WR,
 		.qp_type = IB_QPT_RC,
 		.qp_context = p,
+		.comm = "ipoib-cm",
 	};
 
 	if (!ipoib_cm_has_srq(dev)) {
@@ -1062,7 +1063,8 @@ static struct ib_qp *ipoib_cm_create_tx_qp(struct net_device *dev, struct ipoib_
 		.sq_sig_type		= IB_SIGNAL_ALL_WR,
 		.qp_type		= IB_QPT_RC,
 		.qp_context		= tx,
-		.create_flags		= 0
+		.create_flags		= 0,
+		.comm			= "ipoib-cm",
 	};
 	struct ib_qp *tx_qp;
 
drivers/infiniband/ulp/ipoib/ipoib_verbs.c +1 -0
index 984a88096f39..d41d85f8ac7e 100644
@@ -206,6 +206,7 @@ int ipoib_transport_dev_init(struct net_device *dev, struct ib_device *ca)
 	if (priv->hca_caps & IB_DEVICE_MANAGED_FLOW_STEERING)
 		init_attr.create_flags |= IB_QP_CREATE_NETIF_QP;
 
+	strncpy(init_attr.comm, "ipoib-verbs", TASK_COMM_LEN);
 	priv->qp = ib_create_qp(priv->pd, &init_attr);
 	if (IS_ERR(priv->qp)) {
 		pr_warn("%s: failed to create QP\n", ca->name);
drivers/infiniband/ulp/srp/ib_srp.c +1 -0
index 62d88212c1b0..fb8be6b2ddfb 100644
@@ -521,6 +521,7 @@ static int srp_create_ch_ib(struct srp_rdma_ch *ch)
 	init_attr->send_cq             = send_cq;
 	init_attr->recv_cq             = recv_cq;
 
+	strncpy(init_attr->comm, "srp", TASK_COMM_LEN);
 	qp = ib_create_qp(dev->pd, init_attr);
 	if (IS_ERR(qp)) {
 		ret = PTR_ERR(qp);
drivers/infiniband/ulp/srpt/ib_srpt.c +1 -0
index d78f60dcc2ba..3724132a266e 100644
@@ -1726,6 +1726,7 @@ static int srpt_create_ch_ib(struct srpt_rdma_ch *ch)
 		qp_init->cap.max_recv_sge = qp_init->cap.max_send_sge;
 	}
 
+	strncpy(qp_init->comm, "srpt", TASK_COMM_LEN);
 	ch->qp = ib_create_qp(sdev->pd, qp_init);
 	if (IS_ERR(ch->qp)) {
 		ret = PTR_ERR(ch->qp);
net/smc/smc_ib.c +1 -0
index 90f1a7f9085c..9f5bca333cce 100644
@@ -243,6 +243,7 @@ int smc_ib_create_queue_pair(struct smc_link *lnk)
 		},
 		.sq_sig_type = IB_SIGNAL_REQ_WR,
 		.qp_type = IB_QPT_RC,
+		.comm = "sec-ib",
 	};
 	int rc;
 
Signature · 6 lines
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20180115151255.30167-4-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org

#25 lore raw

RE: [PATCH rdma-next v4 3/7] RDMA: Annotate create QP callers

Quoted 30 lines

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

Update all callers to provide owner name through QP attribute structure and connect create_qp with helper which supports resource tracking.

---
 drivers/infiniband/core/cma.c              | 1 +
 drivers/infiniband/core/mad.c              | 1 +
 drivers/infiniband/core/uverbs_cmd.c       | 3 +--
 drivers/infiniband/core/verbs.c            | 4 ++--
 drivers/infiniband/hw/mlx4/mad.c           | 1 +
 drivers/infiniband/hw/mlx4/qp.c            | 1 +
 drivers/infiniband/hw/mlx5/gsi.c           | 2 ++
 drivers/infiniband/ulp/ipoib/ipoib_cm.c    | 4 +++-
 drivers/infiniband/ulp/ipoib/ipoib_verbs.c | 1 +
 drivers/infiniband/ulp/srp/ib_srp.c        | 1 +
 drivers/infiniband/ulp/srpt/ib_srpt.c      | 1 +
 net/smc/smc_ib.c                           | 1 +
 12 files changed, 16 insertions(+), 5 deletions(-)
drivers/infiniband/core/cma.c +0 -0
index 30d1c32a816f..3810716ea65e 100644

drivers/infiniband/core/cma.c int rdma_create_qp(struct rdma_cm_id *id, struct · line 862

Quoted 5 lines
drivers/infiniband/core/cma.c +1 -0
@@ -858,6 +858,7 @@ int rdma_create_qp(struct rdma_cm_id *id, struct
 ib_pd *pd,
 		return -EINVAL;
 
 	qp_init_attr->port_num = id->port_num;
+	strncpy(qp_init_attr->comm, "rdma-cm", TASK_COMM_LEN);

I think the above strncpy should be done only if the caller of rdma_create_qp() didn't fill in qp_init_attr->comm. IE if the caller ULP fills it out, we want to see it. Otherwise all kernel ULPs end up being "rdma-cm"...

Stevo

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#26 lore raw

Re: [PATCH rdma-next v4 3/7] RDMA: Annotate create QP callers

Quoted 33 lines · Steve Wise
Quoted 33 lines

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

Update all callers to provide owner name through QP attribute structure and connect create_qp with helper which supports resource tracking.

---
 drivers/infiniband/core/cma.c              | 1 +
 drivers/infiniband/core/mad.c              | 1 +
 drivers/infiniband/core/uverbs_cmd.c       | 3 +--
 drivers/infiniband/core/verbs.c            | 4 ++--
 drivers/infiniband/hw/mlx4/mad.c           | 1 +
 drivers/infiniband/hw/mlx4/qp.c            | 1 +
 drivers/infiniband/hw/mlx5/gsi.c           | 2 ++
 drivers/infiniband/ulp/ipoib/ipoib_cm.c    | 4 +++-
 drivers/infiniband/ulp/ipoib/ipoib_verbs.c | 1 +
 drivers/infiniband/ulp/srp/ib_srp.c        | 1 +
 drivers/infiniband/ulp/srpt/ib_srpt.c      | 1 +
 net/smc/smc_ib.c                           | 1 +
 12 files changed, 16 insertions(+), 5 deletions(-)
drivers/infiniband/core/cma.c +0 -0
index 30d1c32a816f..3810716ea65e 100644
@@ -858,6 +858,7 @@ int rdma_create_qp(struct rdma_cm_id *id, struct
 ib_pd *pd,
 		return -EINVAL;
 
Quoted 8 lines

drivers/infiniband/core/cma.c int rdma_create_qp(struct rdma_cm_id *id, struct ib_pd *pd, · line 861

Quoted 2 lines
drivers/infiniband/core/cma.c +1 -0
 	qp_init_attr->port_num = id->port_num;
+	strncpy(qp_init_attr->comm, "rdma-cm", TASK_COMM_LEN);

I think the above strncpy should be done only if the caller of rdma_create_qp() didn't fill in qp_init_attr->comm. IE if the caller ULP fills it out, we want to see it. Otherwise all kernel ULPs end up being "rdma-cm"...

Thanks Steve,

This patch won't be needed in next revision, I found a way to use compiler to generate the kernel name.

Quoted 3 lines

Stevo

#27 lore raw

PATCH v4 4/7 RDMA/core: Add resource tracking for create and destroy CQs

In reply to a message not in the archive

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

Track all creation and destroy of CQ objects.

---
 drivers/infiniband/core/cq.c               | 3 +++
 drivers/infiniband/core/uverbs_cmd.c       | 1 +
 drivers/infiniband/core/uverbs_std_types.c | 2 ++
 drivers/infiniband/core/verbs.c            | 2 ++
 4 files changed, 8 insertions(+)
drivers/infiniband/core/cq.c +3 -0
index f2ae75fa3128..6344e007dbce 100644
@@ -154,6 +154,8 @@ struct ib_cq *ib_alloc_cq(struct ib_device *dev, void *private,
 	if (!cq->wc)
 		goto out_destroy_cq;
 
+	rdma_restrack_add(&cq->res, RDMA_RESTRACK_CQ, NULL);
+
 	switch (cq->poll_ctx) {
 	case IB_POLL_DIRECT:
 		cq->comp_handler = ib_cq_completion_direct;
@@ -208,6 +210,7 @@ void ib_free_cq(struct ib_cq *cq)
 		WARN_ON_ONCE(1);
 	}
 
+	rdma_restrack_del(&cq->res, RDMA_RESTRACK_CQ);
 	kfree(cq->wc);
 	ret = cq->device->destroy_cq(cq);
 	WARN_ON_ONCE(ret);
drivers/infiniband/core/uverbs_cmd.c +1 -0
index 62b3c5d71cce..c6ac0ee57f7c 100644
@@ -1033,6 +1033,7 @@ static struct ib_ucq_object *create_cq(struct ib_uverbs_file *file,
 		goto err_cb;
 
 	uobj_alloc_commit(&obj->uobject);
+	rdma_restrack_add(&cq->res, RDMA_RESTRACK_CQ, NULL);
 
 	return obj;
 
drivers/infiniband/core/uverbs_std_types.c +2 -0
index c3ee5d9b336d..2d0d27865b09 100644
@@ -35,6 +35,7 @@
 #include <rdma/ib_verbs.h>
 #include <linux/bug.h>
 #include <linux/file.h>
+#include <rdma/restrack.h>
 #include "rdma_core.h"
 #include "uverbs.h"
 
@@ -319,6 +320,7 @@ static int uverbs_create_cq_handler(struct ib_device *ib_dev,
 	obj->uobject.object = cq;
 	obj->uobject.user_handle = user_handle;
 	atomic_set(&cq->usecnt, 0);
+	rdma_restrack_add(&cq->res, RDMA_RESTRACK_CQ, NULL);
 
 	ret = uverbs_copy_to(attrs, CREATE_CQ_RESP_CQE, &cq->cqe);
 	if (ret)
drivers/infiniband/core/verbs.c +2 -0
index 8522639cd315..b0c4080eaf6e 100644
@@ -1562,6 +1562,7 @@ struct ib_cq *ib_create_cq(struct ib_device *device,
 		cq->event_handler = event_handler;
 		cq->cq_context    = cq_context;
 		atomic_set(&cq->usecnt, 0);
+		rdma_restrack_add(&cq->res, RDMA_RESTRACK_CQ, NULL);
 	}
 
 	return cq;
@@ -1580,6 +1581,7 @@ int ib_destroy_cq(struct ib_cq *cq)
 	if (atomic_read(&cq->usecnt))
 		return -EBUSY;
 
+	rdma_restrack_del(&cq->res, RDMA_RESTRACK_CQ);
 	return cq->device->destroy_cq(cq);
 }
 EXPORT_SYMBOL(ib_destroy_cq);
Signature · 6 lines
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#28 lore raw

PATCH v4 5/7 RDMA/core: Add resource tracking for create and destroy PDs

In reply to a message not in the archive

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

Track all creation and destroy of PD objects.

---
 drivers/infiniband/core/uverbs_cmd.c | 1 +
 drivers/infiniband/core/verbs.c      | 2 ++
 2 files changed, 3 insertions(+)
drivers/infiniband/core/uverbs_cmd.c +1 -0
index c6ac0ee57f7c..2cf3e20e5152 100644
@@ -340,6 +340,7 @@ ssize_t ib_uverbs_alloc_pd(struct ib_uverbs_file *file,
 	uobj->object = pd;
 	memset(&resp, 0, sizeof resp);
 	resp.pd_handle = uobj->id;
+	rdma_restrack_add(&pd->res, RDMA_RESTRACK_PD, NULL);
 
 	if (copy_to_user(u64_to_user_ptr(cmd.response), &resp, sizeof resp)) {
 		ret = -EFAULT;
drivers/infiniband/core/verbs.c +2 -0
index b0c4080eaf6e..e032b14c4201 100644
@@ -262,6 +262,7 @@ struct ib_pd *__ib_alloc_pd(struct ib_device *device, unsigned int flags,
 		pr_warn("%s: enabling unsafe global rkey\n", caller);
 		mr_access_flags |= IB_ACCESS_REMOTE_READ | IB_ACCESS_REMOTE_WRITE;
 	}
+	rdma_restrack_add(&pd->res, RDMA_RESTRACK_PD, NULL);
 
 	if (mr_access_flags) {
 		struct ib_mr *mr;
@@ -312,6 +313,7 @@ void ib_dealloc_pd(struct ib_pd *pd)
 	   requires the caller to guarantee we can't race here. */
 	WARN_ON(atomic_read(&pd->usecnt));
 
+	rdma_restrack_del(&pd->res, RDMA_RESTRACK_PD);
 	/* Making delalloc_pd a void return is a WIP, no driver should return
 	   an error here. */
 	ret = pd->device->dealloc_pd(pd);
Signature · 6 lines
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#29 lore raw

PATCH v4 6/7 RDMA/nldev: Provide global resource utilization

In reply to a message not in the archive

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

Export through netlink interface, the global device utilization for the rdmatool as the main user of RDMA nldev interface.

Provide both dumpit and doit callbacks.

As an example of possible output from rdmatool for system with 5 Mellanox's card

$ rdma res
1: mlx5_0: qp 4 cq 5 pd 3
2: mlx5_1: qp 4 cq 5 pd 3
3: mlx5_2: qp 4 cq 5 pd 3
4: mlx5_3: qp 2 cq 3 pd 2
5: mlx5_4: qp 4 cq 5 pd 3
---
 drivers/infiniband/core/nldev.c  | 154 +++++++++++++++++++++++++++++++++++++++
 include/uapi/rdma/rdma_netlink.h |  10 +++
 2 files changed, 164 insertions(+)
drivers/infiniband/core/nldev.c +154 -0
index 5d790c507c7e..1a607e940c85 100644
@@ -31,6 +31,8 @@
  */
 
 #include <linux/module.h>
+#include <linux/pid.h>
+#include <linux/pid_namespace.h>
 #include <net/netlink.h>
 #include <rdma/rdma_netlink.h>
 
@@ -52,6 +54,11 @@ static const struct nla_policy nldev_policy[RDMA_NLDEV_ATTR_MAX] = {
 	[RDMA_NLDEV_ATTR_PORT_STATE]	= { .type = NLA_U8 },
 	[RDMA_NLDEV_ATTR_PORT_PHYS_STATE] = { .type = NLA_U8 },
 	[RDMA_NLDEV_ATTR_DEV_NODE_TYPE] = { .type = NLA_U8 },
+	[RDMA_NLDEV_ATTR_RES_SUMMARY]	= { .type = NLA_NESTED },
+	[RDMA_NLDEV_ATTR_RES_SUMMARY_ENTRY]	= { .type = NLA_NESTED },
+	[RDMA_NLDEV_ATTR_RES_SUMMARY_ENTRY_NAME] = { .type = NLA_NUL_STRING,
+					     .len = 16 },
+	[RDMA_NLDEV_ATTR_RES_SUMMARY_ENTRY_CURR] = { .type = NLA_U64 },
 };
 
 static int fill_nldev_handle(struct sk_buff *msg, struct ib_device *device)
@@ -134,6 +141,68 @@ static int fill_port_info(struct sk_buff *msg,
 	return 0;
 }
 
+static int fill_res_info_entry(struct sk_buff *msg,
+			       const char *name, u64 curr)
+{
+	struct nlattr *entry_attr;
+
+	entry_attr = nla_nest_start(msg, RDMA_NLDEV_ATTR_RES_SUMMARY_ENTRY);
+	if (!entry_attr)
+		return -EMSGSIZE;
+
+	if (nla_put_string(msg, RDMA_NLDEV_ATTR_RES_SUMMARY_ENTRY_NAME, name))
+		goto err;
+	if (nla_put_u64_64bit(msg,
+			      RDMA_NLDEV_ATTR_RES_SUMMARY_ENTRY_CURR, curr, 0))
+		goto err;
+
+	nla_nest_end(msg, entry_attr);
+	return 0;
+
+err:
+	nla_nest_cancel(msg, entry_attr);
+	return -EMSGSIZE;
+}
+
+static int fill_res_info(struct sk_buff *msg, struct ib_device *device)
+{
+	static const char *names[_RDMA_RESTRACK_MAX] = {
+		[RDMA_RESTRACK_PD] = "pd",
+		[RDMA_RESTRACK_CQ] = "cq",
+		[RDMA_RESTRACK_QP] = "qp",
+	};
+
+	struct rdma_restrack_root *res = &device->res;
+	struct nlattr *table_attr;
+	int ret, i;
+
+	if (fill_nldev_handle(msg, device))
+		return -EMSGSIZE;
+
+	if (task_active_pid_ns(current) != &init_pid_ns)
+		return 0;
+
+	table_attr = nla_nest_start(msg, RDMA_NLDEV_ATTR_RES_SUMMARY);
+	if (!table_attr)
+		return -EMSGSIZE;
+
+	for (i = 0; i < _RDMA_RESTRACK_MAX; i++) {
+		if (!names[i])
+			continue;
+		ret = fill_res_info_entry(msg, names[i],
+					  rdma_restrack_count(res, i));
+		if (ret)
+			goto err;
+	}
+
+	nla_nest_end(msg, table_attr);
+	return 0;
+
+err:
+	nla_nest_cancel(msg, table_attr);
+	return ret;
+}
+
 static int nldev_get_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
 			  struct netlink_ext_ack *extack)
 {
@@ -329,6 +398,87 @@ static int nldev_port_get_dumpit(struct sk_buff *skb,
 	return skb->len;
 }
 
+static int nldev_res_get_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
+			      struct netlink_ext_ack *extack)
+{
+	struct nlattr *tb[RDMA_NLDEV_ATTR_MAX];
+	struct ib_device *device;
+	struct sk_buff *msg;
+	u32 index;
+	int ret;
+
+	ret = nlmsg_parse(nlh, 0, tb, RDMA_NLDEV_ATTR_MAX - 1,
+			  nldev_policy, extack);
+	if (ret || !tb[RDMA_NLDEV_ATTR_DEV_INDEX])
+		return -EINVAL;
+
+	index = nla_get_u32(tb[RDMA_NLDEV_ATTR_DEV_INDEX]);
+	device = ib_device_get_by_index(index);
+	if (!device)
+		return -EINVAL;
+
+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	if (!msg)
+		goto err;
+
+	nlh = nlmsg_put(msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq,
+			RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, RDMA_NLDEV_CMD_RES_GET),
+			0, 0);
+
+	ret = fill_res_info(msg, device);
+	if (ret)
+		goto err_free;
+
+	nlmsg_end(msg, nlh);
+	put_device(&device->dev);
+	return rdma_nl_unicast(msg, NETLINK_CB(skb).portid);
+
+err_free:
+	nlmsg_free(msg);
+err:
+	put_device(&device->dev);
+	return ret;
+}
+
+static int _nldev_res_get_dumpit(struct ib_device *device,
+				 struct sk_buff *skb,
+				 struct netlink_callback *cb,
+				 unsigned int idx)
+{
+	int start = cb->args[0];
+	struct nlmsghdr *nlh;
+
+	if (idx < start)
+		return 0;
+
+	nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
+			RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, RDMA_NLDEV_CMD_RES_GET),
+			0, NLM_F_MULTI);
+
+	if (fill_res_info(skb, device)) {
+		nlmsg_cancel(skb, nlh);
+		goto out;
+	}
+
+	nlmsg_end(skb, nlh);
+
+	idx++;
+
+out:
+	cb->args[0] = idx;
+	return skb->len;
+}
+
+static int nldev_res_get_dumpit(struct sk_buff *skb,
+				struct netlink_callback *cb)
+{
+	/*
+	 * There is no need to take lock, because
+	 * we are relying on ib_core's lists_rwsem
+	 */
+	return ib_enum_all_devs(_nldev_res_get_dumpit, skb, cb);
+}
+
 static const struct rdma_nl_cbs nldev_cb_table[RDMA_NLDEV_NUM_OPS] = {
 	[RDMA_NLDEV_CMD_GET] = {
 		.doit = nldev_get_doit,
@@ -338,6 +488,10 @@ static const struct rdma_nl_cbs nldev_cb_table[RDMA_NLDEV_NUM_OPS] = {
 		.doit = nldev_port_get_doit,
 		.dump = nldev_port_get_dumpit,
 	},
+	[RDMA_NLDEV_CMD_RES_GET] = {
+		.doit = nldev_res_get_doit,
+		.dump = nldev_res_get_dumpit,
+	},
 };
 
 void __init nldev_init(void)
include/uapi/rdma/rdma_netlink.h +10 -0
index cc002e316d09..e0f5cdc81541 100644
@@ -236,6 +236,11 @@ enum rdma_nldev_command {
 	RDMA_NLDEV_CMD_PORT_NEW,
 	RDMA_NLDEV_CMD_PORT_DEL,
 
+	RDMA_NLDEV_CMD_RES_GET, /* can dump */
+	RDMA_NLDEV_CMD_RES_SET,
+	RDMA_NLDEV_CMD_RES_NEW,
+	RDMA_NLDEV_CMD_RES_DEL,
+
 	RDMA_NLDEV_NUM_OPS
 };
 
@@ -303,6 +308,11 @@ enum rdma_nldev_attr {
 
 	RDMA_NLDEV_ATTR_DEV_NODE_TYPE,		/* u8 */
 
+	RDMA_NLDEV_ATTR_RES_SUMMARY,		/* nested table */
+	RDMA_NLDEV_ATTR_RES_SUMMARY_ENTRY,	/* nested table */
+	RDMA_NLDEV_ATTR_RES_SUMMARY_ENTRY_NAME,	/* string */
+	RDMA_NLDEV_ATTR_RES_SUMMARY_ENTRY_CURR,	/* u64 */
+
 	RDMA_NLDEV_ATTR_MAX
 };
 #endif /* _UAPI_RDMA_NETLINK_H */
Signature · 6 lines
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#30 lore raw

PATCH v4 7/7 RDMA/nldev: Provide detailed QP information

In reply to a message not in the archive

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

Implement RDMA nldev netlink interface to get detailed QP information.

Currently only dumpit variant is implemented.

---
 drivers/infiniband/core/nldev.c  | 237 +++++++++++++++++++++++++++++++++++++++
 include/uapi/rdma/rdma_netlink.h |  45 ++++++++
 2 files changed, 282 insertions(+)
drivers/infiniband/core/nldev.c +237 -0
index 1a607e940c85..ff32667f051e 100644
@@ -59,6 +59,18 @@ static const struct nla_policy nldev_policy[RDMA_NLDEV_ATTR_MAX] = {
 	[RDMA_NLDEV_ATTR_RES_SUMMARY_ENTRY_NAME] = { .type = NLA_NUL_STRING,
 					     .len = 16 },
 	[RDMA_NLDEV_ATTR_RES_SUMMARY_ENTRY_CURR] = { .type = NLA_U64 },
+	[RDMA_NLDEV_ATTR_RES_QP]		= { .type = NLA_NESTED },
+	[RDMA_NLDEV_ATTR_RES_QP_ENTRY]		= { .type = NLA_NESTED },
+	[RDMA_NLDEV_ATTR_RES_LQPN]		= { .type = NLA_U32 },
+	[RDMA_NLDEV_ATTR_RES_RQPN]		= { .type = NLA_U32 },
+	[RDMA_NLDEV_ATTR_RES_RQ_PSN]		= { .type = NLA_U32 },
+	[RDMA_NLDEV_ATTR_RES_SQ_PSN]		= { .type = NLA_U32 },
+	[RDMA_NLDEV_ATTR_RES_PATH_MIG_STATE] = { .type = NLA_U8 },
+	[RDMA_NLDEV_ATTR_RES_TYPE]		= { .type = NLA_U8 },
+	[RDMA_NLDEV_ATTR_RES_STATE]		= { .type = NLA_U8 },
+	[RDMA_NLDEV_ATTR_RES_PID]		= { .type = NLA_U32 },
+	[RDMA_NLDEV_ATTR_RES_KERN_NAME]		= { .type = NLA_NUL_STRING,
+						    .len = TASK_COMM_LEN },
 };
 
 static int fill_nldev_handle(struct sk_buff *msg, struct ib_device *device)
@@ -203,6 +215,78 @@ static int fill_res_info(struct sk_buff *msg, struct ib_device *device)
 	return ret;
 }
 
+static int fill_res_qp_entry(struct sk_buff *msg,
+			     struct ib_qp *qp, uint32_t port)
+{
+	struct rdma_restrack_entry *res = &qp->res;
+	struct ib_qp_init_attr qp_init_attr;
+	struct nlattr *entry_attr;
+	struct ib_qp_attr qp_attr;
+	int ret;
+
+	ret = ib_query_qp(qp, &qp_attr, 0, &qp_init_attr);
+	if (ret)
+		return ret;
+
+	if (port && port != qp_attr.port_num)
+		return 0;
+
+	entry_attr = nla_nest_start(msg, RDMA_NLDEV_ATTR_RES_QP_ENTRY);
+	if (!entry_attr)
+		goto out;
+
+	/* In create_qp() port is not set yet */
+	if (qp_attr.port_num &&
+	    nla_put_u32(msg, RDMA_NLDEV_ATTR_PORT_INDEX, qp_attr.port_num))
+		goto err;
+
+	if (nla_put_u32(msg, RDMA_NLDEV_ATTR_RES_LQPN, qp->qp_num))
+		goto err;
+	if (qp->qp_type == IB_QPT_RC || qp->qp_type == IB_QPT_UC) {
+		if (nla_put_u32(msg, RDMA_NLDEV_ATTR_RES_RQPN,
+				qp_attr.dest_qp_num))
+			goto err;
+		if (nla_put_u32(msg, RDMA_NLDEV_ATTR_RES_RQ_PSN,
+				qp_attr.rq_psn))
+			goto err;
+	}
+
+	if (nla_put_u32(msg, RDMA_NLDEV_ATTR_RES_SQ_PSN, qp_attr.sq_psn))
+		goto err;
+
+	if (qp->qp_type == IB_QPT_RC || qp->qp_type == IB_QPT_UC ||
+	    qp->qp_type == IB_QPT_XRC_INI || qp->qp_type == IB_QPT_XRC_TGT) {
+		if (nla_put_u8(msg, RDMA_NLDEV_ATTR_RES_PATH_MIG_STATE,
+			       qp_attr.path_mig_state))
+			goto err;
+	}
+	if (nla_put_u8(msg, RDMA_NLDEV_ATTR_RES_TYPE, qp->qp_type))
+		goto err;
+	if (nla_put_u8(msg, RDMA_NLDEV_ATTR_RES_STATE, qp_attr.qp_state))
+		goto err;
+
+	/*
+	 * Existence of task means that it is user QP and netlink
+	 * user is invited to go and read /proc/PID/comm to get name
+	 * of the task file and res->task_com should be NULL.
+	 */
+	if (res->task &&
+	    nla_put_u32(msg, RDMA_NLDEV_ATTR_RES_PID, task_pid_vnr(res->task)))
+		goto err;
+
+	if (res->kern_name &&
+	    nla_put_string(msg, RDMA_NLDEV_ATTR_RES_KERN_NAME, res->kern_name))
+		goto err;
+
+	nla_nest_end(msg, entry_attr);
+	return 0;
+
+err:
+	nla_nest_cancel(msg, entry_attr);
+out:
+	return -EMSGSIZE;
+}
+
 static int nldev_get_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
 			  struct netlink_ext_ack *extack)
 {
@@ -479,6 +563,146 @@ static int nldev_res_get_dumpit(struct sk_buff *skb,
 	return ib_enum_all_devs(_nldev_res_get_dumpit, skb, cb);
 }
 
+static int nldev_res_get_qp_dumpit(struct sk_buff *skb,
+				   struct netlink_callback *cb)
+{
+	struct nlattr *tb[RDMA_NLDEV_ATTR_MAX];
+	struct rdma_restrack_entry *res, *nxt;
+	int err, key, ret = 0, idx = 0;
+	struct nlattr *table_attr;
+	struct ib_device *device;
+	int start = cb->args[0];
+	struct ib_qp *qp = NULL;
+	struct nlmsghdr *nlh;
+	u32 index, port = 0;
+
+	err = nlmsg_parse(cb->nlh, 0, tb, RDMA_NLDEV_ATTR_MAX - 1,
+			  nldev_policy, NULL);
+	/*
+	 * Right now, we are expecting the device index to get QP information,
+	 * but it is possible to extend this code to return all devices in
+	 * one shot by checking the existence of RDMA_NLDEV_ATTR_DEV_INDEX.
+	 * if it doesn't exist, we will iterate over all devices.
+	 *
+	 * But it is not needed for now.
+	 */
+	if (err || !tb[RDMA_NLDEV_ATTR_DEV_INDEX])
+		return -EINVAL;
+
+	index = nla_get_u32(tb[RDMA_NLDEV_ATTR_DEV_INDEX]);
+	device = ib_device_get_by_index(index);
+	if (!device)
+		return -EINVAL;
+
+	/*
+	 * If no PORT_INDEX is supplied, we will return QPs from whole device
+	 */
+	if (tb[RDMA_NLDEV_ATTR_PORT_INDEX]) {
+		port = nla_get_u32(tb[RDMA_NLDEV_ATTR_PORT_INDEX]);
+		if (!rdma_is_port_valid(device, port)) {
+			ret = -EINVAL;
+			goto err_index;
+		}
+	}
+
+	nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
+			RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, RDMA_NLDEV_CMD_RES_QP_GET),
+			0, NLM_F_MULTI);
+
+	if (fill_nldev_handle(skb, device)) {
+		ret = -EMSGSIZE;
+		goto err;
+	}
+
+	table_attr = nla_nest_start(skb, RDMA_NLDEV_ATTR_RES_QP);
+	if (!table_attr) {
+		ret = -EMSGSIZE;
+		goto err;
+	}
+
+	rdma_restrack_lock(&device->res, RDMA_RESTRACK_QP);
+	for_each_res_safe(res, nxt, RDMA_RESTRACK_QP, device) {
+		if (idx < start) {
+			idx++;
+			continue;
+		}
+
+		key = srcu_read_lock(&res->srcu);
+
+		if ((!res->task &&
+		     task_active_pid_ns(current) != &init_pid_ns) ||
+		    (res->task &&
+		     task_active_pid_ns(current) != task_active_pid_ns(res->task))) {
+			srcu_read_unlock(&res->srcu, key);
+			/*
+			 * 1. Kernel QPs should be visible in init namsapce only
+			 * 2. Preent only QPs visible in the current namespace
+			 */
+			continue;
+		}
+
+		qp = container_of(res, struct ib_qp, res);
+		/*
+		 * We are releasing the object list lock to ensure that
+		 * object creation/destroy doesn't can progress while
+		 * we are getting information about QP.
+		 *
+		 * It is needed to prevent writer starvation, because of
+		 * large number of reources in object list.
+		 */
+		rdma_restrack_unlock(&device->res, RDMA_RESTRACK_QP);
+		ret = fill_res_qp_entry(skb, qp, port);
+		rdma_restrack_lock(&device->res, RDMA_RESTRACK_QP);
+
+		/*
+		 * There is a need to ensure that next QP is valid,
+		 * because we dropped the lock protecting our linked list.
+		 *
+		 * The RCU protection of "res" ensures that it is safe.
+		 */
+		list_safe_reset_next(res, nxt, list);
+
+		srcu_read_unlock(&res->srcu, key);
+
+		if (ret == -EMSGSIZE)
+			/*
+			 * There is a chance to optimize here.
+			 * It can be done by using list_prepare_entry
+			 * and list_for_each_entry_continue afterwards.
+			 */
+			break;
+		if (ret)
+			goto res_err;
+		idx++;
+	}
+	rdma_restrack_unlock(&device->res, RDMA_RESTRACK_QP);
+
+	nla_nest_end(skb, table_attr);
+	nlmsg_end(skb, nlh);
+	cb->args[0] = idx;
+
+	/*
+	 * No more QPs to fill, cancel the message and
+	 * return 0 to mark end of dumpit.
+	 */
+	if (!qp)
+		goto err;
+
+	put_device(&device->dev);
+	return skb->len;
+
+res_err:
+	nla_nest_cancel(skb, table_attr);
+	rdma_restrack_unlock(&device->res, RDMA_RESTRACK_QP);
+
+err:
+	nlmsg_cancel(skb, nlh);
+
+err_index:
+	put_device(&device->dev);
+	return ret;
+}
+
 static const struct rdma_nl_cbs nldev_cb_table[RDMA_NLDEV_NUM_OPS] = {
 	[RDMA_NLDEV_CMD_GET] = {
 		.doit = nldev_get_doit,
@@ -492,6 +716,19 @@ static const struct rdma_nl_cbs nldev_cb_table[RDMA_NLDEV_NUM_OPS] = {
 		.doit = nldev_res_get_doit,
 		.dump = nldev_res_get_dumpit,
 	},
+	[RDMA_NLDEV_CMD_RES_QP_GET] = {
+		.dump = nldev_res_get_qp_dumpit,
+		/*
+		 * .doit is not implemented yet for two reasons:
+		 * 1. It is not needed yet.
+		 * 2. There is a need to provide identifier, while it is easy
+		 * for the QPs (device index + port index + LQPN), it is not
+		 * the case for the rest of resources (PD and CQ). Because it
+		 * is better to provide similar interface for all resources,
+		 * let's wait till we will have other resources implemented
+		 * too.
+		 */
+	},
 };
 
 void __init nldev_init(void)
include/uapi/rdma/rdma_netlink.h +45 -0

57 lines not on this page · raw message

Signature · 6 lines
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html