ODP information and statistics

PATCH Superseded Leon Romanovsky 3 participants 16 messages Aug 30, 2019 – Sep 9, 2019

Subsystems (2)

InfiniBand Mellanox MLX5 IB driver

lore

Share & tools

lore

Leon Romanovsky

author

2019-08-30 08:16 UTC #0 lore raw

cover ODP information and statistics

From: Leon Romanovsky <[email protected]>

Changelog:
v1:
* Dropped umem patch, because it doesn't follow our IB model, where
UMEM is driver object and ib_core object (Jason).
* Removed the ODP type indicator from ib_umem_odp not needed after
commit fd7dbf035edc ("RDMA/odp: Make it clearer when a umem is an implicit ODP umem")
* Since umems are not part of core MR (from the reason mentioned
above) there is no way to access the odp type as was previously done via nldev (old patch #3). Instead, patch #4 is adding mlx5 implementation for fill_res_entry and dumping ODP type as part of the driver table entry, as its driver details.
* Counter types are now atomic64_t instead of u64.
v0: https://lore.kernel.org/linux-rdma/[email protected]

-----------------------------------------------------------------------------
Hi,

This series from Erez refactors exposes ODP type information (explicit, implicit) and statistics through netlink interface.

Thanks

Erez Alfasi (4):
IB/mlx5: Introduce ODP diagnostic counters RDMA/nldev: Allow different fill function per resource RDMA/nldev: Provide MR statistics
RDMA/mlx5: Return ODP type per MR

 drivers/infiniband/core/device.c      |   1 +
 drivers/infiniband/core/nldev.c       | 109 ++++++++++++++++++++++----
 drivers/infiniband/hw/mlx5/Makefile   |   2 +-
 drivers/infiniband/hw/mlx5/main.c     |  17 ++++
 drivers/infiniband/hw/mlx5/mlx5_ib.h  |   2 +
 drivers/infiniband/hw/mlx5/odp.c      |  18 +++++
 drivers/infiniband/hw/mlx5/restrack.c |  48 ++++++++++++
 include/rdma/ib_umem_odp.h            |  14 ++++
 include/rdma/ib_verbs.h               |   9 +++
 include/rdma/restrack.h               |   3 +
 10 files changed, 207 insertions(+), 16 deletions(-)
 create mode 100644 drivers/infiniband/hw/mlx5/restrack.c

--
2.20.1

Leon Romanovsky

author

2019-08-30 08:16 UTC #1 lore raw

PATCH 1/4 IB/mlx5: Introduce ODP diagnostic counters

From: Erez Alfasi <[email protected]>

Introduce ODP diagnostic counters and count the following per MR within IB/mlx5 driver:
1) Page faults:
Total number of faulted pages.
2) Page invalidations:
Total number of pages invalidated by the OS during all invalidation events. The translations can be no longer valid due to either non-present pages or mapping changes.
3) Prefetched pages:
When prefetching a page, page fault is generated in order to bring the page to the main memory. The prefetched pages counter will be updated during a page fault flow only if it was derived from prefetching operation.

Signed-off-by Erez Alfasi <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/hw/mlx5/odp.c | 18 ++++++++++++++++++
 include/rdma/ib_umem_odp.h       | 14 ++++++++++++++
 2 files changed, 32 insertions(+)

drivers/infiniband/hw/mlx5/odp.c +18 -0

