[PATCH net-next 0/7] netconsole: Add taskname sysdata support

[PATCH net-next 0/7] netconsole: Add taskname sysdata support

[Breno Leitao]

This patchset introduces a new feature to the netconsole extradata
subsystem that enables the inclusion of the current task's name in the
sysdata output of netconsole messages.

This enhancement is particularly valuable for large-scale deployments,
such as Meta's, where netconsole collects messages from millions of
servers and stores them in a data warehouse for analysis. Engineers
often rely on these messages to investigate issues and assess kernel
health.

One common challenge we face is determining the context in which
a particular message was generated. By including the task name
(task->comm) with each message, this feature provides a direct answer to
the frequently asked question: "What was running when this message was
generated?"

This added context will significantly improve our ability to diagnose
and troubleshoot issues, making it easier to interpret output of
netconsole.

The patchset consists of seven patches that implement the following changes:

 * Refactor CPU number formatting into a separate function
 * Prefix CPU_NR sysdata feature with SYSDATA_
 * Add configfs controls for taskname sysdata feature
 * Add taskname to extradata entry count
 * Add support for including task name in netconsole's extra data output
 * Document the task name feature in Documentation/networking/netconsole.rst
 * Add test coverage for the task name feature to the existing sysdata selftest script

These changes allow users to enable or disable the task name feature via
configfs and provide additional context for kernel messages by showing
which task generated each console message.

I have tested these patches on some servers and they seem to work as
expected.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
Breno Leitao (7):
      netconsole: prefix CPU_NR sysdata feature with SYSDATA_
      netconsole: refactor CPU number formatting into separate function
      netconsole: add taskname to extradata entry count
      netconsole: add configfs controls for taskname sysdata feature
      netconsole: add task name to extra data fields
      netconsole: docs: document the task name feature
      netconsole: selftest: add task name append testing

 Documentation/networking/netconsole.rst            | 28 +++++++
 drivers/net/netconsole.c                           | 98 ++++++++++++++++++----
 .../selftests/drivers/net/netcons_sysdata.sh       | 51 +++++++++--
 3 files changed, 156 insertions(+), 21 deletions(-)
---
base-commit: bb3bb6c92e5719c0f5d7adb9d34db7e76705ac33
change-id: 20250217-netcons_current-2c635fa5beda
prerequisite-change-id: 20250212-netdevsim-258d2d628175:v3
prerequisite-patch-id: 4ecfdbc58dd599d2358655e4ad742cbb9dde39f3

Best regards,
-- 
Breno Leitao <leitao@debian.org>

[PATCH net-next 1/7] netconsole: prefix CPU_NR sysdata feature with SYSDATA_

[Breno Leitao]

Rename the CPU_NR enum value to SYSDATA_CPU_NR to establish a consistent
naming convention for sysdata features. This change prepares for
upcoming additions to the sysdata feature set by clearly grouping
related features under the SYSDATA prefix.

