Re: [PATCH net-next] iavf: Avoid a memory allocation in iavf_print_link_message()
From: Christophe JAILLET
Date: Tue Oct 03 2023 - 16:33:57 EST
Le 03/10/2023 à 19:14, Jesse Brandeburg a écrit :
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@xxxxxxxxxx>
---
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.
kasprintf() is much better.
your patch still shows these errors
I built-tested the patch before sending, so this is strange.
However, I got a similar feedback from Greg KH and the "kernel test
robot" for another similar patch.
What version of gcc do you use?
I use 12.3.0, and I suspect that the value range algorithm or how the
diagnostic is done has been improved in recent gcc.
The other report was from 11.3.0.
CJ
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);