* Re: [PATCH net-next v4 2/2] pktgen: Introducing 'SHARED' flag for testing with non-shared skb [not found] ` <20230916132932.361875-2-liangchen.linux@gmail.com> @ 2023-09-18 14:22 ` Benjamin Poirier 2023-09-20 4:00 ` Liang Chen 0 siblings, 1 reply; 2+ messages in thread From: Benjamin Poirier @ 2023-09-18 14:22 UTC (permalink / raw) To: Liang Chen Cc: davem, edumazet, kuba, pabeni, netdev, Jonathan Corbet, Jason A. Donenfeld, Greg Kroah-Hartman, Kees Cook, Darrick J. Wong, Yunsheng Lin, linux-doc 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 <liangchen.linux@gmail.com> > --- In the future, please run scripts/get_maintainer.pl and cc the listed addresses. I'm adding them now. Reviewed-by: Benjamin Poirier <bpoirier@nvidia.com> > 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 > ^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH net-next v4 2/2] pktgen: Introducing 'SHARED' flag for testing with non-shared skb 2023-09-18 14:22 ` [PATCH net-next v4 2/2] pktgen: Introducing 'SHARED' flag for testing with non-shared skb Benjamin Poirier @ 2023-09-20 4:00 ` Liang Chen 0 siblings, 0 replies; 2+ messages in thread From: Liang Chen @ 2023-09-20 4:00 UTC (permalink / raw) To: Benjamin Poirier Cc: davem, edumazet, kuba, pabeni, netdev, Jonathan Corbet, Jason A. Donenfeld, Greg Kroah-Hartman, Kees Cook, Darrick J. Wong, Yunsheng Lin, linux-doc On Mon, Sep 18, 2023 at 10:22 PM Benjamin Poirier <bpoirier@nvidia.com> wrote: > > 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 <liangchen.linux@gmail.com> > > --- > > In the future, please run scripts/get_maintainer.pl and cc the listed > addresses. I'm adding them now. Sure. Thanks! > > Reviewed-by: Benjamin Poirier <bpoirier@nvidia.com> > > > 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 > > ^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-09-20 4:01 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20230916132932.361875-1-liangchen.linux@gmail.com>
[not found] ` <20230916132932.361875-2-liangchen.linux@gmail.com>
2023-09-18 14:22 ` [PATCH net-next v4 2/2] pktgen: Introducing 'SHARED' flag for testing with non-shared skb Benjamin Poirier
2023-09-20 4:00 ` Liang Chen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox