* [PATCH for-next 0/2] Ratelimited ibdev printk functions
@ 2019-07-30 15:18 Gal Pressman
2019-07-30 15:18 ` [PATCH for-next 1/2] RDMA/core: Introduce ratelimited " Gal Pressman
2019-07-30 15:18 ` [PATCH for-next 2/2] RDMA/efa: Rate limit admin queue error prints Gal Pressman
0 siblings, 2 replies; 14+ messages in thread
From: Gal Pressman @ 2019-07-30 15:18 UTC (permalink / raw)
To: Jason Gunthorpe, Doug Ledford; +Cc: linux-rdma, Gal Pressman
Hello,
This implements ratelimited ibdev printk functions, similar to their
dev_*_ratelimited counterparts.
I've submitted the first patch before [1] without usage, I'm now adding
the corresponding EFA patch as well.
cxgb4, hfi1, mlx4, vmw_pvrdma, rdmavt and rxe could also benefit from
this if they move to ibvdev prints:
$ git grep ratelimited -- drivers/infiniband/[hs]w
drivers/infiniband/hw/cxgb4/mem.c: pr_warn_ratelimited("%s: dma map failure (non fatal)\n",
drivers/infiniband/hw/cxgb4/resource.c: pr_warn_ratelimited("%s: Out of RQT memory\n",
drivers/infiniband/hw/hfi1/chip.c: dd_dev_info_ratelimited(dd, "DCC Error: fmconfig error: %s\n",
drivers/infiniband/hw/hfi1/chip.c: dd_dev_info_ratelimited(dd, "DCC Error: PortRcv error: %s\n"
drivers/infiniband/hw/hfi1/chip.c: dd_dev_info_ratelimited(dd, "8051 access to LCB blocked\n");
drivers/infiniband/hw/hfi1/chip.c: dd_dev_info_ratelimited(dd, "host access to LCB blocked\n");
drivers/infiniband/hw/hfi1/chip.c: dd_dev_info_ratelimited(dd, "DCC Error: %s\n",
drivers/infiniband/hw/hfi1/chip.c: dd_dev_info_ratelimited(dd, "%s: PortErrorAction bounce\n",
drivers/infiniband/hw/hfi1/chip.c: dd_dev_info_ratelimited(dd, "SDMA engine %u interrupt, but no status bits set\n",
drivers/infiniband/hw/hfi1/hfi.h:#define dd_dev_err_ratelimited(dd, fmt, ...) \
drivers/infiniband/hw/hfi1/hfi.h: dev_err_ratelimited(&(dd)->pcidev->dev, "%s: " fmt, \
drivers/infiniband/hw/hfi1/hfi.h:#define dd_dev_warn_ratelimited(dd, fmt, ...) \
drivers/infiniband/hw/hfi1/hfi.h: dev_warn_ratelimited(&(dd)->pcidev->dev, "%s: " fmt, \
drivers/infiniband/hw/hfi1/hfi.h:#define dd_dev_info_ratelimited(dd, fmt, ...) \
drivers/infiniband/hw/hfi1/hfi.h: dev_info_ratelimited(&(dd)->pcidev->dev, "%s: " fmt, \
drivers/infiniband/hw/hfi1/mad.c: pr_err_ratelimited("hfi1: Invalid trap 0x%0x dropped. Total dropped: %d\n",
drivers/infiniband/hw/hfi1/mad.c: pr_warn_ratelimited("hfi1: Maximum trap limit reached for 0x%0x traps\n",
drivers/infiniband/hw/mlx4/qp.c: pr_warn_ratelimited("Unexpected event type %d on WQ 0x%06x. Events are not supported for WQs\n",
drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c: dev_warn_ratelimited(&dev->pdev->dev,
drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c: dev_warn_ratelimited(&dev->pdev->dev,
drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c: dev_warn_ratelimited(&dev->pdev->dev,
drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c: dev_warn_ratelimited(&dev->pdev->dev,
drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c: dev_warn_ratelimited(&dev->pdev->dev,
drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c: dev_warn_ratelimited(&dev->pdev->dev,
drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c: dev_warn_ratelimited(&dev->pdev->dev,
drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c: dev_warn_ratelimited(&dev->pdev->dev,
drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c: dev_warn_ratelimited(&dev->pdev->dev,
drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c: dev_warn_ratelimited(&dev->pdev->dev,
drivers/infiniband/sw/rdmavt/cq.c: rvt_pr_err_ratelimited(rdi, "CQ is full!\n");
drivers/infiniband/sw/rdmavt/vt.h:#define rvt_pr_err_ratelimited(rdi, fmt, ...) \
drivers/infiniband/sw/rdmavt/vt.h: __rvt_pr_err_ratelimited((rdi)->driver_f.get_pci_dev(rdi), \
drivers/infiniband/sw/rdmavt/vt.h:#define __rvt_pr_err_ratelimited(pdev, name, fmt, ...) \
drivers/infiniband/sw/rdmavt/vt.h: dev_err_ratelimited(&(pdev)->dev, "%s: " fmt, name, ##__VA_ARGS__)
drivers/infiniband/sw/rxe/rxe.h: pr_warn_ratelimited("failed crc calculation, err: %d\n", err);
drivers/infiniband/sw/rxe/rxe_net.c: pr_err_ratelimited("no route to %pI4\n", &daddr->s_addr);
drivers/infiniband/sw/rxe/rxe_net.c: pr_err_ratelimited("no route to %pI6\n", daddr);
drivers/infiniband/sw/rxe/rxe_recv.c: pr_warn_ratelimited("bad qp type\n");
drivers/infiniband/sw/rxe/rxe_recv.c: pr_warn_ratelimited("bad qp type\n");
drivers/infiniband/sw/rxe/rxe_recv.c: pr_warn_ratelimited("bad qp type\n");
drivers/infiniband/sw/rxe/rxe_recv.c: pr_warn_ratelimited("unsupported qp type\n");
drivers/infiniband/sw/rxe/rxe_recv.c: pr_warn_ratelimited("bad pkey = 0x%x\n", pkey);
drivers/infiniband/sw/rxe/rxe_recv.c: pr_warn_ratelimited("bad pkey = 0x%0x\n", pkey);
drivers/infiniband/sw/rxe/rxe_recv.c: pr_warn_ratelimited("bad qkey, got 0x%x expected 0x%x for qpn 0x%x\n",
drivers/infiniband/sw/rxe/rxe_recv.c: pr_warn_ratelimited("port %d != qp port %d\n",
drivers/infiniband/sw/rxe/rxe_recv.c: pr_warn_ratelimited("dst addr %pI4 != qp source addr %pI4\n",
drivers/infiniband/sw/rxe/rxe_recv.c: pr_warn_ratelimited("source addr %pI4 != qp dst addr %pI4\n",
drivers/infiniband/sw/rxe/rxe_recv.c: pr_warn_ratelimited("dst addr %pI6 != qp source addr %pI6\n",
drivers/infiniband/sw/rxe/rxe_recv.c: pr_warn_ratelimited("source addr %pI6 != qp dst addr %pI6\n",
drivers/infiniband/sw/rxe/rxe_recv.c: pr_warn_ratelimited("bad tver\n");
drivers/infiniband/sw/rxe/rxe_recv.c: pr_warn_ratelimited("no qp matches qpn 0x%x\n", qpn);
drivers/infiniband/sw/rxe/rxe_recv.c: pr_warn_ratelimited("no grh for mcast qpn\n");
drivers/infiniband/sw/rxe/rxe_recv.c: pr_warn_ratelimited("failed matching dgid\n");
drivers/infiniband/sw/rxe/rxe_recv.c: pr_warn_ratelimited("bad ICRC from %pI6c\n",
drivers/infiniband/sw/rxe/rxe_recv.c: pr_warn_ratelimited("bad ICRC from %pI4\n",
drivers/infiniband/sw/rxe/rxe_recv.c: pr_warn_ratelimited("bad ICRC from unknown\n");
drivers/infiniband/sw/rxe/rxe_resp.c: pr_err_ratelimited("Failed sending ack\n");
drivers/infiniband/sw/rxe/rxe_resp.c: pr_err_ratelimited("Failed sending ack\n");
[1] https://patchwork.kernel.org/patch/10926991/
Thanks,
Gal
Gal Pressman (2):
RDMA/core: Introduce ratelimited ibdev printk functions
RDMA/efa: Rate limit admin queue error prints
drivers/infiniband/hw/efa/efa_com.c | 70 ++++++------
drivers/infiniband/hw/efa/efa_com_cmd.c | 136 ++++++++++++++----------
include/rdma/ib_verbs.h | 51 +++++++++
3 files changed, 171 insertions(+), 86 deletions(-)
--
2.22.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH for-next 1/2] RDMA/core: Introduce ratelimited ibdev printk functions
2019-07-30 15:18 [PATCH for-next 0/2] Ratelimited ibdev printk functions Gal Pressman
@ 2019-07-30 15:18 ` Gal Pressman
2019-07-30 15:41 ` Leon Romanovsky
2019-07-30 15:18 ` [PATCH for-next 2/2] RDMA/efa: Rate limit admin queue error prints Gal Pressman
1 sibling, 1 reply; 14+ messages in thread
From: Gal Pressman @ 2019-07-30 15:18 UTC (permalink / raw)
To: Jason Gunthorpe, Doug Ledford; +Cc: linux-rdma, Gal Pressman
Add ratelimited helpers to the ibdev_* printk functions.
Implementation inspired by counterpart dev_*_ratelimited functions.
Signed-off-by: Gal Pressman <galpress@amazon.com>
---
include/rdma/ib_verbs.h | 51 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 51 insertions(+)
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index c5f8a9f17063..356e6a105366 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -107,6 +107,57 @@ static inline
void ibdev_dbg(const struct ib_device *ibdev, const char *format, ...) {}
#endif
+#define ibdev_level_ratelimited(ibdev_level, ibdev, fmt, ...) \
+do { \
+ static DEFINE_RATELIMIT_STATE(_rs, \
+ DEFAULT_RATELIMIT_INTERVAL, \
+ DEFAULT_RATELIMIT_BURST); \
+ if (__ratelimit(&_rs)) \
+ ibdev_level(ibdev, fmt, ##__VA_ARGS__); \
+} while (0)
+
+#define ibdev_emerg_ratelimited(ibdev, fmt, ...) \
+ ibdev_level_ratelimited(ibdev_emerg, ibdev, fmt, ##__VA_ARGS__)
+#define ibdev_alert_ratelimited(ibdev, fmt, ...) \
+ ibdev_level_ratelimited(ibdev_alert, ibdev, fmt, ##__VA_ARGS__)
+#define ibdev_crit_ratelimited(ibdev, fmt, ...) \
+ ibdev_level_ratelimited(ibdev_crit, ibdev, fmt, ##__VA_ARGS__)
+#define ibdev_err_ratelimited(ibdev, fmt, ...) \
+ ibdev_level_ratelimited(ibdev_err, ibdev, fmt, ##__VA_ARGS__)
+#define ibdev_warn_ratelimited(ibdev, fmt, ...) \
+ ibdev_level_ratelimited(ibdev_warn, ibdev, fmt, ##__VA_ARGS__)
+#define ibdev_notice_ratelimited(ibdev, fmt, ...) \
+ ibdev_level_ratelimited(ibdev_notice, ibdev, fmt, ##__VA_ARGS__)
+#define ibdev_info_ratelimited(ibdev, fmt, ...) \
+ ibdev_level_ratelimited(ibdev_info, ibdev, fmt, ##__VA_ARGS__)
+
+#if defined(CONFIG_DYNAMIC_DEBUG)
+/* descriptor check is first to prevent flooding with "callbacks suppressed" */
+#define ibdev_dbg_ratelimited(ibdev, fmt, ...) \
+do { \
+ static DEFINE_RATELIMIT_STATE(_rs, \
+ DEFAULT_RATELIMIT_INTERVAL, \
+ DEFAULT_RATELIMIT_BURST); \
+ DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt); \
+ if (DYNAMIC_DEBUG_BRANCH(descriptor) && __ratelimit(&_rs)) \
+ __dynamic_ibdev_dbg(&descriptor, ibdev, fmt, \
+ ##__VA_ARGS__); \
+} while (0)
+#elif defined(DEBUG)
+#define ibdev_dbg_ratelimited(ibdev, fmt, ...) \
+do { \
+ static DEFINE_RATELIMIT_STATE(_rs, \
+ DEFAULT_RATELIMIT_INTERVAL, \
+ DEFAULT_RATELIMIT_BURST); \
+ if (__ratelimit(&_rs)) \
+ ibdev_printk(KERN_DEBUG, ibdev, fmt, ##__VA_ARGS__); \
+} while (0)
+#else
+__printf(2, 3) __cold
+static inline
+void ibdev_dbg_ratelimited(const struct ib_device *ibdev, const char *format, ...) {}
+#endif
+
union ib_gid {
u8 raw[16];
struct {
--
2.22.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH for-next 2/2] RDMA/efa: Rate limit admin queue error prints
2019-07-30 15:18 [PATCH for-next 0/2] Ratelimited ibdev printk functions Gal Pressman
2019-07-30 15:18 ` [PATCH for-next 1/2] RDMA/core: Introduce ratelimited " Gal Pressman
@ 2019-07-30 15:18 ` Gal Pressman
1 sibling, 0 replies; 14+ messages in thread
From: Gal Pressman @ 2019-07-30 15:18 UTC (permalink / raw)
To: Jason Gunthorpe, Doug Ledford
Cc: linux-rdma, Gal Pressman, Firas JahJah, Yossi Leybovich
Admin queue error prints should never happen unless something wrong
happened to the device. However, in the unfortunate case that it does,
we should take extra care not to flood the log with error messages.
Reviewed-by: Firas JahJah <firasj@amazon.com>
Reviewed-by: Yossi Leybovich <sleybo@amazon.com>
Signed-off-by: Gal Pressman <galpress@amazon.com>
---
drivers/infiniband/hw/efa/efa_com.c | 70 ++++++------
drivers/infiniband/hw/efa/efa_com_cmd.c | 136 ++++++++++++++----------
2 files changed, 120 insertions(+), 86 deletions(-)
diff --git a/drivers/infiniband/hw/efa/efa_com.c b/drivers/infiniband/hw/efa/efa_com.c
index 2cb42484b0f8..3c412bc5b94f 100644
--- a/drivers/infiniband/hw/efa/efa_com.c
+++ b/drivers/infiniband/hw/efa/efa_com.c
@@ -109,17 +109,19 @@ static u32 efa_com_reg_read32(struct efa_com_dev *edev, u16 offset)
} while (time_is_after_jiffies(exp_time));
if (read_resp->req_id != mmio_read->seq_num) {
- ibdev_err(edev->efa_dev,
- "Reading register timed out. expected: req id[%u] offset[%#x] actual: req id[%u] offset[%#x]\n",
- mmio_read->seq_num, offset, read_resp->req_id,
- read_resp->reg_off);
+ ibdev_err_ratelimited(
+ edev->efa_dev,
+ "Reading register timed out. expected: req id[%u] offset[%#x] actual: req id[%u] offset[%#x]\n",
+ mmio_read->seq_num, offset, read_resp->req_id,
+ read_resp->reg_off);
err = EFA_MMIO_READ_INVALID;
goto out;
}
if (read_resp->reg_off != offset) {
- ibdev_err(edev->efa_dev,
- "Reading register failed: wrong offset provided\n");
+ ibdev_err_ratelimited(
+ edev->efa_dev,
+ "Reading register failed: wrong offset provided\n");
err = EFA_MMIO_READ_INVALID;
goto out;
}
@@ -293,9 +295,10 @@ static struct efa_comp_ctx *efa_com_get_comp_ctx(struct efa_com_admin_queue *aq,
u16 ctx_id = cmd_id & (aq->depth - 1);
if (aq->comp_ctx[ctx_id].occupied && capture) {
- ibdev_err(aq->efa_dev,
- "Completion context for command_id %#x is occupied\n",
- cmd_id);
+ ibdev_err_ratelimited(
+ aq->efa_dev,
+ "Completion context for command_id %#x is occupied\n",
+ cmd_id);
return NULL;
}
@@ -401,7 +404,7 @@ static struct efa_comp_ctx *efa_com_submit_admin_cmd(struct efa_com_admin_queue
spin_lock(&aq->sq.lock);
if (!test_bit(EFA_AQ_STATE_RUNNING_BIT, &aq->state)) {
- ibdev_err(aq->efa_dev, "Admin queue is closed\n");
+ ibdev_err_ratelimited(aq->efa_dev, "Admin queue is closed\n");
spin_unlock(&aq->sq.lock);
return ERR_PTR(-ENODEV);
}
@@ -519,8 +522,9 @@ static int efa_com_wait_and_process_admin_cq_polling(struct efa_comp_ctx *comp_c
break;
if (time_is_before_jiffies(timeout)) {
- ibdev_err(aq->efa_dev,
- "Wait for completion (polling) timeout\n");
+ ibdev_err_ratelimited(
+ aq->efa_dev,
+ "Wait for completion (polling) timeout\n");
/* EFA didn't have any completion */
atomic64_inc(&aq->stats.no_completion);
@@ -561,17 +565,19 @@ static int efa_com_wait_and_process_admin_cq_interrupts(struct efa_comp_ctx *com
atomic64_inc(&aq->stats.no_completion);
if (comp_ctx->status == EFA_CMD_COMPLETED)
- ibdev_err(aq->efa_dev,
- "The device sent a completion but the driver didn't receive any MSI-X interrupt for admin cmd %s(%d) status %d (ctx: 0x%p, sq producer: %d, sq consumer: %d, cq consumer: %d)\n",
- efa_com_cmd_str(comp_ctx->cmd_opcode),
- comp_ctx->cmd_opcode, comp_ctx->status,
- comp_ctx, aq->sq.pc, aq->sq.cc, aq->cq.cc);
+ ibdev_err_ratelimited(
+ aq->efa_dev,
+ "The device sent a completion but the driver didn't receive any MSI-X interrupt for admin cmd %s(%d) status %d (ctx: 0x%p, sq producer: %d, sq consumer: %d, cq consumer: %d)\n",
+ efa_com_cmd_str(comp_ctx->cmd_opcode),
+ comp_ctx->cmd_opcode, comp_ctx->status,
+ comp_ctx, aq->sq.pc, aq->sq.cc, aq->cq.cc);
else
- ibdev_err(aq->efa_dev,
- "The device didn't send any completion for admin cmd %s(%d) status %d (ctx 0x%p, sq producer: %d, sq consumer: %d, cq consumer: %d)\n",
- efa_com_cmd_str(comp_ctx->cmd_opcode),
- comp_ctx->cmd_opcode, comp_ctx->status,
- comp_ctx, aq->sq.pc, aq->sq.cc, aq->cq.cc);
+ ibdev_err_ratelimited(
+ aq->efa_dev,
+ "The device didn't send any completion for admin cmd %s(%d) status %d (ctx 0x%p, sq producer: %d, sq consumer: %d, cq consumer: %d)\n",
+ efa_com_cmd_str(comp_ctx->cmd_opcode),
+ comp_ctx->cmd_opcode, comp_ctx->status,
+ comp_ctx, aq->sq.pc, aq->sq.cc, aq->cq.cc);
clear_bit(EFA_AQ_STATE_RUNNING_BIT, &aq->state);
err = -ETIME;
@@ -633,10 +639,11 @@ int efa_com_cmd_exec(struct efa_com_admin_queue *aq,
cmd->aq_common_descriptor.opcode);
comp_ctx = efa_com_submit_admin_cmd(aq, cmd, cmd_size, comp, comp_size);
if (IS_ERR(comp_ctx)) {
- ibdev_err(aq->efa_dev,
- "Failed to submit command %s (opcode %u) err %ld\n",
- efa_com_cmd_str(cmd->aq_common_descriptor.opcode),
- cmd->aq_common_descriptor.opcode, PTR_ERR(comp_ctx));
+ ibdev_err_ratelimited(
+ aq->efa_dev,
+ "Failed to submit command %s (opcode %u) err %ld\n",
+ efa_com_cmd_str(cmd->aq_common_descriptor.opcode),
+ cmd->aq_common_descriptor.opcode, PTR_ERR(comp_ctx));
up(&aq->avail_cmds);
return PTR_ERR(comp_ctx);
@@ -644,11 +651,12 @@ int efa_com_cmd_exec(struct efa_com_admin_queue *aq,
err = efa_com_wait_and_process_admin_cq(comp_ctx, aq);
if (err)
- ibdev_err(aq->efa_dev,
- "Failed to process command %s (opcode %u) comp_status %d err %d\n",
- efa_com_cmd_str(cmd->aq_common_descriptor.opcode),
- cmd->aq_common_descriptor.opcode,
- comp_ctx->comp_status, err);
+ ibdev_err_ratelimited(
+ aq->efa_dev,
+ "Failed to process command %s (opcode %u) comp_status %d err %d\n",
+ efa_com_cmd_str(cmd->aq_common_descriptor.opcode),
+ cmd->aq_common_descriptor.opcode, comp_ctx->comp_status,
+ err);
up(&aq->avail_cmds);
diff --git a/drivers/infiniband/hw/efa/efa_com_cmd.c b/drivers/infiniband/hw/efa/efa_com_cmd.c
index 16b115df63e8..501dce89f275 100644
--- a/drivers/infiniband/hw/efa/efa_com_cmd.c
+++ b/drivers/infiniband/hw/efa/efa_com_cmd.c
@@ -44,7 +44,8 @@ int efa_com_create_qp(struct efa_com_dev *edev,
(struct efa_admin_acq_entry *)&cmd_completion,
sizeof(cmd_completion));
if (err) {
- ibdev_err(edev->efa_dev, "Failed to create qp [%d]\n", err);
+ ibdev_err_ratelimited(edev->efa_dev,
+ "Failed to create qp [%d]\n", err);
return err;
}
@@ -82,9 +83,10 @@ int efa_com_modify_qp(struct efa_com_dev *edev,
(struct efa_admin_acq_entry *)&resp,
sizeof(resp));
if (err) {
- ibdev_err(edev->efa_dev,
- "Failed to modify qp-%u modify_mask[%#x] [%d]\n",
- cmd.qp_handle, cmd.modify_mask, err);
+ ibdev_err_ratelimited(
+ edev->efa_dev,
+ "Failed to modify qp-%u modify_mask[%#x] [%d]\n",
+ cmd.qp_handle, cmd.modify_mask, err);
return err;
}
@@ -109,8 +111,9 @@ int efa_com_query_qp(struct efa_com_dev *edev,
(struct efa_admin_acq_entry *)&resp,
sizeof(resp));
if (err) {
- ibdev_err(edev->efa_dev, "Failed to query qp-%u [%d]\n",
- cmd.qp_handle, err);
+ ibdev_err_ratelimited(edev->efa_dev,
+ "Failed to query qp-%u [%d]\n",
+ cmd.qp_handle, err);
return err;
}
@@ -139,8 +142,9 @@ int efa_com_destroy_qp(struct efa_com_dev *edev,
(struct efa_admin_acq_entry *)&cmd_completion,
sizeof(cmd_completion));
if (err) {
- ibdev_err(edev->efa_dev, "Failed to destroy qp-%u [%d]\n",
- qp_cmd.qp_handle, err);
+ ibdev_err_ratelimited(edev->efa_dev,
+ "Failed to destroy qp-%u [%d]\n",
+ qp_cmd.qp_handle, err);
return err;
}
@@ -173,7 +177,8 @@ int efa_com_create_cq(struct efa_com_dev *edev,
(struct efa_admin_acq_entry *)&cmd_completion,
sizeof(cmd_completion));
if (err) {
- ibdev_err(edev->efa_dev, "Failed to create cq[%d]\n", err);
+ ibdev_err_ratelimited(edev->efa_dev,
+ "Failed to create cq[%d]\n", err);
return err;
}
@@ -201,8 +206,9 @@ int efa_com_destroy_cq(struct efa_com_dev *edev,
sizeof(destroy_resp));
if (err) {
- ibdev_err(edev->efa_dev, "Failed to destroy CQ-%u [%d]\n",
- params->cq_idx, err);
+ ibdev_err_ratelimited(edev->efa_dev,
+ "Failed to destroy CQ-%u [%d]\n",
+ params->cq_idx, err);
return err;
}
@@ -250,7 +256,8 @@ int efa_com_register_mr(struct efa_com_dev *edev,
(struct efa_admin_acq_entry *)&cmd_completion,
sizeof(cmd_completion));
if (err) {
- ibdev_err(edev->efa_dev, "Failed to register mr [%d]\n", err);
+ ibdev_err_ratelimited(edev->efa_dev,
+ "Failed to register mr [%d]\n", err);
return err;
}
@@ -277,9 +284,9 @@ int efa_com_dereg_mr(struct efa_com_dev *edev,
(struct efa_admin_acq_entry *)&cmd_completion,
sizeof(cmd_completion));
if (err) {
- ibdev_err(edev->efa_dev,
- "Failed to de-register mr(lkey-%u) [%d]\n",
- mr_cmd.l_key, err);
+ ibdev_err_ratelimited(edev->efa_dev,
+ "Failed to de-register mr(lkey-%u) [%d]\n",
+ mr_cmd.l_key, err);
return err;
}
@@ -306,8 +313,9 @@ int efa_com_create_ah(struct efa_com_dev *edev,
(struct efa_admin_acq_entry *)&cmd_completion,
sizeof(cmd_completion));
if (err) {
- ibdev_err(edev->efa_dev, "Failed to create ah for %pI6 [%d]\n",
- ah_cmd.dest_addr, err);
+ ibdev_err_ratelimited(edev->efa_dev,
+ "Failed to create ah for %pI6 [%d]\n",
+ ah_cmd.dest_addr, err);
return err;
}
@@ -334,8 +342,9 @@ int efa_com_destroy_ah(struct efa_com_dev *edev,
(struct efa_admin_acq_entry *)&cmd_completion,
sizeof(cmd_completion));
if (err) {
- ibdev_err(edev->efa_dev, "Failed to destroy ah-%d pd-%d [%d]\n",
- ah_cmd.ah, ah_cmd.pd, err);
+ ibdev_err_ratelimited(edev->efa_dev,
+ "Failed to destroy ah-%d pd-%d [%d]\n",
+ ah_cmd.ah, ah_cmd.pd, err);
return err;
}
@@ -367,8 +376,9 @@ static int efa_com_get_feature_ex(struct efa_com_dev *edev,
int err;
if (!efa_com_check_supported_feature_id(edev, feature_id)) {
- ibdev_err(edev->efa_dev, "Feature %d isn't supported\n",
- feature_id);
+ ibdev_err_ratelimited(edev->efa_dev,
+ "Feature %d isn't supported\n",
+ feature_id);
return -EOPNOTSUPP;
}
@@ -396,9 +406,10 @@ static int efa_com_get_feature_ex(struct efa_com_dev *edev,
sizeof(*get_resp));
if (err) {
- ibdev_err(edev->efa_dev,
- "Failed to submit get_feature command %d [%d]\n",
- feature_id, err);
+ ibdev_err_ratelimited(
+ edev->efa_dev,
+ "Failed to submit get_feature command %d [%d]\n",
+ feature_id, err);
return err;
}
@@ -421,8 +432,9 @@ int efa_com_get_network_attr(struct efa_com_dev *edev,
err = efa_com_get_feature(edev, &resp,
EFA_ADMIN_NETWORK_ATTR);
if (err) {
- ibdev_err(edev->efa_dev,
- "Failed to get network attributes %d\n", err);
+ ibdev_err_ratelimited(edev->efa_dev,
+ "Failed to get network attributes %d\n",
+ err);
return err;
}
@@ -441,8 +453,9 @@ int efa_com_get_device_attr(struct efa_com_dev *edev,
err = efa_com_get_feature(edev, &resp, EFA_ADMIN_DEVICE_ATTR);
if (err) {
- ibdev_err(edev->efa_dev, "Failed to get device attributes %d\n",
- err);
+ ibdev_err_ratelimited(edev->efa_dev,
+ "Failed to get device attributes %d\n",
+ err);
return err;
}
@@ -456,9 +469,10 @@ int efa_com_get_device_attr(struct efa_com_dev *edev,
result->db_bar = resp.u.device_attr.db_bar;
if (result->admin_api_version < 1) {
- ibdev_err(edev->efa_dev,
- "Failed to get device attr api version [%u < 1]\n",
- result->admin_api_version);
+ ibdev_err_ratelimited(
+ edev->efa_dev,
+ "Failed to get device attr api version [%u < 1]\n",
+ result->admin_api_version);
return -EINVAL;
}
@@ -466,8 +480,9 @@ int efa_com_get_device_attr(struct efa_com_dev *edev,
err = efa_com_get_feature(edev, &resp,
EFA_ADMIN_QUEUE_ATTR);
if (err) {
- ibdev_err(edev->efa_dev,
- "Failed to get network attributes %d\n", err);
+ ibdev_err_ratelimited(edev->efa_dev,
+ "Failed to get network attributes %d\n",
+ err);
return err;
}
@@ -497,7 +512,8 @@ int efa_com_get_hw_hints(struct efa_com_dev *edev,
err = efa_com_get_feature(edev, &resp, EFA_ADMIN_HW_HINTS);
if (err) {
- ibdev_err(edev->efa_dev, "Failed to get hw hints %d\n", err);
+ ibdev_err_ratelimited(edev->efa_dev,
+ "Failed to get hw hints %d\n", err);
return err;
}
@@ -520,8 +536,9 @@ static int efa_com_set_feature_ex(struct efa_com_dev *edev,
int err;
if (!efa_com_check_supported_feature_id(edev, feature_id)) {
- ibdev_err(edev->efa_dev, "Feature %d isn't supported\n",
- feature_id);
+ ibdev_err_ratelimited(edev->efa_dev,
+ "Feature %d isn't supported\n",
+ feature_id);
return -EOPNOTSUPP;
}
@@ -545,9 +562,10 @@ static int efa_com_set_feature_ex(struct efa_com_dev *edev,
sizeof(*set_resp));
if (err) {
- ibdev_err(edev->efa_dev,
- "Failed to submit set_feature command %d error: %d\n",
- feature_id, err);
+ ibdev_err_ratelimited(
+ edev->efa_dev,
+ "Failed to submit set_feature command %d error: %d\n",
+ feature_id, err);
return err;
}
@@ -574,8 +592,9 @@ int efa_com_set_aenq_config(struct efa_com_dev *edev, u32 groups)
err = efa_com_get_feature(edev, &get_resp, EFA_ADMIN_AENQ_CONFIG);
if (err) {
- ibdev_err(edev->efa_dev, "Failed to get aenq attributes: %d\n",
- err);
+ ibdev_err_ratelimited(edev->efa_dev,
+ "Failed to get aenq attributes: %d\n",
+ err);
return err;
}
@@ -585,9 +604,10 @@ int efa_com_set_aenq_config(struct efa_com_dev *edev, u32 groups)
get_resp.u.aenq.enabled_groups);
if ((get_resp.u.aenq.supported_groups & groups) != groups) {
- ibdev_err(edev->efa_dev,
- "Trying to set unsupported aenq groups[%#x] supported[%#x]\n",
- groups, get_resp.u.aenq.supported_groups);
+ ibdev_err_ratelimited(
+ edev->efa_dev,
+ "Trying to set unsupported aenq groups[%#x] supported[%#x]\n",
+ groups, get_resp.u.aenq.supported_groups);
return -EOPNOTSUPP;
}
@@ -595,8 +615,9 @@ int efa_com_set_aenq_config(struct efa_com_dev *edev, u32 groups)
err = efa_com_set_feature(edev, &set_resp, &cmd,
EFA_ADMIN_AENQ_CONFIG);
if (err) {
- ibdev_err(edev->efa_dev, "Failed to set aenq attributes: %d\n",
- err);
+ ibdev_err_ratelimited(edev->efa_dev,
+ "Failed to set aenq attributes: %d\n",
+ err);
return err;
}
@@ -619,7 +640,8 @@ int efa_com_alloc_pd(struct efa_com_dev *edev,
(struct efa_admin_acq_entry *)&resp,
sizeof(resp));
if (err) {
- ibdev_err(edev->efa_dev, "Failed to allocate pd[%d]\n", err);
+ ibdev_err_ratelimited(edev->efa_dev,
+ "Failed to allocate pd[%d]\n", err);
return err;
}
@@ -645,8 +667,9 @@ int efa_com_dealloc_pd(struct efa_com_dev *edev,
(struct efa_admin_acq_entry *)&resp,
sizeof(resp));
if (err) {
- ibdev_err(edev->efa_dev, "Failed to deallocate pd-%u [%d]\n",
- cmd.pd, err);
+ ibdev_err_ratelimited(edev->efa_dev,
+ "Failed to deallocate pd-%u [%d]\n",
+ cmd.pd, err);
return err;
}
@@ -669,7 +692,8 @@ int efa_com_alloc_uar(struct efa_com_dev *edev,
(struct efa_admin_acq_entry *)&resp,
sizeof(resp));
if (err) {
- ibdev_err(edev->efa_dev, "Failed to allocate uar[%d]\n", err);
+ ibdev_err_ratelimited(edev->efa_dev,
+ "Failed to allocate uar[%d]\n", err);
return err;
}
@@ -695,8 +719,9 @@ int efa_com_dealloc_uar(struct efa_com_dev *edev,
(struct efa_admin_acq_entry *)&resp,
sizeof(resp));
if (err) {
- ibdev_err(edev->efa_dev, "Failed to deallocate uar-%u [%d]\n",
- cmd.uar, err);
+ ibdev_err_ratelimited(edev->efa_dev,
+ "Failed to deallocate uar-%u [%d]\n",
+ cmd.uar, err);
return err;
}
@@ -723,9 +748,10 @@ int efa_com_get_stats(struct efa_com_dev *edev,
(struct efa_admin_acq_entry *)&resp,
sizeof(resp));
if (err) {
- ibdev_err(edev->efa_dev,
- "Failed to get stats type-%u scope-%u.%u [%d]\n",
- cmd.type, cmd.scope, cmd.scope_modifier, err);
+ ibdev_err_ratelimited(
+ edev->efa_dev,
+ "Failed to get stats type-%u scope-%u.%u [%d]\n",
+ cmd.type, cmd.scope, cmd.scope_modifier, err);
return err;
}
--
2.22.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH for-next 1/2] RDMA/core: Introduce ratelimited ibdev printk functions
2019-07-30 15:18 ` [PATCH for-next 1/2] RDMA/core: Introduce ratelimited " Gal Pressman
@ 2019-07-30 15:41 ` Leon Romanovsky
2019-07-31 7:22 ` Gal Pressman
0 siblings, 1 reply; 14+ messages in thread
From: Leon Romanovsky @ 2019-07-30 15:41 UTC (permalink / raw)
To: Gal Pressman; +Cc: Jason Gunthorpe, Doug Ledford, linux-rdma
On Tue, Jul 30, 2019 at 06:18:33PM +0300, Gal Pressman wrote:
> Add ratelimited helpers to the ibdev_* printk functions.
> Implementation inspired by counterpart dev_*_ratelimited functions.
>
> Signed-off-by: Gal Pressman <galpress@amazon.com>
> ---
> include/rdma/ib_verbs.h | 51 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 51 insertions(+)
>
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index c5f8a9f17063..356e6a105366 100644
> --- a/include/rdma/ib_verbs.h
> +++ b/include/rdma/ib_verbs.h
> @@ -107,6 +107,57 @@ static inline
> void ibdev_dbg(const struct ib_device *ibdev, const char *format, ...) {}
> #endif
>
> +#define ibdev_level_ratelimited(ibdev_level, ibdev, fmt, ...) \
> +do { \
> + static DEFINE_RATELIMIT_STATE(_rs, \
> + DEFAULT_RATELIMIT_INTERVAL, \
> + DEFAULT_RATELIMIT_BURST); \
> + if (__ratelimit(&_rs)) \
> + ibdev_level(ibdev, fmt, ##__VA_ARGS__); \
> +} while (0)
> +
> +#define ibdev_emerg_ratelimited(ibdev, fmt, ...) \
> + ibdev_level_ratelimited(ibdev_emerg, ibdev, fmt, ##__VA_ARGS__)
> +#define ibdev_alert_ratelimited(ibdev, fmt, ...) \
> + ibdev_level_ratelimited(ibdev_alert, ibdev, fmt, ##__VA_ARGS__)
> +#define ibdev_crit_ratelimited(ibdev, fmt, ...) \
> + ibdev_level_ratelimited(ibdev_crit, ibdev, fmt, ##__VA_ARGS__)
> +#define ibdev_err_ratelimited(ibdev, fmt, ...) \
> + ibdev_level_ratelimited(ibdev_err, ibdev, fmt, ##__VA_ARGS__)
> +#define ibdev_warn_ratelimited(ibdev, fmt, ...) \
> + ibdev_level_ratelimited(ibdev_warn, ibdev, fmt, ##__VA_ARGS__)
> +#define ibdev_notice_ratelimited(ibdev, fmt, ...) \
> + ibdev_level_ratelimited(ibdev_notice, ibdev, fmt, ##__VA_ARGS__)
> +#define ibdev_info_ratelimited(ibdev, fmt, ...) \
> + ibdev_level_ratelimited(ibdev_info, ibdev, fmt, ##__VA_ARGS__)
> +
> +#if defined(CONFIG_DYNAMIC_DEBUG)
> +/* descriptor check is first to prevent flooding with "callbacks suppressed" */
> +#define ibdev_dbg_ratelimited(ibdev, fmt, ...) \
> +do { \
> + static DEFINE_RATELIMIT_STATE(_rs, \
> + DEFAULT_RATELIMIT_INTERVAL, \
> + DEFAULT_RATELIMIT_BURST); \
> + DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt); \
> + if (DYNAMIC_DEBUG_BRANCH(descriptor) && __ratelimit(&_rs)) \
> + __dynamic_ibdev_dbg(&descriptor, ibdev, fmt, \
> + ##__VA_ARGS__); \
> +} while (0)
> +#elif defined(DEBUG)
When will you see this CONFIG_DEBUG set? I suspect only in private
out-of-tree builds which we are not really care. Also I can't imagine
system with this CONFIG_DEBUG and without CONFIG_DYNAMIC_DEBUG.
Thanks
> +#define ibdev_dbg_ratelimited(ibdev, fmt, ...) \
> +do { \
> + static DEFINE_RATELIMIT_STATE(_rs, \
> + DEFAULT_RATELIMIT_INTERVAL, \
> + DEFAULT_RATELIMIT_BURST); \
> + if (__ratelimit(&_rs)) \
> + ibdev_printk(KERN_DEBUG, ibdev, fmt, ##__VA_ARGS__); \
> +} while (0)
> +#else
> +__printf(2, 3) __cold
> +static inline
> +void ibdev_dbg_ratelimited(const struct ib_device *ibdev, const char *format, ...) {}
> +#endif
> +
> union ib_gid {
> u8 raw[16];
> struct {
> --
> 2.22.0
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH for-next 1/2] RDMA/core: Introduce ratelimited ibdev printk functions
2019-07-30 15:41 ` Leon Romanovsky
@ 2019-07-31 7:22 ` Gal Pressman
2019-07-31 7:41 ` Leon Romanovsky
0 siblings, 1 reply; 14+ messages in thread
From: Gal Pressman @ 2019-07-31 7:22 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: Jason Gunthorpe, Doug Ledford, linux-rdma
On 30/07/2019 18:41, Leon Romanovsky wrote:
> On Tue, Jul 30, 2019 at 06:18:33PM +0300, Gal Pressman wrote:
>> Add ratelimited helpers to the ibdev_* printk functions.
>> Implementation inspired by counterpart dev_*_ratelimited functions.
>>
>> Signed-off-by: Gal Pressman <galpress@amazon.com>
>> ---
>> include/rdma/ib_verbs.h | 51 +++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 51 insertions(+)
>>
>> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
>> index c5f8a9f17063..356e6a105366 100644
>> --- a/include/rdma/ib_verbs.h
>> +++ b/include/rdma/ib_verbs.h
>> @@ -107,6 +107,57 @@ static inline
>> void ibdev_dbg(const struct ib_device *ibdev, const char *format, ...) {}
>> #endif
>>
>> +#define ibdev_level_ratelimited(ibdev_level, ibdev, fmt, ...) \
>> +do { \
>> + static DEFINE_RATELIMIT_STATE(_rs, \
>> + DEFAULT_RATELIMIT_INTERVAL, \
>> + DEFAULT_RATELIMIT_BURST); \
>> + if (__ratelimit(&_rs)) \
>> + ibdev_level(ibdev, fmt, ##__VA_ARGS__); \
>> +} while (0)
>> +
>> +#define ibdev_emerg_ratelimited(ibdev, fmt, ...) \
>> + ibdev_level_ratelimited(ibdev_emerg, ibdev, fmt, ##__VA_ARGS__)
>> +#define ibdev_alert_ratelimited(ibdev, fmt, ...) \
>> + ibdev_level_ratelimited(ibdev_alert, ibdev, fmt, ##__VA_ARGS__)
>> +#define ibdev_crit_ratelimited(ibdev, fmt, ...) \
>> + ibdev_level_ratelimited(ibdev_crit, ibdev, fmt, ##__VA_ARGS__)
>> +#define ibdev_err_ratelimited(ibdev, fmt, ...) \
>> + ibdev_level_ratelimited(ibdev_err, ibdev, fmt, ##__VA_ARGS__)
>> +#define ibdev_warn_ratelimited(ibdev, fmt, ...) \
>> + ibdev_level_ratelimited(ibdev_warn, ibdev, fmt, ##__VA_ARGS__)
>> +#define ibdev_notice_ratelimited(ibdev, fmt, ...) \
>> + ibdev_level_ratelimited(ibdev_notice, ibdev, fmt, ##__VA_ARGS__)
>> +#define ibdev_info_ratelimited(ibdev, fmt, ...) \
>> + ibdev_level_ratelimited(ibdev_info, ibdev, fmt, ##__VA_ARGS__)
>> +
>> +#if defined(CONFIG_DYNAMIC_DEBUG)
>> +/* descriptor check is first to prevent flooding with "callbacks suppressed" */
>> +#define ibdev_dbg_ratelimited(ibdev, fmt, ...) \
>> +do { \
>> + static DEFINE_RATELIMIT_STATE(_rs, \
>> + DEFAULT_RATELIMIT_INTERVAL, \
>> + DEFAULT_RATELIMIT_BURST); \
>> + DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt); \
>> + if (DYNAMIC_DEBUG_BRANCH(descriptor) && __ratelimit(&_rs)) \
>> + __dynamic_ibdev_dbg(&descriptor, ibdev, fmt, \
>> + ##__VA_ARGS__); \
>> +} while (0)
>> +#elif defined(DEBUG)
>
> When will you see this CONFIG_DEBUG set? I suspect only in private
> out-of-tree builds which we are not really care. Also I can't imagine
> system with this CONFIG_DEBUG and without CONFIG_DYNAMIC_DEBUG.
This is the common way to handle debug prints, see:
https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/printk.h#L331
https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/device.h#L1493
https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/netdevice.h#L4743
https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/net.h#L266
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH for-next 1/2] RDMA/core: Introduce ratelimited ibdev printk functions
2019-07-31 7:22 ` Gal Pressman
@ 2019-07-31 7:41 ` Leon Romanovsky
2019-07-31 10:51 ` Gal Pressman
0 siblings, 1 reply; 14+ messages in thread
From: Leon Romanovsky @ 2019-07-31 7:41 UTC (permalink / raw)
To: Gal Pressman; +Cc: Jason Gunthorpe, Doug Ledford, linux-rdma
On Wed, Jul 31, 2019 at 10:22:42AM +0300, Gal Pressman wrote:
> On 30/07/2019 18:41, Leon Romanovsky wrote:
> > On Tue, Jul 30, 2019 at 06:18:33PM +0300, Gal Pressman wrote:
> >> Add ratelimited helpers to the ibdev_* printk functions.
> >> Implementation inspired by counterpart dev_*_ratelimited functions.
> >>
> >> Signed-off-by: Gal Pressman <galpress@amazon.com>
> >> ---
> >> include/rdma/ib_verbs.h | 51 +++++++++++++++++++++++++++++++++++++++++
> >> 1 file changed, 51 insertions(+)
> >>
> >> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> >> index c5f8a9f17063..356e6a105366 100644
> >> --- a/include/rdma/ib_verbs.h
> >> +++ b/include/rdma/ib_verbs.h
> >> @@ -107,6 +107,57 @@ static inline
> >> void ibdev_dbg(const struct ib_device *ibdev, const char *format, ...) {}
> >> #endif
> >>
> >> +#define ibdev_level_ratelimited(ibdev_level, ibdev, fmt, ...) \
> >> +do { \
> >> + static DEFINE_RATELIMIT_STATE(_rs, \
> >> + DEFAULT_RATELIMIT_INTERVAL, \
> >> + DEFAULT_RATELIMIT_BURST); \
> >> + if (__ratelimit(&_rs)) \
> >> + ibdev_level(ibdev, fmt, ##__VA_ARGS__); \
> >> +} while (0)
> >> +
> >> +#define ibdev_emerg_ratelimited(ibdev, fmt, ...) \
> >> + ibdev_level_ratelimited(ibdev_emerg, ibdev, fmt, ##__VA_ARGS__)
> >> +#define ibdev_alert_ratelimited(ibdev, fmt, ...) \
> >> + ibdev_level_ratelimited(ibdev_alert, ibdev, fmt, ##__VA_ARGS__)
> >> +#define ibdev_crit_ratelimited(ibdev, fmt, ...) \
> >> + ibdev_level_ratelimited(ibdev_crit, ibdev, fmt, ##__VA_ARGS__)
> >> +#define ibdev_err_ratelimited(ibdev, fmt, ...) \
> >> + ibdev_level_ratelimited(ibdev_err, ibdev, fmt, ##__VA_ARGS__)
> >> +#define ibdev_warn_ratelimited(ibdev, fmt, ...) \
> >> + ibdev_level_ratelimited(ibdev_warn, ibdev, fmt, ##__VA_ARGS__)
> >> +#define ibdev_notice_ratelimited(ibdev, fmt, ...) \
> >> + ibdev_level_ratelimited(ibdev_notice, ibdev, fmt, ##__VA_ARGS__)
> >> +#define ibdev_info_ratelimited(ibdev, fmt, ...) \
> >> + ibdev_level_ratelimited(ibdev_info, ibdev, fmt, ##__VA_ARGS__)
> >> +
> >> +#if defined(CONFIG_DYNAMIC_DEBUG)
> >> +/* descriptor check is first to prevent flooding with "callbacks suppressed" */
> >> +#define ibdev_dbg_ratelimited(ibdev, fmt, ...) \
> >> +do { \
> >> + static DEFINE_RATELIMIT_STATE(_rs, \
> >> + DEFAULT_RATELIMIT_INTERVAL, \
> >> + DEFAULT_RATELIMIT_BURST); \
> >> + DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt); \
> >> + if (DYNAMIC_DEBUG_BRANCH(descriptor) && __ratelimit(&_rs)) \
> >> + __dynamic_ibdev_dbg(&descriptor, ibdev, fmt, \
> >> + ##__VA_ARGS__); \
> >> +} while (0)
> >> +#elif defined(DEBUG)
> >
> > When will you see this CONFIG_DEBUG set? I suspect only in private
> > out-of-tree builds which we are not really care. Also I can't imagine
> > system with this CONFIG_DEBUG and without CONFIG_DYNAMIC_DEBUG.
>
> This is the common way to handle debug prints, see:
> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/printk.h#L331
> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/device.h#L1493
> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/netdevice.h#L4743
> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/net.h#L266
I'm more interested to know the real usage of this copy/paste and
understand if it makes sense for drivers/infiniband/* or not.
Not everything in netdev is great and worth to borrow.
Thanks
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH for-next 1/2] RDMA/core: Introduce ratelimited ibdev printk functions
2019-07-31 7:41 ` Leon Romanovsky
@ 2019-07-31 10:51 ` Gal Pressman
2019-07-31 11:46 ` Leon Romanovsky
0 siblings, 1 reply; 14+ messages in thread
From: Gal Pressman @ 2019-07-31 10:51 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: Jason Gunthorpe, Doug Ledford, linux-rdma
On 31/07/2019 10:41, Leon Romanovsky wrote:
> On Wed, Jul 31, 2019 at 10:22:42AM +0300, Gal Pressman wrote:
>> On 30/07/2019 18:41, Leon Romanovsky wrote:
>>> On Tue, Jul 30, 2019 at 06:18:33PM +0300, Gal Pressman wrote:
>>>> Add ratelimited helpers to the ibdev_* printk functions.
>>>> Implementation inspired by counterpart dev_*_ratelimited functions.
>>>>
>>>> Signed-off-by: Gal Pressman <galpress@amazon.com>
>>>> ---
>>>> include/rdma/ib_verbs.h | 51 +++++++++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 51 insertions(+)
>>>>
>>>> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
>>>> index c5f8a9f17063..356e6a105366 100644
>>>> --- a/include/rdma/ib_verbs.h
>>>> +++ b/include/rdma/ib_verbs.h
>>>> @@ -107,6 +107,57 @@ static inline
>>>> void ibdev_dbg(const struct ib_device *ibdev, const char *format, ...) {}
>>>> #endif
>>>>
>>>> +#define ibdev_level_ratelimited(ibdev_level, ibdev, fmt, ...) \
>>>> +do { \
>>>> + static DEFINE_RATELIMIT_STATE(_rs, \
>>>> + DEFAULT_RATELIMIT_INTERVAL, \
>>>> + DEFAULT_RATELIMIT_BURST); \
>>>> + if (__ratelimit(&_rs)) \
>>>> + ibdev_level(ibdev, fmt, ##__VA_ARGS__); \
>>>> +} while (0)
>>>> +
>>>> +#define ibdev_emerg_ratelimited(ibdev, fmt, ...) \
>>>> + ibdev_level_ratelimited(ibdev_emerg, ibdev, fmt, ##__VA_ARGS__)
>>>> +#define ibdev_alert_ratelimited(ibdev, fmt, ...) \
>>>> + ibdev_level_ratelimited(ibdev_alert, ibdev, fmt, ##__VA_ARGS__)
>>>> +#define ibdev_crit_ratelimited(ibdev, fmt, ...) \
>>>> + ibdev_level_ratelimited(ibdev_crit, ibdev, fmt, ##__VA_ARGS__)
>>>> +#define ibdev_err_ratelimited(ibdev, fmt, ...) \
>>>> + ibdev_level_ratelimited(ibdev_err, ibdev, fmt, ##__VA_ARGS__)
>>>> +#define ibdev_warn_ratelimited(ibdev, fmt, ...) \
>>>> + ibdev_level_ratelimited(ibdev_warn, ibdev, fmt, ##__VA_ARGS__)
>>>> +#define ibdev_notice_ratelimited(ibdev, fmt, ...) \
>>>> + ibdev_level_ratelimited(ibdev_notice, ibdev, fmt, ##__VA_ARGS__)
>>>> +#define ibdev_info_ratelimited(ibdev, fmt, ...) \
>>>> + ibdev_level_ratelimited(ibdev_info, ibdev, fmt, ##__VA_ARGS__)
>>>> +
>>>> +#if defined(CONFIG_DYNAMIC_DEBUG)
>>>> +/* descriptor check is first to prevent flooding with "callbacks suppressed" */
>>>> +#define ibdev_dbg_ratelimited(ibdev, fmt, ...) \
>>>> +do { \
>>>> + static DEFINE_RATELIMIT_STATE(_rs, \
>>>> + DEFAULT_RATELIMIT_INTERVAL, \
>>>> + DEFAULT_RATELIMIT_BURST); \
>>>> + DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt); \
>>>> + if (DYNAMIC_DEBUG_BRANCH(descriptor) && __ratelimit(&_rs)) \
>>>> + __dynamic_ibdev_dbg(&descriptor, ibdev, fmt, \
>>>> + ##__VA_ARGS__); \
>>>> +} while (0)
>>>> +#elif defined(DEBUG)
>>>
>>> When will you see this CONFIG_DEBUG set? I suspect only in private
>>> out-of-tree builds which we are not really care. Also I can't imagine
>>> system with this CONFIG_DEBUG and without CONFIG_DYNAMIC_DEBUG.
>>
>> This is the common way to handle debug prints, see:
>> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/printk.h#L331
>> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/device.h#L1493
>> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/netdevice.h#L4743
>> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/net.h#L266
>
> I'm more interested to know the real usage of this copy/paste and
> understand if it makes sense for drivers/infiniband/* or not.
>
> Not everything in netdev is great and worth to borrow.
DEBUG exists since the first commit in the tree, and is used in various parts of
the kernel (mlx5 as well). Do you think it should be removed from the kernel?
Regarding combination of both, I don't think DEBUG is related to
CONFIG_DYNAMIC_DEBUG. DEBUG is a generic debug flag (not necessarily to prints)
while CONFIG_DYNAMIC_DEBUG is specific to the dynamic debug prints infrastructure.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH for-next 1/2] RDMA/core: Introduce ratelimited ibdev printk functions
2019-07-31 10:51 ` Gal Pressman
@ 2019-07-31 11:46 ` Leon Romanovsky
2019-07-31 12:56 ` Gal Pressman
0 siblings, 1 reply; 14+ messages in thread
From: Leon Romanovsky @ 2019-07-31 11:46 UTC (permalink / raw)
To: Gal Pressman; +Cc: Jason Gunthorpe, Doug Ledford, linux-rdma
On Wed, Jul 31, 2019 at 01:51:05PM +0300, Gal Pressman wrote:
> On 31/07/2019 10:41, Leon Romanovsky wrote:
> > On Wed, Jul 31, 2019 at 10:22:42AM +0300, Gal Pressman wrote:
> >> On 30/07/2019 18:41, Leon Romanovsky wrote:
> >>> On Tue, Jul 30, 2019 at 06:18:33PM +0300, Gal Pressman wrote:
> >>>> Add ratelimited helpers to the ibdev_* printk functions.
> >>>> Implementation inspired by counterpart dev_*_ratelimited functions.
> >>>>
> >>>> Signed-off-by: Gal Pressman <galpress@amazon.com>
> >>>> ---
> >>>> include/rdma/ib_verbs.h | 51 +++++++++++++++++++++++++++++++++++++++++
> >>>> 1 file changed, 51 insertions(+)
> >>>>
> >>>> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> >>>> index c5f8a9f17063..356e6a105366 100644
> >>>> --- a/include/rdma/ib_verbs.h
> >>>> +++ b/include/rdma/ib_verbs.h
> >>>> @@ -107,6 +107,57 @@ static inline
> >>>> void ibdev_dbg(const struct ib_device *ibdev, const char *format, ...) {}
> >>>> #endif
> >>>>
> >>>> +#define ibdev_level_ratelimited(ibdev_level, ibdev, fmt, ...) \
> >>>> +do { \
> >>>> + static DEFINE_RATELIMIT_STATE(_rs, \
> >>>> + DEFAULT_RATELIMIT_INTERVAL, \
> >>>> + DEFAULT_RATELIMIT_BURST); \
> >>>> + if (__ratelimit(&_rs)) \
> >>>> + ibdev_level(ibdev, fmt, ##__VA_ARGS__); \
> >>>> +} while (0)
> >>>> +
> >>>> +#define ibdev_emerg_ratelimited(ibdev, fmt, ...) \
> >>>> + ibdev_level_ratelimited(ibdev_emerg, ibdev, fmt, ##__VA_ARGS__)
> >>>> +#define ibdev_alert_ratelimited(ibdev, fmt, ...) \
> >>>> + ibdev_level_ratelimited(ibdev_alert, ibdev, fmt, ##__VA_ARGS__)
> >>>> +#define ibdev_crit_ratelimited(ibdev, fmt, ...) \
> >>>> + ibdev_level_ratelimited(ibdev_crit, ibdev, fmt, ##__VA_ARGS__)
> >>>> +#define ibdev_err_ratelimited(ibdev, fmt, ...) \
> >>>> + ibdev_level_ratelimited(ibdev_err, ibdev, fmt, ##__VA_ARGS__)
> >>>> +#define ibdev_warn_ratelimited(ibdev, fmt, ...) \
> >>>> + ibdev_level_ratelimited(ibdev_warn, ibdev, fmt, ##__VA_ARGS__)
> >>>> +#define ibdev_notice_ratelimited(ibdev, fmt, ...) \
> >>>> + ibdev_level_ratelimited(ibdev_notice, ibdev, fmt, ##__VA_ARGS__)
> >>>> +#define ibdev_info_ratelimited(ibdev, fmt, ...) \
> >>>> + ibdev_level_ratelimited(ibdev_info, ibdev, fmt, ##__VA_ARGS__)
> >>>> +
> >>>> +#if defined(CONFIG_DYNAMIC_DEBUG)
> >>>> +/* descriptor check is first to prevent flooding with "callbacks suppressed" */
> >>>> +#define ibdev_dbg_ratelimited(ibdev, fmt, ...) \
> >>>> +do { \
> >>>> + static DEFINE_RATELIMIT_STATE(_rs, \
> >>>> + DEFAULT_RATELIMIT_INTERVAL, \
> >>>> + DEFAULT_RATELIMIT_BURST); \
> >>>> + DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt); \
> >>>> + if (DYNAMIC_DEBUG_BRANCH(descriptor) && __ratelimit(&_rs)) \
> >>>> + __dynamic_ibdev_dbg(&descriptor, ibdev, fmt, \
> >>>> + ##__VA_ARGS__); \
> >>>> +} while (0)
> >>>> +#elif defined(DEBUG)
> >>>
> >>> When will you see this CONFIG_DEBUG set? I suspect only in private
> >>> out-of-tree builds which we are not really care. Also I can't imagine
> >>> system with this CONFIG_DEBUG and without CONFIG_DYNAMIC_DEBUG.
> >>
> >> This is the common way to handle debug prints, see:
> >> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/printk.h#L331
> >> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/device.h#L1493
> >> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/netdevice.h#L4743
> >> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/net.h#L266
> >
> > I'm more interested to know the real usage of this copy/paste and
> > understand if it makes sense for drivers/infiniband/* or not.
> >
> > Not everything in netdev is great and worth to borrow.
>
> DEBUG exists since the first commit in the tree, and is used in various parts of
> the kernel (mlx5 as well). Do you think it should be removed from the kernel?
It is gradually removed when it is spotted, I'll send a patch for mlx5 now.
>
> Regarding combination of both, I don't think DEBUG is related to
> CONFIG_DYNAMIC_DEBUG. DEBUG is a generic debug flag (not necessarily to prints)
> while CONFIG_DYNAMIC_DEBUG is specific to the dynamic debug prints infrastructure.
I know exactly what DEBUG and CONFIG_DYNAMIC_DEBUG mean, but I'm
asking YOU to provide us real and in-tree scenario where DEBUG will
exists and CONFIG_DYNAMIC_DEBUG won't.
Thanks
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH for-next 1/2] RDMA/core: Introduce ratelimited ibdev printk functions
2019-07-31 11:46 ` Leon Romanovsky
@ 2019-07-31 12:56 ` Gal Pressman
2019-07-31 13:33 ` Leon Romanovsky
0 siblings, 1 reply; 14+ messages in thread
From: Gal Pressman @ 2019-07-31 12:56 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: Jason Gunthorpe, Doug Ledford, linux-rdma
On 31/07/2019 14:46, Leon Romanovsky wrote:
> On Wed, Jul 31, 2019 at 01:51:05PM +0300, Gal Pressman wrote:
>> On 31/07/2019 10:41, Leon Romanovsky wrote:
>>> On Wed, Jul 31, 2019 at 10:22:42AM +0300, Gal Pressman wrote:
>>>> On 30/07/2019 18:41, Leon Romanovsky wrote:
>>>>> On Tue, Jul 30, 2019 at 06:18:33PM +0300, Gal Pressman wrote:
>>>>>> Add ratelimited helpers to the ibdev_* printk functions.
>>>>>> Implementation inspired by counterpart dev_*_ratelimited functions.
>>>>>>
>>>>>> Signed-off-by: Gal Pressman <galpress@amazon.com>
>>>>>> ---
>>>>>> include/rdma/ib_verbs.h | 51 +++++++++++++++++++++++++++++++++++++++++
>>>>>> 1 file changed, 51 insertions(+)
>>>>>>
>>>>>> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
>>>>>> index c5f8a9f17063..356e6a105366 100644
>>>>>> --- a/include/rdma/ib_verbs.h
>>>>>> +++ b/include/rdma/ib_verbs.h
>>>>>> @@ -107,6 +107,57 @@ static inline
>>>>>> void ibdev_dbg(const struct ib_device *ibdev, const char *format, ...) {}
>>>>>> #endif
>>>>>>
>>>>>> +#define ibdev_level_ratelimited(ibdev_level, ibdev, fmt, ...) \
>>>>>> +do { \
>>>>>> + static DEFINE_RATELIMIT_STATE(_rs, \
>>>>>> + DEFAULT_RATELIMIT_INTERVAL, \
>>>>>> + DEFAULT_RATELIMIT_BURST); \
>>>>>> + if (__ratelimit(&_rs)) \
>>>>>> + ibdev_level(ibdev, fmt, ##__VA_ARGS__); \
>>>>>> +} while (0)
>>>>>> +
>>>>>> +#define ibdev_emerg_ratelimited(ibdev, fmt, ...) \
>>>>>> + ibdev_level_ratelimited(ibdev_emerg, ibdev, fmt, ##__VA_ARGS__)
>>>>>> +#define ibdev_alert_ratelimited(ibdev, fmt, ...) \
>>>>>> + ibdev_level_ratelimited(ibdev_alert, ibdev, fmt, ##__VA_ARGS__)
>>>>>> +#define ibdev_crit_ratelimited(ibdev, fmt, ...) \
>>>>>> + ibdev_level_ratelimited(ibdev_crit, ibdev, fmt, ##__VA_ARGS__)
>>>>>> +#define ibdev_err_ratelimited(ibdev, fmt, ...) \
>>>>>> + ibdev_level_ratelimited(ibdev_err, ibdev, fmt, ##__VA_ARGS__)
>>>>>> +#define ibdev_warn_ratelimited(ibdev, fmt, ...) \
>>>>>> + ibdev_level_ratelimited(ibdev_warn, ibdev, fmt, ##__VA_ARGS__)
>>>>>> +#define ibdev_notice_ratelimited(ibdev, fmt, ...) \
>>>>>> + ibdev_level_ratelimited(ibdev_notice, ibdev, fmt, ##__VA_ARGS__)
>>>>>> +#define ibdev_info_ratelimited(ibdev, fmt, ...) \
>>>>>> + ibdev_level_ratelimited(ibdev_info, ibdev, fmt, ##__VA_ARGS__)
>>>>>> +
>>>>>> +#if defined(CONFIG_DYNAMIC_DEBUG)
>>>>>> +/* descriptor check is first to prevent flooding with "callbacks suppressed" */
>>>>>> +#define ibdev_dbg_ratelimited(ibdev, fmt, ...) \
>>>>>> +do { \
>>>>>> + static DEFINE_RATELIMIT_STATE(_rs, \
>>>>>> + DEFAULT_RATELIMIT_INTERVAL, \
>>>>>> + DEFAULT_RATELIMIT_BURST); \
>>>>>> + DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt); \
>>>>>> + if (DYNAMIC_DEBUG_BRANCH(descriptor) && __ratelimit(&_rs)) \
>>>>>> + __dynamic_ibdev_dbg(&descriptor, ibdev, fmt, \
>>>>>> + ##__VA_ARGS__); \
>>>>>> +} while (0)
>>>>>> +#elif defined(DEBUG)
>>>>>
>>>>> When will you see this CONFIG_DEBUG set? I suspect only in private
>>>>> out-of-tree builds which we are not really care. Also I can't imagine
>>>>> system with this CONFIG_DEBUG and without CONFIG_DYNAMIC_DEBUG.
>>>>
>>>> This is the common way to handle debug prints, see:
>>>> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/printk.h#L331
>>>> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/device.h#L1493
>>>> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/netdevice.h#L4743
>>>> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/net.h#L266
>>>
>>> I'm more interested to know the real usage of this copy/paste and
>>> understand if it makes sense for drivers/infiniband/* or not.
>>>
>>> Not everything in netdev is great and worth to borrow.
>>
>> DEBUG exists since the first commit in the tree, and is used in various parts of
>> the kernel (mlx5 as well). Do you think it should be removed from the kernel?
>
> It is gradually removed when it is spotted, I'll send a patch for mlx5 now.
Was there an on-list discussion regarding removal of DEBUG usage? Can you please
share a link?
If so, I agree the DEBUG part should be removed.
>
>>
>> Regarding combination of both, I don't think DEBUG is related to
>> CONFIG_DYNAMIC_DEBUG. DEBUG is a generic debug flag (not necessarily to prints)
>> while CONFIG_DYNAMIC_DEBUG is specific to the dynamic debug prints infrastructure.
>
> I know exactly what DEBUG and CONFIG_DYNAMIC_DEBUG mean, but I'm
> asking YOU to provide us real and in-tree scenario where DEBUG will
> exists and CONFIG_DYNAMIC_DEBUG won't.
What's any of this has to do with in-tree? This code and defines are part of the
tree.
The use case doesn't matter, it's a valid permutation. Is there anything that
stops a user from building the kernel this way?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH for-next 1/2] RDMA/core: Introduce ratelimited ibdev printk functions
2019-07-31 12:56 ` Gal Pressman
@ 2019-07-31 13:33 ` Leon Romanovsky
2019-07-31 14:19 ` Gal Pressman
0 siblings, 1 reply; 14+ messages in thread
From: Leon Romanovsky @ 2019-07-31 13:33 UTC (permalink / raw)
To: Gal Pressman; +Cc: Jason Gunthorpe, Doug Ledford, linux-rdma
On Wed, Jul 31, 2019 at 03:56:55PM +0300, Gal Pressman wrote:
> On 31/07/2019 14:46, Leon Romanovsky wrote:
> > On Wed, Jul 31, 2019 at 01:51:05PM +0300, Gal Pressman wrote:
> >> On 31/07/2019 10:41, Leon Romanovsky wrote:
> >>> On Wed, Jul 31, 2019 at 10:22:42AM +0300, Gal Pressman wrote:
> >>>> On 30/07/2019 18:41, Leon Romanovsky wrote:
> >>>>> On Tue, Jul 30, 2019 at 06:18:33PM +0300, Gal Pressman wrote:
> >>>>>> Add ratelimited helpers to the ibdev_* printk functions.
> >>>>>> Implementation inspired by counterpart dev_*_ratelimited functions.
> >>>>>>
> >>>>>> Signed-off-by: Gal Pressman <galpress@amazon.com>
> >>>>>> ---
> >>>>>> include/rdma/ib_verbs.h | 51 +++++++++++++++++++++++++++++++++++++++++
> >>>>>> 1 file changed, 51 insertions(+)
> >>>>>>
> >>>>>> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> >>>>>> index c5f8a9f17063..356e6a105366 100644
> >>>>>> --- a/include/rdma/ib_verbs.h
> >>>>>> +++ b/include/rdma/ib_verbs.h
> >>>>>> @@ -107,6 +107,57 @@ static inline
> >>>>>> void ibdev_dbg(const struct ib_device *ibdev, const char *format, ...) {}
> >>>>>> #endif
> >>>>>>
> >>>>>> +#define ibdev_level_ratelimited(ibdev_level, ibdev, fmt, ...) \
> >>>>>> +do { \
> >>>>>> + static DEFINE_RATELIMIT_STATE(_rs, \
> >>>>>> + DEFAULT_RATELIMIT_INTERVAL, \
> >>>>>> + DEFAULT_RATELIMIT_BURST); \
> >>>>>> + if (__ratelimit(&_rs)) \
> >>>>>> + ibdev_level(ibdev, fmt, ##__VA_ARGS__); \
> >>>>>> +} while (0)
> >>>>>> +
> >>>>>> +#define ibdev_emerg_ratelimited(ibdev, fmt, ...) \
> >>>>>> + ibdev_level_ratelimited(ibdev_emerg, ibdev, fmt, ##__VA_ARGS__)
> >>>>>> +#define ibdev_alert_ratelimited(ibdev, fmt, ...) \
> >>>>>> + ibdev_level_ratelimited(ibdev_alert, ibdev, fmt, ##__VA_ARGS__)
> >>>>>> +#define ibdev_crit_ratelimited(ibdev, fmt, ...) \
> >>>>>> + ibdev_level_ratelimited(ibdev_crit, ibdev, fmt, ##__VA_ARGS__)
> >>>>>> +#define ibdev_err_ratelimited(ibdev, fmt, ...) \
> >>>>>> + ibdev_level_ratelimited(ibdev_err, ibdev, fmt, ##__VA_ARGS__)
> >>>>>> +#define ibdev_warn_ratelimited(ibdev, fmt, ...) \
> >>>>>> + ibdev_level_ratelimited(ibdev_warn, ibdev, fmt, ##__VA_ARGS__)
> >>>>>> +#define ibdev_notice_ratelimited(ibdev, fmt, ...) \
> >>>>>> + ibdev_level_ratelimited(ibdev_notice, ibdev, fmt, ##__VA_ARGS__)
> >>>>>> +#define ibdev_info_ratelimited(ibdev, fmt, ...) \
> >>>>>> + ibdev_level_ratelimited(ibdev_info, ibdev, fmt, ##__VA_ARGS__)
> >>>>>> +
> >>>>>> +#if defined(CONFIG_DYNAMIC_DEBUG)
> >>>>>> +/* descriptor check is first to prevent flooding with "callbacks suppressed" */
> >>>>>> +#define ibdev_dbg_ratelimited(ibdev, fmt, ...) \
> >>>>>> +do { \
> >>>>>> + static DEFINE_RATELIMIT_STATE(_rs, \
> >>>>>> + DEFAULT_RATELIMIT_INTERVAL, \
> >>>>>> + DEFAULT_RATELIMIT_BURST); \
> >>>>>> + DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt); \
> >>>>>> + if (DYNAMIC_DEBUG_BRANCH(descriptor) && __ratelimit(&_rs)) \
> >>>>>> + __dynamic_ibdev_dbg(&descriptor, ibdev, fmt, \
> >>>>>> + ##__VA_ARGS__); \
> >>>>>> +} while (0)
> >>>>>> +#elif defined(DEBUG)
> >>>>>
> >>>>> When will you see this CONFIG_DEBUG set? I suspect only in private
> >>>>> out-of-tree builds which we are not really care. Also I can't imagine
> >>>>> system with this CONFIG_DEBUG and without CONFIG_DYNAMIC_DEBUG.
> >>>>
> >>>> This is the common way to handle debug prints, see:
> >>>> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/printk.h#L331
> >>>> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/device.h#L1493
> >>>> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/netdevice.h#L4743
> >>>> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/net.h#L266
> >>>
> >>> I'm more interested to know the real usage of this copy/paste and
> >>> understand if it makes sense for drivers/infiniband/* or not.
> >>>
> >>> Not everything in netdev is great and worth to borrow.
> >>
> >> DEBUG exists since the first commit in the tree, and is used in various parts of
> >> the kernel (mlx5 as well). Do you think it should be removed from the kernel?
> >
> > It is gradually removed when it is spotted, I'll send a patch for mlx5 now.
>
> Was there an on-list discussion regarding removal of DEBUG usage? Can you please
> share a link?
Sorry, but no, I didn't know that I need to save all discussions I see
in the mailing lists.
> If so, I agree the DEBUG part should be removed.
>
> >
> >>
> >> Regarding combination of both, I don't think DEBUG is related to
> >> CONFIG_DYNAMIC_DEBUG. DEBUG is a generic debug flag (not necessarily to prints)
> >> while CONFIG_DYNAMIC_DEBUG is specific to the dynamic debug prints infrastructure.
> >
> > I know exactly what DEBUG and CONFIG_DYNAMIC_DEBUG mean, but I'm
> > asking YOU to provide us real and in-tree scenario where DEBUG will
> > exists and CONFIG_DYNAMIC_DEBUG won't.
>
> What's any of this has to do with in-tree? This code and defines are part of the
> tree.
>
> The use case doesn't matter, it's a valid permutation. Is there anything that
> stops a user from building the kernel this way?
Like everything else, nothing stops from you to do any modifications to
the source code, before you will build. We are talking about in-tree
builds and distro kernels.
Thanks
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH for-next 1/2] RDMA/core: Introduce ratelimited ibdev printk functions
2019-07-31 13:33 ` Leon Romanovsky
@ 2019-07-31 14:19 ` Gal Pressman
2019-07-31 14:50 ` Leon Romanovsky
0 siblings, 1 reply; 14+ messages in thread
From: Gal Pressman @ 2019-07-31 14:19 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: Jason Gunthorpe, Doug Ledford, linux-rdma
On 31/07/2019 16:33, Leon Romanovsky wrote:
> On Wed, Jul 31, 2019 at 03:56:55PM +0300, Gal Pressman wrote:
>> On 31/07/2019 14:46, Leon Romanovsky wrote:
>>> On Wed, Jul 31, 2019 at 01:51:05PM +0300, Gal Pressman wrote:
>>>> On 31/07/2019 10:41, Leon Romanovsky wrote:
>>>>> On Wed, Jul 31, 2019 at 10:22:42AM +0300, Gal Pressman wrote:
>>>>>> On 30/07/2019 18:41, Leon Romanovsky wrote:
>>>>>>> On Tue, Jul 30, 2019 at 06:18:33PM +0300, Gal Pressman wrote:
>>>>>>>> Add ratelimited helpers to the ibdev_* printk functions.
>>>>>>>> Implementation inspired by counterpart dev_*_ratelimited functions.
>>>>>>>>
>>>>>>>> Signed-off-by: Gal Pressman <galpress@amazon.com>
>>>>>>>> ---
>>>>>>>> include/rdma/ib_verbs.h | 51 +++++++++++++++++++++++++++++++++++++++++
>>>>>>>> 1 file changed, 51 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
>>>>>>>> index c5f8a9f17063..356e6a105366 100644
>>>>>>>> --- a/include/rdma/ib_verbs.h
>>>>>>>> +++ b/include/rdma/ib_verbs.h
>>>>>>>> @@ -107,6 +107,57 @@ static inline
>>>>>>>> void ibdev_dbg(const struct ib_device *ibdev, const char *format, ...) {}
>>>>>>>> #endif
>>>>>>>>
>>>>>>>> +#define ibdev_level_ratelimited(ibdev_level, ibdev, fmt, ...) \
>>>>>>>> +do { \
>>>>>>>> + static DEFINE_RATELIMIT_STATE(_rs, \
>>>>>>>> + DEFAULT_RATELIMIT_INTERVAL, \
>>>>>>>> + DEFAULT_RATELIMIT_BURST); \
>>>>>>>> + if (__ratelimit(&_rs)) \
>>>>>>>> + ibdev_level(ibdev, fmt, ##__VA_ARGS__); \
>>>>>>>> +} while (0)
>>>>>>>> +
>>>>>>>> +#define ibdev_emerg_ratelimited(ibdev, fmt, ...) \
>>>>>>>> + ibdev_level_ratelimited(ibdev_emerg, ibdev, fmt, ##__VA_ARGS__)
>>>>>>>> +#define ibdev_alert_ratelimited(ibdev, fmt, ...) \
>>>>>>>> + ibdev_level_ratelimited(ibdev_alert, ibdev, fmt, ##__VA_ARGS__)
>>>>>>>> +#define ibdev_crit_ratelimited(ibdev, fmt, ...) \
>>>>>>>> + ibdev_level_ratelimited(ibdev_crit, ibdev, fmt, ##__VA_ARGS__)
>>>>>>>> +#define ibdev_err_ratelimited(ibdev, fmt, ...) \
>>>>>>>> + ibdev_level_ratelimited(ibdev_err, ibdev, fmt, ##__VA_ARGS__)
>>>>>>>> +#define ibdev_warn_ratelimited(ibdev, fmt, ...) \
>>>>>>>> + ibdev_level_ratelimited(ibdev_warn, ibdev, fmt, ##__VA_ARGS__)
>>>>>>>> +#define ibdev_notice_ratelimited(ibdev, fmt, ...) \
>>>>>>>> + ibdev_level_ratelimited(ibdev_notice, ibdev, fmt, ##__VA_ARGS__)
>>>>>>>> +#define ibdev_info_ratelimited(ibdev, fmt, ...) \
>>>>>>>> + ibdev_level_ratelimited(ibdev_info, ibdev, fmt, ##__VA_ARGS__)
>>>>>>>> +
>>>>>>>> +#if defined(CONFIG_DYNAMIC_DEBUG)
>>>>>>>> +/* descriptor check is first to prevent flooding with "callbacks suppressed" */
>>>>>>>> +#define ibdev_dbg_ratelimited(ibdev, fmt, ...) \
>>>>>>>> +do { \
>>>>>>>> + static DEFINE_RATELIMIT_STATE(_rs, \
>>>>>>>> + DEFAULT_RATELIMIT_INTERVAL, \
>>>>>>>> + DEFAULT_RATELIMIT_BURST); \
>>>>>>>> + DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt); \
>>>>>>>> + if (DYNAMIC_DEBUG_BRANCH(descriptor) && __ratelimit(&_rs)) \
>>>>>>>> + __dynamic_ibdev_dbg(&descriptor, ibdev, fmt, \
>>>>>>>> + ##__VA_ARGS__); \
>>>>>>>> +} while (0)
>>>>>>>> +#elif defined(DEBUG)
>>>>>>>
>>>>>>> When will you see this CONFIG_DEBUG set? I suspect only in private
>>>>>>> out-of-tree builds which we are not really care. Also I can't imagine
>>>>>>> system with this CONFIG_DEBUG and without CONFIG_DYNAMIC_DEBUG.
>>>>>>
>>>>>> This is the common way to handle debug prints, see:
>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/printk.h#L331
>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/device.h#L1493
>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/netdevice.h#L4743
>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/net.h#L266
>>>>>
>>>>> I'm more interested to know the real usage of this copy/paste and
>>>>> understand if it makes sense for drivers/infiniband/* or not.
>>>>>
>>>>> Not everything in netdev is great and worth to borrow.
>>>>
>>>> DEBUG exists since the first commit in the tree, and is used in various parts of
>>>> the kernel (mlx5 as well). Do you think it should be removed from the kernel?
>>>
>>> It is gradually removed when it is spotted, I'll send a patch for mlx5 now.
>>
>> Was there an on-list discussion regarding removal of DEBUG usage? Can you please
>> share a link?
>
> Sorry, but no, I didn't know that I need to save all discussions I see
> in the mailing lists.
Trying to understand whether "It is gradually removed when it is spotted" is a
well known guideline by the community, should checkpatch produce a warning for this?
>
>> If so, I agree the DEBUG part should be removed.
>>
>>>
>>>>
>>>> Regarding combination of both, I don't think DEBUG is related to
>>>> CONFIG_DYNAMIC_DEBUG. DEBUG is a generic debug flag (not necessarily to prints)
>>>> while CONFIG_DYNAMIC_DEBUG is specific to the dynamic debug prints infrastructure.
>>>
>>> I know exactly what DEBUG and CONFIG_DYNAMIC_DEBUG mean, but I'm
>>> asking YOU to provide us real and in-tree scenario where DEBUG will
>>> exists and CONFIG_DYNAMIC_DEBUG won't.
>>
>> What's any of this has to do with in-tree? This code and defines are part of the
>> tree.
>>
>> The use case doesn't matter, it's a valid permutation. Is there anything that
>> stops a user from building the kernel this way?
>
> Like everything else, nothing stops from you to do any modifications to
> the source code, before you will build. We are talking about in-tree
> builds and distro kernels.
Last I checked turning on DEBUG doesn't require source code changes?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH for-next 1/2] RDMA/core: Introduce ratelimited ibdev printk functions
2019-07-31 14:19 ` Gal Pressman
@ 2019-07-31 14:50 ` Leon Romanovsky
2019-07-31 15:26 ` Gal Pressman
0 siblings, 1 reply; 14+ messages in thread
From: Leon Romanovsky @ 2019-07-31 14:50 UTC (permalink / raw)
To: Gal Pressman; +Cc: Jason Gunthorpe, Doug Ledford, linux-rdma
On Wed, Jul 31, 2019 at 05:19:41PM +0300, Gal Pressman wrote:
> On 31/07/2019 16:33, Leon Romanovsky wrote:
> > On Wed, Jul 31, 2019 at 03:56:55PM +0300, Gal Pressman wrote:
> >> On 31/07/2019 14:46, Leon Romanovsky wrote:
> >>> On Wed, Jul 31, 2019 at 01:51:05PM +0300, Gal Pressman wrote:
> >>>> On 31/07/2019 10:41, Leon Romanovsky wrote:
> >>>>> On Wed, Jul 31, 2019 at 10:22:42AM +0300, Gal Pressman wrote:
> >>>>>> On 30/07/2019 18:41, Leon Romanovsky wrote:
> >>>>>>> On Tue, Jul 30, 2019 at 06:18:33PM +0300, Gal Pressman wrote:
> >>>>>>>> Add ratelimited helpers to the ibdev_* printk functions.
> >>>>>>>> Implementation inspired by counterpart dev_*_ratelimited functions.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Gal Pressman <galpress@amazon.com>
> >>>>>>>> ---
> >>>>>>>> include/rdma/ib_verbs.h | 51 +++++++++++++++++++++++++++++++++++++++++
> >>>>>>>> 1 file changed, 51 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> >>>>>>>> index c5f8a9f17063..356e6a105366 100644
> >>>>>>>> --- a/include/rdma/ib_verbs.h
> >>>>>>>> +++ b/include/rdma/ib_verbs.h
> >>>>>>>> @@ -107,6 +107,57 @@ static inline
> >>>>>>>> void ibdev_dbg(const struct ib_device *ibdev, const char *format, ...) {}
> >>>>>>>> #endif
> >>>>>>>>
> >>>>>>>> +#define ibdev_level_ratelimited(ibdev_level, ibdev, fmt, ...) \
> >>>>>>>> +do { \
> >>>>>>>> + static DEFINE_RATELIMIT_STATE(_rs, \
> >>>>>>>> + DEFAULT_RATELIMIT_INTERVAL, \
> >>>>>>>> + DEFAULT_RATELIMIT_BURST); \
> >>>>>>>> + if (__ratelimit(&_rs)) \
> >>>>>>>> + ibdev_level(ibdev, fmt, ##__VA_ARGS__); \
> >>>>>>>> +} while (0)
> >>>>>>>> +
> >>>>>>>> +#define ibdev_emerg_ratelimited(ibdev, fmt, ...) \
> >>>>>>>> + ibdev_level_ratelimited(ibdev_emerg, ibdev, fmt, ##__VA_ARGS__)
> >>>>>>>> +#define ibdev_alert_ratelimited(ibdev, fmt, ...) \
> >>>>>>>> + ibdev_level_ratelimited(ibdev_alert, ibdev, fmt, ##__VA_ARGS__)
> >>>>>>>> +#define ibdev_crit_ratelimited(ibdev, fmt, ...) \
> >>>>>>>> + ibdev_level_ratelimited(ibdev_crit, ibdev, fmt, ##__VA_ARGS__)
> >>>>>>>> +#define ibdev_err_ratelimited(ibdev, fmt, ...) \
> >>>>>>>> + ibdev_level_ratelimited(ibdev_err, ibdev, fmt, ##__VA_ARGS__)
> >>>>>>>> +#define ibdev_warn_ratelimited(ibdev, fmt, ...) \
> >>>>>>>> + ibdev_level_ratelimited(ibdev_warn, ibdev, fmt, ##__VA_ARGS__)
> >>>>>>>> +#define ibdev_notice_ratelimited(ibdev, fmt, ...) \
> >>>>>>>> + ibdev_level_ratelimited(ibdev_notice, ibdev, fmt, ##__VA_ARGS__)
> >>>>>>>> +#define ibdev_info_ratelimited(ibdev, fmt, ...) \
> >>>>>>>> + ibdev_level_ratelimited(ibdev_info, ibdev, fmt, ##__VA_ARGS__)
> >>>>>>>> +
> >>>>>>>> +#if defined(CONFIG_DYNAMIC_DEBUG)
> >>>>>>>> +/* descriptor check is first to prevent flooding with "callbacks suppressed" */
> >>>>>>>> +#define ibdev_dbg_ratelimited(ibdev, fmt, ...) \
> >>>>>>>> +do { \
> >>>>>>>> + static DEFINE_RATELIMIT_STATE(_rs, \
> >>>>>>>> + DEFAULT_RATELIMIT_INTERVAL, \
> >>>>>>>> + DEFAULT_RATELIMIT_BURST); \
> >>>>>>>> + DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt); \
> >>>>>>>> + if (DYNAMIC_DEBUG_BRANCH(descriptor) && __ratelimit(&_rs)) \
> >>>>>>>> + __dynamic_ibdev_dbg(&descriptor, ibdev, fmt, \
> >>>>>>>> + ##__VA_ARGS__); \
> >>>>>>>> +} while (0)
> >>>>>>>> +#elif defined(DEBUG)
> >>>>>>>
> >>>>>>> When will you see this CONFIG_DEBUG set? I suspect only in private
> >>>>>>> out-of-tree builds which we are not really care. Also I can't imagine
> >>>>>>> system with this CONFIG_DEBUG and without CONFIG_DYNAMIC_DEBUG.
> >>>>>>
> >>>>>> This is the common way to handle debug prints, see:
> >>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/printk.h#L331
> >>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/device.h#L1493
> >>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/netdevice.h#L4743
> >>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/net.h#L266
> >>>>>
> >>>>> I'm more interested to know the real usage of this copy/paste and
> >>>>> understand if it makes sense for drivers/infiniband/* or not.
> >>>>>
> >>>>> Not everything in netdev is great and worth to borrow.
> >>>>
> >>>> DEBUG exists since the first commit in the tree, and is used in various parts of
> >>>> the kernel (mlx5 as well). Do you think it should be removed from the kernel?
> >>>
> >>> It is gradually removed when it is spotted, I'll send a patch for mlx5 now.
> >>
> >> Was there an on-list discussion regarding removal of DEBUG usage? Can you please
> >> share a link?
> >
> > Sorry, but no, I didn't know that I need to save all discussions I see
> > in the mailing lists.
>
> Trying to understand whether "It is gradually removed when it is spotted" is a
> well known guideline by the community, should checkpatch produce a warning for this?
I didn't see checks in checkpatch about tabs<->spaces mix either which you
pointed for hns guys.
>
> >
> >> If so, I agree the DEBUG part should be removed.
> >>
> >>>
> >>>>
> >>>> Regarding combination of both, I don't think DEBUG is related to
> >>>> CONFIG_DYNAMIC_DEBUG. DEBUG is a generic debug flag (not necessarily to prints)
> >>>> while CONFIG_DYNAMIC_DEBUG is specific to the dynamic debug prints infrastructure.
> >>>
> >>> I know exactly what DEBUG and CONFIG_DYNAMIC_DEBUG mean, but I'm
> >>> asking YOU to provide us real and in-tree scenario where DEBUG will
> >>> exists and CONFIG_DYNAMIC_DEBUG won't.
> >>
> >> What's any of this has to do with in-tree? This code and defines are part of the
> >> tree.
> >>
> >> The use case doesn't matter, it's a valid permutation. Is there anything that
> >> stops a user from building the kernel this way?
> >
> > Like everything else, nothing stops from you to do any modifications to
> > the source code, before you will build. We are talking about in-tree
> > builds and distro kernels.
>
> Last I checked turning on DEBUG doesn't require source code changes?
Exciting, how did you enable DEBUG without recompiling source code?
Maybe you find a way to enable DEBUG on running kernel?
And how did it come that v5.3 kernel was compiled with DEBUG but
without DYNAMIC_DEBUG?
Thanks
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH for-next 1/2] RDMA/core: Introduce ratelimited ibdev printk functions
2019-07-31 14:50 ` Leon Romanovsky
@ 2019-07-31 15:26 ` Gal Pressman
2019-07-31 17:54 ` Leon Romanovsky
0 siblings, 1 reply; 14+ messages in thread
From: Gal Pressman @ 2019-07-31 15:26 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: Jason Gunthorpe, Doug Ledford, linux-rdma
On 31/07/2019 17:50, Leon Romanovsky wrote:
> On Wed, Jul 31, 2019 at 05:19:41PM +0300, Gal Pressman wrote:
>> On 31/07/2019 16:33, Leon Romanovsky wrote:
>>> On Wed, Jul 31, 2019 at 03:56:55PM +0300, Gal Pressman wrote:
>>>> On 31/07/2019 14:46, Leon Romanovsky wrote:
>>>>> On Wed, Jul 31, 2019 at 01:51:05PM +0300, Gal Pressman wrote:
>>>>>> On 31/07/2019 10:41, Leon Romanovsky wrote:
>>>>>>> On Wed, Jul 31, 2019 at 10:22:42AM +0300, Gal Pressman wrote:
>>>>>>>> On 30/07/2019 18:41, Leon Romanovsky wrote:
>>>>>>>>> On Tue, Jul 30, 2019 at 06:18:33PM +0300, Gal Pressman wrote:
>>>>>>>>>> Add ratelimited helpers to the ibdev_* printk functions.
>>>>>>>>>> Implementation inspired by counterpart dev_*_ratelimited functions.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Gal Pressman <galpress@amazon.com>
>>>>>>>>>> ---
>>>>>>>>>> include/rdma/ib_verbs.h | 51 +++++++++++++++++++++++++++++++++++++++++
>>>>>>>>>> 1 file changed, 51 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
>>>>>>>>>> index c5f8a9f17063..356e6a105366 100644
>>>>>>>>>> --- a/include/rdma/ib_verbs.h
>>>>>>>>>> +++ b/include/rdma/ib_verbs.h
>>>>>>>>>> @@ -107,6 +107,57 @@ static inline
>>>>>>>>>> void ibdev_dbg(const struct ib_device *ibdev, const char *format, ...) {}
>>>>>>>>>> #endif
>>>>>>>>>>
>>>>>>>>>> +#define ibdev_level_ratelimited(ibdev_level, ibdev, fmt, ...) \
>>>>>>>>>> +do { \
>>>>>>>>>> + static DEFINE_RATELIMIT_STATE(_rs, \
>>>>>>>>>> + DEFAULT_RATELIMIT_INTERVAL, \
>>>>>>>>>> + DEFAULT_RATELIMIT_BURST); \
>>>>>>>>>> + if (__ratelimit(&_rs)) \
>>>>>>>>>> + ibdev_level(ibdev, fmt, ##__VA_ARGS__); \
>>>>>>>>>> +} while (0)
>>>>>>>>>> +
>>>>>>>>>> +#define ibdev_emerg_ratelimited(ibdev, fmt, ...) \
>>>>>>>>>> + ibdev_level_ratelimited(ibdev_emerg, ibdev, fmt, ##__VA_ARGS__)
>>>>>>>>>> +#define ibdev_alert_ratelimited(ibdev, fmt, ...) \
>>>>>>>>>> + ibdev_level_ratelimited(ibdev_alert, ibdev, fmt, ##__VA_ARGS__)
>>>>>>>>>> +#define ibdev_crit_ratelimited(ibdev, fmt, ...) \
>>>>>>>>>> + ibdev_level_ratelimited(ibdev_crit, ibdev, fmt, ##__VA_ARGS__)
>>>>>>>>>> +#define ibdev_err_ratelimited(ibdev, fmt, ...) \
>>>>>>>>>> + ibdev_level_ratelimited(ibdev_err, ibdev, fmt, ##__VA_ARGS__)
>>>>>>>>>> +#define ibdev_warn_ratelimited(ibdev, fmt, ...) \
>>>>>>>>>> + ibdev_level_ratelimited(ibdev_warn, ibdev, fmt, ##__VA_ARGS__)
>>>>>>>>>> +#define ibdev_notice_ratelimited(ibdev, fmt, ...) \
>>>>>>>>>> + ibdev_level_ratelimited(ibdev_notice, ibdev, fmt, ##__VA_ARGS__)
>>>>>>>>>> +#define ibdev_info_ratelimited(ibdev, fmt, ...) \
>>>>>>>>>> + ibdev_level_ratelimited(ibdev_info, ibdev, fmt, ##__VA_ARGS__)
>>>>>>>>>> +
>>>>>>>>>> +#if defined(CONFIG_DYNAMIC_DEBUG)
>>>>>>>>>> +/* descriptor check is first to prevent flooding with "callbacks suppressed" */
>>>>>>>>>> +#define ibdev_dbg_ratelimited(ibdev, fmt, ...) \
>>>>>>>>>> +do { \
>>>>>>>>>> + static DEFINE_RATELIMIT_STATE(_rs, \
>>>>>>>>>> + DEFAULT_RATELIMIT_INTERVAL, \
>>>>>>>>>> + DEFAULT_RATELIMIT_BURST); \
>>>>>>>>>> + DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt); \
>>>>>>>>>> + if (DYNAMIC_DEBUG_BRANCH(descriptor) && __ratelimit(&_rs)) \
>>>>>>>>>> + __dynamic_ibdev_dbg(&descriptor, ibdev, fmt, \
>>>>>>>>>> + ##__VA_ARGS__); \
>>>>>>>>>> +} while (0)
>>>>>>>>>> +#elif defined(DEBUG)
>>>>>>>>>
>>>>>>>>> When will you see this CONFIG_DEBUG set? I suspect only in private
>>>>>>>>> out-of-tree builds which we are not really care. Also I can't imagine
>>>>>>>>> system with this CONFIG_DEBUG and without CONFIG_DYNAMIC_DEBUG.
>>>>>>>>
>>>>>>>> This is the common way to handle debug prints, see:
>>>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/printk.h#L331
>>>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/device.h#L1493
>>>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/netdevice.h#L4743
>>>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/net.h#L266
>>>>>>>
>>>>>>> I'm more interested to know the real usage of this copy/paste and
>>>>>>> understand if it makes sense for drivers/infiniband/* or not.
>>>>>>>
>>>>>>> Not everything in netdev is great and worth to borrow.
>>>>>>
>>>>>> DEBUG exists since the first commit in the tree, and is used in various parts of
>>>>>> the kernel (mlx5 as well). Do you think it should be removed from the kernel?
>>>>>
>>>>> It is gradually removed when it is spotted, I'll send a patch for mlx5 now.
>>>>
>>>> Was there an on-list discussion regarding removal of DEBUG usage? Can you please
>>>> share a link?
>>>
>>> Sorry, but no, I didn't know that I need to save all discussions I see
>>> in the mailing lists.
>>
>> Trying to understand whether "It is gradually removed when it is spotted" is a
>> well known guideline by the community, should checkpatch produce a warning for this?
>
> I didn't see checks in checkpatch about tabs<->spaces mix either which you
> pointed for hns guys.
Ofcourse there are, this patch was full of checkpatch warnings.
But that's not the point, you're avoiding answering a simple question: is DEBUG
usage discouraged by the community?
>
>>
>>>
>>>> If so, I agree the DEBUG part should be removed.
>>>>
>>>>>
>>>>>>
>>>>>> Regarding combination of both, I don't think DEBUG is related to
>>>>>> CONFIG_DYNAMIC_DEBUG. DEBUG is a generic debug flag (not necessarily to prints)
>>>>>> while CONFIG_DYNAMIC_DEBUG is specific to the dynamic debug prints infrastructure.
>>>>>
>>>>> I know exactly what DEBUG and CONFIG_DYNAMIC_DEBUG mean, but I'm
>>>>> asking YOU to provide us real and in-tree scenario where DEBUG will
>>>>> exists and CONFIG_DYNAMIC_DEBUG won't.
>>>>
>>>> What's any of this has to do with in-tree? This code and defines are part of the
>>>> tree.
>>>>
>>>> The use case doesn't matter, it's a valid permutation. Is there anything that
>>>> stops a user from building the kernel this way?
>>>
>>> Like everything else, nothing stops from you to do any modifications to
>>> the source code, before you will build. We are talking about in-tree
>>> builds and distro kernels.
>>
>> Last I checked turning on DEBUG doesn't require source code changes?
>
> Exciting, how did you enable DEBUG without recompiling source code?
Recompiling source code != changing source code.
You can turn on DEBUG when compiling the kernel (i.e running make) with no
source code changes (again, last I checked, did this change lately?).
> Maybe you find a way to enable DEBUG on running kernel?
>
> And how did it come that v5.3 kernel was compiled with DEBUG but
> without DYNAMIC_DEBUG?
Change CONFIG_DYNAMIC_DEBUG=n in your .config and pass DEBUG to make.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH for-next 1/2] RDMA/core: Introduce ratelimited ibdev printk functions
2019-07-31 15:26 ` Gal Pressman
@ 2019-07-31 17:54 ` Leon Romanovsky
0 siblings, 0 replies; 14+ messages in thread
From: Leon Romanovsky @ 2019-07-31 17:54 UTC (permalink / raw)
To: Gal Pressman; +Cc: Jason Gunthorpe, Doug Ledford, linux-rdma
On Wed, Jul 31, 2019 at 06:26:16PM +0300, Gal Pressman wrote:
> On 31/07/2019 17:50, Leon Romanovsky wrote:
> > On Wed, Jul 31, 2019 at 05:19:41PM +0300, Gal Pressman wrote:
> >> On 31/07/2019 16:33, Leon Romanovsky wrote:
> >>> On Wed, Jul 31, 2019 at 03:56:55PM +0300, Gal Pressman wrote:
> >>>> On 31/07/2019 14:46, Leon Romanovsky wrote:
> >>>>> On Wed, Jul 31, 2019 at 01:51:05PM +0300, Gal Pressman wrote:
> >>>>>> On 31/07/2019 10:41, Leon Romanovsky wrote:
> >>>>>>> On Wed, Jul 31, 2019 at 10:22:42AM +0300, Gal Pressman wrote:
> >>>>>>>> On 30/07/2019 18:41, Leon Romanovsky wrote:
> >>>>>>>>> On Tue, Jul 30, 2019 at 06:18:33PM +0300, Gal Pressman wrote:
> >>>>>>>>>> Add ratelimited helpers to the ibdev_* printk functions.
> >>>>>>>>>> Implementation inspired by counterpart dev_*_ratelimited functions.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Gal Pressman <galpress@amazon.com>
> >>>>>>>>>> ---
> >>>>>>>>>> include/rdma/ib_verbs.h | 51 +++++++++++++++++++++++++++++++++++++++++
> >>>>>>>>>> 1 file changed, 51 insertions(+)
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> >>>>>>>>>> index c5f8a9f17063..356e6a105366 100644
> >>>>>>>>>> --- a/include/rdma/ib_verbs.h
> >>>>>>>>>> +++ b/include/rdma/ib_verbs.h
> >>>>>>>>>> @@ -107,6 +107,57 @@ static inline
> >>>>>>>>>> void ibdev_dbg(const struct ib_device *ibdev, const char *format, ...) {}
> >>>>>>>>>> #endif
> >>>>>>>>>>
> >>>>>>>>>> +#define ibdev_level_ratelimited(ibdev_level, ibdev, fmt, ...) \
> >>>>>>>>>> +do { \
> >>>>>>>>>> + static DEFINE_RATELIMIT_STATE(_rs, \
> >>>>>>>>>> + DEFAULT_RATELIMIT_INTERVAL, \
> >>>>>>>>>> + DEFAULT_RATELIMIT_BURST); \
> >>>>>>>>>> + if (__ratelimit(&_rs)) \
> >>>>>>>>>> + ibdev_level(ibdev, fmt, ##__VA_ARGS__); \
> >>>>>>>>>> +} while (0)
> >>>>>>>>>> +
> >>>>>>>>>> +#define ibdev_emerg_ratelimited(ibdev, fmt, ...) \
> >>>>>>>>>> + ibdev_level_ratelimited(ibdev_emerg, ibdev, fmt, ##__VA_ARGS__)
> >>>>>>>>>> +#define ibdev_alert_ratelimited(ibdev, fmt, ...) \
> >>>>>>>>>> + ibdev_level_ratelimited(ibdev_alert, ibdev, fmt, ##__VA_ARGS__)
> >>>>>>>>>> +#define ibdev_crit_ratelimited(ibdev, fmt, ...) \
> >>>>>>>>>> + ibdev_level_ratelimited(ibdev_crit, ibdev, fmt, ##__VA_ARGS__)
> >>>>>>>>>> +#define ibdev_err_ratelimited(ibdev, fmt, ...) \
> >>>>>>>>>> + ibdev_level_ratelimited(ibdev_err, ibdev, fmt, ##__VA_ARGS__)
> >>>>>>>>>> +#define ibdev_warn_ratelimited(ibdev, fmt, ...) \
> >>>>>>>>>> + ibdev_level_ratelimited(ibdev_warn, ibdev, fmt, ##__VA_ARGS__)
> >>>>>>>>>> +#define ibdev_notice_ratelimited(ibdev, fmt, ...) \
> >>>>>>>>>> + ibdev_level_ratelimited(ibdev_notice, ibdev, fmt, ##__VA_ARGS__)
> >>>>>>>>>> +#define ibdev_info_ratelimited(ibdev, fmt, ...) \
> >>>>>>>>>> + ibdev_level_ratelimited(ibdev_info, ibdev, fmt, ##__VA_ARGS__)
> >>>>>>>>>> +
> >>>>>>>>>> +#if defined(CONFIG_DYNAMIC_DEBUG)
> >>>>>>>>>> +/* descriptor check is first to prevent flooding with "callbacks suppressed" */
> >>>>>>>>>> +#define ibdev_dbg_ratelimited(ibdev, fmt, ...) \
> >>>>>>>>>> +do { \
> >>>>>>>>>> + static DEFINE_RATELIMIT_STATE(_rs, \
> >>>>>>>>>> + DEFAULT_RATELIMIT_INTERVAL, \
> >>>>>>>>>> + DEFAULT_RATELIMIT_BURST); \
> >>>>>>>>>> + DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt); \
> >>>>>>>>>> + if (DYNAMIC_DEBUG_BRANCH(descriptor) && __ratelimit(&_rs)) \
> >>>>>>>>>> + __dynamic_ibdev_dbg(&descriptor, ibdev, fmt, \
> >>>>>>>>>> + ##__VA_ARGS__); \
> >>>>>>>>>> +} while (0)
> >>>>>>>>>> +#elif defined(DEBUG)
> >>>>>>>>>
> >>>>>>>>> When will you see this CONFIG_DEBUG set? I suspect only in private
> >>>>>>>>> out-of-tree builds which we are not really care. Also I can't imagine
> >>>>>>>>> system with this CONFIG_DEBUG and without CONFIG_DYNAMIC_DEBUG.
> >>>>>>>>
> >>>>>>>> This is the common way to handle debug prints, see:
> >>>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/printk.h#L331
> >>>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/device.h#L1493
> >>>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/netdevice.h#L4743
> >>>>>>>> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/net.h#L266
> >>>>>>>
> >>>>>>> I'm more interested to know the real usage of this copy/paste and
> >>>>>>> understand if it makes sense for drivers/infiniband/* or not.
> >>>>>>>
> >>>>>>> Not everything in netdev is great and worth to borrow.
> >>>>>>
> >>>>>> DEBUG exists since the first commit in the tree, and is used in various parts of
> >>>>>> the kernel (mlx5 as well). Do you think it should be removed from the kernel?
> >>>>>
> >>>>> It is gradually removed when it is spotted, I'll send a patch for mlx5 now.
> >>>>
> >>>> Was there an on-list discussion regarding removal of DEBUG usage? Can you please
> >>>> share a link?
> >>>
> >>> Sorry, but no, I didn't know that I need to save all discussions I see
> >>> in the mailing lists.
> >>
> >> Trying to understand whether "It is gradually removed when it is spotted" is a
> >> well known guideline by the community, should checkpatch produce a warning for this?
> >
> > I didn't see checks in checkpatch about tabs<->spaces mix either which you
> > pointed for hns guys.
>
> Ofcourse there are, this patch was full of checkpatch warnings.
> But that's not the point, you're avoiding answering a simple question: is DEBUG
> usage discouraged by the community?
Yes, I said it a couple of times, "community" is not excited to see
debug code in final code. The expectation that submitted code works
and doesn't need extra debug. If it is not true, such code is not ready
to be merged.
Anyway, simple grep in our subsystem shows that this "DEBUG" in use in one
place only (fmr) and it predates git era.
_ kernel git:(rdma-next) git grep DEBUG drivers/infiniband/ include/rdma/ | grep ifdef
There is no new code with this "DEBUG" flag.
>
> >
> >>
> >>>
> >>>> If so, I agree the DEBUG part should be removed.
> >>>>
> >>>>>
> >>>>>>
> >>>>>> Regarding combination of both, I don't think DEBUG is related to
> >>>>>> CONFIG_DYNAMIC_DEBUG. DEBUG is a generic debug flag (not necessarily to prints)
> >>>>>> while CONFIG_DYNAMIC_DEBUG is specific to the dynamic debug prints infrastructure.
> >>>>>
> >>>>> I know exactly what DEBUG and CONFIG_DYNAMIC_DEBUG mean, but I'm
> >>>>> asking YOU to provide us real and in-tree scenario where DEBUG will
> >>>>> exists and CONFIG_DYNAMIC_DEBUG won't.
> >>>>
> >>>> What's any of this has to do with in-tree? This code and defines are part of the
> >>>> tree.
> >>>>
> >>>> The use case doesn't matter, it's a valid permutation. Is there anything that
> >>>> stops a user from building the kernel this way?
> >>>
> >>> Like everything else, nothing stops from you to do any modifications to
> >>> the source code, before you will build. We are talking about in-tree
> >>> builds and distro kernels.
> >>
> >> Last I checked turning on DEBUG doesn't require source code changes?
> >
> > Exciting, how did you enable DEBUG without recompiling source code?
>
> Recompiling source code != changing source code.
I'm as a kernel developer don't see the difference.
> You can turn on DEBUG when compiling the kernel (i.e running make) with no
> source code changes (again, last I checked, did this change lately?).
>
> > Maybe you find a way to enable DEBUG on running kernel?
> >
> > And how did it come that v5.3 kernel was compiled with DEBUG but
> > without DYNAMIC_DEBUG?
>
> Change CONFIG_DYNAMIC_DEBUG=n in your .config and pass DEBUG to make.
I asked for a real example and not for allnoconfig compilation.
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-07-31 17:54 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-30 15:18 [PATCH for-next 0/2] Ratelimited ibdev printk functions Gal Pressman
2019-07-30 15:18 ` [PATCH for-next 1/2] RDMA/core: Introduce ratelimited " Gal Pressman
2019-07-30 15:41 ` Leon Romanovsky
2019-07-31 7:22 ` Gal Pressman
2019-07-31 7:41 ` Leon Romanovsky
2019-07-31 10:51 ` Gal Pressman
2019-07-31 11:46 ` Leon Romanovsky
2019-07-31 12:56 ` Gal Pressman
2019-07-31 13:33 ` Leon Romanovsky
2019-07-31 14:19 ` Gal Pressman
2019-07-31 14:50 ` Leon Romanovsky
2019-07-31 15:26 ` Gal Pressman
2019-07-31 17:54 ` Leon Romanovsky
2019-07-30 15:18 ` [PATCH for-next 2/2] RDMA/efa: Rate limit admin queue error prints Gal Pressman
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.