diff mbox series

[RFC,net-next] net/sched: introduce pretty printers for Qdiscs

Message ID 20230602162935.2380811-1-vladimir.oltean@nxp.com (mailing list archive)
State RFC
Delegated to: Netdev Maintainers
Headers show
Series [RFC,net-next] net/sched: introduce pretty printers for Qdiscs | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 11 this patch: 11
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 11 this patch: 11
netdev/checkpatch warning WARNING: Missing or malformed SPDX-License-Identifier tag in line 1 WARNING: Single statement macros should not use a do {} while (0) loop WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? WARNING: else is not generally useful after a break or return WARNING: macros should not use a trailing semicolon WARNING: printk() should include KERN_<LEVEL> facility level
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Vladimir Oltean June 2, 2023, 4:29 p.m. UTC 
Sometimes when debugging Qdiscs it may be confusing to know exactly what
you're looking at, especially since they're hierarchical. Pretty printing
the handle, parent handle and netdev is a bit cumbersome, so this patch
proposes a set of wrappers around __qdisc_printk() which are heavily
inspired from __net_printk().

It is assumed that these printers will be more useful for untalented
kernel hackers such as myself rather than to the overall quality of the
code, since Qdiscs rarely print stuff to the kernel log by design (and
where they do, maybe that should be reconsidered in the first place).

A single demo conversion has been made, there is room for more if the
idea is appreciated.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 include/net/sch_debug.h | 45 +++++++++++++++++++++
 net/sched/Makefile      |  2 +-
 net/sched/sch_api.c     |  4 +-
 net/sched/sch_debug.c   | 86 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 134 insertions(+), 3 deletions(-)
 create mode 100644 include/net/sch_debug.h
 create mode 100644 net/sched/sch_debug.c

Comments

Pedro Tammela June 3, 2023, 1:28 a.m. UTC | #1 
On 02/06/2023 13:29, Vladimir Oltean wrote:
> Sometimes when debugging Qdiscs it may be confusing to know exactly what
> you're looking at, especially since they're hierarchical. Pretty printing
> the handle, parent handle and netdev is a bit cumbersome, so this patch
> proposes a set of wrappers around __qdisc_printk() which are heavily
> inspired from __net_printk().