This change is purely cosmetic and does not modify any functionality.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/net/netconsole.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index f77eddf221850fe2778cd479e49c91ad695aba3c..c086e2fe51f874812379e6f89c421d7d32980f91 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -103,7 +103,7 @@ struct netconsole_target_stats  {
  */
 enum sysdata_feature {
 	/* Populate the CPU that sends the message */
-	CPU_NR = BIT(0),
+	SYSDATA_CPU_NR = BIT(0),
 };
 
 /**
@@ -418,7 +418,7 @@ static ssize_t sysdata_cpu_nr_enabled_show(struct config_item *item, char *buf)
 	bool cpu_nr_enabled;
 
 	mutex_lock(&dynamic_netconsole_mutex);
-	cpu_nr_enabled = !!(nt->sysdata_fields & CPU_NR);
+	cpu_nr_enabled = !!(nt->sysdata_fields & SYSDATA_CPU_NR);
 	mutex_unlock(&dynamic_netconsole_mutex);
 
 	return sysfs_emit(buf, "%d\n", cpu_nr_enabled);
@@ -699,7 +699,7 @@ static size_t count_extradata_entries(struct netconsole_target *nt)
 	/* Userdata entries */
 	entries = list_count_nodes(&nt->userdata_group.cg_children);
 	/* Plus sysdata entries */
-	if (nt->sysdata_fields & CPU_NR)
+	if (nt->sysdata_fields & SYSDATA_CPU_NR)
 		entries += 1;
 
 	return entries;
@@ -850,7 +850,7 @@ static ssize_t sysdata_cpu_nr_enabled_store(struct config_item *item,
 		return ret;
 
 	mutex_lock(&dynamic_netconsole_mutex);
-	curr = nt->sysdata_fields & CPU_NR;
+	curr = nt->sysdata_fields & SYSDATA_CPU_NR;
 	if (cpu_nr_enabled == curr)
 		/* no change requested */
 		goto unlock_ok;
@@ -865,13 +865,13 @@ static ssize_t sysdata_cpu_nr_enabled_store(struct config_item *item,
 	}
 
 	if (cpu_nr_enabled)
-		nt->sysdata_fields |= CPU_NR;
+		nt->sysdata_fields |= SYSDATA_CPU_NR;
 	else
 		/* This is special because extradata_complete might have
 		 * remaining data from previous sysdata, and it needs to be
 		 * cleaned.
 		 */
-		disable_sysdata_feature(nt, CPU_NR);
+		disable_sysdata_feature(nt, SYSDATA_CPU_NR);
 
 unlock_ok:
 	ret = strnlen(buf, count);
@@ -1130,7 +1130,7 @@ static int prepare_extradata(struct netconsole_target *nt)
 	 */
 	extradata_len = nt->userdata_length;
 
-	if (!(nt->sysdata_fields & CPU_NR))
+	if (!(nt->sysdata_fields & SYSDATA_CPU_NR))
 		goto out;
 
 	/* Append cpu=%d at extradata_complete after userdata str */

-- 
2.43.5

[PATCH net-next 2/7] netconsole: refactor CPU number formatting into separate function

[Breno Leitao]

Extract CPU number formatting logic from prepare_extradata() into a new
append_cpu_nr() function.

This refactoring improves code organization by isolating CPU number
formatting into its own function while reducing the complexity of
prepare_extradata().

The change prepares the codebase for the upcoming taskname feature by
establishing a consistent pattern for handling sysdata features.

The CPU number formatting logic itself remains unchanged; only its
location has moved to improve maintainability.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/net/netconsole.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index c086e2fe51f874812379e6f89c421d7d32980f91..26ff2ed4de16bce58e9eeaf8b5b362dfaafaca0a 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -1117,13 +1117,21 @@ static void populate_configfs_item(struct netconsole_target *nt,
 	init_target_config_group(nt, target_name);
 }
 
+static int append_cpu_nr(struct netconsole_target *nt, int offset)
+{
+	/* Append cpu=%d at extradata_complete after userdata str */
+	return scnprintf(&nt->extradata_complete[offset],
+			 MAX_EXTRADATA_ENTRY_LEN, " cpu=%u\n",
+			 raw_smp_processor_id());
+}
+
 /*
  * prepare_extradata - append sysdata at extradata_complete in runtime
  * @nt: target to send message to
  */
 static int prepare_extradata(struct netconsole_target *nt)
 {
-	int sysdata_len, extradata_len;
+	int extradata_len;
 
 	/* userdata was appended when configfs write helper was called
 	 * by update_userdata().
@@ -1133,12 +1141,8 @@ static int prepare_extradata(struct netconsole_target *nt)
 	if (!(nt->sysdata_fields & SYSDATA_CPU_NR))
 		goto out;
 
-	/* Append cpu=%d at extradata_complete after userdata str */
-	sysdata_len = scnprintf(&nt->extradata_complete[nt->userdata_length],
-				MAX_EXTRADATA_ENTRY_LEN, " cpu=%u\n",
-				raw_smp_processor_id());
-
-	extradata_len += sysdata_len;
+	if (nt->sysdata_fields & SYSDATA_CPU_NR)
+		extradata_len += append_cpu_nr(nt, nt->userdata_length);
 
 	WARN_ON_ONCE(extradata_len >
 		     MAX_EXTRADATA_ENTRY_LEN * MAX_EXTRADATA_ITEMS);

-- 
2.43.5

[PATCH net-next 3/7] netconsole: add taskname to extradata entry count

[Breno Leitao]

New SYSDATA_TASKNAME feature flag to track when taskname append is enabled.

Additional check in count_extradata_entries() to include taskname in
total, counting it as an entry in extradata. This function is used to
check if we are not overflowing the number of extradata items.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/net/netconsole.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 26ff2ed4de16bce58e9eeaf8b5b362dfaafaca0a..1b109f46512ffb7628c6b34c6efdfc301376dd53 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -104,6 +104,8 @@ struct netconsole_target_stats  {
 enum sysdata_feature {
 	/* Populate the CPU that sends the message */
 	SYSDATA_CPU_NR = BIT(0),
+	/* Populate the task name (as in current->comm) in sysdata */
+	SYSDATA_TASKNAME = BIT(1),
 };
 
 /**
@@ -701,6 +703,8 @@ static size_t count_extradata_entries(struct netconsole_target *nt)
 	/* Plus sysdata entries */
 	if (nt->sysdata_fields & SYSDATA_CPU_NR)
 		entries += 1;
+	if (nt->sysdata_fields & SYSDATA_TASKNAME)
+		entries += 1;
 
 	return entries;
 }

-- 
2.43.5

[PATCH net-next 4/7] netconsole: add configfs controls for taskname sysdata feature

[Breno Leitao]

Add configfs interface to enable/disable the taskname sysdata feature.
This adds the following functionality:

The implementation follows the same pattern as the existing CPU number
feature, ensuring consistent behavior and error handling across sysdata
features.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/net/netconsole.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 1b109f46512ffb7628c6b34c6efdfc301376dd53..5a29144ae37ee7b487b1a252b0f2ce8574f9cefa 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -426,6 +426,20 @@ static ssize_t sysdata_cpu_nr_enabled_show(struct config_item *item, char *buf)
 	return sysfs_emit(buf, "%d\n", cpu_nr_enabled);
 }
 
+/* configfs helper to display if taskname sysdata feature is enabled */
+static ssize_t sysdata_taskname_enabled_show(struct config_item *item,
+					     char *buf)
+{
+	struct netconsole_target *nt = to_target(item->ci_parent);
+	bool taskname_enabled;
+
+	mutex_lock(&dynamic_netconsole_mutex);
+	taskname_enabled = !!(nt->sysdata_fields & SYSDATA_TASKNAME);
+	mutex_unlock(&dynamic_netconsole_mutex);
+
+	return sysfs_emit(buf, "%d\n", taskname_enabled);
+}
+
 /*
  * This one is special -- targets created through the configfs interface
  * are not enabled (and the corresponding netpoll activated) by default.
@@ -841,6 +855,40 @@ static void disable_sysdata_feature(struct netconsole_target *nt,
 	nt->extradata_complete[nt->userdata_length] = 0;
 }
 
+static ssize_t sysdata_taskname_enabled_store(struct config_item *item,
+					      const char *buf, size_t count)
+{
+	struct netconsole_target *nt = to_target(item->ci_parent);
+	bool taskname_enabled, curr;
+	ssize_t ret;
+
+	ret = kstrtobool(buf, &taskname_enabled);
+	if (ret)
+		return ret;
+
+	mutex_lock(&dynamic_netconsole_mutex);
+	curr = nt->sysdata_fields & SYSDATA_TASKNAME;
+	if (taskname_enabled == curr)
+		goto unlock_ok;
+
+	if (taskname_enabled &&
+	    count_extradata_entries(nt) >= MAX_EXTRADATA_ITEMS) {
+		ret = -ENOSPC;
+		goto unlock;
+	}
+
+	if (taskname_enabled)
+		nt->sysdata_fields |= SYSDATA_TASKNAME;
+	else
+		disable_sysdata_feature(nt, SYSDATA_TASKNAME);
+
+unlock_ok:
+	ret = strnlen(buf, count);
+unlock:
+	mutex_unlock(&dynamic_netconsole_mutex);
+	return ret;
+}
+
 /* configfs helper to sysdata cpu_nr feature */
 static ssize_t sysdata_cpu_nr_enabled_store(struct config_item *item,
 					    const char *buf, size_t count)
@@ -886,6 +934,7 @@ static ssize_t sysdata_cpu_nr_enabled_store(struct config_item *item,
 
 CONFIGFS_ATTR(userdatum_, value);
 CONFIGFS_ATTR(sysdata_, cpu_nr_enabled);
+CONFIGFS_ATTR(sysdata_, taskname_enabled);
 
 static struct configfs_attribute *userdatum_attrs[] = {
 	&userdatum_attr_value,
@@ -946,6 +995,7 @@ static void userdatum_drop(struct config_group *group, struct config_item *item)
 
 static struct configfs_attribute *userdata_attrs[] = {
 	&sysdata_attr_cpu_nr_enabled,
+	&sysdata_attr_taskname_enabled,
 	NULL,
 };
 

-- 
2.43.5

[PATCH net-next 5/7] netconsole: add task name to extra data fields

[Breno Leitao]

This is the core patch for this whole patchset. Add support for
including the current task's name in netconsole's extra data output.
This adds a new append_taskname() function that writes the task name
(from current->comm) into the target's extradata buffer, similar to how
CPU numbers are handled.

The task name is included when the SYSDATA_TASKNAME field is set,
appearing in the format "taskname=<name>" in the output. This additional
context can help with debugging by showing which task generated each
console message.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/net/netconsole.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 5a29144ae37ee7b487b1a252b0f2ce8574f9cefa..625f4c0be11d8deb454139b1c526abc842697219 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -1179,12 +1179,22 @@ static int append_cpu_nr(struct netconsole_target *nt, int offset)
 			 raw_smp_processor_id());
 }
 
+static int append_taskname(struct netconsole_target *nt, int offset)
+{
+	if (WARN_ON_ONCE(!current))
+		return 0;
+
+	return scnprintf(&nt->extradata_complete[offset],
+			 MAX_EXTRADATA_ENTRY_LEN, " taskname=%s\n",
+			 current->comm);
+}
 /*
  * prepare_extradata - append sysdata at extradata_complete in runtime
  * @nt: target to send message to
  */
 static int prepare_extradata(struct netconsole_target *nt)
 {
+	u32 fields = SYSDATA_CPU_NR | SYSDATA_TASKNAME;
 	int extradata_len;
 
 	/* userdata was appended when configfs write helper was called
@@ -1192,11 +1202,13 @@ static int prepare_extradata(struct netconsole_target *nt)
 	 */
 	extradata_len = nt->userdata_length;
 
-	if (!(nt->sysdata_fields & SYSDATA_CPU_NR))
+	if (!(nt->sysdata_fields & fields))
 		goto out;
 
 	if (nt->sysdata_fields & SYSDATA_CPU_NR)
 		extradata_len += append_cpu_nr(nt, nt->userdata_length);
+	if (nt->sysdata_fields & SYSDATA_TASKNAME)
+		extradata_len += append_taskname(nt, extradata_len);
 
 	WARN_ON_ONCE(extradata_len >
 		     MAX_EXTRADATA_ENTRY_LEN * MAX_EXTRADATA_ITEMS);

-- 
2.43.5

[PATCH net-next 6/7] netconsole: docs: document the task name feature

[Breno Leitao]

Add documentation for the netconsole task name feature in
Documentation/networking/netconsole.rst. This explains how to enable
task name via configfs and demonstrates the output format.

The documentation includes:
- How to enable/disable the feature via taskname_enabled
- The format of the task name in the output
- An example showing the task name appearing in messages

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 Documentation/networking/netconsole.rst | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/Documentation/networking/netconsole.rst b/Documentation/networking/netconsole.rst
index 84803c59968a3237012fab821f432eb531aba45c..ae82a6337a8d8a07a0d691e2da170f6cf70ae86f 100644
--- a/Documentation/networking/netconsole.rst
+++ b/Documentation/networking/netconsole.rst
@@ -240,6 +240,34 @@ Delete `userdata` entries with `rmdir`::
 
    It is recommended to not write user data values with newlines.
 
+Task name auto population in userdata
+-------------------------------------
+
+Inside the netconsole configfs hierarchy, there is a file called
+`taskname_enabled` under the `userdata` directory. This file is used to enable
+or disable the automatic task name population feature. This feature
+automatically populates the current task name that is scheduled in the CPU
+sneding the message.
+
+To enable task name auto-population::
+
+  echo 1 > /sys/kernel/config/netconsole/target1/userdata/taskname_enabled
+
+When this option is enabled, the netconsole messages will include an additional
+line in the userdata field with the format `taskname=<task name>`. This allows
+the receiver of the netconsole messages to easily find which application was
+currently scheduled when that message was generated, providing extra context
+for kernel messages and helping to categorize them.
+
+Example::
+
+  echo "This is a message" > /dev/kmsg
+  12,607,22085407756,-;This is a message
+   taskname=echo
+
+In this example, the message was generated while "echo" was the current
+scheduled process.
+
 CPU number auto population in userdata
 --------------------------------------
 

-- 
2.43.5

[PATCH net-next 7/7] netconsole: selftest: add task name append testing

[Breno Leitao]

Add test coverage for the netconsole task name feature to the existing
sysdata selftest script. This extends the test infrastructure to verify
that task names are correctly appended when enabled and absent when
disabled.

The test validates that:
  - Task names appear in the expected format "taskname=<name>"
  - Task names are included when the feature is enabled
  - Task names are excluded when the feature is disabled
  - The feature works correctly alongside other sysdata fields like CPU

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 .../selftests/drivers/net/netcons_sysdata.sh       | 51 +++++++++++++++++++---
 1 file changed, 44 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/netcons_sysdata.sh b/tools/testing/selftests/drivers/net/netcons_sysdata.sh
index 2b78fd1f5982fee81b379bf9ac6f64a38d915974..f351206ed1bda4d46269a521b27b9b8e56d524be 100755
--- a/tools/testing/selftests/drivers/net/netcons_sysdata.sh
+++ b/tools/testing/selftests/drivers/net/netcons_sysdata.sh
@@ -31,17 +31,38 @@ function set_cpu_nr() {
 	echo 1 > "${NETCONS_PATH}/userdata/cpu_nr_enabled"
 }
 
+# Enable the taskname to be appended to sysdata
+function set_taskname() {
+	if [[ ! -f "${NETCONS_PATH}/userdata/taskname_enabled" ]]
+	then
+		echo "Not able to enable taskname sysdata append. Configfs not available in ${NETCONS_PATH}/userdata/taskname_enabled" >&2
+		exit "${ksft_skip}"
+	fi
+
+	echo 1 > "${NETCONS_PATH}/userdata/taskname_enabled"
+}
+
 # Disable the sysdata cpu_nr feature
 function unset_cpu_nr() {
 	echo 0 > "${NETCONS_PATH}/userdata/cpu_nr_enabled"
 }
 
-# Test if MSG content and `cpu=${CPU}` exists in OUTPUT_FILE
-function validate_sysdata_cpu_exists() {
+# Once called, taskname=<..> will not be appended anymore
+function unset_taskname() {
+	echo 0 > "${NETCONS_PATH}/userdata/taskname_enabled"
+}
+
+# Test if MSG contains sysdata
+function validate_sysdata() {
 	# OUTPUT_FILE will contain something like:
 	# 6.11.1-0_fbk0_rc13_509_g30d75cea12f7,13,1822,115075213798,-;netconsole selftest: netcons_gtJHM
 	#  userdatakey=userdatavalue
 	#  cpu=X
+	#  taskname=<taskname>
+
+	# Echo is what this test uses to create the message. See runtest()
+	# function
+	SENDER="echo"
 
 	if [ ! -f "$OUTPUT_FILE" ]; then
 		echo "FAIL: File was not generated." >&2
@@ -62,12 +83,19 @@ function validate_sysdata_cpu_exists() {
 		exit "${ksft_fail}"
 	fi
 
+	if ! grep -q "taskname=${SENDER}" "${OUTPUT_FILE}"; then
+		echo "FAIL: 'taskname=echo' not found in ${OUTPUT_FILE}" >&2
+		cat "${OUTPUT_FILE}" >&2
+		exit "${ksft_fail}"
+	fi
+
 	rm "${OUTPUT_FILE}"
 	pkill_socat
 }
 
-# Test if MSG content exists in OUTPUT_FILE but no `cpu=` string
-function validate_sysdata_no_cpu() {
+# Test if MSG content exists in OUTPUT_FILE but no `cpu=` and `taskname=`
+# strings
+function validate_no_sysdata() {
 	if [ ! -f "$OUTPUT_FILE" ]; then
 		echo "FAIL: File was not generated." >&2
 		exit "${ksft_fail}"
@@ -85,6 +113,12 @@ function validate_sysdata_no_cpu() {
 		exit "${ksft_fail}"
 	fi
 
+	if grep -q "taskname=" "${OUTPUT_FILE}"; then
+		echo "FAIL: 'taskname=  found in ${OUTPUT_FILE}" >&2
+		cat "${OUTPUT_FILE}" >&2
+		exit "${ksft_fail}"
+	fi
+
 	rm "${OUTPUT_FILE}"
 }
 
@@ -133,10 +167,12 @@ OUTPUT_FILE="/tmp/${TARGET}_1"
 MSG="Test #1 from CPU${CPU}"
 # Enable the auto population of cpu_nr
 set_cpu_nr
+# Enable taskname to be appended to sysdata
+set_taskname
 runtest
 # Make sure the message was received in the dst part
 # and exit
-validate_sysdata_cpu_exists
+validate_sysdata
 
 #====================================================
 # TEST #2
@@ -148,7 +184,7 @@ OUTPUT_FILE="/tmp/${TARGET}_2"
 MSG="Test #2 from CPU${CPU}"
 set_user_data
 runtest
-validate_sysdata_cpu_exists
+validate_sysdata
 
 # ===================================================
 # TEST #3
@@ -160,8 +196,9 @@ OUTPUT_FILE="/tmp/${TARGET}_3"
 MSG="Test #3 from CPU${CPU}"
 # Enable the auto population of cpu_nr
 unset_cpu_nr
+unset_taskname
 runtest
 # At this time, cpu= shouldn't be present in the msg
-validate_sysdata_no_cpu
+validate_no_sysdata
 
 exit "${ksft_pass}"

-- 
2.43.5

Re: [PATCH net-next 2/7] netconsole: refactor CPU number formatting into separate function

[Simon Horman]

On Fri, Feb 21, 2025 at 05:52:07AM -0800, Breno Leitao wrote:
> Extract CPU number formatting logic from prepare_extradata() into a new
> append_cpu_nr() function.
> 
> This refactoring improves code organization by isolating CPU number
> formatting into its own function while reducing the complexity of
> prepare_extradata().
> 
> The change prepares the codebase for the upcoming taskname feature by
> establishing a consistent pattern for handling sysdata features.
> 
> The CPU number formatting logic itself remains unchanged; only its
> location has moved to improve maintainability.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  drivers/net/netconsole.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index c086e2fe51f874812379e6f89c421d7d32980f91..26ff2ed4de16bce58e9eeaf8b5b362dfaafaca0a 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -1117,13 +1117,21 @@ static void populate_configfs_item(struct netconsole_target *nt,
>  	init_target_config_group(nt, target_name);
>  }
>  
> +static int append_cpu_nr(struct netconsole_target *nt, int offset)
> +{
> +	/* Append cpu=%d at extradata_complete after userdata str */
> +	return scnprintf(&nt->extradata_complete[offset],
> +			 MAX_EXTRADATA_ENTRY_LEN, " cpu=%u\n",
> +			 raw_smp_processor_id());
> +}
> +
>  /*
>   * prepare_extradata - append sysdata at extradata_complete in runtime
>   * @nt: target to send message to
>   */
>  static int prepare_extradata(struct netconsole_target *nt)
>  {
> -	int sysdata_len, extradata_len;
> +	int extradata_len;
>  
>  	/* userdata was appended when configfs write helper was called
>  	 * by update_userdata().
> @@ -1133,12 +1141,8 @@ static int prepare_extradata(struct netconsole_target *nt)
>  	if (!(nt->sysdata_fields & SYSDATA_CPU_NR))
>  		goto out;
>  
> -	/* Append cpu=%d at extradata_complete after userdata str */
> -	sysdata_len = scnprintf(&nt->extradata_complete[nt->userdata_length],
> -				MAX_EXTRADATA_ENTRY_LEN, " cpu=%u\n",
> -				raw_smp_processor_id());
> -
> -	extradata_len += sysdata_len;
> +	if (nt->sysdata_fields & SYSDATA_CPU_NR)
> +		extradata_len += append_cpu_nr(nt, nt->userdata_length);

Hi Breno,

As this is the only caller of append_cpu_nr() I'm wondering
if it would be nicer if nt was the only argument to append_cpu_nr().

Not a big deal either way, so the above notwithstanding:

Reviewed-by: Simon Horman <horms@kernel.org>

>  
>  	WARN_ON_ONCE(extradata_len >
>  		     MAX_EXTRADATA_ENTRY_LEN * MAX_EXTRADATA_ITEMS);
> 
> -- 
> 2.43.5
> 
> 

Re: [PATCH net-next 5/7] netconsole: add task name to extra data fields

[Simon Horman]

On Fri, Feb 21, 2025 at 05:52:10AM -0800, Breno Leitao wrote:
> This is the core patch for this whole patchset. Add support for
> including the current task's name in netconsole's extra data output.
> This adds a new append_taskname() function that writes the task name
> (from current->comm) into the target's extradata buffer, similar to how
> CPU numbers are handled.
> 
> The task name is included when the SYSDATA_TASKNAME field is set,
> appearing in the format "taskname=<name>" in the output. This additional
> context can help with debugging by showing which task generated each
> console message.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  drivers/net/netconsole.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 5a29144ae37ee7b487b1a252b0f2ce8574f9cefa..625f4c0be11d8deb454139b1c526abc842697219 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -1179,12 +1179,22 @@ static int append_cpu_nr(struct netconsole_target *nt, int offset)
>  			 raw_smp_processor_id());
>  }
>  
> +static int append_taskname(struct netconsole_target *nt, int offset)
> +{
> +	if (WARN_ON_ONCE(!current))
> +		return 0;

Hi Breno,

I gather that theoretically this could occur, but it isn't expected
to happen in practice. Is that right?

...

Re: [PATCH net-next 1/7] netconsole: prefix CPU_NR sysdata feature with SYSDATA_

[Simon Horman]

On Fri, Feb 21, 2025 at 05:52:06AM -0800, Breno Leitao wrote:
> Rename the CPU_NR enum value to SYSDATA_CPU_NR to establish a consistent
> naming convention for sysdata features. This change prepares for
> upcoming additions to the sysdata feature set by clearly grouping
> related features under the SYSDATA prefix.
> 
> This change is purely cosmetic and does not modify any functionality.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next 3/7] netconsole: add taskname to extradata entry count

[Simon Horman]

On Fri, Feb 21, 2025 at 05:52:08AM -0800, Breno Leitao wrote:
> New SYSDATA_TASKNAME feature flag to track when taskname append is enabled.
> 
> Additional check in count_extradata_entries() to include taskname in
> total, counting it as an entry in extradata. This function is used to
> check if we are not overflowing the number of extradata items.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next 4/7] netconsole: add configfs controls for taskname sysdata feature

[Simon Horman]

On Fri, Feb 21, 2025 at 05:52:09AM -0800, Breno Leitao wrote:
> Add configfs interface to enable/disable the taskname sysdata feature.
> This adds the following functionality:
> 
> The implementation follows the same pattern as the existing CPU number
> feature, ensuring consistent behavior and error handling across sysdata
> features.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next 6/7] netconsole: docs: document the task name feature

[Simon Horman]

On Fri, Feb 21, 2025 at 05:52:11AM -0800, Breno Leitao wrote:
> Add documentation for the netconsole task name feature in
> Documentation/networking/netconsole.rst. This explains how to enable
> task name via configfs and demonstrates the output format.
> 
> The documentation includes:
> - How to enable/disable the feature via taskname_enabled
> - The format of the task name in the output
> - An example showing the task name appearing in messages
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next 7/7] netconsole: selftest: add task name append testing

[Simon Horman]

On Fri, Feb 21, 2025 at 05:52:12AM -0800, Breno Leitao wrote:
> Add test coverage for the netconsole task name feature to the existing
> sysdata selftest script. This extends the test infrastructure to verify
> that task names are correctly appended when enabled and absent when
> disabled.
> 
> The test validates that:
>   - Task names appear in the expected format "taskname=<name>"
>   - Task names are included when the feature is enabled
>   - Task names are excluded when the feature is disabled
>   - The feature works correctly alongside other sysdata fields like CPU
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next 2/7] netconsole: refactor CPU number formatting into separate function

[Breno Leitao]

Hello Simon,

On Tue, Feb 25, 2025 at 10:17:48AM +0000, Simon Horman wrote:
> On Fri, Feb 21, 2025 at 05:52:07AM -0800, Breno Leitao wrote:
> > Extract CPU number formatting logic from prepare_extradata() into a new
> > append_cpu_nr() function.
> > 
> > This refactoring improves code organization by isolating CPU number
> > formatting into its own function while reducing the complexity of
> > prepare_extradata().
> > 
> > The change prepares the codebase for the upcoming taskname feature by
> > establishing a consistent pattern for handling sysdata features.
> > 
> > The CPU number formatting logic itself remains unchanged; only its
> > location has moved to improve maintainability.
> > 
> > Signed-off-by: Breno Leitao <leitao@debian.org>
> > ---
> >  drivers/net/netconsole.c | 18 +++++++++++-------
> >  1 file changed, 11 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > index c086e2fe51f874812379e6f89c421d7d32980f91..26ff2ed4de16bce58e9eeaf8b5b362dfaafaca0a 100644
> > --- a/drivers/net/netconsole.c
> > +++ b/drivers/net/netconsole.c
> > @@ -1117,13 +1117,21 @@ static void populate_configfs_item(struct netconsole_target *nt,
> >  	init_target_config_group(nt, target_name);
> >  }
> >  
> > +static int append_cpu_nr(struct netconsole_target *nt, int offset)
> > +{
> > +	/* Append cpu=%d at extradata_complete after userdata str */
> > +	return scnprintf(&nt->extradata_complete[offset],
> > +			 MAX_EXTRADATA_ENTRY_LEN, " cpu=%u\n",
> > +			 raw_smp_processor_id());
> > +}
> > +
> >  /*
> >   * prepare_extradata - append sysdata at extradata_complete in runtime
> >   * @nt: target to send message to
> >   */
> >  static int prepare_extradata(struct netconsole_target *nt)
> >  {
> > -	int sysdata_len, extradata_len;
> > +	int extradata_len;
> >  
> >  	/* userdata was appended when configfs write helper was called
> >  	 * by update_userdata().
> > @@ -1133,12 +1141,8 @@ static int prepare_extradata(struct netconsole_target *nt)
> >  	if (!(nt->sysdata_fields & SYSDATA_CPU_NR))
> >  		goto out;
> >  
> > -	/* Append cpu=%d at extradata_complete after userdata str */
> > -	sysdata_len = scnprintf(&nt->extradata_complete[nt->userdata_length],
> > -				MAX_EXTRADATA_ENTRY_LEN, " cpu=%u\n",
> > -				raw_smp_processor_id());
> > -
> > -	extradata_len += sysdata_len;
> > +	if (nt->sysdata_fields & SYSDATA_CPU_NR)
> > +		extradata_len += append_cpu_nr(nt, nt->userdata_length);
> 
> Hi Breno,
> 
> As this is the only caller of append_cpu_nr() I'm wondering
> if it would be nicer if nt was the only argument to append_cpu_nr().

Yes, I can do it. I just kept both functions the same:

  static int append_taskname(struct netconsole_target *nt, int offset)
  static int append_cpu_nr(struct netconsole_target *nt, int offset)

Another option is to use extradata_len as the second argument, instead
of nt->userdata_length. That might(?) make the code easier to read? it
would look like the following:

          extradata_len = nt->userdata_length;
          if (nt->sysdata_fields & SYSDATA_CPU_NR)
                  extradata_len += append_cpu_nr(nt, extradata_len);
          if (nt->sysdata_fields & SYSDATA_TASKNAME)
                  extradata_len += append_taskname(nt, extradata_len);

What would you write yourself?

Thank you very much for the review,
--breno

Re: [PATCH net-next 5/7] netconsole: add task name to extra data fields

[Breno Leitao]

Hello Simon,

On Tue, Feb 25, 2025 at 10:19:10AM +0000, Simon Horman wrote:
> On Fri, Feb 21, 2025 at 05:52:10AM -0800, Breno Leitao wrote:
> > This is the core patch for this whole patchset. Add support for
> > including the current task's name in netconsole's extra data output.
> > This adds a new append_taskname() function that writes the task name
> > (from current->comm) into the target's extradata buffer, similar to how
> > CPU numbers are handled.
> > 
> > The task name is included when the SYSDATA_TASKNAME field is set,
> > appearing in the format "taskname=<name>" in the output. This additional
> > context can help with debugging by showing which task generated each
> > console message.
> > 
> > Signed-off-by: Breno Leitao <leitao@debian.org>
> > ---
> >  drivers/net/netconsole.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > index 5a29144ae37ee7b487b1a252b0f2ce8574f9cefa..625f4c0be11d8deb454139b1c526abc842697219 100644
> > --- a/drivers/net/netconsole.c
> > +++ b/drivers/net/netconsole.c
> > @@ -1179,12 +1179,22 @@ static int append_cpu_nr(struct netconsole_target *nt, int offset)
> >  			 raw_smp_processor_id());
> >  }
> >  
> > +static int append_taskname(struct netconsole_target *nt, int offset)
> > +{
> > +	if (WARN_ON_ONCE(!current))
> > +		return 0;
> 
> Hi Breno,
> 
> I gather that theoretically this could occur, but it isn't expected
> to happen in practice. Is that right?

That's correct. `current` isn't expected to be NULL in practice.
I've been running this code on several servers for days and have never
encountered this warning. 

While the taskname feature isn't enabled during early boot, netconsole
might be active at that time, which is why I exercised extra caution
here.

If you also think this is safe, I am more than happy to remove this
check.

Thanks for the review,
--breno

Re: [PATCH net-next 4/7] netconsole: add configfs controls for taskname sysdata feature

[Paolo Abeni]

On 2/21/25 2:52 PM, Breno Leitao wrote:
> Add configfs interface to enable/disable the taskname sysdata feature.
> This adds the following functionality:
> 
> The implementation follows the same pattern as the existing CPU number
> feature, ensuring consistent behavior and error handling across sysdata
> features.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  drivers/net/netconsole.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 1b109f46512ffb7628c6b34c6efdfc301376dd53..5a29144ae37ee7b487b1a252b0f2ce8574f9cefa 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -426,6 +426,20 @@ static ssize_t sysdata_cpu_nr_enabled_show(struct config_item *item, char *buf)
>  	return sysfs_emit(buf, "%d\n", cpu_nr_enabled);
>  }
>  
> +/* configfs helper to display if taskname sysdata feature is enabled */
> +static ssize_t sysdata_taskname_enabled_show(struct config_item *item,
> +					     char *buf)
> +{
> +	struct netconsole_target *nt = to_target(item->ci_parent);
> +	bool taskname_enabled;
> +
> +	mutex_lock(&dynamic_netconsole_mutex);
> +	taskname_enabled = !!(nt->sysdata_fields & SYSDATA_TASKNAME);
> +	mutex_unlock(&dynamic_netconsole_mutex);
> +
> +	return sysfs_emit(buf, "%d\n", taskname_enabled);
> +}
> +
>  /*
>   * This one is special -- targets created through the configfs interface
>   * are not enabled (and the corresponding netpoll activated) by default.
> @@ -841,6 +855,40 @@ static void disable_sysdata_feature(struct netconsole_target *nt,
>  	nt->extradata_complete[nt->userdata_length] = 0;
>  }
>  
> +static ssize_t sysdata_taskname_enabled_store(struct config_item *item,
> +					      const char *buf, size_t count)
> +{
> +	struct netconsole_target *nt = to_target(item->ci_parent);
> +	bool taskname_enabled, curr;
> +	ssize_t ret;
> +
> +	ret = kstrtobool(buf, &taskname_enabled);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&dynamic_netconsole_mutex);
> +	curr = nt->sysdata_fields & SYSDATA_TASKNAME;

Minor nit:
	curr = !!(nt->sysdata_fields & SYSDATA_TASKNAME);

would be preferable, and more robust if later on other SYSDATA_ bits are
added, 'moving down' SYSDATA_TASKNAME definition.

Also it would be more consistent with previous usage in
`sysdata_taskname_enabled_show()`

Cheers,

Paolo

Re: [PATCH net-next 5/7] netconsole: add task name to extra data fields

[Paolo Abeni]

On 2/25/25 12:17 PM, Breno Leitao wrote:
> On Tue, Feb 25, 2025 at 10:19:10AM +0000, Simon Horman wrote:
>> On Fri, Feb 21, 2025 at 05:52:10AM -0800, Breno Leitao wrote:
>>> This is the core patch for this whole patchset. Add support for
>>> including the current task's name in netconsole's extra data output.
>>> This adds a new append_taskname() function that writes the task name
>>> (from current->comm) into the target's extradata buffer, similar to how
>>> CPU numbers are handled.
>>>
>>> The task name is included when the SYSDATA_TASKNAME field is set,
>>> appearing in the format "taskname=<name>" in the output. This additional
>>> context can help with debugging by showing which task generated each
>>> console message.
>>>
>>> Signed-off-by: Breno Leitao <leitao@debian.org>
>>> ---
>>>  drivers/net/netconsole.c | 14 +++++++++++++-
>>>  1 file changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
>>> index 5a29144ae37ee7b487b1a252b0f2ce8574f9cefa..625f4c0be11d8deb454139b1c526abc842697219 100644
>>> --- a/drivers/net/netconsole.c
>>> +++ b/drivers/net/netconsole.c
>>> @@ -1179,12 +1179,22 @@ static int append_cpu_nr(struct netconsole_target *nt, int offset)
>>>  			 raw_smp_processor_id());
>>>  }
>>>  
>>> +static int append_taskname(struct netconsole_target *nt, int offset)
>>> +{
>>> +	if (WARN_ON_ONCE(!current))
>>> +		return 0;
>>
>> Hi Breno,
>>
>> I gather that theoretically this could occur, but it isn't expected
>> to happen in practice. Is that right?
> 
> That's correct. `current` isn't expected to be NULL in practice.
> I've been running this code on several servers for days and have never
> encountered this warning. 
> 
> While the taskname feature isn't enabled during early boot, netconsole
> might be active at that time, which is why I exercised extra caution
> here.

So `current` can't be NULL here. I think it's better to drop such check,
it's presence would be misleading. i.e. like adding checks for UDP stack
being initialized before calling send_msg_fragmented()

Cheers,

Paolo

Re: [PATCH net-next 5/7] netconsole: add task name to extra data fields

[Breno Leitao]

Hello Paolo,

On Tue, Feb 25, 2025 at 12:53:49PM +0100, Paolo Abeni wrote:
> On 2/25/25 12:17 PM, Breno Leitao wrote:
> > On Tue, Feb 25, 2025 at 10:19:10AM +0000, Simon Horman wrote:
> >> On Fri, Feb 21, 2025 at 05:52:10AM -0800, Breno Leitao wrote:
> >>> This is the core patch for this whole patchset. Add support for
> >>> including the current task's name in netconsole's extra data output.
> >>> This adds a new append_taskname() function that writes the task name
> >>> (from current->comm) into the target's extradata buffer, similar to how
> >>> CPU numbers are handled.
> >>>
> >>> The task name is included when the SYSDATA_TASKNAME field is set,
> >>> appearing in the format "taskname=<name>" in the output. This additional
> >>> context can help with debugging by showing which task generated each
> >>> console message.
> >>>
> >>> Signed-off-by: Breno Leitao <leitao@debian.org>
> >>> ---
> >>>  drivers/net/netconsole.c | 14 +++++++++++++-
> >>>  1 file changed, 13 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> >>> index 5a29144ae37ee7b487b1a252b0f2ce8574f9cefa..625f4c0be11d8deb454139b1c526abc842697219 100644
> >>> --- a/drivers/net/netconsole.c
> >>> +++ b/drivers/net/netconsole.c
> >>> @@ -1179,12 +1179,22 @@ static int append_cpu_nr(struct netconsole_target *nt, int offset)
> >>>  			 raw_smp_processor_id());
> >>>  }
> >>>  
> >>> +static int append_taskname(struct netconsole_target *nt, int offset)
> >>> +{
> >>> +	if (WARN_ON_ONCE(!current))
> >>> +		return 0;
> >>
> >> Hi Breno,
> >>
> >> I gather that theoretically this could occur, but it isn't expected
> >> to happen in practice. Is that right?
> > 
> > That's correct. `current` isn't expected to be NULL in practice.
> > I've been running this code on several servers for days and have never
> > encountered this warning. 
> > 
> > While the taskname feature isn't enabled during early boot, netconsole
> > might be active at that time, which is why I exercised extra caution
> > here.
> 
> So `current` can't be NULL here. I think it's better to drop such check,
> it's presence would be misleading. i.e. like adding checks for UDP stack
> being initialized before calling send_msg_fragmented()

Ack. I will remove the check then, and check if the UDP stack has been
initialized before calling netpoll helpers.

What is the best way to make sure taht the UDP stack has been
initialized?

Thanks for the review,
--breno

Re: [PATCH net-next 4/7] netconsole: add configfs controls for taskname sysdata feature

[Breno Leitao]

On Tue, Feb 25, 2025 at 12:41:50PM +0100, Paolo Abeni wrote:
> On 2/21/25 2:52 PM, Breno Leitao wrote:
> > Add configfs interface to enable/disable the taskname sysdata feature.
> > This adds the following functionality:
> > 
> > The implementation follows the same pattern as the existing CPU number
> > feature, ensuring consistent behavior and error handling across sysdata
> > features.
> > 
> > Signed-off-by: Breno Leitao <leitao@debian.org>
> > ---
> >  drivers/net/netconsole.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 50 insertions(+)
> > 
> > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > index 1b109f46512ffb7628c6b34c6efdfc301376dd53..5a29144ae37ee7b487b1a252b0f2ce8574f9cefa 100644
> > --- a/drivers/net/netconsole.c
> > +++ b/drivers/net/netconsole.c
> > @@ -426,6 +426,20 @@ static ssize_t sysdata_cpu_nr_enabled_show(struct config_item *item, char *buf)
> >  	return sysfs_emit(buf, "%d\n", cpu_nr_enabled);
> >  }
> >  
> > +/* configfs helper to display if taskname sysdata feature is enabled */
> > +static ssize_t sysdata_taskname_enabled_show(struct config_item *item,
> > +					     char *buf)
> > +{
> > +	struct netconsole_target *nt = to_target(item->ci_parent);
> > +	bool taskname_enabled;
> > +
> > +	mutex_lock(&dynamic_netconsole_mutex);
> > +	taskname_enabled = !!(nt->sysdata_fields & SYSDATA_TASKNAME);
> > +	mutex_unlock(&dynamic_netconsole_mutex);
> > +
> > +	return sysfs_emit(buf, "%d\n", taskname_enabled);
> > +}
> > +
> >  /*
> >   * This one is special -- targets created through the configfs interface
> >   * are not enabled (and the corresponding netpoll activated) by default.
> > @@ -841,6 +855,40 @@ static void disable_sysdata_feature(struct netconsole_target *nt,
> >  	nt->extradata_complete[nt->userdata_length] = 0;
> >  }
> >  
> > +static ssize_t sysdata_taskname_enabled_store(struct config_item *item,
> > +					      const char *buf, size_t count)
> > +{
> > +	struct netconsole_target *nt = to_target(item->ci_parent);
> > +	bool taskname_enabled, curr;
> > +	ssize_t ret;
> > +
> > +	ret = kstrtobool(buf, &taskname_enabled);
> > +	if (ret)
> > +		return ret;
> > +
> > +	mutex_lock(&dynamic_netconsole_mutex);
> > +	curr = nt->sysdata_fields & SYSDATA_TASKNAME;
> 
> Minor nit:
> 	curr = !!(nt->sysdata_fields & SYSDATA_TASKNAME);
> 
> would be preferable, and more robust if later on other SYSDATA_ bits are
> added, 'moving down' SYSDATA_TASKNAME definition.
> 
> Also it would be more consistent with previous usage in
> `sysdata_taskname_enabled_show()`

Good point, in fact. I will update!

Thanks for the review,
--breno

Re: [PATCH net-next 2/7] netconsole: refactor CPU number formatting into separate function

[Simon Horman]

On Tue, Feb 25, 2025 at 03:09:20AM -0800, Breno Leitao wrote:
> Hello Simon,
> 
> On Tue, Feb 25, 2025 at 10:17:48AM +0000, Simon Horman wrote:
> > On Fri, Feb 21, 2025 at 05:52:07AM -0800, Breno Leitao wrote:
> > > Extract CPU number formatting logic from prepare_extradata() into a new
> > > append_cpu_nr() function.
> > > 
> > > This refactoring improves code organization by isolating CPU number
> > > formatting into its own function while reducing the complexity of
> > > prepare_extradata().
> > > 
> > > The change prepares the codebase for the upcoming taskname feature by
> > > establishing a consistent pattern for handling sysdata features.
> > > 
> > > The CPU number formatting logic itself remains unchanged; only its
> > > location has moved to improve maintainability.
> > > 
> > > Signed-off-by: Breno Leitao <leitao@debian.org>
> > > ---
> > >  drivers/net/netconsole.c | 18 +++++++++++-------
> > >  1 file changed, 11 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > > index c086e2fe51f874812379e6f89c421d7d32980f91..26ff2ed4de16bce58e9eeaf8b5b362dfaafaca0a 100644
> > > --- a/drivers/net/netconsole.c
> > > +++ b/drivers/net/netconsole.c
> > > @@ -1117,13 +1117,21 @@ static void populate_configfs_item(struct netconsole_target *nt,
> > >  	init_target_config_group(nt, target_name);
> > >  }
> > >  
> > > +static int append_cpu_nr(struct netconsole_target *nt, int offset)
> > > +{
> > > +	/* Append cpu=%d at extradata_complete after userdata str */
> > > +	return scnprintf(&nt->extradata_complete[offset],
> > > +			 MAX_EXTRADATA_ENTRY_LEN, " cpu=%u\n",
> > > +			 raw_smp_processor_id());
> > > +}
> > > +
> > >  /*
> > >   * prepare_extradata - append sysdata at extradata_complete in runtime
> > >   * @nt: target to send message to
> > >   */
> > >  static int prepare_extradata(struct netconsole_target *nt)
> > >  {
> > > -	int sysdata_len, extradata_len;
> > > +	int extradata_len;
> > >  
> > >  	/* userdata was appended when configfs write helper was called
> > >  	 * by update_userdata().
> > > @@ -1133,12 +1141,8 @@ static int prepare_extradata(struct netconsole_target *nt)
> > >  	if (!(nt->sysdata_fields & SYSDATA_CPU_NR))
> > >  		goto out;
> > >  
> > > -	/* Append cpu=%d at extradata_complete after userdata str */
> > > -	sysdata_len = scnprintf(&nt->extradata_complete[nt->userdata_length],
> > > -				MAX_EXTRADATA_ENTRY_LEN, " cpu=%u\n",
> > > -				raw_smp_processor_id());
> > > -
> > > -	extradata_len += sysdata_len;
> > > +	if (nt->sysdata_fields & SYSDATA_CPU_NR)
> > > +		extradata_len += append_cpu_nr(nt, nt->userdata_length);
> > 
> > Hi Breno,
> > 
> > As this is the only caller of append_cpu_nr() I'm wondering
> > if it would be nicer if nt was the only argument to append_cpu_nr().
> 
> Yes, I can do it. I just kept both functions the same:
> 
>   static int append_taskname(struct netconsole_target *nt, int offset)
>   static int append_cpu_nr(struct netconsole_target *nt, int offset)
> 
> Another option is to use extradata_len as the second argument, instead
> of nt->userdata_length. That might(?) make the code easier to read? it
> would look like the following:
> 
>           extradata_len = nt->userdata_length;
>           if (nt->sysdata_fields & SYSDATA_CPU_NR)
>                   extradata_len += append_cpu_nr(nt, extradata_len);
>           if (nt->sysdata_fields & SYSDATA_TASKNAME)
>                   extradata_len += append_taskname(nt, extradata_len);
> 
> What would you write yourself?

I think that I would reduce the number of parameters of append_cpu_nr() and
append_taskname(). But really, any of the options, including this patch
as-is, are fine. So please chose whichever you think is best.

Re: [PATCH net-next 5/7] netconsole: add task name to extra data fields

[Simon Horman]

On Tue, Feb 25, 2025 at 03:17:33AM -0800, Breno Leitao wrote:
> Hello Simon,
> 
> On Tue, Feb 25, 2025 at 10:19:10AM +0000, Simon Horman wrote:
> > On Fri, Feb 21, 2025 at 05:52:10AM -0800, Breno Leitao wrote:
> > > This is the core patch for this whole patchset. Add support for
> > > including the current task's name in netconsole's extra data output.
> > > This adds a new append_taskname() function that writes the task name
> > > (from current->comm) into the target's extradata buffer, similar to how
> > > CPU numbers are handled.
> > > 
> > > The task name is included when the SYSDATA_TASKNAME field is set,
> > > appearing in the format "taskname=<name>" in the output. This additional
> > > context can help with debugging by showing which task generated each
> > > console message.
> > > 
> > > Signed-off-by: Breno Leitao <leitao@debian.org>
> > > ---
> > >  drivers/net/netconsole.c | 14 +++++++++++++-
> > >  1 file changed, 13 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > > index 5a29144ae37ee7b487b1a252b0f2ce8574f9cefa..625f4c0be11d8deb454139b1c526abc842697219 100644
> > > --- a/drivers/net/netconsole.c
> > > +++ b/drivers/net/netconsole.c
> > > @@ -1179,12 +1179,22 @@ static int append_cpu_nr(struct netconsole_target *nt, int offset)
> > >  			 raw_smp_processor_id());
> > >  }
> > >  
> > > +static int append_taskname(struct netconsole_target *nt, int offset)
> > > +{
> > > +	if (WARN_ON_ONCE(!current))
> > > +		return 0;
> > 
> > Hi Breno,
> > 
> > I gather that theoretically this could occur, but it isn't expected
> > to happen in practice. Is that right?
> 
> That's correct. `current` isn't expected to be NULL in practice.
> I've been running this code on several servers for days and have never
> encountered this warning. 
> 
> While the taskname feature isn't enabled during early boot, netconsole
> might be active at that time, which is why I exercised extra caution
> here.
> 
> If you also think this is safe, I am more than happy to remove this
> check.

Thanks for the clarification.

Given the above explanation I would have added the check too.
So from my PoV we are good.

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next 5/7] netconsole: add task name to extra data fields

[Paolo Abeni]

On 2/25/25 2:11 PM, Breno Leitao wrote:
> Ack. I will remove the check then, and check if the UDP stack has been
> initialized before calling netpoll helpers.
> 
> What is the best way to make sure taht the UDP stack has been
> initialized?

[brosers!] Sorry for the latency. My statement was intended as a
negative example: "adding such check would be as misleading as checking
if the UDP stack has been before calling netpoll helpers". The UDP stack
is always initialized when called from netpoll, due to the relevant
"init" order.

/P