Message-ID: <jejsptfg7cqmbcm467tb72gg3mwsqge6iz4qy4wy5ifev2sgim@hukyfgsr74xj>
Date: Fri, 7 Nov 2025 05:23:26 -0800
From: Breno Leitao <leitao@...ian.org>
To: Gustavo Luiz Duarte <gustavold@...il.com>
Cc: Andre Carvalho <asantostc@...il.com>, Simon Horman <horms@...nel.org>, 
	Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Shuah Khan <shuah@...nel.org>, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next 2/4] netconsole: Split userdata and sysdata

On Wed, Nov 05, 2025 at 09:06:44AM -0800, Gustavo Luiz Duarte wrote:
> Separate userdata and sysdata into distinct buffers to enable independent
> management. Previously, both were stored in a single extradata_complete
> buffer with a fixed size that accommodated both types of data.
> 
> This separation allows:
> - userdata to grow dynamically (in subsequent patch)
> - sysdata to remain in a small static buffer
> - removal of complex entry counting logic that tracked both types together
> 
> The split also simplifies the code by eliminating the need to check total
> entry count across both userdata and sysdata when enabling features,
> which allows to drop holding su_mutex on sysdata_*_enabled_store().
> 
> No functional change in this patch, just structural preparation for
> dynamic userdata allocation.
> 
> Signed-off-by: Gustavo Luiz Duarte <gustavold@...il.com>
> ---
>  drivers/net/netconsole.c | 204 +++++++++++++++++++----------------------------
>  1 file changed, 84 insertions(+), 120 deletions(-)
> 
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 0a8ba7c4bc9d..e780c884db83 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -50,7 +50,8 @@ MODULE_LICENSE("GPL");
>  /* The number 3 comes from userdata entry format characters (' ', '=', '\n') */
>  #define MAX_EXTRADATA_NAME_LEN		(MAX_EXTRADATA_ENTRY_LEN - \
>  					MAX_EXTRADATA_VALUE_LEN - 3)
> -#define MAX_EXTRADATA_ITEMS		16
> +#define MAX_USERDATA_ITEMS		16

Do we still need to have MAX_USERDATA_ITEMS cap with your new approach?

> +#define MAX_SYSDATA_ITEMS		4

Can we have this one inside enum sysdata_feature?

Something as:

  enum sysdata_feature {
      SYSDATA_CPU_NR = BIT(0),
      SYSDATA_TASKNAME = BIT(1),
      SYSDATA_RELEASE = BIT(2),
      SYSDATA_MSGID = BIT(3),
      MAX_SYSDATA_ITEMS = 4  /* Sentinel: highest bit position */
  };

