diff mbox series

[net-next] net: ethernet: mlx4: Avoid assigning a value to ring_cons but not used it anymore in mlx4_en_xmit()

Message ID 1599898095-10712-1-git-send-email-luojiaxing@huawei.com
State New
Headers show
Series [net-next] net: ethernet: mlx4: Avoid assigning a value to ring_cons but not used it anymore in mlx4_en_xmit() | expand

Commit Message

luojiaxing Sept. 12, 2020, 8:08 a.m. UTC
We found a set but not used variable 'ring_cons' in mlx4_en_xmit(), it will
cause a warning when build the kernel. And after checking the commit record
of this function, we found that it was introduced by a previous patch.

So, We delete this redundant assignment code.

Fixes: 488a9b48e398 ("net/mlx4_en: Wake TX queues only when there's enough room")

Signed-off-by: Luo Jiaxing <luojiaxing@huawei.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_tx.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Tariq Toukan Sept. 13, 2020, 10:12 a.m. UTC | #1
On 9/13/2020 4:22 AM, David Miller wrote:
> From: Luo Jiaxing <luojiaxing@huawei.com>
> Date: Sat, 12 Sep 2020 16:08:15 +0800
> 
>> We found a set but not used variable 'ring_cons' in mlx4_en_xmit(), it will
>> cause a warning when build the kernel. And after checking the commit record
>> of this function, we found that it was introduced by a previous patch.
>>
>> So, We delete this redundant assignment code.
>>
>> Fixes: 488a9b48e398 ("net/mlx4_en: Wake TX queues only when there's enough room")
>>
>> Signed-off-by: Luo Jiaxing <luojiaxing@huawei.com>
> 
> Looks good, applied, thanks.
> 

Hi Luo,

I didn't get a chance to review it during the weekend.

The ring_cons local variable is used in line 903:
https://elixir.bootlin.com/linux/v5.9-rc4/source/drivers/net/ethernet/mellanox/mlx4/en_tx.c#L903

AVG_PERF_COUNTER depends on the compile-time definition of 
MLX4_EN_PERF_STAT. Otherwise it is a nop.

1. Your patch causes a degradation to the case when MLX4_EN_PERF_STAT is 
defined.
2. When MLX4_EN_PERF_STAT is not defined, we should totally remove the 
local variable declaration, not only its usage.

Please let me know if you're planning to fix this. Otherwise I'll do.

Regards,
Tariq
David Miller Sept. 13, 2020, 9:33 p.m. UTC | #2
From: Tariq Toukan <ttoukan.linux@gmail.com>
Date: Sun, 13 Sep 2020 13:12:05 +0300

> 
> 
> On 9/13/2020 4:22 AM, David Miller wrote:
>> From: Luo Jiaxing <luojiaxing@huawei.com>
>> Date: Sat, 12 Sep 2020 16:08:15 +0800
>> 
>>> We found a set but not used variable 'ring_cons' in mlx4_en_xmit(), it
>>> will
>>> cause a warning when build the kernel. And after checking the commit
>>> record
>>> of this function, we found that it was introduced by a previous patch.
>>>
>>> So, We delete this redundant assignment code.
>>>
>>> Fixes: 488a9b48e398 ("net/mlx4_en: Wake TX queues only when there's
>>> enough room")
>>>
>>> Signed-off-by: Luo Jiaxing <luojiaxing@huawei.com>
>> Looks good, applied, thanks.
>> 
> 
> Hi Luo,
> 
> I didn't get a chance to review it during the weekend.

Tariq, what are you even commenting on?  Are you responding to this patch
which removes a %100 obviously unused variable set, or on the commit
mentioned in the Fixes: tag?

> The ring_cons local variable is used in line 903:
> https://elixir.bootlin.com/linux/v5.9-rc4/source/drivers/net/ethernet/mellanox/mlx4/en_tx.c#L903

He is removing an assignment to ring_cons much later in the function
and therefore has no effect on this line.

> 1. Your patch causes a degradation to the case when MLX4_EN_PERF_STAT
> is defined.

This is not true, see above.
Jakub Kicinski Sept. 14, 2020, 8:02 p.m. UTC | #3
On Sun, 13 Sep 2020 13:12:05 +0300 Tariq Toukan wrote:
> 2. When MLX4_EN_PERF_STAT is not defined, we should totally remove the 
> local variable declaration, not only its usage.

I was actually wondering about this when working on the pause stat
patch. Where is MLX4_EN_PERF_STAT ever defined?

$ git grep MLX4_EN_PERF_STAT
drivers/net/ethernet/mellanox/mlx4/mlx4_en.h:#ifdef MLX4_EN_PERF_STAT
drivers/net/ethernet/mellanox/mlx4/mlx4_en.h:#endif /* MLX4_EN_PERF_STAT */
drivers/net/ethernet/mellanox/mlx4/mlx4_stats.h:#ifdef MLX4_EN_PERF_STAT
Tariq Toukan Sept. 15, 2020, 12:06 p.m. UTC | #4
On 9/14/2020 11:02 PM, Jakub Kicinski wrote:
> On Sun, 13 Sep 2020 13:12:05 +0300 Tariq Toukan wrote:
>> 2. When MLX4_EN_PERF_STAT is not defined, we should totally remove the
>> local variable declaration, not only its usage.
> 
> I was actually wondering about this when working on the pause stat
> patch. Where is MLX4_EN_PERF_STAT ever defined?
> 
> $ git grep MLX4_EN_PERF_STAT
> drivers/net/ethernet/mellanox/mlx4/mlx4_en.h:#ifdef MLX4_EN_PERF_STAT
> drivers/net/ethernet/mellanox/mlx4/mlx4_en.h:#endif /* MLX4_EN_PERF_STAT */
> drivers/net/ethernet/mellanox/mlx4/mlx4_stats.h:#ifdef MLX4_EN_PERF_STAT
> 

Good point.

This was introduced long ago, since day 1 of mlx4 driver.
I believe it had off-tree usage back then, not sure though...

Anyway, I don't find it useful anymore.
Should be removed. I'll prepare a cleanup patch for net-next.

Thanks,
Tariq
diff mbox series

Patch

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
index 9dff7b0..d554344 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
@@ -1075,7 +1075,6 @@  netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
 		 */
 		smp_rmb();
 
-		ring_cons = READ_ONCE(ring->cons);
 		if (unlikely(!mlx4_en_is_tx_ring_full(ring))) {
 			netif_tx_wake_queue(ring->tx_queue);
 			ring->wake_queue++;