Re: [PATCH net-next] iavf: Avoid a memory allocation in iavf_print_link_message()

[Jesse Brandeburg <jesse.brandeburg@intel.com>]

On 9/23/2023 5:17 AM, Christophe JAILLET wrote:
> IAVF_MAX_SPEED_STRLEN is only 13 and 'speed' is allocated and freed within
> iavf_print_link_message().
> 
> 'speed' is only used with some snprintf() and netdev_info() calls.
> 
> So there is no real use to kzalloc()/free() it. Use the stack instead.
> This saves a memory allocation.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  drivers/net/ethernet/intel/iavf/iavf_virtchnl.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> index 8ce6389b5815..980dc69d7fbe 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> @@ -1389,18 +1389,14 @@ void iavf_disable_vlan_insertion_v2(struct iavf_adapter *adapter, u16 tpid)
>  static void iavf_print_link_message(struct iavf_adapter *adapter)
>  {
>  	struct net_device *netdev = adapter->netdev;
> +	char speed[IAVF_MAX_SPEED_STRLEN];
>  	int link_speed_mbps;
> -	char *speed;
>  
>  	if (!adapter->link_up) {
>  		netdev_info(netdev, "NIC Link is Down\n");
>  		return;
>  	}
>  
> -	speed = kzalloc(IAVF_MAX_SPEED_STRLEN, GFP_KERNEL);
> -	if (!speed)
> -		return;
> -
>  	if (ADV_LINK_SUPPORT(adapter)) {
>  		link_speed_mbps = adapter->link_speed_mbps;
>  		goto print_link_msg;
> @@ -1452,7 +1448,6 @@ static void iavf_print_link_message(struct iavf_adapter *adapter)
>  	}
>  
>  	netdev_info(netdev, "NIC Link is Up Speed is %s Full Duplex\n", speed);
> -	kfree(speed);
>  }
>  
>  /**

Hi Christophe!

I had a slightly different proposal that gets rid of all the -Wformat=2
warnings in this code by using kasprintf to handle the varying string
lengths.

any thoughts about this instead and drop yours? I'm less worried about
the "extra allocation" here in this function since it's slow path, and
the same comment applies to your patch as well.

your patch still shows these errors
> drivers/net/ethernet/intel/iavf/iavf_virtchnl.c: In function ‘iavf_virtchnl_completion’:
> drivers/net/ethernet/intel/iavf/iavf_virtchnl.c:1446:60: warning: ‘%s’ directive output may be truncated writing 4 bytes into a region of size between 1 and 11 [-Wformat-truncation=]
>  1446 |                 snprintf(speed, IAVF_MAX_SPEED_STRLEN, "%d %s",
>       |                                                            ^~
>  1447 |                          link_speed_mbps, "Mbps");
>       |                                           ~~~~~~
> In function ‘iavf_print_link_message’,
>     inlined from ‘iavf_virtchnl_completion’ at drivers/net/ethernet/intel/iavf/iavf_virtchnl.c:1965:4:
> drivers/net/ethernet/intel/iavf/iavf_virtchnl.c:1446:17: note: ‘snprintf’ output between 7 and 17 bytes into a destination of size 13
>  1446 |                 snprintf(speed, IAVF_MAX_SPEED_STRLEN, "%d %s",
>       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  1447 |                          link_speed_mbps, "Mbps");
>       |                          ~~~~~~~~~~~~~~~~~~~~~~~~


<my iavf patch pasted as a quote so my mail client won't wrap the lines...>


> diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> index 8ce6389b5815..82b84a93bcc8 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> @@ -1378,8 +1378,6 @@ void iavf_disable_vlan_insertion_v2(struct iavf_adapter *adapter, u16 tpid)
>                                   VIRTCHNL_OP_DISABLE_VLAN_INSERTION_V2);
>  }
> 
> -#define IAVF_MAX_SPEED_STRLEN  13
> -
>  /**
>   * iavf_print_link_message - print link up or down
>   * @adapter: adapter structure
> @@ -1397,10 +1395,6 @@ static void iavf_print_link_message(struct iavf_adapter *adapter)
>                 return;
>         }
> 
> -       speed = kzalloc(IAVF_MAX_SPEED_STRLEN, GFP_KERNEL);
> -       if (!speed)
> -               return;
> -
>         if (ADV_LINK_SUPPORT(adapter)) {
>                 link_speed_mbps = adapter->link_speed_mbps;
>                 goto print_link_msg;
> @@ -1438,17 +1432,17 @@ static void iavf_print_link_message(struct iavf_adapter *adapter)
> 
>  print_link_msg:
>         if (link_speed_mbps > SPEED_1000) {
> -               if (link_speed_mbps == SPEED_2500)
> -                       snprintf(speed, IAVF_MAX_SPEED_STRLEN, "2.5 Gbps");
> -               else
> +               if (link_speed_mbps == SPEED_2500) {
> +                       speed = kasprintf(GFP_KERNEL, "%s", "2.5 Gbps");
> +               } else {
>                         /* convert to Gbps inline */
> -                       snprintf(speed, IAVF_MAX_SPEED_STRLEN, "%d %s",
> -                                link_speed_mbps / 1000, "Gbps");
> +                       speed = kasprintf(GFP_KERNEL, "%d Gbps",
> +                                         link_speed_mbps / 1000);
> +               }
>         } else if (link_speed_mbps == SPEED_UNKNOWN) {
> -               snprintf(speed, IAVF_MAX_SPEED_STRLEN, "%s", "Unknown Mbps");
> +               speed = kasprintf(GFP_KERNEL, "%s", "Unknown Mbps");
>         } else {
> -               snprintf(speed, IAVF_MAX_SPEED_STRLEN, "%d %s",
> -                        link_speed_mbps, "Mbps");
> +               speed = kasprintf(GFP_KERNEL, "%d Mbps", link_speed_mbps);
>         }
> 
>         netdev_info(netdev, "NIC Link is Up Speed is %s Full Duplex\n", speed);