index 905936423a03..b7c8a49ac753 100644
@@ -287,6 +287,10 @@ void mlx5_ib_invalidate_range(struct ib_umem_odp *umem_odp, unsigned long start,
 
 	ib_umem_odp_unmap_dma_pages(umem_odp, start, end);
 
+	/* Count page invalidations */
+	ib_update_odp_stats(umem_odp, invalidations,
+			    ib_umem_odp_num_pages(umem_odp));
+
 	if (unlikely(!umem_odp->npages && mr->parent &&
 		     !umem_odp->dying)) {
 		WRITE_ONCE(umem_odp->dying, 1);

@@ -801,6 +805,20 @@ static int pagefault_single_data_segment(struct mlx5_ib_dev *dev,
 		if (ret < 0)
 			goto srcu_unlock;
 
+		/*
+		 * When prefetching a page, page fault is generated
+		 * in order to bring the page to the main memory.
+		 * In the current flow, page faults are being counted.
+		 * Prefetched pages counter will be updated as well
+		 * only if the current page fault flow was derived
+		 * from prefetching flow.
+		 */
+		ib_update_odp_stats(to_ib_umem_odp(mr->umem), faults, ret);
+
+		if (prefetch)
+			ib_update_odp_stats(to_ib_umem_odp(mr->umem),
+					    prefetched, ret);
+
 		npages += ret;
 		ret = 0;
 		break;

include/rdma/ib_umem_odp.h +14 -0

index b37c674b7fe6..3359e34516da 100644
@@ -37,6 +37,12 @@
 #include <rdma/ib_verbs.h>
 #include <linux/interval_tree.h>
 
+struct ib_odp_counters {
+	atomic64_t faults;
+	atomic64_t invalidations;
+	atomic64_t prefetched;
+};
+
 struct ib_umem_odp {
 	struct ib_umem umem;
 	struct ib_ucontext_per_mm *per_mm;

@@ -62,6 +68,11 @@ struct ib_umem_odp {
 	struct mutex		umem_mutex;
 	void			*private; /* for the HW driver to use. */
 
+	/*
+	 * ODP diagnostic counters.
+	 */
+	struct ib_odp_counters odp_stats;
+
 	int notifiers_seq;
 	int notifiers_count;
 	int npages;

@@ -106,6 +117,9 @@ static inline size_t ib_umem_odp_num_pages(struct ib_umem_odp *umem_odp)
 	       umem_odp->page_shift;
 }
 
+#define ib_update_odp_stats(umem_odp, counter_name, value)		    \
+	atomic64_add(value, &(umem_odp->odp_stats.counter_name))
+
 /*
  * The lower 2 bits of the DMA address signal the R/W permissions for
  * the entry. To upgrade the permissions, provide the appropriate

Signature · 1 line

2.20.1

Jason Gunthorpe

2019-09-09 08:45 UTC #2 lore raw

Re: [PATCH rdma-next v1 1/4] IB/mlx5: Introduce ODP diagnostic counters

Quoted 27 lines · Leon Romanovsky

From: Erez Alfasi <[email protected]>
Introduce ODP diagnostic counters and count the following per MR within IB/mlx5 driver:
1) Page faults:
Total number of faulted pages.
2) Page invalidations:
Total number of pages invalidated by the OS during all invalidation events. The translations can be no longer valid due to either non-present pages or mapping changes.
3) Prefetched pages:
When prefetching a page, page fault is generated in order to bring the page to the main memory. The prefetched pages counter will be updated during a page fault flow only if it was derived from prefetching operation.
Signed-off-by Erez Alfasi <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
 drivers/infiniband/hw/mlx5/odp.c | 18 ++++++++++++++++++
 include/rdma/ib_umem_odp.h       | 14 ++++++++++++++
 2 files changed, 32 insertions(+)
drivers/infiniband/hw/mlx5/odp.c +1 -0
index 905936423a03..b7c8a49ac753 100644
+++ b/drivers/infiniband/hw/mlx5/odp.c

drivers/infiniband/hw/mlx5/odp.c void mlx5_ib_invalidate_range(struct ib_umem_odp *umem_odp, unsigned long start, · line 292

Quoted 6 lines

drivers/infiniband/hw/mlx5/odp.c +3 -0

@@ -287,6 +287,10 @@ void mlx5_ib_invalidate_range(struct ib_umem_odp *umem_odp, unsigned long start,
 
 	ib_umem_odp_unmap_dma_pages(umem_odp, start, end);
 
+	/* Count page invalidations */
+	ib_update_odp_stats(umem_odp, invalidations,
+			    ib_umem_odp_num_pages(umem_odp));

This doesn't seem right, it should only count the number of pages that were actually removed from the mapping

Quoted 17 lines

include/rdma/ib_umem_odp.h +7 -0

index b37c674b7fe6..3359e34516da 100644
+++ b/include/rdma/ib_umem_odp.h

@@ -37,6 +37,12 @@
 #include <rdma/ib_verbs.h>
 #include <linux/interval_tree.h>
 
+struct ib_odp_counters {
+	atomic64_t faults;
+	atomic64_t invalidations;
+	atomic64_t prefetched;
+};
+
 struct ib_umem_odp {
 	struct ib_umem umem;
 	struct ib_ucontext_per_mm *per_mm;

include/rdma/ib_umem_odp.h struct ib_umem_odp { · line 74

Quoted 7 lines

include/rdma/ib_umem_odp.h +4 -0

@@ -62,6 +68,11 @@ struct ib_umem_odp {
 	struct mutex		umem_mutex;
 	void			*private; /* for the HW driver to use. */
 
+	/*
+	 * ODP diagnostic counters.
+	 */
+	struct ib_odp_counters odp_stats;

This really belongs in the MR not the umem

include/rdma/ib_umem_odp.h static inline size_t ib_umem_odp_num_pages(struct ib_umem_odp *umem_odp) · line 121

Quoted 9 lines

include/rdma/ib_umem_odp.h +2 -0

 	int notifiers_seq;
 	int notifiers_count;
 	int npages;

@@ -106,6 +117,9 @@ static inline size_t ib_umem_odp_num_pages(struct ib_umem_odp *umem_odp)
 	       umem_odp->page_shift;
 }
 
+#define ib_update_odp_stats(umem_odp, counter_name, value)		    \
+	atomic64_add(value, &(umem_odp->odp_stats.counter_name))

Missing brackets in a macro

Jason

Leon Romanovsky

author

2019-09-09 09:40 UTC #3 lore raw

Re: [PATCH rdma-next v1 1/4] IB/mlx5: Introduce ODP diagnostic counters

Quoted 29 lines · Jason Gunthorpe

Quoted 28 lines · Leon Romanovsky
From: Erez Alfasi <[email protected]>
Introduce ODP diagnostic counters and count the following per MR within IB/mlx5 driver:
1) Page faults:
Total number of faulted pages.
2) Page invalidations:
Total number of pages invalidated by the OS during all invalidation events. The translations can be no longer valid due to either non-present pages or mapping changes.
3) Prefetched pages:
When prefetching a page, page fault is generated in order to bring the page to the main memory. The prefetched pages counter will be updated during a page fault flow only if it was derived from prefetching operation.
Signed-off-by Erez Alfasi <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
 drivers/infiniband/hw/mlx5/odp.c | 18 ++++++++++++++++++
 include/rdma/ib_umem_odp.h       | 14 ++++++++++++++
 2 files changed, 32 insertions(+)
drivers/infiniband/hw/mlx5/odp.c +1 -0
index 905936423a03..b7c8a49ac753 100644
+++ b/drivers/infiniband/hw/mlx5/odp.c
@@ -287,6 +287,10 @@ void mlx5_ib_invalidate_range(struct ib_umem_odp *umem_odp, unsigned long start,
 

Quoted 8 lines

drivers/infiniband/hw/mlx5/odp.c void mlx5_ib_invalidate_range(struct ib_umem_odp *umem_odp, unsigned long start, · line 292
Quoted 5 lines
drivers/infiniband/hw/mlx5/odp.c +3 -0
 	ib_umem_odp_unmap_dma_pages(umem_odp, start, end);
 
+	/* Count page invalidations */
+	ib_update_odp_stats(umem_odp, invalidations,
+			    ib_umem_odp_num_pages(umem_odp));
This doesn't seem right, it should only count the number of pages that were actually removed from the mapping

Absolutely, It is my mistake, I think that I mislead Erez back then.

Quoted 21 lines

include/rdma/ib_umem_odp.h +7 -0

index b37c674b7fe6..3359e34516da 100644
+++ b/include/rdma/ib_umem_odp.h

@@ -37,6 +37,12 @@
 #include <rdma/ib_verbs.h>
 #include <linux/interval_tree.h>
 
+struct ib_odp_counters {
+	atomic64_t faults;
+	atomic64_t invalidations;
+	atomic64_t prefetched;
+};
+
 struct ib_umem_odp {
 	struct ib_umem umem;
 	struct ib_ucontext_per_mm *per_mm;

@@ -62,6 +68,11 @@ struct ib_umem_odp {
 	struct mutex		umem_mutex;
 	void			*private; /* for the HW driver to use. */

Quoted 6 lines

include/rdma/ib_umem_odp.h struct ib_umem_odp { · line 74
Quoted 4 lines
include/rdma/ib_umem_odp.h +4 -0
+	/*
+	 * ODP diagnostic counters.
+	 */
+	struct ib_odp_counters odp_stats;
This really belongs in the MR not the umem

Bu why? We have umem_odp, all our statistics calculations are done in umem_odp level.

Quoted 8 lines

include/rdma/ib_umem_odp.h +0 -0

 	int notifiers_seq;
 	int notifiers_count;
 	int npages;

@@ -106,6 +117,9 @@ static inline size_t ib_umem_odp_num_pages(struct ib_umem_odp *umem_odp)
 	       umem_odp->page_shift;
 }

Quoted 4 lines

include/rdma/ib_umem_odp.h static inline size_t ib_umem_odp_num_pages(struct ib_umem_odp *umem_odp) · line 121
Quoted 2 lines
include/rdma/ib_umem_odp.h +2 -0
+#define ib_update_odp_stats(umem_odp, counter_name, value)		    \
+	atomic64_add(value, &(umem_odp->odp_stats.counter_name))
Missing brackets in a macro

I am doubt about that, because it is one to one replace and not "real" macro.

Thanks

Quoted 2 lines

Jason

Leon Romanovsky

author

2019-08-30 08:16 UTC #4 lore raw

PATCH 2/4 RDMA/nldev: Allow different fill function per resource

In reply to Leon Romanovsky

From: Erez Alfasi <[email protected]>

So far res_get_common_{dumpit, doit} was using the default resource fill function which was defined as part of the nldev_fill_res_entry fill_entries.

Add a fill function pointer as an argument allows us to use different fill function in case we want to dump different values then 'rdma resource' flow do, but still use the same existing general resources dumping flow.

If a NULL value is passed, it will be using the default fill function that was defined in 'fill_entries' for a given resource type.

Signed-off-by Erez Alfasi <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/nldev.c | 42 +++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 13 deletions(-)

drivers/infiniband/core/nldev.c +29 ~~-13~~

index cc08218f1ef7..47f7fe5432db 100644
@@ -1181,7 +1181,10 @@ static const struct nldev_fill_res_entry fill_entries[RDMA_RESTRACK_MAX] = {
 
 static int res_get_common_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
 			       struct netlink_ext_ack *extack,
-			       enum rdma_restrack_type res_type)
+			       enum rdma_restrack_type res_type,
+			       int (*fill_func)(struct sk_buff*, bool,
+						struct rdma_restrack_entry*,
+						uint32_t))
 {
 	const struct nldev_fill_res_entry *fe = &fill_entries[res_type];
 	struct nlattr *tb[RDMA_NLDEV_ATTR_MAX];

@@ -1244,7 +1247,12 @@ static int res_get_common_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
 	}
 
 	has_cap_net_admin = netlink_capable(skb, CAP_NET_ADMIN);
-	ret = fe->fill_res_func(msg, has_cap_net_admin, res, port);
+
+	if (fill_func)
+		ret = fill_func(msg, has_cap_net_admin, res, port);
+	else
+		ret = fe->fill_res_func(msg, has_cap_net_admin, res, port);
+
 	rdma_restrack_put(res);
 	if (ret)
 		goto err_free;

@@ -1264,7 +1272,10 @@ static int res_get_common_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 static int res_get_common_dumpit(struct sk_buff *skb,
 				 struct netlink_callback *cb,
-				 enum rdma_restrack_type res_type)
+				 enum rdma_restrack_type res_type,
+				 int (*fill_func)(struct sk_buff*, bool,
+						  struct rdma_restrack_entry*,
+						  uint32_t))
 {
 	const struct nldev_fill_res_entry *fe = &fill_entries[res_type];
 	struct nlattr *tb[RDMA_NLDEV_ATTR_MAX];

@@ -1352,7 +1363,12 @@ static int res_get_common_dumpit(struct sk_buff *skb,
 			goto msg_full;
 		}
 
-		ret = fe->fill_res_func(skb, has_cap_net_admin, res, port);
+		if (fill_func)
+			ret = fill_func(skb, has_cap_net_admin, res, port);
+		else
+			ret = fe->fill_res_func(skb, has_cap_net_admin,
+						res, port);
+
 		rdma_restrack_put(res);
 
 		if (ret) {

@@ -1395,17 +1411,17 @@ next:		idx++;
 	return ret;
 }
 
-#define RES_GET_FUNCS(name, type)                                              \
-	static int nldev_res_get_##name##_dumpit(struct sk_buff *skb,          \
+#define RES_GET_FUNCS(name, type)					       \
+	static int nldev_res_get_##name##_dumpit(struct sk_buff *skb,	       \
 						 struct netlink_callback *cb)  \
-	{                                                                      \
-		return res_get_common_dumpit(skb, cb, type);                   \
-	}                                                                      \
-	static int nldev_res_get_##name##_doit(struct sk_buff *skb,            \
-					       struct nlmsghdr *nlh,           \
+	{								       \
+		return res_get_common_dumpit(skb, cb, type, NULL);	       \
+	}								       \
+	static int nldev_res_get_##name##_doit(struct sk_buff *skb,	       \
+					       struct nlmsghdr *nlh,	       \
 					       struct netlink_ext_ack *extack) \
-	{                                                                      \
-		return res_get_common_doit(skb, nlh, extack, type);            \
+	{								       \
+		return res_get_common_doit(skb, nlh, extack, type, NULL);      \
 	}
 
 RES_GET_FUNCS(qp, RDMA_RESTRACK_QP);

Signature · 1 line

2.20.1

Jason Gunthorpe

2019-09-09 08:48 UTC #5 lore raw

Re: [PATCH rdma-next v1 2/4] RDMA/nldev: Allow different fill function per resource

Quoted 24 lines · Leon Romanovsky

From: Erez Alfasi <[email protected]>
So far res_get_common_{dumpit, doit} was using the default resource fill function which was defined as part of the nldev_fill_res_entry fill_entries.
Add a fill function pointer as an argument allows us to use different fill function in case we want to dump different values then 'rdma resource' flow do, but still use the same existing general resources dumping flow.
If a NULL value is passed, it will be using the default fill function that was defined in 'fill_entries' for a given resource type.
Signed-off-by Erez Alfasi <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
 drivers/infiniband/core/nldev.c | 42 +++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 13 deletions(-)
drivers/infiniband/core/nldev.c +1 -0
index cc08218f1ef7..47f7fe5432db 100644
+++ b/drivers/infiniband/core/nldev.c

drivers/infiniband/core/nldev.c static const struct nldev_fill_res_entry fill_entries[RDMA_RESTRACK_MAX] = { · line 1187

Quoted 8 lines

drivers/infiniband/core/nldev.c +4 -1

@@ -1181,7 +1181,10 @@ static const struct nldev_fill_res_entry fill_entries[RDMA_RESTRACK_MAX] = {
 
 static int res_get_common_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
 			       struct netlink_ext_ack *extack,
-			       enum rdma_restrack_type res_type)
+			       enum rdma_restrack_type res_type,
+			       int (*fill_func)(struct sk_buff*, bool,
+						struct rdma_restrack_entry*,
+						uint32_t))

Use a typedef?

drivers/infiniband/core/nldev.c static int res_get_common_doit(struct sk_buff *skb, struct nlmsghdr *nlh, · line 1254

Quoted 13 lines

drivers/infiniband/core/nldev.c +5 -1

 {
 	const struct nldev_fill_res_entry *fe = &fill_entries[res_type];
 	struct nlattr *tb[RDMA_NLDEV_ATTR_MAX];

@@ -1244,7 +1247,12 @@ static int res_get_common_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
 	}
 
 	has_cap_net_admin = netlink_capable(skb, CAP_NET_ADMIN);
-	ret = fe->fill_res_func(msg, has_cap_net_admin, res, port);
+
+	if (fill_func)
+		ret = fill_func(msg, has_cap_net_admin, res, port);
+	else
+		ret = fe->fill_res_func(msg, has_cap_net_admin, res, port);

Weird to now be choosing between two function pointers

Quoted 11 lines

drivers/infiniband/core/nldev.c +3 -7

-#define RES_GET_FUNCS(name, type)                                              \
-	static int nldev_res_get_##name##_dumpit(struct sk_buff *skb,          \
+#define RES_GET_FUNCS(name, type)					       \
+	static int nldev_res_get_##name##_dumpit(struct sk_buff *skb,	       \
 						 struct netlink_callback *cb)  \
-	{                                                                      \
-		return res_get_common_dumpit(skb, cb, type);                   \
-	}                                                                      \
-	static int nldev_res_get_##name##_doit(struct sk_buff *skb,            \
-					       struct nlmsghdr *nlh,           \
+	{								       \

drivers/infiniband/core/nldev.c next: idx++; · line 1425

Quoted 10 lines

drivers/infiniband/core/nldev.c +6 -2

+		return res_get_common_dumpit(skb, cb, type, NULL);	       \
+	}								       \
+	static int nldev_res_get_##name##_doit(struct sk_buff *skb,	       \
+					       struct nlmsghdr *nlh,	       \
 					       struct netlink_ext_ack *extack) \
-	{                                                                      \
-		return res_get_common_doit(skb, nlh, extack, type);            \
+	{								       \
+		return res_get_common_doit(skb, nlh, extack, type, NULL);      \
 	}

ie the NULL should be fill_entries[type]->fill_res_func?

Jason

Leon Romanovsky

author

2019-09-09 09:46 UTC #6 lore raw

Re: [PATCH rdma-next v1 2/4] RDMA/nldev: Allow different fill function per resource

Quoted 26 lines · Jason Gunthorpe

Quoted 25 lines · Leon Romanovsky
From: Erez Alfasi <[email protected]>
So far res_get_common_{dumpit, doit} was using the default resource fill function which was defined as part of the nldev_fill_res_entry fill_entries.
Add a fill function pointer as an argument allows us to use different fill function in case we want to dump different values then 'rdma resource' flow do, but still use the same existing general resources dumping flow.
If a NULL value is passed, it will be using the default fill function that was defined in 'fill_entries' for a given resource type.
Signed-off-by Erez Alfasi <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
 drivers/infiniband/core/nldev.c | 42 +++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 13 deletions(-)
drivers/infiniband/core/nldev.c +1 -0
index cc08218f1ef7..47f7fe5432db 100644
+++ b/drivers/infiniband/core/nldev.c
@@ -1181,7 +1181,10 @@ static const struct nldev_fill_res_entry fill_entries[RDMA_RESTRACK_MAX] = {
 

Quoted 9 lines

drivers/infiniband/core/nldev.c static const struct nldev_fill_res_entry fill_entries[RDMA_RESTRACK_MAX] = { · line 1187

Quoted 7 lines

drivers/infiniband/core/nldev.c +4 -1

 static int res_get_common_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
 			       struct netlink_ext_ack *extack,
-			       enum rdma_restrack_type res_type)
+			       enum rdma_restrack_type res_type,
+			       int (*fill_func)(struct sk_buff*, bool,
+						struct rdma_restrack_entry*,
+						uint32_t))

Use a typedef?

I'll take a look on that, but it is not fully clear to me what are the gains here.

Quoted 7 lines

drivers/infiniband/core/nldev.c +0 -0

 {
 	const struct nldev_fill_res_entry *fe = &fill_entries[res_type];
 	struct nlattr *tb[RDMA_NLDEV_ATTR_MAX];

@@ -1244,7 +1247,12 @@ static int res_get_common_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
 	}

Quoted 9 lines

drivers/infiniband/core/nldev.c static int res_get_common_doit(struct sk_buff *skb, struct nlmsghdr *nlh, · line 1254
Quoted 7 lines
drivers/infiniband/core/nldev.c +5 -1
 	has_cap_net_admin = netlink_capable(skb, CAP_NET_ADMIN);
-	ret = fe->fill_res_func(msg, has_cap_net_admin, res, port);
+
+	if (fill_func)
+		ret = fill_func(msg, has_cap_net_admin, res, port);
+	else
+		ret = fe->fill_res_func(msg, has_cap_net_admin, res, port);
Weird to now be choosing between two function pointers

I didn't like this either, but we didn't find an easy solution to do chains of fill functions. In our case, we are requesting COUNTER object which will call to MR object to fill statistic.

Quoted 16 lines

drivers/infiniband/core/nldev.c +7 -7

-#define RES_GET_FUNCS(name, type)                                              \
-	static int nldev_res_get_##name##_dumpit(struct sk_buff *skb,          \
+#define RES_GET_FUNCS(name, type)					       \
+	static int nldev_res_get_##name##_dumpit(struct sk_buff *skb,	       \
 						 struct netlink_callback *cb)  \
-	{                                                                      \
-		return res_get_common_dumpit(skb, cb, type);                   \
-	}                                                                      \
-	static int nldev_res_get_##name##_doit(struct sk_buff *skb,            \
-					       struct nlmsghdr *nlh,           \
+	{								       \
+		return res_get_common_dumpit(skb, cb, type, NULL);	       \
+	}								       \
+	static int nldev_res_get_##name##_doit(struct sk_buff *skb,	       \
+					       struct nlmsghdr *nlh,	       \

Quoted 8 lines

drivers/infiniband/core/nldev.c next: idx++; · line 1425
Quoted 6 lines
drivers/infiniband/core/nldev.c +2 -2
 					       struct netlink_ext_ack *extack) \
-	{                                                                      \
-		return res_get_common_doit(skb, nlh, extack, type);            \
+	{								       \
+		return res_get_common_doit(skb, nlh, extack, type, NULL);      \
 	}
ie the NULL should be fill_entries[type]->fill_res_func?

The "if (fill_func) " above will do the trick.

Quoted 2 lines

Jason

Leon Romanovsky

author

2019-08-30 08:16 UTC #7 lore raw

PATCH 3/4 RDMA/nldev: Provide MR statistics

In reply to Leon Romanovsky

From: Erez Alfasi <[email protected]>

Add RDMA nldev netlink interface for dumping MR
statistics information.

Output example:
ereza@dev~$: ./ibv_rc_pingpong -o -P -s 500000000
local address: LID 0x0001, QPN 0x00008a, PSN 0xf81096, GID ::

ereza@dev~$: rdma stat show mr
dev mlx5_0 mrn 2 page_faults 122071 page_invalidations 0 prefetched_pages 122071

Signed-off-by Erez Alfasi <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/device.c  |  1 +
 drivers/infiniband/core/nldev.c   | 54 +++++++++++++++++++++++++++++--
 drivers/infiniband/hw/mlx5/main.c | 16 +++++++++
 include/rdma/ib_verbs.h           |  9 ++++++
 4 files changed, 78 insertions(+), 2 deletions(-)

drivers/infiniband/core/device.c +1 -0

index 99c4a55545cf..34a9e37c5c61 100644
@@ -2610,6 +2610,7 @@ void ib_set_device_ops(struct ib_device *dev, const struct ib_device_ops *ops)
 	SET_DEVICE_OP(dev_ops, get_dma_mr);
 	SET_DEVICE_OP(dev_ops, get_hw_stats);
 	SET_DEVICE_OP(dev_ops, get_link_layer);
+	SET_DEVICE_OP(dev_ops, fill_odp_stats);
 	SET_DEVICE_OP(dev_ops, get_netdev);
 	SET_DEVICE_OP(dev_ops, get_port_immutable);
 	SET_DEVICE_OP(dev_ops, get_vector_affinity);

drivers/infiniband/core/nldev.c +52 -2

78 lines not on this page · raw message

drivers/infiniband/hw/mlx5/main.c +16 -0

index 07aecba16019..05095fda03cc 100644
@@ -67,6 +67,7 @@
 #include <rdma/uverbs_std_types.h>
 #include <rdma/mlx5_user_ioctl_verbs.h>
 #include <rdma/mlx5_user_ioctl_cmds.h>
+#include <rdma/ib_umem_odp.h>
 
 #define UVERBS_MODULE_NAME mlx5_ib
 #include <rdma/uverbs_named_ioctl.h>

@@ -121,6 +122,20 @@ struct mlx5_ib_dev *mlx5_ib_get_ibdev_from_mpi(struct mlx5_ib_multiport_info *mp
 	return dev;
 }
 
+static bool mlx5_ib_fill_odp_stats(struct ib_mr *ibmr,
+				   struct ib_odp_counters *cnt)
+{
+	struct mlx5_ib_mr *mr = to_mmr(ibmr);
+
+	if (!is_odp_mr(mr))
+		return false;
+
+	memcpy(cnt, &to_ib_umem_odp(mr->umem)->odp_stats,
+	       sizeof(struct ib_odp_counters));
+
+	return true;
+}
+
 static enum rdma_link_layer
 mlx5_port_type_cap_to_rdma_ll(int port_type_cap)
 {

@@ -6316,6 +6331,7 @@ static const struct ib_device_ops mlx5_ib_dev_ops = {
 	.get_dev_fw_str = get_dev_fw_str,
 	.get_dma_mr = mlx5_ib_get_dma_mr,
 	.get_link_layer = mlx5_ib_port_link_layer,
+	.fill_odp_stats = mlx5_ib_fill_odp_stats,
 	.map_mr_sg = mlx5_ib_map_mr_sg,
 	.map_mr_sg_pi = mlx5_ib_map_mr_sg_pi,
 	.mmap = mlx5_ib_mmap,

include/rdma/ib_verbs.h +9 -0

index de5bc352f473..48d6513b3b59 100644
@@ -72,6 +72,7 @@
 #define IB_FW_VERSION_NAME_MAX	ETHTOOL_FWVERS_LEN
 
 struct ib_umem_odp;
+struct ib_odp_counters;
 
 extern struct workqueue_struct *ib_wq;
 extern struct workqueue_struct *ib_comp_wq;

@@ -2566,6 +2567,14 @@ struct ib_device_ops {
 	 */
 	int (*counter_update_stats)(struct rdma_counter *counter);
 
+	/**
+	 * fill_odp_stats - Fill MR ODP stats into a given
+	 * ib_odp_counters struct.
+	 * Return value - true in case counters has been filled,
+	 * false otherwise (if its non-ODP registered MR for example).
+	 */
+	bool (*fill_odp_stats)(struct ib_mr *mr, struct ib_odp_counters *cnt);
+
 	DECLARE_RDMA_OBJ_SIZE(ib_ah);
 	DECLARE_RDMA_OBJ_SIZE(ib_cq);
 	DECLARE_RDMA_OBJ_SIZE(ib_pd);

Signature · 1 line

2.20.1

Parav Pandit

2019-08-30 10:19 UTC #8 lore raw

RE: [PATCH rdma-next v1 3/4] RDMA/nldev: Provide MR statistics

Quoted 203 lines

-----Original Message-----
From: [email protected] <linux-rdma- [email protected]> On Behalf Of Leon Romanovsky
Sent: Friday, August 30, 2019 1:46 PM
To: Doug Ledford <[email protected]>; Jason Gunthorpe <[email protected]>
Cc: Leon Romanovsky <[email protected]>; RDMA mailing list <linux- [email protected]>; Erez Alfasi <[email protected]>
Subject: [PATCH rdma-next v1 3/4] RDMA/nldev: Provide MR statistics

From: Erez Alfasi <[email protected]>

Add RDMA nldev netlink interface for dumping MR statistics information.

Output example:
ereza@dev~$: ./ibv_rc_pingpong -o -P -s 500000000
local address: LID 0x0001, QPN 0x00008a, PSN 0xf81096, GID ::

ereza@dev~$: rdma stat show mr
dev mlx5_0 mrn 2 page_faults 122071 page_invalidations 0 prefetched_pages 122071

Signed-off-by Erez Alfasi <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/device.c  |  1 +
 drivers/infiniband/core/nldev.c   | 54 +++++++++++++++++++++++++++++--
 drivers/infiniband/hw/mlx5/main.c | 16 +++++++++
 include/rdma/ib_verbs.h           |  9 ++++++
 4 files changed, 78 insertions(+), 2 deletions(-)

drivers/infiniband/core/device.c +1 -0

index 99c4a55545cf..34a9e37c5c61 100644
@@ -2610,6 +2610,7 @@ void ib_set_device_ops(struct ib_device *dev,
 const struct ib_device_ops *ops)
 	SET_DEVICE_OP(dev_ops, get_dma_mr);
 	SET_DEVICE_OP(dev_ops, get_hw_stats);
 	SET_DEVICE_OP(dev_ops, get_link_layer);
+	SET_DEVICE_OP(dev_ops, fill_odp_stats);
 	SET_DEVICE_OP(dev_ops, get_netdev);
 	SET_DEVICE_OP(dev_ops, get_port_immutable);

SET_DEVICE_OP(dev_ops, get_vector_affinity); diff --git
a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c index 47f7fe5432db..47fee3d68cb9 100644

drivers/infiniband/core/nldev.c +47 -1

@@ -37,6 +37,7 @@
 #include <net/netlink.h>
 #include <rdma/rdma_cm.h>
 #include <rdma/rdma_netlink.h>
+#include <rdma/ib_umem_odp.h>
 
 #include "core_priv.h"
 #include "cma_priv.h"

@@ -748,6 +749,49 @@ static int fill_stat_hwcounter_entry(struct sk_buff
 *msg,
 	return -EMSGSIZE;
 }
 
+static int fill_stat_mr_entry(struct sk_buff *msg, bool has_cap_net_admin,
+			      struct rdma_restrack_entry *res, uint32_t port) {
+	struct ib_mr *mr = container_of(res, struct ib_mr, res);
+	struct ib_device *dev = mr->pd->device;
+	struct ib_odp_counters odp_stats;
+	struct nlattr *table_attr;
+
+	if (nla_put_u32(msg, RDMA_NLDEV_ATTR_RES_MRN, res->id))
+		goto err;
+
+	if (!dev->ops.fill_odp_stats)
+		return 0;
+
+	if (!dev->ops.fill_odp_stats(mr, &odp_stats))
+		return 0;
+
+	table_attr = nla_nest_start(msg,
+				    RDMA_NLDEV_ATTR_STAT_HWCOUNTERS);
+
+	if (!table_attr)
+		return -EMSGSIZE;
+
+	if (fill_stat_hwcounter_entry(msg, "page_faults",
+				      (u64)atomic64_read(&odp_stats.faults)))
+		goto err_table;
+	if (fill_stat_hwcounter_entry(
+		    msg, "page_invalidations",
+		    (u64)atomic64_read(&odp_stats.invalidations)))
+		goto err_table;
+	if (fill_stat_hwcounter_entry(msg, "prefetched_pages",
+
 (u64)atomic64_read(&odp_stats.prefetched)))
+		goto err_table;
+
+	nla_nest_end(msg, table_attr);
+	return 0;
+
+err_table:
+	nla_nest_cancel(msg, table_attr);
+err:
+	return -EMSGSIZE;
+}
+
 static int fill_stat_counter_hwcounters(struct sk_buff *msg,
 					struct rdma_counter *counter)
 {

@@ -2008,7 +2052,10 @@ static int nldev_stat_get_doit(struct sk_buff *skb,
 struct nlmsghdr *nlh,
 	case RDMA_NLDEV_ATTR_RES_QP:
 		ret = stat_get_doit_qp(skb, nlh, extack, tb);
 		break;
-
+	case RDMA_NLDEV_ATTR_RES_MR:
+		ret = res_get_common_doit(skb, nlh, extack,
 RDMA_RESTRACK_MR,
+					  fill_stat_mr_entry);
+		break;
 	default:

drivers/infiniband/core/nldev.c +4 -1

 		ret = -EINVAL;
 		break;

@@ -2032,7 +2079,10 @@ static int nldev_stat_get_dumpit(struct sk_buff
 *skb,
 	case RDMA_NLDEV_ATTR_RES_QP:
 		ret = nldev_res_get_counter_dumpit(skb, cb);
 		break;
-
+	case RDMA_NLDEV_ATTR_RES_MR:
+		ret = res_get_common_dumpit(skb, cb,
 RDMA_RESTRACK_MR,
+					    fill_stat_mr_entry);
+		break;
 	default:

drivers/infiniband/core/nldev.c +0 -0

 		ret = -EINVAL;
 		break;

drivers/infiniband/hw/mlx5/main.c +0 -0

index 07aecba16019..05095fda03cc 100644
@@ -67,6 +67,7 @@
 #include <rdma/uverbs_std_types.h>
 #include <rdma/mlx5_user_ioctl_verbs.h>  #include

<rdma/mlx5_user_ioctl_cmds.h>

drivers/infiniband/hw/mlx5/main.c +15 -0

+#include <rdma/ib_umem_odp.h>
 
 #define UVERBS_MODULE_NAME mlx5_ib
 #include <rdma/uverbs_named_ioctl.h>

@@ -121,6 +122,20 @@ struct mlx5_ib_dev
 *mlx5_ib_get_ibdev_from_mpi(struct mlx5_ib_multiport_info *mp
 	return dev;
 }
 
+static bool mlx5_ib_fill_odp_stats(struct ib_mr *ibmr,
+				   struct ib_odp_counters *cnt)
+{
+	struct mlx5_ib_mr *mr = to_mmr(ibmr);
+
+	if (!is_odp_mr(mr))
+		return false;
+
+	memcpy(cnt, &to_ib_umem_odp(mr->umem)->odp_stats,
+	       sizeof(struct ib_odp_counters));
+
+	return true;
+}
+
 static enum rdma_link_layer
 mlx5_port_type_cap_to_rdma_ll(int port_type_cap)  { @@ -6316,6 +6331,7

@@ static const struct ib_device_ops mlx5_ib_dev_ops = {

drivers/infiniband/hw/mlx5/main.c +1 -0

 	.get_dev_fw_str = get_dev_fw_str,
 	.get_dma_mr = mlx5_ib_get_dma_mr,
 	.get_link_layer = mlx5_ib_port_link_layer,
+	.fill_odp_stats = mlx5_ib_fill_odp_stats,
 	.map_mr_sg = mlx5_ib_map_mr_sg,
 	.map_mr_sg_pi = mlx5_ib_map_mr_sg_pi,
 	.mmap = mlx5_ib_mmap,

include/rdma/ib_verbs.h index renamed from include/rdma/ib_verbs.h +0 -0

de5bc352f473..48d6513b3b59 100644

include/rdma/ib_verbs.h +1 -0

@@ -72,6 +72,7 @@
 #define IB_FW_VERSION_NAME_MAX	ETHTOOL_FWVERS_LEN
 
 struct ib_umem_odp;
+struct ib_odp_counters;
 
 extern struct workqueue_struct *ib_wq;
 extern struct workqueue_struct *ib_comp_wq; @@ -2566,6 +2567,14 @@

struct ib_device_ops {

include/rdma/ib_verbs.h +7 -0

 	 */
 	int (*counter_update_stats)(struct rdma_counter *counter);
 
+	/**
+	 * fill_odp_stats - Fill MR ODP stats into a given
+	 * ib_odp_counters struct.
+	 * Return value - true in case counters has been filled,
+	 * false otherwise (if its non-ODP registered MR for example).
+	 */
+	bool (*fill_odp_stats)(struct ib_mr *mr, struct ib_odp_counters

Quoted 2 lines

*cnt);
+

Requesting ODP stats on non-ODP MR is an error.
Instead of returning bool, please return int = -EINVAL as an error for non ODP MRs.

Quoted 5 lines

include/rdma/ib_verbs.h +0 -1

 	DECLARE_RDMA_OBJ_SIZE(ib_ah);
 	DECLARE_RDMA_OBJ_SIZE(ib_cq);
 	DECLARE_RDMA_OBJ_SIZE(ib_pd);
--

2.20.1

Leon Romanovsky

author

2019-08-30 11:12 UTC #9 lore raw

Re: [PATCH rdma-next v1 3/4] RDMA/nldev: Provide MR statistics

Quoted 27 lines · Parav Pandit

Quoted 27 lines
-----Original Message-----
From: [email protected] <linux-rdma- [email protected]> On Behalf Of Leon Romanovsky
Sent: Friday, August 30, 2019 1:46 PM
To: Doug Ledford <[email protected]>; Jason Gunthorpe <[email protected]>
Cc: Leon Romanovsky <[email protected]>; RDMA mailing list <linux- [email protected]>; Erez Alfasi <[email protected]>
Subject: [PATCH rdma-next v1 3/4] RDMA/nldev: Provide MR statistics
From: Erez Alfasi <[email protected]>
Add RDMA nldev netlink interface for dumping MR statistics information.
Output example:
ereza@dev~$: ./ibv_rc_pingpong -o -P -s 500000000
local address: LID 0x0001, QPN 0x00008a, PSN 0xf81096, GID ::
ereza@dev~$: rdma stat show mr
dev mlx5_0 mrn 2 page_faults 122071 page_invalidations 0 prefetched_pages 122071
Signed-off-by Erez Alfasi <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---

<...>

Quoted 4 lines

Quoted 4 lines
struct ib_device_ops {
include/rdma/ib_verbs.h +0 -0
 	 */
 	int (*counter_update_stats)(struct rdma_counter *counter);

Quoted 11 lines

Quoted 9 lines
include/rdma/ib_verbs.h +7 -0
+	/**
+	 * fill_odp_stats - Fill MR ODP stats into a given
+	 * ib_odp_counters struct.
+	 * Return value - true in case counters has been filled,
+	 * false otherwise (if its non-ODP registered MR for example).
+	 */
+	bool (*fill_odp_stats)(struct ib_mr *mr, struct ib_odp_counters
*cnt);
+
Requesting ODP stats on non-ODP MR is an error.
Instead of returning bool, please return int = -EINVAL as an error for non ODP MRs.

We don't want to add extra checks in the drivers for something that is supposed to be checked by upper layer. If caller asks for ODP statistics, he will check that MR is ODP backed. Especially if the need to have ODP MR is embedded into the function name.

Thanks

Parav Pandit

2019-08-30 12:07 UTC #10 lore raw

RE: [PATCH rdma-next v1 3/4] RDMA/nldev: Provide MR statistics

Quoted 57 lines

-----Original Message-----
From: Leon Romanovsky <[email protected]>
Sent: Friday, August 30, 2019 4:43 PM
To: Parav Pandit <[email protected]>
Cc: Doug Ledford <[email protected]>; Jason Gunthorpe <[email protected]>; RDMA mailing list <[email protected]>; Erez Alfasi <[email protected]>
Subject: Re: [PATCH rdma-next v1 3/4] RDMA/nldev: Provide MR statistics
Quoted 27 lines · Parav Pandit
Quoted 27 lines
-----Original Message-----
From: [email protected] <linux-rdma- [email protected]> On Behalf Of Leon Romanovsky
Sent: Friday, August 30, 2019 1:46 PM
To: Doug Ledford <[email protected]>; Jason Gunthorpe <[email protected]>
Cc: Leon Romanovsky <[email protected]>; RDMA mailing list <linux- [email protected]>; Erez Alfasi <[email protected]>
Subject: [PATCH rdma-next v1 3/4] RDMA/nldev: Provide MR statistics
From: Erez Alfasi <[email protected]>
Add RDMA nldev netlink interface for dumping MR statistics information.
Output example:
ereza@dev~$: ./ibv_rc_pingpong -o -P -s 500000000
local address: LID 0x0001, QPN 0x00008a, PSN 0xf81096, GID ::
ereza@dev~$: rdma stat show mr
dev mlx5_0 mrn 2 page_faults 122071 page_invalidations 0 prefetched_pages 122071
Signed-off-by Erez Alfasi <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
<...>
Quoted 4 lines
Quoted 4 lines
struct ib_device_ops {
include/rdma/ib_verbs.h +0 -0
 	 */
 	int (*counter_update_stats)(struct rdma_counter *counter);
Quoted 11 lines
Quoted 9 lines
include/rdma/ib_verbs.h +7 -0
+	/**
+	 * fill_odp_stats - Fill MR ODP stats into a given
+	 * ib_odp_counters struct.
+	 * Return value - true in case counters has been filled,
+	 * false otherwise (if its non-ODP registered MR for example).
+	 */
+	bool (*fill_odp_stats)(struct ib_mr *mr, struct ib_odp_counters
*cnt);
+
Requesting ODP stats on non-ODP MR is an error.
Instead of returning bool, please return int = -EINVAL as an error for non ODP

Quoted 7 lines

MRs.
We don't want to add extra checks in the drivers for something that is supposed to be checked by upper layer. If caller asks for ODP statistics, he will check that MR is ODP backed. Especially if the need to have ODP MR is embedded into the function name.

That make sense. In that case return type should be void. Comment says - "false otherwise (if its non-ODP registered MR for example)."

And with that logic,
Below code should be in upper layer instead of mlx5/main.c

+ if (!is_odp_mr(mr))
+ return false;

Jason Gunthorpe

2019-09-09 08:52 UTC #11 lore raw

Re: [PATCH rdma-next v1 3/4] RDMA/nldev: Provide MR statistics

In reply to Leon Romanovsky

Quoted 25 lines · Leon Romanovsky

From: Erez Alfasi <[email protected]>
Add RDMA nldev netlink interface for dumping MR
statistics information.
Output example:
ereza@dev~$: ./ibv_rc_pingpong -o -P -s 500000000
local address: LID 0x0001, QPN 0x00008a, PSN 0xf81096, GID ::
ereza@dev~$: rdma stat show mr
dev mlx5_0 mrn 2 page_faults 122071 page_invalidations 0 prefetched_pages 122071
Signed-off-by Erez Alfasi <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
 drivers/infiniband/core/device.c  |  1 +
 drivers/infiniband/core/nldev.c   | 54 +++++++++++++++++++++++++++++--
 drivers/infiniband/hw/mlx5/main.c | 16 +++++++++
 include/rdma/ib_verbs.h           |  9 ++++++
 4 files changed, 78 insertions(+), 2 deletions(-)
drivers/infiniband/core/device.c +1 -0
index 99c4a55545cf..34a9e37c5c61 100644
+++ b/drivers/infiniband/core/device.c

drivers/infiniband/core/device.c void ib_set_device_ops(struct ib_device *dev, const struct ib_device_ops *ops) · line 2616

Quoted 7 lines

drivers/infiniband/core/device.c +1 -0

@@ -2610,6 +2610,7 @@ void ib_set_device_ops(struct ib_device *dev, const struct ib_device_ops *ops)
 	SET_DEVICE_OP(dev_ops, get_dma_mr);
 	SET_DEVICE_OP(dev_ops, get_hw_stats);
 	SET_DEVICE_OP(dev_ops, get_link_layer);
+	SET_DEVICE_OP(dev_ops, fill_odp_stats);
 	SET_DEVICE_OP(dev_ops, get_netdev);
 	SET_DEVICE_OP(dev_ops, get_port_immutable);
 	SET_DEVICE_OP(dev_ops, get_vector_affinity);

I'm now curious what motivated placing the line here, apparently randomly in a sorted list?

Quoted 6 lines

drivers/infiniband/core/device.c +6 -0

+static int fill_stat_mr_entry(struct sk_buff *msg, bool has_cap_net_admin,
+			      struct rdma_restrack_entry *res, uint32_t port)
+{
+	struct ib_mr *mr = container_of(res, struct ib_mr, res);
+	struct ib_device *dev = mr->pd->device;
+	struct ib_odp_counters odp_stats;

drivers/infiniband/core/device.c

Quoted 10 lines

drivers/infiniband/core/device.c +10 -0

+	struct nlattr *table_attr;
+
+	if (nla_put_u32(msg, RDMA_NLDEV_ATTR_RES_MRN, res->id))
+		goto err;
+
+	if (!dev->ops.fill_odp_stats)
+		return 0;
+
+	if (!dev->ops.fill_odp_stats(mr, &odp_stats))
+		return 0;

As Parav says this seems to be wrong for !ODP MRs. Can we even detect !ODP at this point?

drivers/infiniband/core/device.c

Quoted 9 lines

drivers/infiniband/core/device.c +9 -0

+
+	table_attr = nla_nest_start(msg,
+				    RDMA_NLDEV_ATTR_STAT_HWCOUNTERS);
+
+	if (!table_attr)
+		return -EMSGSIZE;
+
+	if (fill_stat_hwcounter_entry(msg, "page_faults",
+				      (u64)atomic64_read(&odp_stats.faults)))

Why the cast?

drivers/infiniband/core/device.c

Quoted 10 lines

drivers/infiniband/core/device.c +10 -0

+static bool mlx5_ib_fill_odp_stats(struct ib_mr *ibmr,
+				   struct ib_odp_counters *cnt)
+{
+	struct mlx5_ib_mr *mr = to_mmr(ibmr);
+
+	if (!is_odp_mr(mr))
+		return false;
+
+	memcpy(cnt, &to_ib_umem_odp(mr->umem)->odp_stats,
+	       sizeof(struct ib_odp_counters));

Can't memcpy atomic64, have to open code a copy using atomic64_read.

drivers/infiniband/core/device.c static const struct ib_device_ops mlx5_ib_dev_ops = { · line 6334

Quoted 5 lines

drivers/infiniband/core/device.c +1 -0

@@ -6316,6 +6331,7 @@ static const struct ib_device_ops mlx5_ib_dev_ops = {
 	.get_dev_fw_str = get_dev_fw_str,
 	.get_dma_mr = mlx5_ib_get_dma_mr,
 	.get_link_layer = mlx5_ib_port_link_layer,
+	.fill_odp_stats = mlx5_ib_fill_odp_stats,

Randomly again..

Jason

Leon Romanovsky

author

2019-09-09 10:00 UTC #12 lore raw

Re: [PATCH rdma-next v1 3/4] RDMA/nldev: Provide MR statistics

Quoted 22 lines · Jason Gunthorpe

Quoted 21 lines · Leon Romanovsky
From: Erez Alfasi <[email protected]>
Add RDMA nldev netlink interface for dumping MR
statistics information.
Output example:
ereza@dev~$: ./ibv_rc_pingpong -o -P -s 500000000
local address: LID 0x0001, QPN 0x00008a, PSN 0xf81096, GID ::
ereza@dev~$: rdma stat show mr
dev mlx5_0 mrn 2 page_faults 122071 page_invalidations 0 prefetched_pages 122071
Signed-off-by Erez Alfasi <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
 drivers/infiniband/core/device.c  |  1 +
 drivers/infiniband/core/nldev.c   | 54 +++++++++++++++++++++++++++++--
 drivers/infiniband/hw/mlx5/main.c | 16 +++++++++
 include/rdma/ib_verbs.h           |  9 ++++++
 4 files changed, 78 insertions(+), 2 deletions(-)

Quoted 14 lines

drivers/infiniband/core/device.c void ib_set_device_ops(struct ib_device *dev, const struct ib_device_ops *ops) · line 2616
Quoted 11 lines
drivers/infiniband/core/device.c +2 -0
index 99c4a55545cf..34a9e37c5c61 100644
+++ b/drivers/infiniband/core/device.c
@@ -2610,6 +2610,7 @@ void ib_set_device_ops(struct ib_device *dev, const struct ib_device_ops *ops)
 	SET_DEVICE_OP(dev_ops, get_dma_mr);
 	SET_DEVICE_OP(dev_ops, get_hw_stats);
 	SET_DEVICE_OP(dev_ops, get_link_layer);
+	SET_DEVICE_OP(dev_ops, fill_odp_stats);
 	SET_DEVICE_OP(dev_ops, get_netdev);
 	SET_DEVICE_OP(dev_ops, get_port_immutable);
 	SET_DEVICE_OP(dev_ops, get_vector_affinity);
I'm now curious what motivated placing the line here, apparently randomly in a sorted list?

desire to be different and express yourself?

Quoted 12 lines

drivers/infiniband/core/device.c +11 -0

+static int fill_stat_mr_entry(struct sk_buff *msg, bool has_cap_net_admin,
+			      struct rdma_restrack_entry *res, uint32_t port)
+{
+	struct ib_mr *mr = container_of(res, struct ib_mr, res);
+	struct ib_device *dev = mr->pd->device;
+	struct ib_odp_counters odp_stats;
+	struct nlattr *table_attr;
+
+	if (nla_put_u32(msg, RDMA_NLDEV_ATTR_RES_MRN, res->id))
+		goto err;
+

Quoted 8 lines

drivers/infiniband/core/device.c
Quoted 5 lines
drivers/infiniband/core/device.c +5 -0
+	if (!dev->ops.fill_odp_stats)
+		return 0;
+
+	if (!dev->ops.fill_odp_stats(mr, &odp_stats))
+		return 0;
As Parav says this seems to be wrong for !ODP MRs. Can we even detect !ODP at this point?

ODP is decided on UMEM level which you said should be driver property and it means that driver should distinguish between odp/not-odp.

Quoted 12 lines

drivers/infiniband/core/device.c

Quoted 10 lines

drivers/infiniband/core/device.c +9 -0

+
+	table_attr = nla_nest_start(msg,
+				    RDMA_NLDEV_ATTR_STAT_HWCOUNTERS);
+
+	if (!table_attr)
+		return -EMSGSIZE;
+
+	if (fill_stat_hwcounter_entry(msg, "page_faults",
+				      (u64)atomic64_read(&odp_stats.faults)))

Why the cast?

atomic64_read returns s64 and not u64, I didn't see need to investigate if s64 == u64 in all architectures.

Quoted 14 lines

drivers/infiniband/core/device.c

Quoted 12 lines

drivers/infiniband/core/device.c +10 -0

+static bool mlx5_ib_fill_odp_stats(struct ib_mr *ibmr,
+				   struct ib_odp_counters *cnt)
+{
+	struct mlx5_ib_mr *mr = to_mmr(ibmr);
+
+	if (!is_odp_mr(mr))
+		return false;
+
+	memcpy(cnt, &to_ib_umem_odp(mr->umem)->odp_stats,
+	       sizeof(struct ib_odp_counters));

Can't memcpy atomic64, have to open code a copy using atomic64_read.

Right

Quoted 10 lines

drivers/infiniband/core/device.c static const struct ib_device_ops mlx5_ib_dev_ops = { · line 6334
Quoted 6 lines
drivers/infiniband/core/device.c +1 -0
@@ -6316,6 +6331,7 @@ static const struct ib_device_ops mlx5_ib_dev_ops = {
 	.get_dev_fw_str = get_dev_fw_str,
 	.get_dma_mr = mlx5_ib_get_dma_mr,
 	.get_link_layer = mlx5_ib_port_link_layer,
+	.fill_odp_stats = mlx5_ib_fill_odp_stats,
Randomly again..
Jason

Leon Romanovsky

author

2019-08-30 08:16 UTC #13 lore raw

PATCH 4/4 RDMA/mlx5: Return ODP type per MR

In reply to Leon Romanovsky

From: Erez Alfasi <[email protected]>

Provide an ODP explicit/implicit type as part
of 'rdma -dd resource show mr' dump.

For example:
~$: rdma -dd resource show mr
dev mlx5_0 mrn 1 rkey 0xa99a lkey 0xa99a mrlen 50000000 pdn 9 pid 7372 comm ibv_rc_pingpong drv_odp explicit

For non-ODP MRs, we won't print "drv_odp ..." at all.

Signed-off-by Erez Alfasi <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/nldev.c       | 13 ++++++++
 drivers/infiniband/hw/mlx5/Makefile   |  2 +-
 drivers/infiniband/hw/mlx5/main.c     |  1 +
 drivers/infiniband/hw/mlx5/mlx5_ib.h  |  2 ++
 drivers/infiniband/hw/mlx5/restrack.c | 48 +++++++++++++++++++++++++++
 include/rdma/restrack.h               |  3 ++
 6 files changed, 68 insertions(+), 1 deletion(-)
 create mode 100644 drivers/infiniband/hw/mlx5/restrack.c

drivers/infiniband/core/nldev.c +13 -0

index 47fee3d68cb9..82ed8e339d8f 100644
@@ -181,6 +181,19 @@ static int _rdma_nl_put_driver_u64(struct sk_buff *msg, const char *name,
 	return 0;
 }
 
+int rdma_nl_put_driver_string(struct sk_buff *msg, const char *name,
+			      const char *str)
+{
+	if (put_driver_name_print_type(msg, name,
+				       RDMA_NLDEV_PRINT_TYPE_UNSPEC))
+		return -EMSGSIZE;
+	if (nla_put_string(msg, RDMA_NLDEV_ATTR_DRIVER_STRING, str))
+		return -EMSGSIZE;
+
+	return 0;
+}
+EXPORT_SYMBOL(rdma_nl_put_driver_string);
+
 int rdma_nl_put_driver_u32(struct sk_buff *msg, const char *name, u32 value)
 {
 	return _rdma_nl_put_driver_u32(msg, name, RDMA_NLDEV_PRINT_TYPE_UNSPEC,

drivers/infiniband/hw/mlx5/Makefile +1 -1

index 9924be8384d8..d0a043ccbe58 100644
@@ -3,7 +3,7 @@ obj-$(CONFIG_MLX5_INFINIBAND)	+= mlx5_ib.o
 
 mlx5_ib-y :=	main.o cq.o doorbell.o qp.o mem.o srq_cmd.o \
 		srq.o mr.o ah.o mad.o gsi.o ib_virt.o cmd.o \
-		cong.o
+		cong.o restrack.o
 mlx5_ib-$(CONFIG_INFINIBAND_ON_DEMAND_PAGING) += odp.o
 mlx5_ib-$(CONFIG_MLX5_ESWITCH) += ib_rep.o
 mlx5_ib-$(CONFIG_INFINIBAND_USER_ACCESS) += devx.o

drivers/infiniband/hw/mlx5/main.c +1 -0

index 05095fda03cc..084145757549 100644
@@ -6328,6 +6328,7 @@ static const struct ib_device_ops mlx5_ib_dev_ops = {
 	.disassociate_ucontext = mlx5_ib_disassociate_ucontext,
 	.drain_rq = mlx5_ib_drain_rq,
 	.drain_sq = mlx5_ib_drain_sq,
+	.fill_res_entry = mlx5_ib_fill_res_entry,
 	.get_dev_fw_str = get_dev_fw_str,
 	.get_dma_mr = mlx5_ib_get_dma_mr,
 	.get_link_layer = mlx5_ib_port_link_layer,

drivers/infiniband/hw/mlx5/mlx5_ib.h +2 -0

index 6abfbf3a69b7..c67ede31d7dc 100644
@@ -1334,6 +1334,8 @@ struct mlx5_core_dev *mlx5_ib_get_native_port_mdev(struct mlx5_ib_dev *dev,
 						   u8 *native_port_num);
 void mlx5_ib_put_native_port_mdev(struct mlx5_ib_dev *dev,
 				  u8 port_num);
+int mlx5_ib_fill_res_entry(struct sk_buff *msg,
+			   struct rdma_restrack_entry *res);
 
 #if IS_ENABLED(CONFIG_INFINIBAND_USER_ACCESS)
 int mlx5_ib_devx_create(struct mlx5_ib_dev *dev, bool is_user);

drivers/infiniband/hw/mlx5/restrack.c new file +48 -0

48 lines not on this page · raw message

include/rdma/restrack.h +3 -0

index b0fc6b26bdf5..cb779b311703 100644
@@ -157,6 +157,9 @@ int rdma_nl_put_driver_u32_hex(struct sk_buff *msg, const char *name,
 int rdma_nl_put_driver_u64(struct sk_buff *msg, const char *name, u64 value);
 int rdma_nl_put_driver_u64_hex(struct sk_buff *msg, const char *name,
 			       u64 value);
+int rdma_nl_put_driver_string(struct sk_buff *msg, const char *name,
+			      const char *str);
+
 struct rdma_restrack_entry *rdma_restrack_get_byid(struct ib_device *dev,
 						   enum rdma_restrack_type type,
 						   u32 id);

Signature · 1 line

2.20.1

Jason Gunthorpe

2019-09-09 08:54 UTC #14 lore raw

Re: [PATCH rdma-next v1 4/4] RDMA/mlx5: Return ODP type per MR

Quoted 19 lines · Leon Romanovsky

drivers/infiniband/hw/mlx5/restrack.c new file +19 -0

+static int fill_res_mr_entry(struct sk_buff *msg,
+			     struct rdma_restrack_entry *res)
+{
+	struct ib_mr *ibmr = container_of(res, struct ib_mr, res);
+	struct mlx5_ib_mr *mr = to_mmr(ibmr);
+	struct nlattr *table_attr;
+
+	if (!is_odp_mr(mr))
+		return 0;
+
+	table_attr = nla_nest_start(msg, RDMA_NLDEV_ATTR_DRIVER);
+	if (!table_attr)
+		goto err;
+
+	if (to_ib_umem_odp(mr->umem)->is_implicit_odp) {
+		if (rdma_nl_put_driver_string(msg, "odp", "implicit"))
+			goto err;
+	} else {
+		if (rdma_nl_put_driver_string(msg, "odp", "explicit"))

drivers/infiniband/hw/mlx5/restrack.c line 39

Quoted 10 lines

drivers/infiniband/hw/mlx5/restrack.c new file +10 -0

+			goto err;
+	}
+
+	nla_nest_end(msg, table_attr);
+	return 0;
+
+err:
+	nla_nest_cancel(msg, table_attr);
+	return -EMSGSIZE;
+}

If we are having a custom fill function why not fill the ODP stats here instead of adding that callback and memcopies/etc

rdma_nl_put_odp_stats(...)

Jason

Leon Romanovsky

author

2019-09-09 10:01 UTC #15 lore raw

Re: [PATCH rdma-next v1 4/4] RDMA/mlx5: Return ODP type per MR

Quoted 26 lines · Jason Gunthorpe

Quoted 24 lines · Leon Romanovsky

drivers/infiniband/hw/mlx5/restrack.c new file +24 -0

+static int fill_res_mr_entry(struct sk_buff *msg,
+			     struct rdma_restrack_entry *res)
+{
+	struct ib_mr *ibmr = container_of(res, struct ib_mr, res);
+	struct mlx5_ib_mr *mr = to_mmr(ibmr);
+	struct nlattr *table_attr;
+
+	if (!is_odp_mr(mr))
+		return 0;
+
+	table_attr = nla_nest_start(msg, RDMA_NLDEV_ATTR_DRIVER);
+	if (!table_attr)
+		goto err;
+
+	if (to_ib_umem_odp(mr->umem)->is_implicit_odp) {
+		if (rdma_nl_put_driver_string(msg, "odp", "implicit"))
+			goto err;
+	} else {
+		if (rdma_nl_put_driver_string(msg, "odp", "explicit"))
+			goto err;
+	}
+
+	nla_nest_end(msg, table_attr);
+	return 0;

Quoted 8 lines

drivers/infiniband/hw/mlx5/restrack.c line 39
Quoted 5 lines
drivers/infiniband/hw/mlx5/restrack.c new file +5 -0
+
+err:
+	nla_nest_cancel(msg, table_attr);
+	return -EMSGSIZE;
+}
If we are having a custom fill function why not fill the ODP stats here instead of adding that callback and memcopies/etc

It makes sense.

Thanks

Quoted 4 lines

rdma_nl_put_odp_stats(...)
Jason