Re: [dpdk-dev] [PATCH v2] net/af_packet: reinsert the stripped vlan tag

[Tudor Cornea <tudor.cornea@gmail.com>]

Thanks, Ferruh

I will perform the suggested recommendations in version 3 of the patch.

On Mon, 20 Sept 2021 at 18:41, Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 9/8/2021 9:59 AM, Tudor Cornea wrote:
> > The af_packet pmd driver binds to a raw socket and allows
> > sending and receiving of packets through the kernel.
> >
> > Since commit [1], the kernel strips the vlan tags early in
> > __netif_receive_skb_core(), so we receive untagged packets while
> > running with the af_packet pmd.
> >
> > Luckily for us, the skb vlan-related fields are still populated from the
> > stripped vlan tags, so we end up having all the information
> > that we need in the mbuf.
> >
> > Having the PMD driver support DEV_RX_OFFLOAD_VLAN_STRIP allows the
> > application to control the desired vlan stripping behavior.
> >
> > [1]
> https://github.com/torvalds/linux/commit/bcc6d47903612c3861201cc3a866fb604f26b8b2
> >
> > Signed-off-by: Tudor Cornea <tudor.cornea@gmail.com>
> >
>
> Hi Tudor,
>
> The concern was unexpected performance degradation (user not setting any
> offload
> will have performance drop). But since your measurements show no
> significant
> drop, I think it is fair to make driver behave same as other drivers.
> (Until we have a way to describe offloads that can't be disabled by PMDs.)
>
> Can you do a few minor updates:
> - Put your performance measurements into to the commit log to record them
> - Update the af_packet documentation (doc/guides/nics/af_packet.rst) to
> document
> PMD behavior with packets with VLAN tag
> - Update release note (doc/guides/rel_notes/release_21_11.rst) with a
> one/two
> sentences to document the change, to notify possible users of the
> af_packet with
> the change.
>
> Thanks,
> ferruh
>
> > ---
> > v2:
> > * Add DEV_RX_OFFLOAD_VLAN_STRIP to rxmode->offloads
> > ---
> >  drivers/net/af_packet/rte_eth_af_packet.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/net/af_packet/rte_eth_af_packet.c
> b/drivers/net/af_packet/rte_eth_af_packet.c
> > index b73b211..5ed9dd6 100644
> > --- a/drivers/net/af_packet/rte_eth_af_packet.c
> > +++ b/drivers/net/af_packet/rte_eth_af_packet.c
> > @@ -48,6 +48,7 @@ struct pkt_rx_queue {
> >
> >       struct rte_mempool *mb_pool;
> >       uint16_t in_port;
> > +     uint8_t vlan_strip;
> >
> >       volatile unsigned long rx_pkts;
> >       volatile unsigned long rx_bytes;
> > @@ -78,6 +79,7 @@ struct pmd_internals {
> >
> >       struct pkt_rx_queue *rx_queue;
> >       struct pkt_tx_queue *tx_queue;
> > +     uint8_t vlan_strip;
> >  };
> >
> >  static const char *valid_arguments[] = {
> > @@ -148,6 +150,9 @@ eth_af_packet_rx(void *queue, struct rte_mbuf
> **bufs, uint16_t nb_pkts)
> >               if (ppd->tp_status & TP_STATUS_VLAN_VALID) {
> >                       mbuf->vlan_tci = ppd->tp_vlan_tci;
> >                       mbuf->ol_flags |= (PKT_RX_VLAN |
> PKT_RX_VLAN_STRIPPED);
> > +
> > +                     if (!pkt_q->vlan_strip && rte_vlan_insert(&mbuf))
> > +                             PMD_LOG(ERR, "Failed to reinsert VLAN
> tag");
> >               }
> >
> >               /* release incoming frame and advance ring buffer */
> > @@ -302,6 +307,11 @@ eth_dev_stop(struct rte_eth_dev *dev)
> >  static int
> >  eth_dev_configure(struct rte_eth_dev *dev __rte_unused)
> >  {
> > +     struct rte_eth_conf *dev_conf = &dev->data->dev_conf;
> > +     const struct rte_eth_rxmode *rxmode = &dev_conf->rxmode;
> > +     struct pmd_internals *internals = dev->data->dev_private;
> > +
> > +     internals->vlan_strip = !!(rxmode->offloads &
> DEV_RX_OFFLOAD_VLAN_STRIP);
> >       return 0;
> >  }
> >
> > @@ -318,6 +328,7 @@ eth_dev_info(struct rte_eth_dev *dev, struct
> rte_eth_dev_info *dev_info)
> >       dev_info->min_rx_bufsize = 0;
> >       dev_info->tx_offload_capa = DEV_TX_OFFLOAD_MULTI_SEGS |
> >               DEV_TX_OFFLOAD_VLAN_INSERT;
> > +     dev_info->rx_offload_capa = DEV_RX_OFFLOAD_VLAN_STRIP;
> >
> >       return 0;
> >  }
> > @@ -448,6 +459,7 @@ eth_rx_queue_setup(struct rte_eth_dev *dev,
> >
> >       dev->data->rx_queues[rx_queue_id] = pkt_q;
> >       pkt_q->in_port = dev->data->port_id;
> > +     pkt_q->vlan_strip = internals->vlan_strip;
> >
> >       return 0;
> >  }
> >
>
>