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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Sep 18, 2023 at 10:22 PM Benjamin Poirier <bpoirier@xxxxxxxxxx> 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@xxxxxxxxx>
> > ---
>
> 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@xxxxxxxxxx>
>
> >  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
> >




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux