* [PATCH net-next] net: ethernet: mlx4: Avoid assigning a value to ring_cons but not used it anymore in mlx4_en_xmit()
@ 2020-09-12 8:08 Luo Jiaxing
2020-09-13 1:22 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: Luo Jiaxing @ 2020-09-12 8:08 UTC (permalink / raw)
To: davem, kuba, idos, ogerlitz; +Cc: netdev, linux-kernel, linuxarm
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(-)
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: ethernet: mlx4: Avoid assigning a value to ring_cons but not used it anymore in mlx4_en_xmit()
2020-09-12 8:08 [PATCH net-next] net: ethernet: mlx4: Avoid assigning a value to ring_cons but not used it anymore in mlx4_en_xmit() Luo Jiaxing
@ 2020-09-13 1:22 ` David Miller
2020-09-13 10:12 ` Tariq Toukan
0 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2020-09-13 1:22 UTC (permalink / raw)
To: luojiaxing; +Cc: kuba, idos, ogerlitz, netdev, linux-kernel, linuxarm
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: ethernet: mlx4: Avoid assigning a value to ring_cons but not used it anymore in mlx4_en_xmit()
2020-09-13 1:22 ` David Miller
@ 2020-09-13 10:12 ` Tariq Toukan
2020-09-13 21:33 ` David Miller
2020-09-14 20:02 ` Jakub Kicinski
0 siblings, 2 replies; 6+ messages in thread
From: Tariq Toukan @ 2020-09-13 10:12 UTC (permalink / raw)
To: David Miller, luojiaxing
Cc: kuba, idos, ogerlitz, netdev, linux-kernel, linuxarm
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: ethernet: mlx4: Avoid assigning a value to ring_cons but not used it anymore in mlx4_en_xmit()
2020-09-13 10:12 ` Tariq Toukan
@ 2020-09-13 21:33 ` David Miller
2020-09-14 20:02 ` Jakub Kicinski
1 sibling, 0 replies; 6+ messages in thread
From: David Miller @ 2020-09-13 21:33 UTC (permalink / raw)
To: ttoukan.linux
Cc: luojiaxing, kuba, idos, ogerlitz, netdev, linux-kernel, linuxarm
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: ethernet: mlx4: Avoid assigning a value to ring_cons but not used it anymore in mlx4_en_xmit()
2020-09-13 10:12 ` Tariq Toukan
2020-09-13 21:33 ` David Miller
@ 2020-09-14 20:02 ` Jakub Kicinski
2020-09-15 12:06 ` Tariq Toukan
1 sibling, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2020-09-14 20:02 UTC (permalink / raw)
To: Tariq Toukan
Cc: David Miller, luojiaxing, idos, ogerlitz, netdev, linux-kernel, linuxarm
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: ethernet: mlx4: Avoid assigning a value to ring_cons but not used it anymore in mlx4_en_xmit()
2020-09-14 20:02 ` Jakub Kicinski
@ 2020-09-15 12:06 ` Tariq Toukan
0 siblings, 0 replies; 6+ messages in thread
From: Tariq Toukan @ 2020-09-15 12:06 UTC (permalink / raw)
To: Jakub Kicinski
Cc: David Miller, luojiaxing, idos, ogerlitz, netdev, linux-kernel, linuxarm
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-09-15 12:11 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-12 8:08 [PATCH net-next] net: ethernet: mlx4: Avoid assigning a value to ring_cons but not used it anymore in mlx4_en_xmit() Luo Jiaxing
2020-09-13 1:22 ` David Miller
2020-09-13 10:12 ` Tariq Toukan
2020-09-13 21:33 ` David Miller
2020-09-14 20:02 ` Jakub Kicinski
2020-09-15 12:06 ` Tariq Toukan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).