Probably worth mentioning drgn 
(https://drgn.readthedocs.io/en/latest/user_guide.html)
I've picked it up recently after Peilin mentioned it and it's quite helpful.

> 
> It is assumed that these printers will be more useful for untalented
> kernel hackers such as myself rather than to the overall quality of the
> code, since Qdiscs rarely print stuff to the kernel log by design (and
> where they do, maybe that should be reconsidered in the first place).
> 
> A single demo conversion has been made, there is room for more if the
> idea is appreciated.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>   include/net/sch_debug.h | 45 +++++++++++++++++++++
>   net/sched/Makefile      |  2 +-
>   net/sched/sch_api.c     |  4 +-
>   net/sched/sch_debug.c   | 86 +++++++++++++++++++++++++++++++++++++++++
>   4 files changed, 134 insertions(+), 3 deletions(-)
>   create mode 100644 include/net/sch_debug.h
>   create mode 100644 net/sched/sch_debug.c
> 
> diff --git a/include/net/sch_debug.h b/include/net/sch_debug.h
> new file mode 100644
> index 000000000000..032de4710671
> --- /dev/null
> +++ b/include/net/sch_debug.h
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __NET_SCHED_DEBUG_H
> +#define __NET_SCHED_DEBUG_H
> +
> +#include <linux/bug.h>
> +#include <linux/kern_levels.h>
> +
> +struct Qdisc;
> +
> +__printf(3, 4) __cold
> +void qdisc_printk(const char *level, const struct Qdisc *sch,
> +		  const char *format, ...);
> +__printf(2, 3) __cold
> +void qdisc_emerg(const struct Qdisc *sch, const char *format, ...);
> +__printf(2, 3) __cold
> +void qdisc_alert(const struct Qdisc *sch, const char *format, ...);
> +__printf(2, 3) __cold
> +void qdisc_crit(const struct Qdisc *sch, const char *format, ...);
> +__printf(2, 3) __cold
> +void qdisc_err(const struct Qdisc *sch, const char *format, ...);
> +__printf(2, 3) __cold
> +void qdisc_warn(const struct Qdisc *sch, const char *format, ...);
> +__printf(2, 3) __cold
> +void qdisc_notice(const struct Qdisc *sch, const char *format, ...);
> +__printf(2, 3) __cold
> +void qdisc_info(const struct Qdisc *sch, const char *format, ...);
> +
> +#if defined(CONFIG_DYNAMIC_DEBUG) || \
> +	(defined(CONFIG_DYNAMIC_DEBUG_CORE) && defined(DYNAMIC_DEBUG_MODULE))
> +#define qdisc_dbg(__sch, format, args...)			\
> +do {								\
> +	dynamic_qdisc_dbg(__sch, format, ##args);		\
> +} while (0)
> +#elif defined(DEBUG)
> +#define qdisc_dbg(__sch, format, args...)			\
> +	qdisc_printk(KERN_DEBUG, __sch, format, ##args)
> +#else
> +#define qdisc_dbg(__sch, format, args...)			\
> +({								\
> +	if (0)							\
> +		qdisc_printk(KERN_DEBUG, __sch, format, ##args); \
> +})
> +#endif
> +
> +#endif
> diff --git a/net/sched/Makefile b/net/sched/Makefile
> index b5fd49641d91..ab13bf7db283 100644
> --- a/net/sched/Makefile
> +++ b/net/sched/Makefile
> @@ -6,7 +6,7 @@
>   obj-y	:= sch_generic.o sch_mq.o
>   
>   obj-$(CONFIG_INET)		+= sch_frag.o
> -obj-$(CONFIG_NET_SCHED)		+= sch_api.o sch_blackhole.o
> +obj-$(CONFIG_NET_SCHED)		+= sch_api.o sch_blackhole.o sch_debug.o
>   obj-$(CONFIG_NET_CLS)		+= cls_api.o
>   obj-$(CONFIG_NET_CLS_ACT)	+= act_api.o
>   obj-$(CONFIG_NET_ACT_POLICE)	+= act_police.o
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index fdb8f429333d..a6bfe2e40f89 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -31,6 +31,7 @@
>   #include <net/netlink.h>
>   #include <net/pkt_sched.h>
>   #include <net/pkt_cls.h>
> +#include <net/sch_debug.h>
>   #include <net/tc_wrapper.h>
>   
>   #include <trace/events/qdisc.h>
> @@ -597,8 +598,7 @@ EXPORT_SYMBOL(__qdisc_calculate_pkt_len);
>   void qdisc_warn_nonwc(const char *txt, struct Qdisc *qdisc)
>   {
>   	if (!(qdisc->flags & TCQ_F_WARN_NONWC)) {
> -		pr_warn("%s: %s qdisc %X: is non-work-conserving?\n",
> -			txt, qdisc->ops->id, qdisc->handle >> 16);
> +		qdisc_warn(qdisc, "%s: qdisc is non-work-conserving?\n", txt);

Looks much better

>   		qdisc->flags |= TCQ_F_WARN_NONWC;
>   	}
>   }
> diff --git a/net/sched/sch_debug.c b/net/sched/sch_debug.c
> new file mode 100644
> index 000000000000..1f47dac88c6e
> --- /dev/null
> +++ b/net/sched/sch_debug.c
> @@ -0,0 +1,86 @@ > +#include <linux/pkt_sched.h>
> +#include <net/sch_generic.h>
> +#include <net/sch_debug.h>
> +
> +static void qdisc_handle_str(u32 handle, char *str)
> +{
> +	if (handle == TC_H_ROOT) {
> +		sprintf(str, "root");
> +		return;
> +	} else if (handle == TC_H_INGRESS) {
> +		sprintf(str, "ingress");
> +		return;
> +	} else {
> +		sprintf(str, "%x:%x", TC_H_MAJ(handle) >> 16, TC_H_MIN(handle));
> +		return;
> +	}

nit: Did you consider a switch?

> +}
> +
> +static void __qdisc_printk(const char *level, const struct Qdisc *sch,
> +			   struct va_format *vaf)
> +{
> +	struct net_device *dev = qdisc_dev(sch);
> +	char handle_str[10], parent_str[10];
> +
> +	qdisc_handle_str(sch->handle, handle_str);
> +	qdisc_handle_str(sch->parent, parent_str);
> +
> +	if (dev && dev->dev.parent) {
> +		dev_printk_emit(level[1] - '0',
> +				dev->dev.parent,
> +				"%s %s %s%s %s %s parent %s: %pV",
> +				dev_driver_string(dev->dev.parent),
> +				dev_name(dev->dev.parent),
> +				netdev_name(dev), netdev_reg_state(dev),
> +				sch->ops->id, handle_str, parent_str, vaf);
> +	} else if (dev) {
> +		printk("%s%s%s %s %s parent %s: %pV",
> +		       level, netdev_name(dev), netdev_reg_state(dev),
> +		       sch->ops->id, handle_str, parent_str, vaf);
> +	} else {
> +		printk("%s(NULL net_device) %s %s parent %s: %pV", level,
> +		       sch->ops->id, handle_str, parent_str, vaf);
> +	}
> +}
> +
> +void qdisc_printk(const char *level, const struct Qdisc *sch,
> +		  const char *format, ...)
> +{
> +	struct va_format vaf;
> +	va_list args;
> +
> +	va_start(args, format);
> +
> +	vaf.fmt = format;
> +	vaf.va = &args;
> +
> +	__qdisc_printk(level, sch, &vaf);
> +
> +	va_end(args);
> +}
> +EXPORT_SYMBOL(qdisc_printk);
> +
> +#define define_qdisc_printk_level(func, level)			\
> +void func(const struct Qdisc *sch, const char *fmt, ...)	\
> +{								\
> +	struct va_format vaf;					\
> +	va_list args;						\
> +								\
> +	va_start(args, fmt);					\
> +								\
> +	vaf.fmt = fmt;						\
> +	vaf.va = &args;						\
> +								\
> +	__qdisc_printk(level, sch, &vaf);			\
> +								\
> +	va_end(args);						\
> +}								\
> +EXPORT_SYMBOL(func);
> +
> +define_qdisc_printk_level(qdisc_emerg, KERN_EMERG);
> +define_qdisc_printk_level(qdisc_alert, KERN_ALERT);
> +define_qdisc_printk_level(qdisc_crit, KERN_CRIT);
> +define_qdisc_printk_level(qdisc_err, KERN_ERR);
> +define_qdisc_printk_level(qdisc_warn, KERN_WARNING);
> +define_qdisc_printk_level(qdisc_notice, KERN_NOTICE);
> +define_qdisc_printk_level(qdisc_info, KERN_INFO);
diff mbox series

Patch

diff --git a/include/net/sch_debug.h b/include/net/sch_debug.h
new file mode 100644
index 000000000000..032de4710671
--- /dev/null
+++ b/include/net/sch_debug.h
@@ -0,0 +1,45 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __NET_SCHED_DEBUG_H
+#define __NET_SCHED_DEBUG_H
+
+#include <linux/bug.h>
+#include <linux/kern_levels.h>
+
+struct Qdisc;
+
+__printf(3, 4) __cold
+void qdisc_printk(const char *level, const struct Qdisc *sch,
+		  const char *format, ...);
+__printf(2, 3) __cold
+void qdisc_emerg(const struct Qdisc *sch, const char *format, ...);
+__printf(2, 3) __cold
+void qdisc_alert(const struct Qdisc *sch, const char *format, ...);
+__printf(2, 3) __cold
+void qdisc_crit(const struct Qdisc *sch, const char *format, ...);
+__printf(2, 3) __cold
+void qdisc_err(const struct Qdisc *sch, const char *format, ...);
+__printf(2, 3) __cold
+void qdisc_warn(const struct Qdisc *sch, const char *format, ...);
+__printf(2, 3) __cold
+void qdisc_notice(const struct Qdisc *sch, const char *format, ...);
+__printf(2, 3) __cold
+void qdisc_info(const struct Qdisc *sch, const char *format, ...);
+
+#if defined(CONFIG_DYNAMIC_DEBUG) || \
+	(defined(CONFIG_DYNAMIC_DEBUG_CORE) && defined(DYNAMIC_DEBUG_MODULE))
+#define qdisc_dbg(__sch, format, args...)			\
+do {								\
+	dynamic_qdisc_dbg(__sch, format, ##args);		\
+} while (0)
+#elif defined(DEBUG)
+#define qdisc_dbg(__sch, format, args...)			\
+	qdisc_printk(KERN_DEBUG, __sch, format, ##args)
+#else
+#define qdisc_dbg(__sch, format, args...)			\
+({								\
+	if (0)							\
+		qdisc_printk(KERN_DEBUG, __sch, format, ##args); \
+})
+#endif
+
+#endif
diff --git a/net/sched/Makefile b/net/sched/Makefile
index b5fd49641d91..ab13bf7db283 100644
--- a/net/sched/Makefile
+++ b/net/sched/Makefile
@@ -6,7 +6,7 @@ 
 obj-y	:= sch_generic.o sch_mq.o
 
 obj-$(CONFIG_INET)		+= sch_frag.o
-obj-$(CONFIG_NET_SCHED)		+= sch_api.o sch_blackhole.o
+obj-$(CONFIG_NET_SCHED)		+= sch_api.o sch_blackhole.o sch_debug.o
 obj-$(CONFIG_NET_CLS)		+= cls_api.o
 obj-$(CONFIG_NET_CLS_ACT)	+= act_api.o
 obj-$(CONFIG_NET_ACT_POLICE)	+= act_police.o
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index fdb8f429333d..a6bfe2e40f89 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -31,6 +31,7 @@ 
 #include <net/netlink.h>
 #include <net/pkt_sched.h>
 #include <net/pkt_cls.h>
+#include <net/sch_debug.h>
 #include <net/tc_wrapper.h>
 
 #include <trace/events/qdisc.h>
@@ -597,8 +598,7 @@  EXPORT_SYMBOL(__qdisc_calculate_pkt_len);
 void qdisc_warn_nonwc(const char *txt, struct Qdisc *qdisc)
 {
 	if (!(qdisc->flags & TCQ_F_WARN_NONWC)) {
-		pr_warn("%s: %s qdisc %X: is non-work-conserving?\n",
-			txt, qdisc->ops->id, qdisc->handle >> 16);
+		qdisc_warn(qdisc, "%s: qdisc is non-work-conserving?\n", txt);
 		qdisc->flags |= TCQ_F_WARN_NONWC;
 	}
 }
diff --git a/net/sched/sch_debug.c b/net/sched/sch_debug.c
new file mode 100644
index 000000000000..1f47dac88c6e
--- /dev/null
+++ b/net/sched/sch_debug.c
@@ -0,0 +1,86 @@ 
+#include <linux/pkt_sched.h>
+#include <net/sch_generic.h>
+#include <net/sch_debug.h>
+
+static void qdisc_handle_str(u32 handle, char *str)
+{
+	if (handle == TC_H_ROOT) {
+		sprintf(str, "root");
+		return;
+	} else if (handle == TC_H_INGRESS) {
+		sprintf(str, "ingress");
+		return;
+	} else {
+		sprintf(str, "%x:%x", TC_H_MAJ(handle) >> 16, TC_H_MIN(handle));
+		return;
+	}
+}
+
+static void __qdisc_printk(const char *level, const struct Qdisc *sch,
+			   struct va_format *vaf)
+{
+	struct net_device *dev = qdisc_dev(sch);
+	char handle_str[10], parent_str[10];
+
+	qdisc_handle_str(sch->handle, handle_str);
+	qdisc_handle_str(sch->parent, parent_str);
+
+	if (dev && dev->dev.parent) {
+		dev_printk_emit(level[1] - '0',
+				dev->dev.parent,
+				"%s %s %s%s %s %s parent %s: %pV",
+				dev_driver_string(dev->dev.parent),
+				dev_name(dev->dev.parent),
+				netdev_name(dev), netdev_reg_state(dev),
+				sch->ops->id, handle_str, parent_str, vaf);
+	} else if (dev) {
+		printk("%s%s%s %s %s parent %s: %pV",
+		       level, netdev_name(dev), netdev_reg_state(dev),
+		       sch->ops->id, handle_str, parent_str, vaf);
+	} else {
+		printk("%s(NULL net_device) %s %s parent %s: %pV", level,
+		       sch->ops->id, handle_str, parent_str, vaf);
+	}
+}
+
+void qdisc_printk(const char *level, const struct Qdisc *sch,
+		  const char *format, ...)
+{
+	struct va_format vaf;
+	va_list args;
+
+	va_start(args, format);
+
+	vaf.fmt = format;
+	vaf.va = &args;
+
+	__qdisc_printk(level, sch, &vaf);
+
+	va_end(args);
+}
+EXPORT_SYMBOL(qdisc_printk);
+
+#define define_qdisc_printk_level(func, level)			\
+void func(const struct Qdisc *sch, const char *fmt, ...)	\
+{								\
+	struct va_format vaf;					\
+	va_list args;						\
+								\
+	va_start(args, fmt);					\
+								\
+	vaf.fmt = fmt;						\
+	vaf.va = &args;						\
+								\
+	__qdisc_printk(level, sch, &vaf);			\
+								\
+	va_end(args);						\
+}								\
+EXPORT_SYMBOL(func);
+
+define_qdisc_printk_level(qdisc_emerg, KERN_EMERG);
+define_qdisc_printk_level(qdisc_alert, KERN_ALERT);
+define_qdisc_printk_level(qdisc_crit, KERN_CRIT);
+define_qdisc_printk_level(qdisc_err, KERN_ERR);
+define_qdisc_printk_level(qdisc_warn, KERN_WARNING);
+define_qdisc_printk_level(qdisc_notice, KERN_NOTICE);
+define_qdisc_printk_level(qdisc_info, KERN_INFO);