[PATCH net-next v5 1/2] pktgen: Automate flag enumeration for unknown flag handling

[PATCH net-next v5 1/2] pktgen: Automate flag enumeration for unknown flag handling

[Liang Chen]

When specifying an unknown flag, it will print all available flags.
Currently, these flags are provided as fixed strings, which requires
manual updates when flags change. Replacing it with automated flag
enumeration.

Signed-off-by: Liang Chen <liangchen.linux@gmail.com>
Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
---
 Changes from v3:
- check "n == IPSEC_SHIFT" instead of string comparison
- use snprintf and check that the result does not overrun pkg_dev->result[]
- avoid double '\n' at the end
- move "return" in the OK case to remove "else" and decrease indent
---
 net/core/pktgen.c | 38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index f56b8d697014..48306a101fd9 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -1318,9 +1318,10 @@ static ssize_t pktgen_if_write(struct file *file,
 		return count;
 	}
 	if (!strcmp(name, "flag")) {
+		bool disable = false;
 		__u32 flag;
 		char f[32];
-		bool disable = false;
+		char *end;
 
 		memset(f, 0, 32);
 		len = strn_len(&user_buffer[i], sizeof(f) - 1);
@@ -1332,28 +1333,33 @@ static ssize_t pktgen_if_write(struct file *file,
 		i += len;
 
 		flag = pktgen_read_flag(f, &disable);
-
 		if (flag) {
 			if (disable)
 				pkt_dev->flags &= ~flag;
 			else
 				pkt_dev->flags |= flag;
-		} else {
-			sprintf(pg_result,
-				"Flag -:%s:- unknown\nAvailable flags, (prepend ! to un-set flag):\n%s",
-				f,
-				"IPSRC_RND, IPDST_RND, UDPSRC_RND, UDPDST_RND, "
-				"MACSRC_RND, MACDST_RND, TXSIZE_RND, IPV6, "
-				"MPLS_RND, VID_RND, SVID_RND, FLOW_SEQ, "
-				"QUEUE_MAP_RND, QUEUE_MAP_CPU, UDPCSUM, "
-				"NO_TIMESTAMP, "
-#ifdef CONFIG_XFRM
-				"IPSEC, "
-#endif
-				"NODE_ALLOC\n");
+
+			sprintf(pg_result, "OK: flags=0x%x", pkt_dev->flags);
 			return count;
 		}
-		sprintf(pg_result, "OK: flags=0x%x", pkt_dev->flags);
+
+		/* Unknown flag */
+		end = pkt_dev->result + sizeof(pkt_dev->result);
+		pg_result += sprintf(pg_result,
+			"Flag -:%s:- unknown\n"
+			"Available flags, (prepend ! to un-set flag):\n", f);
+
+		for (int n = 0; n < NR_PKT_FLAGS && pg_result < end; n++) {
+			if (!IS_ENABLED(CONFIG_XFRM) && n == IPSEC_SHIFT)
+				continue;
+			pg_result += snprintf(pg_result, end - pg_result,
+					      "%s, ", pkt_flag_names[n]);
+		}
+		if (!WARN_ON_ONCE(pg_result >= end)) {
+			/* Remove the comma and whitespace at the end */
+			*(pg_result - 2) = '\0';
+		}
+
 		return count;
 	}
 	if (!strcmp(name, "dst_min") || !strcmp(name, "dst")) {
-- 
2.31.1

[PATCH net-next v5 2/2] pktgen: Introducing 'SHARED' flag for testing with non-shared skb

[Liang Chen]

Currently, skbs generated by pktgen always have their reference count
incremented before transmission, causing their reference count to be
always greater than 1, leading to two issues:
  1. Only the code paths for shared skbs can be tested.
  2. In certain situations, skbs can only be released by pktgen.
To enhance testing comprehensiveness, we are introducing the "SHARED"
flag to indicate whether an SKB is shared. This flag is enabled by
default, aligning with the current behavior. However, disabling this
flag allows skbs with a reference count of 1 to be transmitted.
So we can test non-shared skbs and code paths where skbs are released
within the stack.

Signed-off-by: Liang Chen <liangchen.linux@gmail.com>
Reviewed-by: Benjamin Poirier <bpoirier@nvidia.com>
---
 Changes from v4:
- read the shared flag once in pktgen_xmit to avoid inconsistent burst,
  clone and shared flag values.
---
 Documentation/networking/pktgen.rst | 12 ++++++
 net/core/pktgen.c                   | 64 ++++++++++++++++++++++++-----
 2 files changed, 66 insertions(+), 10 deletions(-)

diff --git a/Documentation/networking/pktgen.rst b/Documentation/networking/pktgen.rst
index 1225f0f63ff0..c945218946e1 100644
--- a/Documentation/networking/pktgen.rst
+++ b/Documentation/networking/pktgen.rst
@@ -178,6 +178,7 @@ Examples::
 			      IPSEC # IPsec encapsulation (needs CONFIG_XFRM)
 			      NODE_ALLOC # node specific memory allocation
 			      NO_TIMESTAMP # disable timestamping
+			      SHARED # enable shared SKB
  pgset 'flag ![name]'    Clear a flag to determine behaviour.
 			 Note that you might need to use single quote in
 			 interactive mode, so that your shell wouldn't expand
@@ -288,6 +289,16 @@ To avoid breaking existing testbed scripts for using AH type and tunnel mode,
 you can use "pgset spi SPI_VALUE" to specify which transformation mode
 to employ.
 
+Disable shared SKB
+==================
+By default, SKBs sent by pktgen are shared (user count > 1).
+To test with non-shared SKBs, remove the "SHARED" flag by simply setting::
+
+	pg_set "flag !SHARED"
+
+However, if the "clone_skb" or "burst" parameters are configured, the skb
+still needs to be held by pktgen for further access. Hence the skb must be
+shared.
 
 Current commands and configuration options
 ==========================================
@@ -357,6 +368,7 @@ Current commands and configuration options
     IPSEC
     NODE_ALLOC
     NO_TIMESTAMP
+    SHARED
 
     spi (ipsec)
 
diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 48306a101fd9..5e865af82e5b 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -200,6 +200,7 @@
 	pf(VID_RND)		/* Random VLAN ID */			\
 	pf(SVID_RND)		/* Random SVLAN ID */			\
 	pf(NODE)		/* Node memory alloc*/			\
+	pf(SHARED)		/* Shared SKB */			\
 
 #define pf(flag)		flag##_SHIFT,
 enum pkt_flags {
@@ -1198,7 +1199,8 @@ static ssize_t pktgen_if_write(struct file *file,
 		    ((pkt_dev->xmit_mode == M_NETIF_RECEIVE) ||
 		     !(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING)))
 			return -ENOTSUPP;
-		if (value > 0 && pkt_dev->n_imix_entries > 0)
+		if (value > 0 && (pkt_dev->n_imix_entries > 0 ||
+				  !(pkt_dev->flags & F_SHARED)))
 			return -EINVAL;
 
 		i += len;
@@ -1257,6 +1259,10 @@ static ssize_t pktgen_if_write(struct file *file,
 		     ((pkt_dev->xmit_mode == M_START_XMIT) &&
 		     (!(pkt_dev->odev->priv_flags & IFF_TX_SKB_SHARING)))))
 			return -ENOTSUPP;
+
+		if (value > 1 && !(pkt_dev->flags & F_SHARED))
+			return -EINVAL;
+
 		pkt_dev->burst = value < 1 ? 1 : value;
 		sprintf(pg_result, "OK: burst=%u", pkt_dev->burst);
 		return count;
@@ -1334,10 +1340,19 @@ static ssize_t pktgen_if_write(struct file *file,
 
 		flag = pktgen_read_flag(f, &disable);
 		if (flag) {
-			if (disable)
+			if (disable) {
+				/* If "clone_skb", or "burst" parameters are
+				 * configured, it means that the skb still
+				 * needs to be referenced by the pktgen, so
+				 * the skb must be shared.
+				 */
+				if (flag == F_SHARED && (pkt_dev->clone_skb ||
+							 pkt_dev->burst > 1))
+					return -EINVAL;
 				pkt_dev->flags &= ~flag;
-			else
+			} else {
 				pkt_dev->flags |= flag;
+			}
 
 			sprintf(pg_result, "OK: flags=0x%x", pkt_dev->flags);
 			return count;
@@ -3446,12 +3461,24 @@ static void pktgen_wait_for_skb(struct pktgen_dev *pkt_dev)
 
 static void pktgen_xmit(struct pktgen_dev *pkt_dev)
 {
-	unsigned int burst = READ_ONCE(pkt_dev->burst);
+	bool skb_shared = !!(READ_ONCE(pkt_dev->flags) & F_SHARED);
 	struct net_device *odev = pkt_dev->odev;
 	struct netdev_queue *txq;
+	unsigned int burst = 1;
 	struct sk_buff *skb;
+	int clone_skb = 0;
 	int ret;
 
+	/* If 'skb_shared' is false, the read of possible
+	 * new values (if any) for 'burst' and 'clone_skb' will be skipped to
+	 * prevent some concurrent changes from slipping in. And the stabilized
+	 * config will be read in during the next run of pktgen_xmit.
+	 */
+	if (skb_shared) {
+		burst = READ_ONCE(pkt_dev->burst);
+		clone_skb = READ_ONCE(pkt_dev->clone_skb);
+	}
+
 	/* If device is offline, then don't send */
 	if (unlikely(!netif_running(odev) || !netif_carrier_ok(odev))) {
 		pktgen_stop_device(pkt_dev);
@@ -3468,7 +3495,7 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
 
 	/* If no skb or clone count exhausted then get new one */
 	if (!pkt_dev->skb || (pkt_dev->last_ok &&
-			      ++pkt_dev->clone_count >= pkt_dev->clone_skb)) {
+			      ++pkt_dev->clone_count >= clone_skb)) {
 		/* build a new pkt */
 		kfree_skb(pkt_dev->skb);
 
@@ -3489,7 +3516,8 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
 	if (pkt_dev->xmit_mode == M_NETIF_RECEIVE) {
 		skb = pkt_dev->skb;
 		skb->protocol = eth_type_trans(skb, skb->dev);
-		refcount_add(burst, &skb->users);
+		if (skb_shared)
+			refcount_add(burst, &skb->users);
 		local_bh_disable();
 		do {
 			ret = netif_receive_skb(skb);
@@ -3497,6 +3525,10 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
 				pkt_dev->errors++;
 			pkt_dev->sofar++;
 			pkt_dev->seq_num++;
+			if (unlikely(!skb_shared)) {
+				pkt_dev->skb = NULL;
+				break;
+			}
 			if (refcount_read(&skb->users) != burst) {
 				/* skb was queued by rps/rfs or taps,
 				 * so cannot reuse this skb
@@ -3515,9 +3547,14 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
 		goto out; /* Skips xmit_mode M_START_XMIT */
 	} else if (pkt_dev->xmit_mode == M_QUEUE_XMIT) {
 		local_bh_disable();
-		refcount_inc(&pkt_dev->skb->users);
+		if (skb_shared)
+			refcount_inc(&pkt_dev->skb->users);
 
 		ret = dev_queue_xmit(pkt_dev->skb);
+
+		if (!skb_shared && dev_xmit_complete(ret))
+			pkt_dev->skb = NULL;
+
 		switch (ret) {
 		case NET_XMIT_SUCCESS:
 			pkt_dev->sofar++;
@@ -3555,11 +3592,15 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
 		pkt_dev->last_ok = 0;
 		goto unlock;
 	}
-	refcount_add(burst, &pkt_dev->skb->users);
+	if (skb_shared)
+		refcount_add(burst, &pkt_dev->skb->users);
 
 xmit_more:
 	ret = netdev_start_xmit(pkt_dev->skb, odev, txq, --burst > 0);
 
+	if (!skb_shared && dev_xmit_complete(ret))
+		pkt_dev->skb = NULL;
+
 	switch (ret) {
 	case NETDEV_TX_OK:
 		pkt_dev->last_ok = 1;
@@ -3581,7 +3622,8 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
 		fallthrough;
 	case NETDEV_TX_BUSY:
 		/* Retry it next time */
-		refcount_dec(&(pkt_dev->skb->users));
+		if (skb_shared)
+			refcount_dec(&pkt_dev->skb->users);
 		pkt_dev->last_ok = 0;
 	}
 	if (unlikely(burst))
@@ -3594,7 +3636,8 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
 
 	/* If pkt_dev->count is zero, then run forever */
 	if ((pkt_dev->count != 0) && (pkt_dev->sofar >= pkt_dev->count)) {
-		pktgen_wait_for_skb(pkt_dev);
+		if (pkt_dev->skb)
+			pktgen_wait_for_skb(pkt_dev);
 
 		/* Done with this */
 		pktgen_stop_device(pkt_dev);
@@ -3777,6 +3820,7 @@ static int pktgen_add_device(struct pktgen_thread *t, const char *ifname)
 	pkt_dev->svlan_id = 0xffff;
 	pkt_dev->burst = 1;
 	pkt_dev->node = NUMA_NO_NODE;
+	pkt_dev->flags = F_SHARED;	/* SKB shared by default */
 
 	err = pktgen_setup_dev(t->net, pkt_dev, ifname);
 	if (err)
-- 
2.31.1

Re: [PATCH net-next v5 1/2] pktgen: Automate flag enumeration for unknown flag handling

[Simon Horman]

On Wed, Sep 20, 2023 at 08:56:57PM +0800, Liang Chen wrote:
> When specifying an unknown flag, it will print all available flags.
> Currently, these flags are provided as fixed strings, which requires
> manual updates when flags change. Replacing it with automated flag
> enumeration.
> 
> Signed-off-by: Liang Chen <liangchen.linux@gmail.com>
> Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
> ---
>  Changes from v3:
> - check "n == IPSEC_SHIFT" instead of string comparison
> - use snprintf and check that the result does not overrun pkg_dev->result[]
> - avoid double '\n' at the end
> - move "return" in the OK case to remove "else" and decrease indent
> ---
>  net/core/pktgen.c | 38 ++++++++++++++++++++++----------------
>  1 file changed, 22 insertions(+), 16 deletions(-)
> 
> diff --git a/net/core/pktgen.c b/net/core/pktgen.c
> index f56b8d697014..48306a101fd9 100644
> --- a/net/core/pktgen.c
> +++ b/net/core/pktgen.c
> @@ -1318,9 +1318,10 @@ static ssize_t pktgen_if_write(struct file *file,
>  		return count;
>  	}
>  	if (!strcmp(name, "flag")) {
> +		bool disable = false;
>  		__u32 flag;
>  		char f[32];
> -		bool disable = false;
> +		char *end;
>  
>  		memset(f, 0, 32);
>  		len = strn_len(&user_buffer[i], sizeof(f) - 1);
> @@ -1332,28 +1333,33 @@ static ssize_t pktgen_if_write(struct file *file,
>  		i += len;
>  
>  		flag = pktgen_read_flag(f, &disable);
> -
>  		if (flag) {
>  			if (disable)
>  				pkt_dev->flags &= ~flag;
>  			else
>  				pkt_dev->flags |= flag;
> -		} else {
> -			sprintf(pg_result,
> -				"Flag -:%s:- unknown\nAvailable flags, (prepend ! to un-set flag):\n%s",
> -				f,
> -				"IPSRC_RND, IPDST_RND, UDPSRC_RND, UDPDST_RND, "
> -				"MACSRC_RND, MACDST_RND, TXSIZE_RND, IPV6, "
> -				"MPLS_RND, VID_RND, SVID_RND, FLOW_SEQ, "
> -				"QUEUE_MAP_RND, QUEUE_MAP_CPU, UDPCSUM, "
> -				"NO_TIMESTAMP, "
> -#ifdef CONFIG_XFRM
> -				"IPSEC, "
> -#endif
> -				"NODE_ALLOC\n");
> +
> +			sprintf(pg_result, "OK: flags=0x%x", pkt_dev->flags);
>  			return count;
>  		}
> -		sprintf(pg_result, "OK: flags=0x%x", pkt_dev->flags);
> +
> +		/* Unknown flag */
> +		end = pkt_dev->result + sizeof(pkt_dev->result);
> +		pg_result += sprintf(pg_result,
> +			"Flag -:%s:- unknown\n"
> +			"Available flags, (prepend ! to un-set flag):\n", f);
> +
> +		for (int n = 0; n < NR_PKT_FLAGS && pg_result < end; n++) {
> +			if (!IS_ENABLED(CONFIG_XFRM) && n == IPSEC_SHIFT)
> +				continue;
> +			pg_result += snprintf(pg_result, end - pg_result,
> +					      "%s, ", pkt_flag_names[n]);
> +		}
> +		if (!WARN_ON_ONCE(pg_result >= end)) {
> +			/* Remove the comma and whitespace at the end */
> +			*(pg_result - 2) = '\0';

Hi Liang Chen,

Should the string have a trailing '\n' in keeping with the current formatting?

> +		}
> +
>  		return count;
>  	}
>  	if (!strcmp(name, "dst_min") || !strcmp(name, "dst")) {
> -- 
> 2.31.1
> 
> 

Re: [PATCH net-next v5 1/2] pktgen: Automate flag enumeration for unknown flag handling

[Benjamin Poirier]

On 2023-09-28 13:21 +0200, Simon Horman wrote:
> On Wed, Sep 20, 2023 at 08:56:57PM +0800, Liang Chen wrote:
> > When specifying an unknown flag, it will print all available flags.
> > Currently, these flags are provided as fixed strings, which requires
> > manual updates when flags change. Replacing it with automated flag
> > enumeration.
> > 
> > Signed-off-by: Liang Chen <liangchen.linux@gmail.com>
> > Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
> > ---
> >  Changes from v3:
> > - check "n == IPSEC_SHIFT" instead of string comparison
> > - use snprintf and check that the result does not overrun pkg_dev->result[]
> > - avoid double '\n' at the end

      ^

[...]

> > -		} else {
> > -			sprintf(pg_result,
> > -				"Flag -:%s:- unknown\nAvailable flags, (prepend ! to un-set flag):\n%s",
> > -				f,
> > -				"IPSRC_RND, IPDST_RND, UDPSRC_RND, UDPDST_RND, "
> > -				"MACSRC_RND, MACDST_RND, TXSIZE_RND, IPV6, "
> > -				"MPLS_RND, VID_RND, SVID_RND, FLOW_SEQ, "
> > -				"QUEUE_MAP_RND, QUEUE_MAP_CPU, UDPCSUM, "
> > -				"NO_TIMESTAMP, "
> > -#ifdef CONFIG_XFRM
> > -				"IPSEC, "
> > -#endif
> > -				"NODE_ALLOC\n");
> > +
> > +			sprintf(pg_result, "OK: flags=0x%x", pkt_dev->flags);
> >  			return count;
> >  		}
> > -		sprintf(pg_result, "OK: flags=0x%x", pkt_dev->flags);
> > +
> > +		/* Unknown flag */
> > +		end = pkt_dev->result + sizeof(pkt_dev->result);
> > +		pg_result += sprintf(pg_result,
> > +			"Flag -:%s:- unknown\n"
> > +			"Available flags, (prepend ! to un-set flag):\n", f);
> > +
> > +		for (int n = 0; n < NR_PKT_FLAGS && pg_result < end; n++) {
> > +			if (!IS_ENABLED(CONFIG_XFRM) && n == IPSEC_SHIFT)
> > +				continue;
> > +			pg_result += snprintf(pg_result, end - pg_result,
> > +					      "%s, ", pkt_flag_names[n]);
> > +		}
> > +		if (!WARN_ON_ONCE(pg_result >= end)) {
> > +			/* Remove the comma and whitespace at the end */
> > +			*(pg_result - 2) = '\0';
> 
> Hi Liang Chen,
> 
> Should the string have a trailing '\n' in keeping with the current formatting?

A '\n' is already added when the string is output by pktgen_if_show() so
if the string above has a trailing '\n', it leads to an empty line in
the output.

If my count is correct, before this patch there are 56 cases that output
to pkt_dev->result/pg_result in pktgen_if_write() and only 3 of them
include a trailing '\n', arguably by mistake.

So, I think it's better to remove the empty line than to keep the
current formatting.

Re: [PATCH net-next v5 1/2] pktgen: Automate flag enumeration for unknown flag handling

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Wed, 20 Sep 2023 20:56:57 +0800 you wrote:
> When specifying an unknown flag, it will print all available flags.
> Currently, these flags are provided as fixed strings, which requires
> manual updates when flags change. Replacing it with automated flag
> enumeration.
> 
> Signed-off-by: Liang Chen <liangchen.linux@gmail.com>
> Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
> 
> [...]

Here is the summary with links:
  - [net-next,v5,1/2] pktgen: Automate flag enumeration for unknown flag handling
    https://git.kernel.org/netdev/net-next/c/057708a9ca59
  - [net-next,v5,2/2] pktgen: Introducing 'SHARED' flag for testing with non-shared skb
    https://git.kernel.org/netdev/net-next/c/7c7dd1d64910

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next v5 1/2] pktgen: Automate flag enumeration for unknown flag handling

[Simon Horman]

On Thu, Sep 28, 2023 at 09:18:08AM -0400, Benjamin Poirier wrote:
> On 2023-09-28 13:21 +0200, Simon Horman wrote:
> > On Wed, Sep 20, 2023 at 08:56:57PM +0800, Liang Chen wrote:
> > > When specifying an unknown flag, it will print all available flags.
> > > Currently, these flags are provided as fixed strings, which requires
> > > manual updates when flags change. Replacing it with automated flag
> > > enumeration.
> > > 
> > > Signed-off-by: Liang Chen <liangchen.linux@gmail.com>
> > > Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
> > > ---
> > >  Changes from v3:
> > > - check "n == IPSEC_SHIFT" instead of string comparison
> > > - use snprintf and check that the result does not overrun pkg_dev->result[]
> > > - avoid double '\n' at the end
> 
>       ^
> 
> [...]
> 
> > > -		} else {
> > > -			sprintf(pg_result,
> > > -				"Flag -:%s:- unknown\nAvailable flags, (prepend ! to un-set flag):\n%s",
> > > -				f,
> > > -				"IPSRC_RND, IPDST_RND, UDPSRC_RND, UDPDST_RND, "
> > > -				"MACSRC_RND, MACDST_RND, TXSIZE_RND, IPV6, "
> > > -				"MPLS_RND, VID_RND, SVID_RND, FLOW_SEQ, "
> > > -				"QUEUE_MAP_RND, QUEUE_MAP_CPU, UDPCSUM, "
> > > -				"NO_TIMESTAMP, "
> > > -#ifdef CONFIG_XFRM
> > > -				"IPSEC, "
> > > -#endif
> > > -				"NODE_ALLOC\n");
> > > +
> > > +			sprintf(pg_result, "OK: flags=0x%x", pkt_dev->flags);
> > >  			return count;
> > >  		}
> > > -		sprintf(pg_result, "OK: flags=0x%x", pkt_dev->flags);
> > > +
> > > +		/* Unknown flag */
> > > +		end = pkt_dev->result + sizeof(pkt_dev->result);
> > > +		pg_result += sprintf(pg_result,
> > > +			"Flag -:%s:- unknown\n"
> > > +			"Available flags, (prepend ! to un-set flag):\n", f);
> > > +
> > > +		for (int n = 0; n < NR_PKT_FLAGS && pg_result < end; n++) {
> > > +			if (!IS_ENABLED(CONFIG_XFRM) && n == IPSEC_SHIFT)
> > > +				continue;
> > > +			pg_result += snprintf(pg_result, end - pg_result,
> > > +					      "%s, ", pkt_flag_names[n]);
> > > +		}
> > > +		if (!WARN_ON_ONCE(pg_result >= end)) {
> > > +			/* Remove the comma and whitespace at the end */
> > > +			*(pg_result - 2) = '\0';
> > 
> > Hi Liang Chen,
> > 
> > Should the string have a trailing '\n' in keeping with the current formatting?
> 
> A '\n' is already added when the string is output by pktgen_if_show() so
> if the string above has a trailing '\n', it leads to an empty line in
> the output.
> 
> If my count is correct, before this patch there are 56 cases that output
> to pkt_dev->result/pg_result in pktgen_if_write() and only 3 of them
> include a trailing '\n', arguably by mistake.
> 
> So, I think it's better to remove the empty line than to keep the
> current formatting.

Thanks for the clarification.

I have no further objections, but if the patch is resupn for some other
reason, then it might be worth mentioning this in the patch description.

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