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