Re: [PATCH 1/1] net/mlx5: add dynamic logging for mlx5_dump_err_cqe

From: Aru
Date: Thu Oct 20 2022 - 04:25:23 EST

On 10/18/22 12:47 AM, Leon Romanovsky wrote:
On Fri, Oct 14, 2022 at 12:12:36PM -0700, Aru wrote:
Hi Leon,

Thank you for reviewing the patch.

The method you mentioned disables the dump permanently for the kernel.
We thought vendor might have enabled it for their consumption when needed.
Hence we made it dynamic, so that it can be enabled/disabled at run time.

Especially, in a production environment, having the option to turn this log
on/off
at runtime will be helpful.
While you are interested on/off this specific warning, your change will
cause "to hide" all syndromes as it is unlikely that anyone runs in
production with debug prints.

- mlx5_ib_warn(dev, "dump error cqe\n");
+ mlx5_ib_dbg(dev, "dump error cqe\n");

Something like this will do the trick without interrupting to the others.

diff --git a/drivers/infiniband/hw/mlx5/cq.c b/drivers/infiniband/hw/mlx5/cq.c
index 457f57b088c6..966206085eb3 100644
--- a/drivers/infiniband/hw/mlx5/cq.c
+++ b/drivers/infiniband/hw/mlx5/cq.c
@@ -267,10 +267,29 @@ static void handle_responder(struct ib_wc *wc, struct mlx5_cqe64 *cqe,
wc->wc_flags |= IB_WC_WITH_NETWORK_HDR_TYPE;
}
-static void dump_cqe(struct mlx5_ib_dev *dev, struct mlx5_err_cqe *cqe)
+static void dump_cqe(struct mlx5_ib_dev *dev, struct mlx5_err_cqe *cqe,
+ struct ib_wc *wc, int dump)
{
- mlx5_ib_warn(dev, "dump error cqe\n");
- mlx5_dump_err_cqe(dev->mdev, cqe);
+ const char *level;
+
+ if (!dump)
+ return;
+
+ mlx5_ib_warn(dev, "WC error: %d, Message: %s\n", wc->status,
+ ib_wc_status_msg(wc->status));
+
+ if (dump == 1) {
+ mlx5_ib_warn(dev, "dump error cqe\n");
+ level = KERN_WARNING;
+ }
+
+ if (dump == 2) {
+ mlx5_ib_dbg(dev, "dump error cqe\n");
+ level = KERN_DEBUG;
+ }
+
+ print_hex_dump(level, "", DUMP_PREFIX_OFFSET, 16, 1, cqe, sizeof(*cqe),
+ false);
}
Hi Leon,

Thank you for the reply and your suggested method to handle this debug logging.

We set 'dump=2' for the syndromes applicable to our scenario:  MLX5_CQE_SYNDROME_REMOTE_ACCESS_ERR,
MLX5_CQE_SYNDROME_REMOTE_OP_ERR and MLX5_CQE_SYNDROME_LOCAL_PROT_ERR.
We verified this code change and by default, the dump_cqe is not printed to syslog until
the level is changed to KERN_DEBUG level. This works as expected.

I will send out another email with the patch using your method.

Is it fine with you If I add your name in the 'suggested-by' field in the new patch?

Thanks
Aru

static void mlx5_handle_error_cqe(struct mlx5_ib_dev *dev,
@@ -300,6 +319,7 @@ static void mlx5_handle_error_cqe(struct mlx5_ib_dev *dev,
wc->status = IB_WC_BAD_RESP_ERR;
break;
case MLX5_CQE_SYNDROME_LOCAL_ACCESS_ERR:
+ dump = 2;
wc->status = IB_WC_LOC_ACCESS_ERR;
break;
case MLX5_CQE_SYNDROME_REMOTE_INVAL_REQ_ERR:
@@ -328,11 +348,7 @@ static void mlx5_handle_error_cqe(struct mlx5_ib_dev *dev,
}
wc->vendor_err = cqe->vendor_err_synd;
- if (dump) {
- mlx5_ib_warn(dev, "WC error: %d, Message: %s\n", wc->status,
- ib_wc_status_msg(wc->status));
- dump_cqe(dev, cqe);
- }
+ dump_cqe(dev, cqe, wc, dump);
}
static void handle_atomics(struct mlx5_ib_qp *qp, struct mlx5_cqe64 *cqe64,

Feel free to share your thoughts.
And please don't top-post.

Thanks
Thanks,
Aru

On 10/13/22 3:43 AM, Leon Romanovsky wrote:
On Wed, Oct 12, 2022 at 04:52:52PM -0700, Aru Kolappan wrote:
From: Arumugam Kolappan <aru.kolappan@xxxxxxxxxx>

Presently, mlx5 driver dumps error CQE by default for few syndromes. Some
syndromes are expected due to application behavior[Ex: REMOTE_ACCESS_ERR
for revoking rkey before RDMA operation is completed]. There is no option
to disable the log if the application decided to do so. This patch
converts the log into dynamic print and by default, this debug print is
disabled. Users can enable/disable this logging at runtime if needed.

Suggested-by: Manjunath Patil <manjunath.b.patil@xxxxxxxxxx>
Signed-off-by: Arumugam Kolappan <aru.kolappan@xxxxxxxxxx>
---
drivers/infiniband/hw/mlx5/cq.c | 2 +-
include/linux/mlx5/cq.h | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/hw/mlx5/cq.c b/drivers/infiniband/hw/mlx5/cq.c
index be189e0..890cdc3 100644
--- a/drivers/infiniband/hw/mlx5/cq.c
+++ b/drivers/infiniband/hw/mlx5/cq.c
@@ -269,7 +269,7 @@ static void handle_responder(struct ib_wc *wc, struct mlx5_cqe64 *cqe,
static void dump_cqe(struct mlx5_ib_dev *dev, struct mlx5_err_cqe *cqe)
{
- mlx5_ib_warn(dev, "dump error cqe\n");
+ mlx5_ib_dbg(dev, "dump error cqe\n");
This path should be handled in switch<->case of mlx5_handle_error_cqe()
by skipping dump_cqe for MLX5_CQE_SYNDROME_REMOTE_ACCESS_ERR.

diff --git a/drivers/infiniband/hw/mlx5/cq.c b/drivers/infiniband/hw/mlx5/cq.c
index be189e0525de..2d75c3071a1e 100644
--- a/drivers/infiniband/hw/mlx5/cq.c
+++ b/drivers/infiniband/hw/mlx5/cq.c
@@ -306,6 +306,7 @@ static void mlx5_handle_error_cqe(struct mlx5_ib_dev *dev,
wc->status = IB_WC_REM_INV_REQ_ERR;
break;
case MLX5_CQE_SYNDROME_REMOTE_ACCESS_ERR:
+ dump = 0;
wc->status = IB_WC_REM_ACCESS_ERR;
break;
case MLX5_CQE_SYNDROME_REMOTE_OP_ERR:

Thanks