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

[rkudurumalla <rkudurumalla@caviumnetworks.com>]

On 07/04/2018 11:06 PM, Ferruh Yigit wrote:
> External Email
> 
> 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?
> 
> It is still partial because Tx VLAN offload(insertion of vlan header >
for tx packets) is yet to be Implemented
> 
> <...>
> 
>> @@ -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?

> if application disables vlan_strip under this condition, hardware
doesn't strip vlan header from the packets and hence
cqe_rx_w0.vlan_stripped field is zero and pkt->ol_flags is update with zero

> 
> <...>
> 
>> +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 alread>
> After enabling vlan strip , if we receive plain packets pkt->ol_flags
is updated with zero value.