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.kuduruma...@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 <rkuduruma...@caviumnetworks.com>
>> Signed-off-by: Pavan Nikhilesh <pbhagavat...@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.
