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

From: Benjamin Poirier <[email-masked]>
Date: Mon, 18 Sep 2023 10:22:00 -0400

On 2023-09-16 21:29 +0800, Liang Chen wrote:
> 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 <[email-masked]>
> ---

In the future, please run scripts/get_maintainer.pl and cc the listed
addresses. I'm adding them now.

Reviewed-by: Benjamin Poirier <[email-masked]>

>  Documentation/networking/pktgen.rst | 12 ++++++++
>  net/core/pktgen.c                   | 48 ++++++++++++++++++++++++-----
>  2 files changed, 52 insertions(+), 8 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..c4e0814df325 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;
> @@ -3489,7 +3504,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 (pkt_dev->flags & F_SHARED)
> +			refcount_add(burst, &skb->users);
>  		local_bh_disable();
>  		do {
>  			ret = netif_receive_skb(skb);
> @@ -3497,6 +3513,10 @@ static void pktgen_xmit(struct pktgen_dev *pkt_dev)
>  				pkt_dev->errors++;
>  			pkt_dev->sofar++;
>  			pkt_dev->seq_num++;
> +			if (unlikely(!(pkt_dev->flags & F_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 +3535,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 (pkt_dev->flags & F_SHARED)
> +			refcount_inc(&pkt_dev->skb->users);
>  
>  		ret = dev_queue_xmit(pkt_dev->skb);
> +
> +		if (!(pkt_dev->flags & F_SHARED) && dev_xmit_complete(ret))
> +			pkt_dev->skb = NULL;
> +
>  		switch (ret) {
>  		case NET_XMIT_SUCCESS:
>  			pkt_dev->sofar++;
> @@ -3555,11 +3580,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 (pkt_dev->flags & F_SHARED)
> +		refcount_add(burst, &pkt_dev->skb->users);
>  
>  xmit_more:
>  	ret = netdev_start_xmit(pkt_dev->skb, odev, txq, --burst > 0);
>  
> +	if (!(pkt_dev->flags & F_SHARED) && dev_xmit_complete(ret))
> +		pkt_dev->skb = NULL;
> +
>  	switch (ret) {
>  	case NETDEV_TX_OK:
>  		pkt_dev->last_ok = 1;
> @@ -3581,7 +3610,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 (pkt_dev->flags & F_SHARED)
> +			refcount_dec(&pkt_dev->skb->users);
>  		pkt_dev->last_ok = 0;
>  	}
>  	if (unlikely(burst))
> @@ -3594,7 +3624,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 +3808,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.40.1
>