[dpdk-dev,v2,2/5] net/i40e: fix bitmask of supported Tx flags

  Some Tx offload flags are missed in bitmask of all supported packet
Tx flags by i40e.
This patch fixes it.

CC: helin.zhang@intel.com
Fixes: 3f33e643e5c6 ("net/i40e: add Tx preparation")
Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
---
 drivers/net/i40e/i40e_rxtx.c | 36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)
  

On 2/4/2017 3:36 AM, Jingjing Wu wrote:
> Some Tx offload flags are missed in bitmask of all supported packet
> Tx flags by i40e.
> This patch fixes it.
> 
> CC: helin.zhang@intel.com
> Fixes: 3f33e643e5c6 ("net/i40e: add Tx preparation")
> Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
> ---
>  drivers/net/i40e/i40e_rxtx.c | 36 ++++++++++++++++++++++++++++--------
>  1 file changed, 28 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
> index 608685f..4dd45f3 100644
> --- a/drivers/net/i40e/i40e_rxtx.c
> +++ b/drivers/net/i40e/i40e_rxtx.c
> @@ -80,15 +80,35 @@
>  		PKT_TX_L4_MASK |		 \
>  		PKT_TX_TCP_SEG |		 \
>  		PKT_TX_OUTER_IP_CKSUM)
> +#ifdef RTE_LIBRTE_IEEE1588
> +#define I40E_TX_OFFLOAD_MASK (		 \
> +		PKT_TX_IP_CKSUM |	 \
> +		PKT_TX_IPV4 |		 \
> +		PKT_TX_IPV6 |		 \
> +		PKT_TX_L4_MASK |	 \
> +		PKT_TX_OUTER_IP_CKSUM |	 \
> +		PKT_TX_OUTER_IPV4 |	 \
> +		PKT_TX_OUTER_IPV6 |	 \
> +		PKT_TX_IEEE1588_TMST |	 \
> +		PKT_TX_TCP_SEG |	 \
> +		PKT_TX_QINQ_PKT |	 \
> +		PKT_TX_VLAN_PKT |	 \
> +		PKT_TX_TUNNEL_MASK)
> +#else
> +#define I40E_TX_OFFLOAD_MASK (		 \
> +		PKT_TX_IP_CKSUM |	 \
> +		PKT_TX_IPV4 |		 \
> +		PKT_TX_IPV6 |		 \
> +		PKT_TX_L4_MASK |	 \
> +		PKT_TX_OUTER_IP_CKSUM |	 \
> +		PKT_TX_OUTER_IPV4 |	 \
> +		PKT_TX_OUTER_IPV6 |	 \
> +		PKT_TX_TCP_SEG |	 \
> +		PKT_TX_QINQ_PKT |	 \
> +		PKT_TX_VLAN_PKT |	 \
> +		PKT_TX_TUNNEL_MASK)

Functionally will be same, but what do you think about following, to
make easy to see what define adds:

+#define I40E_TX_OFFLOAD_MASK (		 \
+		PKT_TX_IP_CKSUM |	 \
+		PKT_TX_IPV4 |		 \
+		PKT_TX_IPV6 |		 \
+		PKT_TX_L4_MASK |	 \
+		PKT_TX_OUTER_IP_CKSUM |	 \
+		PKT_TX_OUTER_IPV4 |	 \
+		PKT_TX_OUTER_IPV6 |	 \

+#ifdef RTE_LIBRTE_IEEE1588
+		PKT_TX_IEEE1588_TMST |	 \
+#endif

+		PKT_TX_TCP_SEG |	 \
+		PKT_TX_QINQ_PKT |	 \
+		PKT_TX_VLAN_PKT |	 \
+		PKT_TX_TUNNEL_MASK)

