Re: [PATCH] net/thunderx: add support for Rx VLAN offload

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

On 7/1/2018 5:46 PM, Pavan Nikhilesh wrote:
> From: "Kudurumalla, Rakesh" <rakesh.kudurumalla@cavium.com>
> 
> This feature is used to offload stripping of vlan header from recevied
> packets and update vlan_tci field in mbuf when
> DEV_RX_OFFLOAD_VLAN_STRIP & ETH_VLAN_STRIP_MASK flag is set.
> 
> Signed-off-by: Rakesh Kudurumalla <rkudurumalla@caviumnetworks.com>
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
> ---
>  drivers/net/thunderx/base/nicvf_hw.c |  1 +
>  drivers/net/thunderx/nicvf_ethdev.c  | 59 +++++++++++++++++------
>  drivers/net/thunderx/nicvf_rxtx.c    | 70 ++++++++++++++++++++++++----
>  drivers/net/thunderx/nicvf_rxtx.h    | 15 ++++--
>  drivers/net/thunderx/nicvf_struct.h  |  1 +

In thunderx.ini, "VLAN offload" already marked as P(Partially) is it still
partially? Why?


<...>

> @@ -1590,9 +1595,9 @@ nicvf_vf_start(struct rte_eth_dev *dev, struct nicvf *nic, uint32_t rbdrsz)
>  		     nic->rbdr->tail, nb_rbdr_desc, nic->vf_id);
>  
>  	/* Configure VLAN Strip */
> -	vlan_strip = !!(dev->data->dev_conf.rxmode.offloads &
> -			DEV_RX_OFFLOAD_VLAN_STRIP);
> -	nicvf_vlan_hw_strip(nic, vlan_strip);
> +	mask = ETH_VLAN_STRIP_MASK | ETH_VLAN_FILTER_MASK |
> +		ETH_VLAN_EXTEND_MASK;

You don't need anything more than ETH_VLAN_STRIP_MASK but agreed no issue add
more if you prefer.

> +	ret = nicvf_vlan_offload_config(dev, mask);
>  
>  	/* Based on the packet type(IPv4 or IPv6), the nicvf HW aligns L3 data
>  	 * to the 64bit memory address.
> @@ -1983,6 +1988,7 @@ static const struct eth_dev_ops nicvf_eth_dev_ops = {
>  	.dev_infos_get            = nicvf_dev_info_get,
>  	.dev_supported_ptypes_get = nicvf_dev_supported_ptypes_get,
>  	.mtu_set                  = nicvf_dev_set_mtu,
> +	.vlan_offload_set         = nicvf_vlan_offload_set,

Not related to this patch but I believe this name 'vlan_offload_set' is
confusing, it enable/disable VLAN related config:
- vlan strip offload
- vlan filtering package (drop/accept specific vlans)
- double vlan feature (not offload if I am not missing anything)
We can think about a more proper name later...

Also rte_eth_dev_set_vlan_offload() API may have a defect, it seems not taking
capability flags into account, cc'ed Shahaf and Andrew for information.

And I have a question about DEV_TX_OFFLOAD_VLAN_INSERT, perhaps goes to Olivier,
if DEV_TX_OFFLOAD_VLAN_INSERT enabled what is the correct way to provide
vlan_tci to insert?
And do we need something like PKT_RX_VLAN_INSERT and use mbuf->vlan_tci value to
have the ability to insert VLAN to some packets?

<...>

> +static int
> +nicvf_vlan_offload_set(struct rte_eth_dev *dev, int mask)
> +{
> +	nicvf_vlan_offload_config(dev, mask);

Don't you need to change rx_pkt_burst function according request here.

Like if driver was using nicvf_recv_pkts_vlan_strip() and application disabled
vlan_strip, what will happen?

<...>

> +uint16_t __hot
> +nicvf_recv_pkts_vlan_strip(void *rx_queue, struct rte_mbuf **rx_pkts,
> +		uint16_t nb_pkts)
> +{
> +	return nicvf_recv_pkts(rx_queue, rx_pkts, nb_pkts,
> +			NICVF_RX_OFFLOAD_NONE | NICVF_RX_OFFLOAD_VLAN_STRIP);

Why do you OR NICVF_RX_OFFLOAD_NONE, this cause zeroing the pkt->ol_flags which
will be overriten because of NICVF_RX_OFFLOAD_VLAN_STRIP already?