diff mbox series

net: fix Tx VLAN flag for offload emulation

Message ID 20190325150541.12087-1-3chas3@gmail.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series net: fix Tx VLAN flag for offload emulation |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/intel-Performance-Testing success Performance Testing PASS
ci/mellanox-Performance-Testing success Performance Testing PASS

Commit Message

Chas Williams March 25, 2019, 3:05 p.m. UTC 
  From: Bill Hong <bhong@brocade.com>

A PMD might use rte_vlan_insert to implement Tx VLAN offload.  Typically
the PMD will insert the VLAN header in the transmit path and then attempt
to send the packets. If this fails, the packets are returned to
the application which may attempt to send these packets again. If the
PKT_TX_VLAN flag is not cleared, the transmit path may attempt to insert
the VLAN header again.

Fixes: 47aa48b969f8 ("net: fix stripped VLAN flag for offload emulation");
Cc: stable@dpdk.org

Signed-off-by: Bill Hong <bhong@brocade.com>
Signed-off-by: Chas Williams <chas3@att.com>
---
 lib/librte_net/rte_ether.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ferruh Yigit March 26, 2019, 4:50 p.m. UTC | #1 
On 3/25/2019 3:05 PM, Chas Williams wrote:
> From: Bill Hong <bhong@brocade.com>
> 
> A PMD might use rte_vlan_insert to implement Tx VLAN offload.  Typically
> the PMD will insert the VLAN header in the transmit path and then attempt
> to send the packets. If this fails, the packets are returned to
> the application which may attempt to send these packets again. If the
> PKT_TX_VLAN flag is not cleared, the transmit path may attempt to insert
> the VLAN header again.
> 
> Fixes: 47aa48b969f8 ("net: fix stripped VLAN flag for offload emulation");
> Cc: stable@dpdk.org
> 
> Signed-off-by: Bill Hong <bhong@brocade.com>
> Signed-off-by: Chas Williams <chas3@att.com>
> ---
>  lib/librte_net/rte_ether.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
> index c2c5e249f..e0d831113 100644
> --- a/lib/librte_net/rte_ether.h
> +++ b/lib/librte_net/rte_ether.h
> @@ -408,7 +408,7 @@ static inline int rte_vlan_insert(struct rte_mbuf **m)
>  	vh = (struct vlan_hdr *) (nh + 1);
>  	vh->vlan_tci = rte_cpu_to_be_16((*m)->vlan_tci);
>  
> -	(*m)->ol_flags &= ~PKT_RX_VLAN_STRIPPED;
> +	(*m)->ol_flags &= ~(PKT_RX_VLAN_STRIPPED | PKT_TX_VLAN);

Hi Chas,

Looks reasonable, AFAIK, PKT_TX_VLAN flag means 'mbuf->vlan_tci' has valid value
and a request from application to driver to insert VLAN information.
After successful insertion flag can be removed.

But in mbuf header, flags documented as:
PKT_TX_VLAN: TX packet is a 802.1q VLAN packet.
PKT_TX_QINQ: TX packet with double VLAN inserted.


Hi Oliver,

Is above comments correct, or I am missing something J


Also here cleaning 'PKT_RX_VLAN_STRIPPED' looks like to say 'vlan' information
is not stripped but in the packet data, but 'RX' in this context can be
confusing, should we have a more generic 'PKT_VLAN_STRIPPED' ?
Chas Williams March 26, 2019, 6:01 p.m. UTC | #2 
On 3/26/19 12:50 PM, Ferruh Yigit wrote:
> On 3/25/2019 3:05 PM, Chas Williams wrote:
>> From: Bill Hong <bhong@brocade.com>
>>
>> A PMD might use rte_vlan_insert to implement Tx VLAN offload.  Typically
>> the PMD will insert the VLAN header in the transmit path and then attempt
>> to send the packets. If this fails, the packets are returned to
>> the application which may attempt to send these packets again. If the
>> PKT_TX_VLAN flag is not cleared, the transmit path may attempt to insert
>> the VLAN header again.
>>
>> Fixes: 47aa48b969f8 ("net: fix stripped VLAN flag for offload emulation");
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Bill Hong <bhong@brocade.com>
>> Signed-off-by: Chas Williams <chas3@att.com>
>> ---
>>   lib/librte_net/rte_ether.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
>> index c2c5e249f..e0d831113 100644
>> --- a/lib/librte_net/rte_ether.h
>> +++ b/lib/librte_net/rte_ether.h
>> @@ -408,7 +408,7 @@ static inline int rte_vlan_insert(struct rte_mbuf **m)
>>   	vh = (struct vlan_hdr *) (nh + 1);
>>   	vh->vlan_tci = rte_cpu_to_be_16((*m)->vlan_tci);
>>   
>> -	(*m)->ol_flags &= ~PKT_RX_VLAN_STRIPPED;
>> +	(*m)->ol_flags &= ~(PKT_RX_VLAN_STRIPPED | PKT_TX_VLAN);
> 
> Hi Chas,
> 
> Looks reasonable, AFAIK, PKT_TX_VLAN flag means 'mbuf->vlan_tci' has valid value
> and a request from application to driver to insert VLAN information.
> After successful insertion flag can be removed.
> 
> But in mbuf header, flags documented as:
> PKT_TX_VLAN: TX packet is a 802.1q VLAN packet.
> PKT_TX_QINQ: TX packet with double VLAN inserted.

Note the usage slightly below:

	/**
	 * Bitmask of all supported packet Tx offload features flags,
	 * which can be set for packet.
	 */
	#define PKT_TX_OFFLOAD_MASK (    \
			PKT_TX_OUTER_IPV6 |      \
			PKT_TX_OUTER_IPV4 |      \
			PKT_TX_OUTER_IP_CKSUM |  \
			PKT_TX_VLAN_PKT |        \