>  
> -#define I40E_TX_OFFLOAD_MASK (  \
> -		PKT_TX_IP_CKSUM |       \
> -		PKT_TX_L4_MASK |        \
> -		PKT_TX_OUTER_IP_CKSUM | \
> -		PKT_TX_TCP_SEG |        \
> -		PKT_TX_QINQ_PKT |       \
> -		PKT_TX_VLAN_PKT)
> -
> +#endif
>  #define I40E_TX_OFFLOAD_NOTSUP_MASK \
>  		(PKT_TX_OFFLOAD_MASK ^ I40E_TX_OFFLOAD_MASK)
>  
>
  

> 
> Functionally will be same, but what do you think about following, to make
> easy to see what define adds:
> 
> +#define I40E_TX_OFFLOAD_MASK (		 \
> +		PKT_TX_IP_CKSUM |	 \
> +		PKT_TX_IPV4 |		 \
> +		PKT_TX_IPV6 |		 \
> +		PKT_TX_L4_MASK |	 \
> +		PKT_TX_OUTER_IP_CKSUM |	 \
> +		PKT_TX_OUTER_IPV4 |	 \
> +		PKT_TX_OUTER_IPV6 |	 \
> 
> +#ifdef RTE_LIBRTE_IEEE1588
> +		PKT_TX_IEEE1588_TMST |	 \
> +#endif
> 
> +		PKT_TX_TCP_SEG |	 \
> +		PKT_TX_QINQ_PKT |	 \
> +		PKT_TX_VLAN_PKT |	 \
> +		PKT_TX_TUNNEL_MASK)
> 

Hi, Ferruh

As I know, the above change is incorrect in C code. We cannot use #ifdef  #endif inside #define

Thanks
Jingjing
  

Hi Jingjing,

On Sat,  4 Feb 2017 11:36:12 +0800, Jingjing Wu <jingjing.wu@intel.com>
wrote:
> Some Tx offload flags are missed in bitmask of all supported packet
> Tx flags by i40e.
> This patch fixes it.

Could you detail which flag was missing?
Is it PKT_TX_TUNNEL_MASK?
If yes, shouldn't we have a "Fixes:" line?

I think most of the patchset should be merged in one patch, because
changing only the mbuf part (PKT_TX_OFFLOAD_MASK) would break the
drivers that checks the offload bits at init, and this is not suitable,
especially if we want to be able to do git bisect.


My suggestion is to have:

1- fix i40 (add missing tunnel mask?)
2- fix missing MACSET in TX_OFFLOAD_MASK
3- change TX_OFFLOAD_MASK to include all flags (this impacts all
   drivers using TX_OFFLOAD_MASK)


Regards,
Olivier
  

On Mon, 6 Feb 2017 03:02:12 +0000, "Wu, Jingjing"
<jingjing.wu@intel.com> wrote:
> > 
> > Functionally will be same, but what do you think about following,
> > to make easy to see what define adds:
> > 
> > +#define I40E_TX_OFFLOAD_MASK (		 \
> > +		PKT_TX_IP_CKSUM |	 \
> > +		PKT_TX_IPV4 |		 \
> > +		PKT_TX_IPV6 |		 \
> > +		PKT_TX_L4_MASK |	 \
> > +		PKT_TX_OUTER_IP_CKSUM |	 \
> > +		PKT_TX_OUTER_IPV4 |	 \
> > +		PKT_TX_OUTER_IPV6 |	 \
> > 
> > +#ifdef RTE_LIBRTE_IEEE1588
> > +		PKT_TX_IEEE1588_TMST |	 \
> > +#endif
> > 
> > +		PKT_TX_TCP_SEG |	 \
> > +		PKT_TX_QINQ_PKT |	 \
> > +		PKT_TX_VLAN_PKT |	 \
> > +		PKT_TX_TUNNEL_MASK)
> >   
> 
> Hi, Ferruh
> 
> As I know, the above change is incorrect in C code. We cannot use
> #ifdef  #endif inside #define
> 
> Thanks
> Jingjing


You can do:

