diff mbox series

[RFC,net-next,2/2] ice: make use of DECLARE_FLEX() in ice_switch.c

Message ID 20230801111923.118268-3-przemyslaw.kitszel@intel.com (mailing list archive)
State RFC
Delegated to: Netdev Maintainers
Headers show
Series introduce DECLARE_FLEX() macro | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1328 this patch: 1328
netdev/cc_maintainers warning 6 maintainers not CCed: kuba@kernel.org jesse.brandeburg@intel.com davem@davemloft.net anthony.l.nguyen@intel.com pabeni@redhat.com edumazet@google.com
netdev/build_clang success Errors and warnings before: 1351 this patch: 1351
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1351 this patch: 1351
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 124 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Przemek Kitszel Aug. 1, 2023, 11:19 a.m. UTC
Use DECLARE_FLEX() macro for 1-elem flex array members of ice_switch.c

Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_switch.c | 53 +++++----------------
 1 file changed, 12 insertions(+), 41 deletions(-)

Comments

Alexander Lobakin Aug. 1, 2023, 1:48 p.m. UTC | #1
From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Date: Tue, 1 Aug 2023 13:19:23 +0200

> Use DECLARE_FLEX() macro for 1-elem flex array members of ice_switch.c
> 
> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_switch.c | 53 +++++----------------
>  1 file changed, 12 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c
> index a7afb612fe32..41679cb6b548 100644
> --- a/drivers/net/ethernet/intel/ice/ice_switch.c
> +++ b/drivers/net/ethernet/intel/ice/ice_switch.c
> @@ -1812,15 +1812,11 @@ ice_aq_alloc_free_vsi_list(struct ice_hw *hw, u16 *vsi_list_id,
>  			   enum ice_sw_lkup_type lkup_type,
>  			   enum ice_adminq_opc opc)
>  {
> -	struct ice_aqc_alloc_free_res_elem *sw_buf;
> +	DECLARE_FLEX(struct ice_aqc_alloc_free_res_elem *, sw_buf, elem, 1);
> +	u16 buf_len = struct_size(sw_buf, elem, 1);
>  	struct ice_aqc_res_elem *vsi_ele;
> -	u16 buf_len;
>  	int status;
>  
> -	buf_len = struct_size(sw_buf, elem, 1);
> -	sw_buf = devm_kzalloc(ice_hw_to_dev(hw), buf_len, GFP_KERNEL);
> -	if (!sw_buf)
> -		return -ENOMEM;
>  	sw_buf->num_elems = cpu_to_le16(1);
>  
>  	if (lkup_type == ICE_SW_LKUP_MAC ||
> @@ -1840,8 +1836,7 @@ ice_aq_alloc_free_vsi_list(struct ice_hw *hw, u16 *vsi_list_id,
>  			sw_buf->res_type =
>  				cpu_to_le16(ICE_AQC_RES_TYPE_VSI_LIST_PRUNE);
>  	} else {
> -		status = -EINVAL;
> -		goto ice_aq_alloc_free_vsi_list_exit;
> +		return -EINVAL;
>  	}
>  
>  	if (opc == ice_aqc_opc_free_res)

bloat-o-meter results would be nice to have in the commitmsg.

[...]

Thanks,
Olek
Przemek Kitszel Aug. 1, 2023, 2:36 p.m. UTC | #2
On 8/1/23 15:48, Alexander Lobakin wrote:
> From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Date: Tue, 1 Aug 2023 13:19:23 +0200
> 
>> Use DECLARE_FLEX() macro for 1-elem flex array members of ice_switch.c
>>
>> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>> ---
>>   drivers/net/ethernet/intel/ice/ice_switch.c | 53 +++++----------------
>>   1 file changed, 12 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c
>> index a7afb612fe32..41679cb6b548 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_switch.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_switch.c
>> @@ -1812,15 +1812,11 @@ ice_aq_alloc_free_vsi_list(struct ice_hw *hw, u16 *vsi_list_id,
>>   			   enum ice_sw_lkup_type lkup_type,
>>   			   enum ice_adminq_opc opc)
>>   {
>> -	struct ice_aqc_alloc_free_res_elem *sw_buf;
>> +	DECLARE_FLEX(struct ice_aqc_alloc_free_res_elem *, sw_buf, elem, 1);
>> +	u16 buf_len = struct_size(sw_buf, elem, 1);
>>   	struct ice_aqc_res_elem *vsi_ele;
>> -	u16 buf_len;
>>   	int status;
>>   
>> -	buf_len = struct_size(sw_buf, elem, 1);
>> -	sw_buf = devm_kzalloc(ice_hw_to_dev(hw), buf_len, GFP_KERNEL);
>> -	if (!sw_buf)
>> -		return -ENOMEM;
>>   	sw_buf->num_elems = cpu_to_le16(1);
>>   
>>   	if (lkup_type == ICE_SW_LKUP_MAC ||
>> @@ -1840,8 +1836,7 @@ ice_aq_alloc_free_vsi_list(struct ice_hw *hw, u16 *vsi_list_id,
>>   			sw_buf->res_type =
>>   				cpu_to_le16(ICE_AQC_RES_TYPE_VSI_LIST_PRUNE);
>>   	} else {
>> -		status = -EINVAL;
>> -		goto ice_aq_alloc_free_vsi_list_exit;
>> +		return -EINVAL;
>>   	}
>>   
>>   	if (opc == ice_aqc_opc_free_res)
> 
> bloat-o-meter results would be nice to have in the commitmsg.

I will do next time, perhaps you could tell me if I get the results 
right here:

./scripts/bloat-o-meter ice_switch.o{ld,}
add/remove: 2/2 grow/shrink: 3/5 up/down: 560/-483 (77)
Function                                     old     new   delta
ice_create_vsi_list_rule                       -     241    +241
ice_remove_vsi_list_rule                     139     270    +131
ice_add_adv_rule                            6047    6139     +92
ice_add_sw_recipe                           2892    2972     +80
__pfx_ice_create_vsi_list_rule                 -      16     +16
ice_alloc_recipe                             124     113     -11
__pfx_ice_aq_alloc_free_vsi_list              16       -     -16
ice_free_res_cntr                            185     155     -30
ice_alloc_res_cntr                           154     124     -30
ice_add_update_vsi_list                     1037     994     -43
ice_add_vlan_internal                       1027     953     -74
ice_aq_alloc_free_vsi_list                   279       -    -279
Total: Before=42183, After=42260, chg +0.18%

My guess here is that compiler did different decisions about what to 
inline where, what is biggest difference.
And biggest gain here is avoidance of heap allocs, perhaps that enables 
gcc to shuffle things a bit too.
Another guess is that b-o-m ignores heap bloat, so slight growth is 
expected.

Values reported for ice.ko are the same, with bigger base to compute the 
percent off.

 > [...]
 >
 > Thanks,
 > Olek

Thank you too, also for our initial talk about on the topic.
Alexander Lobakin Aug. 1, 2023, 5:22 p.m. UTC | #3
From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Date: Tue, 1 Aug 2023 16:36:57 +0200

> On 8/1/23 15:48, Alexander Lobakin wrote:
>> From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>> Date: Tue, 1 Aug 2023 13:19:23 +0200

[...]

>>> -        status = -EINVAL;
>>> -        goto ice_aq_alloc_free_vsi_list_exit;
>>> +        return -EINVAL;
>>>       }
>>>         if (opc == ice_aqc_opc_free_res)
>>
>> bloat-o-meter results would be nice to have in the commitmsg.
> 
> I will do next time, perhaps you could tell me if I get the results
> right here:
> 
> ./scripts/bloat-o-meter ice_switch.o{ld,}
> add/remove: 2/2 grow/shrink: 3/5 up/down: 560/-483 (77)
> Function                                     old     new   delta
> ice_create_vsi_list_rule                       -     241    +241
> ice_remove_vsi_list_rule                     139     270    +131
> ice_add_adv_rule                            6047    6139     +92
> ice_add_sw_recipe                           2892    2972     +80
> __pfx_ice_create_vsi_list_rule                 -      16     +16
> ice_alloc_recipe                             124     113     -11
> __pfx_ice_aq_alloc_free_vsi_list              16       -     -16
> ice_free_res_cntr                            185     155     -30
> ice_alloc_res_cntr                           154     124     -30
> ice_add_update_vsi_list                     1037     994     -43
> ice_add_vlan_internal                       1027     953     -74
> ice_aq_alloc_free_vsi_list                   279       -    -279
> Total: Before=42183, After=42260, chg +0.18%
> 
> My guess here is that compiler did different decisions about what to
> inline where, what is biggest difference.

77 bytes is very good result, because see below.

> And biggest gain here is avoidance of heap allocs, perhaps that enables
> gcc to shuffle things a bit too.

Exactly, having the stack grown only by 77 bytes after avoiding -- how
many? -- a lot of heap allocations sounds great.

> Another guess is that b-o-m ignores heap bloat, so slight growth is
> expected.

BOM can't calculate any heap usage, it simply compares symbol sizes in
object files.

(BTW, passing /dev/null as the first "object file" is legit, in case you
 just want to see sorted symbol sizes in your module or vmlinux)

> 
> Values reported for ice.ko are the same, with bigger base to compute the
> percent off.
> 
>> [...]
>>
>> Thanks,
>> Olek
> 
> Thank you too, also for our initial talk about on the topic.

No problem, I really feel like this macro would be very useful.

Thanks,
Olek
Kees Cook Aug. 1, 2023, 10:35 p.m. UTC | #4
On Tue, Aug 01, 2023 at 01:19:23PM +0200, Przemek Kitszel wrote:
> Use DECLARE_FLEX() macro for 1-elem flex array members of ice_switch.c
> 
> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_switch.c | 53 +++++----------------
>  1 file changed, 12 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c
> index a7afb612fe32..41679cb6b548 100644
> --- a/drivers/net/ethernet/intel/ice/ice_switch.c
> +++ b/drivers/net/ethernet/intel/ice/ice_switch.c
> @@ -1812,15 +1812,11 @@ ice_aq_alloc_free_vsi_list(struct ice_hw *hw, u16 *vsi_list_id,
>  			   enum ice_sw_lkup_type lkup_type,
>  			   enum ice_adminq_opc opc)
>  {
> -	struct ice_aqc_alloc_free_res_elem *sw_buf;
> +	DECLARE_FLEX(struct ice_aqc_alloc_free_res_elem *, sw_buf, elem, 1);
> +	u16 buf_len = struct_size(sw_buf, elem, 1);

With the macro I suggested, I think this line can become:

	u16 buf_len = __builtin_object_size(sw_buf, 1);

but either is fine. (N.B. the "1" here is a bitfield, not the "1" size
above).

-Kees
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c
index a7afb612fe32..41679cb6b548 100644
--- a/drivers/net/ethernet/intel/ice/ice_switch.c
+++ b/drivers/net/ethernet/intel/ice/ice_switch.c
@@ -1812,15 +1812,11 @@  ice_aq_alloc_free_vsi_list(struct ice_hw *hw, u16 *vsi_list_id,
 			   enum ice_sw_lkup_type lkup_type,
 			   enum ice_adminq_opc opc)
 {
-	struct ice_aqc_alloc_free_res_elem *sw_buf;
+	DECLARE_FLEX(struct ice_aqc_alloc_free_res_elem *, sw_buf, elem, 1);
+	u16 buf_len = struct_size(sw_buf, elem, 1);
 	struct ice_aqc_res_elem *vsi_ele;
-	u16 buf_len;
 	int status;
 
-	buf_len = struct_size(sw_buf, elem, 1);
-	sw_buf = devm_kzalloc(ice_hw_to_dev(hw), buf_len, GFP_KERNEL);
-	if (!sw_buf)
-		return -ENOMEM;
 	sw_buf->num_elems = cpu_to_le16(1);
 
 	if (lkup_type == ICE_SW_LKUP_MAC ||
@@ -1840,8 +1836,7 @@  ice_aq_alloc_free_vsi_list(struct ice_hw *hw, u16 *vsi_list_id,
 			sw_buf->res_type =
 				cpu_to_le16(ICE_AQC_RES_TYPE_VSI_LIST_PRUNE);
 	} else {
-		status = -EINVAL;
-		goto ice_aq_alloc_free_vsi_list_exit;
+		return -EINVAL;
 	}
 
 	if (opc == ice_aqc_opc_free_res)
@@ -1849,16 +1844,14 @@  ice_aq_alloc_free_vsi_list(struct ice_hw *hw, u16 *vsi_list_id,
 
 	status = ice_aq_alloc_free_res(hw, 1, sw_buf, buf_len, opc, NULL);
 	if (status)
-		goto ice_aq_alloc_free_vsi_list_exit;
+		return status;
 
 	if (opc == ice_aqc_opc_alloc_res) {
 		vsi_ele = &sw_buf->elem[0];
 		*vsi_list_id = le16_to_cpu(vsi_ele->e.sw_resp);
 	}
 
-ice_aq_alloc_free_vsi_list_exit:
-	devm_kfree(ice_hw_to_dev(hw), sw_buf);
-	return status;
+	return 0;
 }
 
 /**
@@ -2088,15 +2081,10 @@  ice_aq_get_recipe_to_profile(struct ice_hw *hw, u32 profile_id, u8 *r_bitmap,
  */
 int ice_alloc_recipe(struct ice_hw *hw, u16 *rid)
 {
-	struct ice_aqc_alloc_free_res_elem *sw_buf;
-	u16 buf_len;
+	DECLARE_FLEX(struct ice_aqc_alloc_free_res_elem *, sw_buf, elem, 1);
+	u16 buf_len = struct_size(sw_buf, elem, 1);
 	int status;
 
-	buf_len = struct_size(sw_buf, elem, 1);
-	sw_buf = kzalloc(buf_len, GFP_KERNEL);
-	if (!sw_buf)
-		return -ENOMEM;
-
 	sw_buf->num_elems = cpu_to_le16(1);
 	sw_buf->res_type = cpu_to_le16((ICE_AQC_RES_TYPE_RECIPE <<
 					ICE_AQC_RES_TYPE_S) |
@@ -2105,7 +2093,6 @@  int ice_alloc_recipe(struct ice_hw *hw, u16 *rid)
 				       ice_aqc_opc_alloc_res, NULL);
 	if (!status)
 		*rid = le16_to_cpu(sw_buf->elem[0].e.sw_resp);
-	kfree(sw_buf);
 
 	return status;
 }
@@ -4482,16 +4469,10 @@  int
 ice_alloc_res_cntr(struct ice_hw *hw, u8 type, u8 alloc_shared, u16 num_items,
 		   u16 *counter_id)
 {
-	struct ice_aqc_alloc_free_res_elem *buf;
-	u16 buf_len;
+	DECLARE_FLEX(struct ice_aqc_alloc_free_res_elem *, buf, elem, 1);
+	u16 buf_len = struct_size(buf, elem, 1);
 	int status;
 
-	/* Allocate resource */
-	buf_len = struct_size(buf, elem, 1);
-	buf = kzalloc(buf_len, GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
-
 	buf->num_elems = cpu_to_le16(num_items);
 	buf->res_type = cpu_to_le16(((type << ICE_AQC_RES_TYPE_S) &
 				      ICE_AQC_RES_TYPE_M) | alloc_shared);
@@ -4499,12 +4480,9 @@  ice_alloc_res_cntr(struct ice_hw *hw, u8 type, u8 alloc_shared, u16 num_items,
 	status = ice_aq_alloc_free_res(hw, 1, buf, buf_len,
 				       ice_aqc_opc_alloc_res, NULL);
 	if (status)
-		goto exit;
+		return status;
 
 	*counter_id = le16_to_cpu(buf->elem[0].e.sw_resp);
-
-exit:
-	kfree(buf);
 	return status;
 }
 
@@ -4520,16 +4498,10 @@  int
 ice_free_res_cntr(struct ice_hw *hw, u8 type, u8 alloc_shared, u16 num_items,
 		  u16 counter_id)
 {
-	struct ice_aqc_alloc_free_res_elem *buf;
-	u16 buf_len;
+	DECLARE_FLEX(struct ice_aqc_alloc_free_res_elem *, buf, elem, 1);
+	u16 buf_len = struct_size(buf, elem, 1);
 	int status;
 
-	/* Free resource */
-	buf_len = struct_size(buf, elem, 1);
-	buf = kzalloc(buf_len, GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
-
 	buf->num_elems = cpu_to_le16(num_items);
 	buf->res_type = cpu_to_le16(((type << ICE_AQC_RES_TYPE_S) &
 				      ICE_AQC_RES_TYPE_M) | alloc_shared);
@@ -4540,7 +4512,6 @@  ice_free_res_cntr(struct ice_hw *hw, u8 type, u8 alloc_shared, u16 num_items,
 	if (status)
 		ice_debug(hw, ICE_DBG_SW, "counter resource could not be freed\n");
 
-	kfree(buf);
 	return status;
 }