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

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

Thanks,

I hope I understood correctly the above comments.

I'm thinking of adding DEV_RX_OFFLOAD_VLAN_STRIP to
dev_info->rx_offload_capa

eth_dev_info()
+dev_info->rx_offload_capa = DEV_RX_OFFLOAD_VLAN_STRIP;

then populating pmd_internals->vlan_strip with the vlan stripping
option that the application requests

eth_dev_configure()
+internals->vlan_strip = !!(rxmode->offloads & DEV_RX_OFFLOAD_VLAN_STRIP);

From the internals structure, we could populate a newly-added field in the
pkt_rx_queue structure 'vlan_strip'

eth_rx_queue_setup()
+pkt_q->vlan_strip = internals->vlan_strip;

And attempt to re-insert the vlan only if required in eth_af_packet_rx

eth_af_packet_rx()
+if (!pkt_q->vlan_strip && rte_vlan_insert(&mbuf))

I've attempted a simple benchmark to understand if the change could cause a
sizable performance hit.

Setup:
Tx: vmxnet3 PMD
Rx: af_packet (running on top of a vmxnet3 interface)
Packet size :68 (packet contains a vlan tag)

Rates:
Tx - 1.419 Mpps
Rx (without vlan insertion) -   1227636 pps
Rx (with vlan insertion) - 1220081 pps

I don't seem to have a large degradation in terms of packet rate at first
glance, but maybe the experiment could be repeated on different setups as
I'm using a virtual environment.

Would it be reasonable if I send v2 of the patch for review, with the above
changes ?


On Thu, 2 Sept 2021 at 13:49, Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 9/1/2021 10:34 PM, Stephen Hemminger wrote:
> > On Wed, 1 Sep 2021 22:07:22 +0300
> > Tudor Cornea <tudor.cornea@gmail.com> wrote:
> >
> >> Indeed, the vlan insertion could be a costly operation. We should
> probably
> >> do it only if the user specifically asks to have the vlan tag in the
> packet.
> >> Otherwise, af_packet PMD users might pay a price in terms of performance
> >> for something they didn't ask for.
> >>
> >> I was thinking of avoiding having to change the application in order to
> >> re-insert the vlan tag.
> >> Doing this operation inside the PMD driver seemed like a good fit.
> >>
> >> Looking at the netvsc driver (drivers/net/netvsc), the vlan insertion is
> >> guarded by a check to hv->vlan_strip
> >>
> >>   if (!hv->vlan_strip && rte_vlan_insert(&m)) {
> >>
> >> hv->vlan_strip seems to be initialized in hn_dev_configure() in the
> >> following way
> >>
> >>  hv->vlan_strip = !!(rxmode->offloads & DEV_RX_OFFLOAD_VLAN_STRIP);
> >>
> >> while 'hv' seems to be stored in rte_eth_dev->data->dev_private
> >>
> >> I am thinking of doing something similar for the af_packet PMD.
> >> The 'pmd_internals' structure could potentially hold a field, say
> >> vlan_strip', which could be initialized if the application enables the
> >> DEV_RX_OFFLOAD_VLAN_STRIP in rxmode->offloads
> >>
> >> This way, I'm thinking that the application could potentially control
> the
> >> effect of vlan stripping for the af_packet PMD, in an uniform way,
> similar
> >> to other PMDs.
> >> Would this be considered an acceptable solution ?
> >>
> >>
> >>
> >>
> >> On Tue, 31 Aug 2021 at 18:31, Ferruh Yigit <ferruh.yigit@intel.com>
> wrote:
> >>
> >>> On 8/20/2021 1:46 PM, 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 bcc6d47903 [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.
> >>>>
> >>>> We would like to have the the vlan tag inside the mbuf.
> >>>> Let's take a shot at it by trying to reinsert the stripped vlan tag.
> >>>>
> >>>
> >>> PMD already sets 'mbuf->vlan_tci' and 'PKT_RX_VLAN |
> PKT_RX_VLAN_STRIPPED'
> >>> flags, so application can be aware of the vlan tag and can consume it.
> >>>
> >>> Inserting the vlan tag back to packet is costly, what is the
> motivation to
> >>> do so?
> >>>
> >>>> As a side note, something similar was done for the netvsc pmd.
> >>>>
> >>>> [1]
> >>>
> https://github.com/torvalds/linux/commit/bcc6d47903612c3861201cc3a866fb604f26b8b2
>
> >>>>
> >>>> Signed-off-by: Tudor Cornea <tudor.cornea@gmail.com>
> >
> > The netvsc PMD has to handle some subtle cases where VLAN stripping
> > is done by the VF but the slow path over vmbus does not.
> > Since most traffic goes over the VF path, it makes sense for the
> > netvsc PMD to advertise and handle VLAN stripping even if it has
> > to do it in software.
> >
> > Ferruh is right the mbuf generated by current AF_PACKET PMD is
> > valid with VLAN stripped correctly. I think you are also correct
> > that the stripping needs to be controllable by the application.
> > And yes the kernel always strips the VLAN; there is no option
> > to tell socket(AF_PACKET) not to do that.
> >
>
> When application doesn't set VLAN_STRIP offload, expectation is VLAN tag
> to be
> in the packet and no additional work is done.
>
> But that is not the case for af_packet.
> If your change is applied, not requesting any offload, default confing, may
> cause unintended work for af_packet, since it will insert the already
> stripped
> vlan tag back.
>
> And we don't have a way to say any specific offload can't be disabled by
> the
> PMD/device, although we hit this case a few times previously.
> Proper fix can be adding this support to offloads, but it is more invasive
> change. + Andrew, Thomas, Jerin for this discussion.
>
>
> For short term at least 'DEV_RX_OFFLOAD_VLAN_STRIP' offload should be
> added to
> the af_packet capability.
> It is also possible to set this offload in the config by PMD itself even
> application doesn't request it, to be correct in the config. Not sure how
> much
> it helps to applications (there is a new API proposed this release to get
> config
> to application, perhaps after configuration step app can request the
> config and
> recognize that VLAN_STRIP offload is set by PMD, but this is some
> overhead).
>