* [PATCH net-next v4 1/2] pktgen: Automate flag enumeration for unknown flag handling @ 2023-09-16 13:29 Liang Chen 2023-09-16 13:29 ` [PATCH net-next v4 2/2] pktgen: Introducing 'SHARED' flag for testing with non-shared skb Liang Chen 0 siblings, 1 reply; 8+ messages in thread From: Liang Chen @ 2023-09-16 13:29 UTC (permalink / raw) To: davem, edumazet, kuba, pabeni, benjamin.poirier Cc: netdev, liangchen.linux, Benjamin Poirier 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.40.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH net-next v4 2/2] pktgen: Introducing 'SHARED' flag for testing with non-shared skb 2023-09-16 13:29 [PATCH net-next v4 1/2] pktgen: Automate flag enumeration for unknown flag handling Liang Chen @ 2023-09-16 13:29 ` Liang Chen 2023-09-18 14:22 ` Benjamin Poirier 2023-09-18 14:28 ` Eric Dumazet 0 siblings, 2 replies; 8+ messages in thread From: Liang Chen @ 2023-09-16 13:29 UTC (permalink / raw) To: davem, edumazet, kuba, pabeni, benjamin.poirier; +Cc: netdev, liangchen.linux 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> --- 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 related [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v4 2/2] pktgen: Introducing 'SHARED' flag for testing with non-shared skb 2023-09-16 13:29 ` [PATCH net-next v4 2/2] pktgen: Introducing 'SHARED' flag for testing with non-shared skb Liang Chen @ 2023-09-18 14:22 ` Benjamin Poirier 2023-09-20 4:00 ` Liang Chen 2023-09-18 14:28 ` Eric Dumazet 1 sibling, 1 reply; 8+ 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] 8+ 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 ` Benjamin Poirier @ 2023-09-20 4:00 ` Liang Chen 0 siblings, 0 replies; 8+ 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] 8+ messages in thread
* Re: [PATCH net-next v4 2/2] pktgen: Introducing 'SHARED' flag for testing with non-shared skb 2023-09-16 13:29 ` [PATCH net-next v4 2/2] pktgen: Introducing 'SHARED' flag for testing with non-shared skb Liang Chen 2023-09-18 14:22 ` Benjamin Poirier @ 2023-09-18 14:28 ` Eric Dumazet 2023-09-19 8:09 ` Paolo Abeni 1 sibling, 1 reply; 8+ messages in thread From: Eric Dumazet @ 2023-09-18 14:28 UTC (permalink / raw) To: Liang Chen; +Cc: davem, kuba, pabeni, benjamin.poirier, netdev On Sat, Sep 16, 2023 at 3:30 PM Liang Chen <liangchen.linux@gmail.com> 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> > --- > 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++; Since pkt_dev->flags can change under us, I would rather read pkt_dev->flags once in pktgen_xmit() to avoid surprises... syzbot probably never figured out how to run pktgen, it is a matter of time... ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v4 2/2] pktgen: Introducing 'SHARED' flag for testing with non-shared skb 2023-09-18 14:28 ` Eric Dumazet @ 2023-09-19 8:09 ` Paolo Abeni 2023-09-20 4:09 ` Liang Chen 0 siblings, 1 reply; 8+ messages in thread From: Paolo Abeni @ 2023-09-19 8:09 UTC (permalink / raw) To: Eric Dumazet, Liang Chen; +Cc: davem, kuba, benjamin.poirier, netdev On Mon, 2023-09-18 at 16:28 +0200, Eric Dumazet wrote: > On Sat, Sep 16, 2023 at 3:30 PM Liang Chen <liangchen.linux@gmail.com> 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> > > --- > > 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++; > > Since pkt_dev->flags can change under us, I would rather read pkt_dev->flags > once in pktgen_xmit() to avoid surprises... Additionally I *think* we can't assume pkt_dev->burst and pkt_dev- >flags have consistent values in pktgen_xmit(). The user-space (syzkaller) could flip burst and flag in between the read access in pktgen_xmit(). There is a later: if (unlikely(burst)) WARN_ON(refcount_sub_and_test(burst, &pkt_dev->skb->users)); that will need explicit check for 'pkt_dev->skb' not being NULL. Cheers, Paolo ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v4 2/2] pktgen: Introducing 'SHARED' flag for testing with non-shared skb 2023-09-19 8:09 ` Paolo Abeni @ 2023-09-20 4:09 ` Liang Chen 2023-09-20 6:01 ` Paolo Abeni 0 siblings, 1 reply; 8+ messages in thread From: Liang Chen @ 2023-09-20 4:09 UTC (permalink / raw) To: Paolo Abeni; +Cc: Eric Dumazet, davem, kuba, benjamin.poirier, netdev On Tue, Sep 19, 2023 at 4:09 PM Paolo Abeni <pabeni@redhat.com> wrote: > > On Mon, 2023-09-18 at 16:28 +0200, Eric Dumazet wrote: > > On Sat, Sep 16, 2023 at 3:30 PM Liang Chen <liangchen.linux@gmail.com> 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> > > > --- > > > 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++; > > > > Since pkt_dev->flags can change under us, I would rather read pkt_dev->flags > > once in pktgen_xmit() to avoid surprises... > > Additionally I *think* we can't assume pkt_dev->burst and pkt_dev- > >flags have consistent values in pktgen_xmit(). The user-space > (syzkaller) could flip burst and flag in between the read access in > pktgen_xmit(). > Thanks for pointing out the issue! We are trying to fix it in the following way, 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) { + burst = READ_ONCE(pkt_dev->burst); + clone_skb = READ_ONCE(pkt_dev->clone_skb); + } + So that pktgen_xmit will have consistent 'burst', 'clone_skb', and 'skb_shared' values. 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 in the next run of pktgen_xmit. This doesn't prevent the loads of 'READ_ONCE(pkt_dev->burst); and READ_ONCE(pkt_dev->clone_skb);' to be speculatively run at the an early time, but only if 'skb_shared' is true these loads will be committed. And burst and clone_skb can change freely with a true value of skb_shared. > There is a later: > > if (unlikely(burst)) > WARN_ON(refcount_sub_and_test(burst, &pkt_dev->skb->users)); This seems no longer an issue If 'burst' and 'skb_shared' have consistent values throughout the function, 'pkt_dev->skb' will not be NULL here. Thanks, Liang > > that will need explicit check for 'pkt_dev->skb' not being NULL. > > Cheers, > > Paolo > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next v4 2/2] pktgen: Introducing 'SHARED' flag for testing with non-shared skb 2023-09-20 4:09 ` Liang Chen @ 2023-09-20 6:01 ` Paolo Abeni 0 siblings, 0 replies; 8+ messages in thread From: Paolo Abeni @ 2023-09-20 6:01 UTC (permalink / raw) To: Liang Chen; +Cc: Eric Dumazet, davem, kuba, benjamin.poirier, netdev On Wed, 2023-09-20 at 12:09 +0800, Liang Chen wrote: > On Tue, Sep 19, 2023 at 4:09 PM Paolo Abeni <pabeni@redhat.com> wrote: > > > > On Mon, 2023-09-18 at 16:28 +0200, Eric Dumazet wrote: > > > On Sat, Sep 16, 2023 at 3:30 PM Liang Chen <liangchen.linux@gmail.com> 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> > > > > --- > > > > 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++; > > > > > > Since pkt_dev->flags can change under us, I would rather read pkt_dev->flags > > > once in pktgen_xmit() to avoid surprises... > > > > Additionally I *think* we can't assume pkt_dev->burst and pkt_dev- > > > flags have consistent values in pktgen_xmit(). The user-space > > (syzkaller) could flip burst and flag in between the read access in > > pktgen_xmit(). > > > > Thanks for pointing out the issue! We are trying to fix it in the following way, > > 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) { > + burst = READ_ONCE(pkt_dev->burst); > + clone_skb = READ_ONCE(pkt_dev->clone_skb); > + } > + > > So that pktgen_xmit will have consistent 'burst', 'clone_skb', and > 'skb_shared' values. I agree it makes sense and address the potential issues. Thanks, Paolo ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-09-20 6:01 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-09-16 13:29 [PATCH net-next v4 1/2] pktgen: Automate flag enumeration for unknown flag handling Liang Chen 2023-09-16 13:29 ` [PATCH net-next v4 2/2] pktgen: Introducing 'SHARED' flag for testing with non-shared skb Liang Chen 2023-09-18 14:22 ` Benjamin Poirier 2023-09-20 4:00 ` Liang Chen 2023-09-18 14:28 ` Eric Dumazet 2023-09-19 8:09 ` Paolo Abeni 2023-09-20 4:09 ` Liang Chen 2023-09-20 6:01 ` Paolo Abeni
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).