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 [off-list ref] wrote:

On 9/1/2021 10:34 PM, Stephen Hemminger wrote:

quoted

On Wed, 1 Sep 2021 22:07:22 +0300
Tudor Cornea [off-list ref] wrote:

quoted

Indeed, the vlan insertion could be a costly operation. We should

probably

quoted

quoted

do it only if the user specifically asks to have the vlan tag in the

packet.

quoted

quoted

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

quoted

quoted

effect of vlan stripping for the af_packet PMD, in an uniform way,

similar

quoted

quoted

to other PMDs.
Would this be considered an acceptable solution ?




On Tue, 31 Aug 2021 at 18:31, Ferruh Yigit [off-list ref]

wrote:

quoted

quoted

quoted

On 8/20/2021 1:46 PM, Tudor Cornea wrote:

quoted

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

quoted

quoted

quoted

quoted

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'

quoted

quoted

quoted

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

quoted

quoted

quoted

do so?

quoted

As a side note, something similar was done for the netvsc pmd.

[1]

https://github.com/torvalds/linux/commit/bcc6d47903612c3861201cc3a866fb604f26b8b2

quoted

quoted

quoted

quoted

Signed-off-by: Tudor Cornea <redacted>

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).