2020-09-12 08:12:13

by Luo Jiaxing

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

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 <[email protected]>
---
drivers/net/ethernet/mellanox/mlx4/en_tx.c | 1 -
1 file changed, 1 deletion(-)

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++;
--
2.7.4


2020-09-13 01:24:31

by David Miller

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

From: Luo Jiaxing <[email protected]>
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 <[email protected]>

Looks good, applied, thanks.

2020-09-13 10:13:30

by Tariq Toukan

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



On 9/13/2020 4:22 AM, David Miller wrote:
> From: Luo Jiaxing <[email protected]>
> 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 <[email protected]>
>
> 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

2020-09-13 21:34:08

by David Miller

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

From: Tariq Toukan <[email protected]>
Date: Sun, 13 Sep 2020 13:12:05 +0300

>
>
> On 9/13/2020 4:22 AM, David Miller wrote:
>> From: Luo Jiaxing <[email protected]>
>> 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 <[email protected]>
>> 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.

2020-09-14 20:04:16

by Jakub Kicinski

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

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

2020-09-15 12:15:32

by Tariq Toukan

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



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