So this implies that PKT_TX_VLAN_PKT (which is PKT_TX_VLAN's old name)
is an offload feature, not that the actual packet has an 802.1q header
(offloaded or otherwise).

The same seems to apply for PKT_TX_QINQ.

> Hi Oliver,
> 
> Is above comments correct, or I am missing something J
> 
> 
> Also here cleaning 'PKT_RX_VLAN_STRIPPED' looks like to say 'vlan' information
> is not stripped but in the packet data, but 'RX' in this context can be
> confusing, should we have a more generic 'PKT_VLAN_STRIPPED' ?
>
Ferruh Yigit April 1, 2019, 8:17 p.m. UTC | #3 
On 3/26/2019 6:01 PM, Chas Williams wrote:
> 
> 
> On 3/26/19 12:50 PM, Ferruh Yigit wrote:
>> On 3/25/2019 3:05 PM, Chas Williams wrote:
>>> From: Bill Hong <bhong@brocade.com>
>>>
>>> A PMD might use rte_vlan_insert to implement Tx VLAN offload.  Typically
>>> the PMD will insert the VLAN header in the transmit path and then attempt
>>> to send the packets. If this fails, the packets are returned to
>>> the application which may attempt to send these packets again. If the
>>> PKT_TX_VLAN flag is not cleared, the transmit path may attempt to insert
>>> the VLAN header again.
>>>
>>> Fixes: 47aa48b969f8 ("net: fix stripped VLAN flag for offload emulation");
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Bill Hong <bhong@brocade.com>
>>> Signed-off-by: Chas Williams <chas3@att.com>
>>> ---
>>>   lib/librte_net/rte_ether.h | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
>>> index c2c5e249f..e0d831113 100644
>>> --- a/lib/librte_net/rte_ether.h
>>> +++ b/lib/librte_net/rte_ether.h
>>> @@ -408,7 +408,7 @@ static inline int rte_vlan_insert(struct rte_mbuf **m)
>>>   	vh = (struct vlan_hdr *) (nh + 1);
>>>   	vh->vlan_tci = rte_cpu_to_be_16((*m)->vlan_tci);
>>>   
>>> -	(*m)->ol_flags &= ~PKT_RX_VLAN_STRIPPED;
>>> +	(*m)->ol_flags &= ~(PKT_RX_VLAN_STRIPPED | PKT_TX_VLAN);
>>
>> Hi Chas,
>>
>> Looks reasonable, AFAIK, PKT_TX_VLAN flag means 'mbuf->vlan_tci' has valid value
>> and a request from application to driver to insert VLAN information.
>> After successful insertion flag can be removed.
>>
>> But in mbuf header, flags documented as:
>> PKT_TX_VLAN: TX packet is a 802.1q VLAN packet.
>> PKT_TX_QINQ: TX packet with double VLAN inserted.

Hi Olivier,

Any comment on them, should they be fixed?

> 
> Note the usage slightly below:
> 
> 	/**
> 	 * Bitmask of all supported packet Tx offload features flags,
> 	 * which can be set for packet.
> 	 */
> 	#define PKT_TX_OFFLOAD_MASK (    \
> 			PKT_TX_OUTER_IPV6 |      \
> 			PKT_TX_OUTER_IPV4 |      \
> 			PKT_TX_OUTER_IP_CKSUM |  \
> 			PKT_TX_VLAN_PKT |        \
> 
> So this implies that PKT_TX_VLAN_PKT (which is PKT_TX_VLAN's old name)
> is an offload feature, not that the actual packet has an 802.1q header
> (offloaded or otherwise).
> 
> The same seems to apply for PKT_TX_QINQ.

Agree, I just would like to confirm, so lgtm:
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

> 
>> Hi Oliver,
>>
>> Is above comments correct, or I am missing something J
>>
>>
>> Also here cleaning 'PKT_RX_VLAN_STRIPPED' looks like to say 'vlan' information
>> is not stripped but in the packet data, but 'RX' in this context can be
>> confusing, should we have a more generic 'PKT_VLAN_STRIPPED' ?
>>
Ferruh Yigit April 4, 2019, 5:07 p.m. UTC | #4 
On 3/25/2019 3:05 PM, Chas Williams wrote:
> From: Bill Hong <bhong@brocade.com>
> 
> A PMD might use rte_vlan_insert to implement Tx VLAN offload.  Typically
> the PMD will insert the VLAN header in the transmit path and then attempt
> to send the packets. If this fails, the packets are returned to
> the application which may attempt to send these packets again. If the
> PKT_TX_VLAN flag is not cleared, the transmit path may attempt to insert
> the VLAN header again.
> 
> Fixes: 47aa48b969f8 ("net: fix stripped VLAN flag for offload emulation");
> Cc: stable@dpdk.org
> 
> Signed-off-by: Bill Hong <bhong@brocade.com>
> Signed-off-by: Chas Williams <chas3@att.com>

Applied to dpdk-next-net/master, thanks.
diff mbox series

Patch

diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
index c2c5e249f..e0d831113 100644
--- a/lib/librte_net/rte_ether.h
+++ b/lib/librte_net/rte_ether.h
@@ -408,7 +408,7 @@  static inline int rte_vlan_insert(struct rte_mbuf **m)
 	vh = (struct vlan_hdr *) (nh + 1);
 	vh->vlan_tci = rte_cpu_to_be_16((*m)->vlan_tci);
 
-	(*m)->ol_flags &= ~PKT_RX_VLAN_STRIPPED;
+	(*m)->ol_flags &= ~(PKT_RX_VLAN_STRIPPED | PKT_TX_VLAN);
 
 	return 0;
 }