From: Benjamin Poirier <bpoirier@nvidia.com>
To: Liang Chen <liangchen.linux@gmail.com>
Cc: davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
pabeni@redhat.com, netdev@vger.kernel.org,
Jonathan Corbet <corbet@lwn.net>,
"Jason A. Donenfeld" <Jason@zx2c4.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Kees Cook <keescook@chromium.org>,
"Darrick J. Wong" <djwong@kernel.org>,
Yunsheng Lin <linyunsheng@huawei.com>,
linux-doc@vger.kernel.org
Subject: Re: [PATCH net-next v4 2/2] pktgen: Introducing 'SHARED' flag for testing with non-shared skb
Date: Mon, 18 Sep 2023 10:22:00 -0400 [thread overview]
Message-ID: <ZQhdCHiqy2R6N3n0@d3> (raw)
In-Reply-To: <20230916132932.361875-2-liangchen.linux@gmail.com>
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
>
next prev parent reply other threads:[~2023-09-18 15:16 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZQhdCHiqy2R6N3n0@d3 \
--to=bpoirier@nvidia.com \
--cc=Jason@zx2c4.com \
--cc=corbet@lwn.net \
--cc=davem@davemloft.net \
--cc=djwong@kernel.org \
--cc=edumazet@google.com \
--cc=gregkh@linuxfoundation.org \
--cc=keescook@chromium.org \
--cc=kuba@kernel.org \
--cc=liangchen.linux@gmail.com \
--cc=linux-doc@vger.kernel.org \
--cc=linyunsheng@huawei.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.