Re: [Intel-wired-lan] [PATCH net v1] ice: fix undersized tx_flags field

Re: [Intel-wired-lan] [PATCH net v1] ice: fix undersized tx_flags field

[Alexander Lobakin]

From: Jan Sokolowski <jan.sokolowski@intel.com>
Date: Tue, 11 Apr 2023 09:37:07 +0200

Please always add original authors to Ccs when you modify some code. I
found this mail only by scrolling IWL, while I should've got it from the
start.

+ Cc netdev (no idea why you didn't do that)

> As not all ICE_TX_FLAGS_* fit in current 16-bit limited
> tx_flags field, some flags would not properly apply.

Could you give more details here? With the actual definitions and also
how it was found and what's the regression is.
I found that there's VLAN tag which uses upper 16 bits only by browsing
the code, while I'd say you should've written it here.

> 
> Fix that by removing 16 bit limitation.
> 
> Signed-off-by: Jan Sokolowski <jan.sokolowski@intel.com>
> Fixes: aa1d3faf71a6 ("ice: Robustify cleaning/completing XDP Tx buffers")

Your SoB must go last, i.e. "Fixes:" should be placed above it.

> ---
>  drivers/net/ethernet/intel/ice/ice_txrx.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.h b/drivers/net/ethernet/intel/ice/ice_txrx.h
> index fff0efe28373..46c108cc5283 100644
> --- a/drivers/net/ethernet/intel/ice/ice_txrx.h
> +++ b/drivers/net/ethernet/intel/ice/ice_txrx.h
> @@ -183,7 +183,7 @@ struct ice_tx_buf {
>  		unsigned int nr_frags;	/* used for mbuf XDP */
>  	};
>  	u32 type:16;			/* &ice_tx_buf_type */
> -	u32 tx_flags:16;
> +	u32 tx_flags;

Please always provide pahole output when you change fields
size/structure. Here you create a 16-bit hole and increase structure
size with no mentioning.
I wouldn't say the fix is optimal. From what I see, we have such flags
(correct me if I'm wrong):

TSO			BIT(0)
[bits 1-7 are used]
OUTER_SINGLE_VLAN	BIT(8)
[bits 9-15 are UNused]
VLAN_S (shift)		16
[bits 16-31 are used for VLAN tag]

So you have 7 free bits to reuse for &ice_tx_buf_type, but you just
restored the before-commit ::tx_flags size =\
I would do the following:

	u32 tx_flags:12;
	u32 type:4;
	u32 vid:16;

* no structure size change (even no layout change);
* ::type range is 0-15 -- more than enough, as the last &ice_tx_buf_type
  value is 6;
* ::tx_flags still has 3 free bits left (9, 10, and 11);
* ::vid makes it easier to set a VLAN tag (no explicit masking-shifting,
  just don't forget to adjust the places where %ICE_TX_VLAN_{M,S} are
  used).

Don't just use "first that works" approach =\

>  	DEFINE_DMA_UNMAP_LEN(len);
>  	DEFINE_DMA_UNMAP_ADDR(dma);
>  };

Thanks,
Olek

RE: [Intel-wired-lan] [PATCH net v1] ice: fix undersized tx_flags field

[Sokolowski, Jan]

From: Lobakin, Aleksander <aleksander.lobakin@intel.com> 
Sent: Wednesday, April 12, 2023 6:49 PM
To: Sokolowski, Jan <jan.sokolowski@intel.com>
Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org
>Subject: Re: [Intel-wired-lan] [PATCH net v1] ice: fix undersized tx_flags field
>>From: Jan Sokolowski <jan.sokolowski@intel.com>
>>Date: Tue, 11 Apr 2023 09:37:07 +0200
>
>Please always add original authors to Ccs when you modify some code. I
>found this mail only by scrolling IWL, while I should've got it from the
>start.
>
>+ Cc netdev (no idea why you didn't do that)
>> As not all ICE_TX_FLAGS_* fit in current 16-bit limited
>> tx_flags field, some flags would not properly apply.
>
>Could you give more details here? With the actual definitions and also
>how it was found and what's the regression is.
>I found that there's VLAN tag which uses upper 16 bits only by browsing
>the code, while I'd say you should've written it here.

The definitions are ICE_TX_FLAGS_VLAN_* ones, for example ICE_TX_FLAGS_VLAN_M that is a
0xffff0000 mask.

The regression found was with some vlan traffic no longer passing through after commit 
aa1d3faf71a6 ("ice: Robustify cleaning/completing XDP Tx buffers")

>> 
>> Fix that by removing 16 bit limitation.
>> 
>> Signed-off-by: Jan Sokolowski <jan.sokolowski@intel.com>
>> Fixes: aa1d3faf71a6 ("ice: Robustify cleaning/completing XDP Tx buffers")
>
>Your SoB must go last, i.e. "Fixes:" should be placed above it.
>
>> ---
>>  drivers/net/ethernet/intel/ice/ice_txrx.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.h b/drivers/net/ethernet/intel/ice/ice_txrx.h
>> index fff0efe28373..46c108cc5283 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_txrx.h
>> +++ b/drivers/net/ethernet/intel/ice/ice_txrx.h
>> @@ -183,7 +183,7 @@ struct ice_tx_buf {
>>  		unsigned int nr_frags;	/* used for mbuf XDP */
>>  	};
>>  	u32 type:16;			/* &ice_tx_buf_type */
>> -	u32 tx_flags:16;
>> +	u32 tx_flags;
>
>Please always provide pahole output when you change fields
>size/structure. Here you create a 16-bit hole and increase structure
>size with no mentioning.

Ok, will do that. 

>I wouldn't say the fix is optimal. From what I see, we have such flags
>(correct me if I'm wrong):
>
>TSO			BIT(0)
>[bits 1-7 are used]
>OUTER_SINGLE_VLAN	BIT(8)
>[bits 9-15 are UNused]
>VLAN_S (shift)		16
>[bits 16-31 are used for VLAN tag]
>
>So you have 7 free bits to reuse for &ice_tx_buf_type, but you just
>restored the before-commit ::tx_flags size =\
>I would do the following:
>
>	u32 tx_flags:12;
>	u32 type:4;
>	u32 vid:16;

Ok, will try to refactor it in this way and see whether it'll work.

>* no structure size change (even no layout change);
>* ::type range is 0-15 -- more than enough, as the last &ice_tx_buf_type
>  value is 6;
>* ::tx_flags still has 3 free bits left (9, 10, and 11);
>* ::vid makes it easier to set a VLAN tag (no explicit masking-shifting,
>  just don't forget to adjust the places where %ICE_TX_VLAN_{M,S} are
>  used).
>
>Don't just use "first that works" approach =\
>
>>  	DEFINE_DMA_UNMAP_LEN(len);
>>  	DEFINE_DMA_UNMAP_ADDR(dma);
>>  };
>
>Thanks,
>Olek