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

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 13.07.2018 17:16, rkudurumalla wrote:
> 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.

Yes, the function could check if corresponding offloads are supported.

Right now we have unified interface to control offloads on device configure
and queues setup. This API to change VLAN offloads looks really legacy now.
If we really need API to control offloads at run time (I'm not sure), it 
should
be generic API for all offloads and corresponding information in dev_info
which specifies offloads are controllable at run time.

>> 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?

As I understand ol_flags should have PKT_TX_VLAN. Right now the description
does not mentione it, however the description for double-tagged insertion
PKT_TX_QINQ does.