>  #define MAX_PRINT_CHUNK			1000
>  
>  static char config[MAX_PARAM_LENGTH];
> @@ -122,8 +123,9 @@ enum sysdata_feature {
>   * @list:	Links this target into the target_list.
>   * @group:	Links us into the configfs subsystem hierarchy.
>   * @userdata_group:	Links to the userdata configfs hierarchy
> - * @extradata_complete:	Cached, formatted string of append
> - * @userdata_length:	String length of usedata in extradata_complete.
> + * @userdata:		Cached, formatted string of append
> + * @userdata_length:	String length of userdata.
> + * @sysdata:		Cached, formatted string of append
>   * @sysdata_fields:	Sysdata features enabled.
>   * @msgcounter:	Message sent counter.
>   * @stats:	Packet send stats for the target. Used for debugging.
> @@ -152,8 +154,10 @@ struct netconsole_target {
>  #ifdef	CONFIG_NETCONSOLE_DYNAMIC
>  	struct config_group	group;
>  	struct config_group	userdata_group;
> -	char extradata_complete[MAX_EXTRADATA_ENTRY_LEN * MAX_EXTRADATA_ITEMS];
> +	char			userdata[MAX_EXTRADATA_ENTRY_LEN * MAX_USERDATA_ITEMS];
>  	size_t			userdata_length;
> +	char			sysdata[MAX_EXTRADATA_ENTRY_LEN * MAX_SYSDATA_ITEMS];
> +
>  	/* bit-wise with sysdata_feature bits */
>  	u32			sysdata_fields;
>  	/* protected by target_list_lock */
> @@ -802,28 +806,14 @@ static ssize_t remote_ip_store(struct config_item *item, const char *buf,
>  	return ret;
>  }
>  
> -/* Count number of entries we have in extradata.
> - * This is important because the extradata_complete only supports
> - * MAX_EXTRADATA_ITEMS entries. Before enabling any new {user,sys}data
> - * feature, number of entries needs to checked for available space.
> +/* Count number of entries we have in userdata.
> + * This is important because userdata only supports MAX_USERDATA_ITEMS
> + * entries. Before enabling any new userdata feature, number of entries needs
> + * to checked for available space.
>   */
> -static size_t count_extradata_entries(struct netconsole_target *nt)
> +static size_t count_userdata_entries(struct netconsole_target *nt)
>  {
> -	size_t entries;
> -
> -	/* Userdata entries */
> -	entries = list_count_nodes(&nt->userdata_group.cg_children);
> -	/* Plus sysdata entries */
> -	if (nt->sysdata_fields & SYSDATA_CPU_NR)
> -		entries += 1;
> -	if (nt->sysdata_fields & SYSDATA_TASKNAME)
> -		entries += 1;
> -	if (nt->sysdata_fields & SYSDATA_RELEASE)
> -		entries += 1;
> -	if (nt->sysdata_fields & SYSDATA_MSGID)
> -		entries += 1;
> -
> -	return entries;
> +	return list_count_nodes(&nt->userdata_group.cg_children);
>  }
>  
>  static ssize_t remote_mac_store(struct config_item *item, const char *buf,
> @@ -894,13 +884,13 @@ static void update_userdata(struct netconsole_target *nt)
>  
>  	/* Clear the current string in case the last userdatum was deleted */
>  	nt->userdata_length = 0;
> -	nt->extradata_complete[0] = 0;
> +	nt->userdata[0] = 0;
>  
>  	list_for_each(entry, &nt->userdata_group.cg_children) {
>  		struct userdatum *udm_item;
>  		struct config_item *item;
>  
> -		if (child_count >= MAX_EXTRADATA_ITEMS) {
> +		if (child_count >= MAX_USERDATA_ITEMS) {
>  			spin_unlock_irqrestore(&target_list_lock, flags);
>  			WARN_ON_ONCE(1);
>  			return;
> @@ -914,11 +904,11 @@ static void update_userdata(struct netconsole_target *nt)
>  		if (strnlen(udm_item->value, MAX_EXTRADATA_VALUE_LEN) == 0)
>  			continue;
>  
> -		/* This doesn't overflow extradata_complete since it will write
> -		 * one entry length (1/MAX_EXTRADATA_ITEMS long), entry count is
> +		/* This doesn't overflow userdata since it will write
> +		 * one entry length (1/MAX_USERDATA_ITEMS long), entry count is
>  		 * checked to not exceed MAX items with child_count above
>  		 */
> -		nt->userdata_length += scnprintf(&nt->extradata_complete[nt->userdata_length],
> +		nt->userdata_length += scnprintf(&nt->userdata[nt->userdata_length],
>  						 MAX_EXTRADATA_ENTRY_LEN, " %s=%s\n",
>  						 item->ci_name, udm_item->value);
>  	}
> @@ -960,7 +950,7 @@ static void disable_sysdata_feature(struct netconsole_target *nt,
>  				    enum sysdata_feature feature)
>  {
>  	nt->sysdata_fields &= ~feature;
> -	nt->extradata_complete[nt->userdata_length] = 0;
> +	nt->sysdata[0] = 0;
>  }
>  
>  static ssize_t sysdata_msgid_enabled_store(struct config_item *item,
> @@ -979,12 +969,6 @@ static ssize_t sysdata_msgid_enabled_store(struct config_item *item,
>  	if (msgid_enabled == curr)
>  		goto unlock_ok;
>  
> -	if (msgid_enabled &&
> -	    count_extradata_entries(nt) >= MAX_EXTRADATA_ITEMS) {
> -		ret = -ENOSPC;
> -		goto unlock;
> -	}
> -
>  	if (msgid_enabled)
>  		nt->sysdata_fields |= SYSDATA_MSGID;
>  	else
> @@ -992,7 +976,6 @@ static ssize_t sysdata_msgid_enabled_store(struct config_item *item,
>  
>  unlock_ok:
>  	ret = strnlen(buf, count);
> -unlock:
>  	mutex_unlock(&dynamic_netconsole_mutex);
>  	return ret;
>  }
> @@ -1013,12 +996,6 @@ static ssize_t sysdata_release_enabled_store(struct config_item *item,
>  	if (release_enabled == curr)
>  		goto unlock_ok;
>  
> -	if (release_enabled &&
> -	    count_extradata_entries(nt) >= MAX_EXTRADATA_ITEMS) {
> -		ret = -ENOSPC;
> -		goto unlock;
> -	}
> -
>  	if (release_enabled)
>  		nt->sysdata_fields |= SYSDATA_RELEASE;
>  	else
> @@ -1026,7 +1003,6 @@ static ssize_t sysdata_release_enabled_store(struct config_item *item,
>  
>  unlock_ok:
>  	ret = strnlen(buf, count);
> -unlock:
>  	mutex_unlock(&dynamic_netconsole_mutex);
>  	return ret;
>  }
> @@ -1047,12 +1023,6 @@ static ssize_t sysdata_taskname_enabled_store(struct config_item *item,
>  	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
> @@ -1060,7 +1030,6 @@ static ssize_t sysdata_taskname_enabled_store(struct config_item *item,
>  
>  unlock_ok:
>  	ret = strnlen(buf, count);
> -unlock:
>  	mutex_unlock(&dynamic_netconsole_mutex);
>  	return ret;
>  }
> @@ -1083,27 +1052,16 @@ static ssize_t sysdata_cpu_nr_enabled_store(struct config_item *item,
>  		/* no change requested */
>  		goto unlock_ok;
>  
> -	if (cpu_nr_enabled &&
> -	    count_extradata_entries(nt) >= MAX_EXTRADATA_ITEMS) {
> -		/* user wants the new feature, but there is no space in the
> -		 * buffer.
> -		 */
> -		ret = -ENOSPC;
> -		goto unlock;
> -	}
> -
>  	if (cpu_nr_enabled)
>  		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.
> +		/* This is special because sysdata might have remaining data
> +		 * from previous sysdata, and it needs to be cleaned.
>  		 */
>  		disable_sysdata_feature(nt, SYSDATA_CPU_NR);
>  
>  unlock_ok:
>  	ret = strnlen(buf, count);
> -unlock:
>  	mutex_unlock(&dynamic_netconsole_mutex);
>  	return ret;
>  }
> @@ -1146,7 +1104,7 @@ static struct config_item *userdatum_make_item(struct config_group *group,
>  
>  	ud = to_userdata(&group->cg_item);
>  	nt = userdata_to_target(ud);
> -	if (count_extradata_entries(nt) >= MAX_EXTRADATA_ITEMS)
> +	if (count_userdata_entries(nt) >= MAX_USERDATA_ITEMS)
>  		return ERR_PTR(-ENOSPC);
>  
>  	udm = kzalloc(sizeof(*udm), GFP_KERNEL);
> @@ -1353,22 +1311,21 @@ static void populate_configfs_item(struct netconsole_target *nt,
>  
>  static int sysdata_append_cpu_nr(struct netconsole_target *nt, int offset)
>  {
> -	/* Append cpu=%d at extradata_complete after userdata str */
> -	return scnprintf(&nt->extradata_complete[offset],
> +	return scnprintf(&nt->sysdata[offset],
>  			 MAX_EXTRADATA_ENTRY_LEN, " cpu=%u\n",

This is confusing. It is writing to sysdata but checking extradata entry len.

>  static int sysdata_append_taskname(struct netconsole_target *nt, int offset)
>  {
> -	return scnprintf(&nt->extradata_complete[offset],
> +	return scnprintf(&nt->sysdata[offset],
>  			 MAX_EXTRADATA_ENTRY_LEN, " taskname=%s\n",

Same as above.

>  }
>  
>  static int sysdata_append_release(struct netconsole_target *nt, int offset)
>  {
> -	return scnprintf(&nt->extradata_complete[offset],
> +	return scnprintf(&nt->sysdata[offset],
>  			 MAX_EXTRADATA_ENTRY_LEN, " release=%s\n",
>  			 init_utsname()->release);
>  }
> @@ -1376,46 +1333,36 @@ static int sysdata_append_release(struct netconsole_target *nt, int offset)
>  static int sysdata_append_msgid(struct netconsole_target *nt, int offset)
>  {
>  	wrapping_assign_add(nt->msgcounter, 1);
> -	return scnprintf(&nt->extradata_complete[offset],
> +	return scnprintf(&nt->sysdata[offset],
>  			 MAX_EXTRADATA_ENTRY_LEN, " msgid=%u\n",
>  			 nt->msgcounter);
>  }
>  
>  /*
> - * prepare_extradata - append sysdata at extradata_complete in runtime
> + * prepare_sysdata - append sysdata in runtime
>   * @nt: target to send message to
>   */
> -static int prepare_extradata(struct netconsole_target *nt)
> +static int prepare_sysdata(struct netconsole_target *nt)
>  {
> -	int extradata_len;
> -
> -	/* userdata was appended when configfs write helper was called
> -	 * by update_userdata().
> -	 */
> -	extradata_len = nt->userdata_length;
> +	int sysdata_len = 0;
>  
>  	if (!nt->sysdata_fields)
>  		goto out;
>  
>  	if (nt->sysdata_fields & SYSDATA_CPU_NR)
> -		extradata_len += sysdata_append_cpu_nr(nt, extradata_len);
> +		sysdata_len += sysdata_append_cpu_nr(nt, sysdata_len);
>  	if (nt->sysdata_fields & SYSDATA_TASKNAME)
> -		extradata_len += sysdata_append_taskname(nt, extradata_len);
> +		sysdata_len += sysdata_append_taskname(nt, sysdata_len);
>  	if (nt->sysdata_fields & SYSDATA_RELEASE)
> -		extradata_len += sysdata_append_release(nt, extradata_len);
> +		sysdata_len += sysdata_append_release(nt, sysdata_len);
>  	if (nt->sysdata_fields & SYSDATA_MSGID)
> -		extradata_len += sysdata_append_msgid(nt, extradata_len);
> +		sysdata_len += sysdata_append_msgid(nt, sysdata_len);
>  
> -	WARN_ON_ONCE(extradata_len >
> -		     MAX_EXTRADATA_ENTRY_LEN * MAX_EXTRADATA_ITEMS);
> +	WARN_ON_ONCE(sysdata_len >
> +		     MAX_EXTRADATA_ENTRY_LEN * MAX_SYSDATA_ITEMS);
>  
>  out:
> -	return extradata_len;
> -}
> -#else /* CONFIG_NETCONSOLE_DYNAMIC not set */
> -static int prepare_extradata(struct netconsole_target *nt)
> -{
> -	return 0;
> +	return sysdata_len;
>  }
>  #endif	/* CONFIG_NETCONSOLE_DYNAMIC */
>  
> @@ -1517,13 +1464,8 @@ static void send_msg_no_fragmentation(struct netconsole_target *nt,
>  				      int msg_len,
>  				      int release_len)
>  {
> -	const char *extradata = NULL;
>  	const char *release;
>  
> -#ifdef CONFIG_NETCONSOLE_DYNAMIC
> -	extradata = nt->extradata_complete;
> -#endif
> -
>  	if (release_len) {
>  		release = init_utsname()->release;
>  
> @@ -1533,11 +1475,11 @@ static void send_msg_no_fragmentation(struct netconsole_target *nt,
>  		memcpy(nt->buf, msg, msg_len);
>  	}
>  
> -	if (extradata)
> -		msg_len += scnprintf(&nt->buf[msg_len],
> -				     MAX_PRINT_CHUNK - msg_len,
> -				     "%s", extradata);
> -
> +#ifdef CONFIG_NETCONSOLE_DYNAMIC
> +	msg_len += scnprintf(&nt->buf[msg_len],
> +			     MAX_PRINT_CHUNK - msg_len,
> +			     "%s%s", nt->userdata, nt->sysdata);
> +#endif

I am not sure I like this ifdef in here. Can you if userdata or sysdata are
valid, and then scnprintf() instead of using ifdef?

>  				 const char *msgbody, int header_len,
> -				 int msgbody_len, int extradata_len)
> +				 int msgbody_len, int sysdata_len)
>  {
> -	const char *extradata = NULL;
> +	const char *userdata = NULL;
> +	const char *sysdata = NULL;
>  	int body_len, offset = 0;
> -	int extradata_offset = 0;
> +	int userdata_offset = 0;
>  	int msgbody_offset = 0;
> +	int sysdata_offset = 0;
> +	int userdata_len = 0;
>  
>  #ifdef CONFIG_NETCONSOLE_DYNAMIC
> -	extradata = nt->extradata_complete;
> +	userdata = nt->userdata;
> +	sysdata = nt->sysdata;
> +	userdata_len = nt->userdata_length;
>  #endif
> -	if (WARN_ON_ONCE(!extradata && extradata_len != 0))
> +	if (WARN_ON_ONCE(!userdata && userdata_len != 0))
> +		return;
> +
> +	if (WARN_ON_ONCE(!sysdata && sysdata_len != 0))
>  		return;
>  
>  	/* body_len represents the number of bytes that will be sent. This is
>  	 * bigger than MAX_PRINT_CHUNK, thus, it will be split in multiple
>  	 * packets
>  	 */
> -	body_len = msgbody_len + extradata_len;
> +	body_len = msgbody_len + userdata_len + sysdata_len;
>  
>  	/* In each iteration of the while loop below, we send a packet
>  	 * containing the header and a portion of the body. The body is
> -	 * composed of two parts: msgbody and extradata. We keep track of how
> -	 * many bytes have been sent so far using the offset variable, which
> -	 * ranges from 0 to the total length of the body.
> +	 * composed of three parts: msgbody, userdata and sysdata.
> +	 * We keep track of how many bytes have been sent from each part using
> +	 * the *_offset variables.
> +	 * We keep track of how many bytes have been sent overall using the
> +	 * offset variable, which ranges from 0 to the total length of the
> +	 * body.
>  	 */
>  	while (offset < body_len) {
>  		int this_header = header_len;
> @@ -1594,12 +1547,20 @@ static void send_fragmented_body(struct netconsole_target *nt,
>  		msgbody_offset += this_chunk;
>  		this_offset += this_chunk;
>  
> -		/* after msgbody, append extradata */
> -		this_chunk = min(extradata_len - extradata_offset,
> +		/* after msgbody, append userdata */
> +		this_chunk = min(userdata_len - userdata_offset,

Please assign this "userdata_len - userdata_offset" to a variable and give it a
name, so, it help us to reason about the code. 

>  				 MAX_PRINT_CHUNK - this_header - this_offset);
>  		memcpy(nt->buf + this_header + this_offset,
> -		       extradata + extradata_offset, this_chunk);
> -		extradata_offset += this_chunk;
> +		       userdata + userdata_offset, this_chunk);
> +		userdata_offset += this_chunk;
> +		this_offset += this_chunk;
> +
> +		/* after userdata, append sysdata */
> +		this_chunk = min(sysdata_len - sysdata_offset,
> +				 MAX_PRINT_CHUNK - this_header - this_offset);
> +		memcpy(nt->buf + this_header + this_offset,
> +		       sysdata + sysdata_offset, this_chunk);

s/this_header/this_header_offset?

Now that you are touching this code, please review these variable to keep them named correct.

Maybe adding _ptr for pointer, and _offset for offsets and _len for lenghts?

Thank you for your work reasong about all of this!
--breno

Powered by blists - more mailing lists