| 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 |
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
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.
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
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 --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++;
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(-)