#ifdef RTE_LIBRTE_IEEE1588
#define I40_TX_IEEE1588_TMST PKT_TX_IEEE1588_TMST
#else
#define I40_TX_IEEE1588_TMST 0
#endif

#define I40E_TX_OFFLOAD_MASK (   \
	I40_TX_IEEE1588_TMST |   \
	PKT_TX_IP_CKSUM |	 \
	...


Regards,
Olivier
  

> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Monday, February 6, 2017 6:29 PM
> To: Wu, Jingjing <jingjing.wu@intel.com>
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; dev@dpdk.org; Zhang, Helin
> <helin.zhang@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 2/5] net/i40e: fix bitmask of supported Tx
> flags
> 
> On Mon, 6 Feb 2017 03:02:12 +0000, "Wu, Jingjing"
> <jingjing.wu@intel.com> wrote:
> > >
> > > Functionally will be same, but what do you think about following, to
> > > make easy to see what define adds:
> > >
> > > +#define I40E_TX_OFFLOAD_MASK (		 \
> > > +		PKT_TX_IP_CKSUM |	 \
> > > +		PKT_TX_IPV4 |		 \
> > > +		PKT_TX_IPV6 |		 \
> > > +		PKT_TX_L4_MASK |	 \
> > > +		PKT_TX_OUTER_IP_CKSUM |	 \
> > > +		PKT_TX_OUTER_IPV4 |	 \
> > > +		PKT_TX_OUTER_IPV6 |	 \
> > >
> > > +#ifdef RTE_LIBRTE_IEEE1588
> > > +		PKT_TX_IEEE1588_TMST |	 \
> > > +#endif
> > >
> > > +		PKT_TX_TCP_SEG |	 \
> > > +		PKT_TX_QINQ_PKT |	 \
> > > +		PKT_TX_VLAN_PKT |	 \
> > > +		PKT_TX_TUNNEL_MASK)
> > >
> >
> > Hi, Ferruh
> >
> > As I know, the above change is incorrect in C code. We cannot use
> > #ifdef  #endif inside #define
> >
> > Thanks
> > Jingjing
> 
> 
> You can do:
> 
> #ifdef RTE_LIBRTE_IEEE1588
> #define I40_TX_IEEE1588_TMST PKT_TX_IEEE1588_TMST #else #define
> I40_TX_IEEE1588_TMST 0 #endif
> 
> #define I40E_TX_OFFLOAD_MASK (   \
> 	I40_TX_IEEE1588_TMST |   \
> 	PKT_TX_IP_CKSUM |	 \
> 	...
> 

Thanks for the suggestion.
> 
> Regards,
> Olivier
  

> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Monday, February 6, 2017 6:27 PM
> To: Wu, Jingjing <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; Zhang, Helin <helin.zhang@intel.com>; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 2/5] net/i40e: fix bitmask of supported Tx
> flags
> 
> Hi Jingjing,
> 
> On Sat,  4 Feb 2017 11:36:12 +0800, Jingjing Wu <jingjing.wu@intel.com>
> wrote:
> > Some Tx offload flags are missed in bitmask of all supported packet Tx
> > flags by i40e.
> > This patch fixes it.
> 
> Could you detail which flag was missing?
> Is it PKT_TX_TUNNEL_MASK?
> If yes, shouldn't we have a "Fixes:" line?
> 
> I think most of the patchset should be merged in one patch, because
> changing only the mbuf part (PKT_TX_OFFLOAD_MASK) would break the
> drivers that checks the offload bits at init, and this is not suitable, especially if
> we want to be able to do git bisect.
> 
> 
> My suggestion is to have:
> 
> 1- fix i40 (add missing tunnel mask?)
> 2- fix missing MACSET in TX_OFFLOAD_MASK
> 3- change TX_OFFLOAD_MASK to include all flags (this impacts all
>    drivers using TX_OFFLOAD_MASK)
> 
>
OK. Will change according to your suggestion. It's clearer. Thanks

Jingjing