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

[Ferruh Yigit <ferruh.yigit@intel.com>]

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;
>  }
>