* [PATCH net v2] net/mlx5e: Fix race in mlx5e_sw_stats and mlx5e_vport_stats @ 2017-04-19 23:32 Martin KaFai Lau 2017-04-20 14:00 ` Saeed Mahameed 0 siblings, 1 reply; 6+ messages in thread From: Martin KaFai Lau @ 2017-04-19 23:32 UTC (permalink / raw) To: netdev; +Cc: Saeed Mahameed, Eric Dumazet, kernel-team We have observed a sudden spike in rx/tx_packets and rx/tx_bytes reported under /proc/net/dev. There is a race in mlx5e_update_stats() and some of the get-stats functions (the one that we hit is the mlx5e_get_stats() which is called by ndo_get_stats64()). In particular, the very first thing mlx5e_update_sw_counters() does is 'memset(s, 0, sizeof(*s))'. For example, if mlx5e_get_stats() is unlucky at one point, rx_bytes and rx_packets could be 0. One second later, a normal (and much bigger than 0) value will be reported. This patch is to use a 'struct mlx5e_sw_stats temp' to avoid a direct memset zero on priv->stats.sw. mlx5e_update_vport_counters() has a similar race. Hence, addressed together. I am lucky enough to catch this 0-reset in rx multicast: eth0: 41457665 76804 70 0 0 70 0 47085 15586634 87502 3 0 0 0 3 0 eth0: 41459860 76815 70 0 0 70 0 47094 15588376 87516 3 0 0 0 3 0 eth0: 41460577 76822 70 0 0 70 0 0 15589083 87521 3 0 0 0 3 0 eth0: 41463293 76838 70 0 0 70 0 47108 15595872 87538 3 0 0 0 3 0 eth0: 41463379 76839 70 0 0 70 0 47116 15596138 87539 3 0 0 0 3 0 Cc: Saeed Mahameed <saeedm@mellanox.com> Suggested-by: Eric Dumazet <eric.dumazet@gmail.com> Signed-off-by: Martin KaFai Lau <kafai@fb.com> --- drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c index 66c133757a5e..246786bb861b 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c @@ -174,7 +174,7 @@ static void mlx5e_tx_timeout_work(struct work_struct *work) static void mlx5e_update_sw_counters(struct mlx5e_priv *priv) { - struct mlx5e_sw_stats *s = &priv->stats.sw; + struct mlx5e_sw_stats temp, *s = &temp; struct mlx5e_rq_stats *rq_stats; struct mlx5e_sq_stats *sq_stats; u64 tx_offload_none = 0; @@ -229,12 +229,14 @@ static void mlx5e_update_sw_counters(struct mlx5e_priv *priv) s->link_down_events_phy = MLX5_GET(ppcnt_reg, priv->stats.pport.phy_counters, counter_set.phys_layer_cntrs.link_down_events); + memcpy(&priv->stats.sw, s, sizeof(*s)); } static void mlx5e_update_vport_counters(struct mlx5e_priv *priv) { + struct mlx5e_vport_stats temp; int outlen = MLX5_ST_SZ_BYTES(query_vport_counter_out); - u32 *out = (u32 *)priv->stats.vport.query_vport_out; + u32 *out = (u32 *)temp.query_vport_out; u32 in[MLX5_ST_SZ_DW(query_vport_counter_in)] = {0}; struct mlx5_core_dev *mdev = priv->mdev; @@ -245,6 +247,7 @@ static void mlx5e_update_vport_counters(struct mlx5e_priv *priv) memset(out, 0, outlen); mlx5_cmd_exec(mdev, in, sizeof(in), out, outlen); + memcpy(priv->stats.vport.query_vport_out, out, outlen); } static void mlx5e_update_pport_counters(struct mlx5e_priv *priv) -- 2.9.3 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net v2] net/mlx5e: Fix race in mlx5e_sw_stats and mlx5e_vport_stats 2017-04-19 23:32 [PATCH net v2] net/mlx5e: Fix race in mlx5e_sw_stats and mlx5e_vport_stats Martin KaFai Lau @ 2017-04-20 14:00 ` Saeed Mahameed 2017-04-20 14:15 ` Martin KaFai Lau 0 siblings, 1 reply; 6+ messages in thread From: Saeed Mahameed @ 2017-04-20 14:00 UTC (permalink / raw) To: Martin KaFai Lau, Gal Pressman Cc: Linux Netdev List, Saeed Mahameed, Eric Dumazet, Kernel Team On Thu, Apr 20, 2017 at 2:32 AM, Martin KaFai Lau <kafai@fb.com> wrote: > We have observed a sudden spike in rx/tx_packets and rx/tx_bytes > reported under /proc/net/dev. There is a race in mlx5e_update_stats() > and some of the get-stats functions (the one that we hit is the > mlx5e_get_stats() which is called by ndo_get_stats64()). > > In particular, the very first thing mlx5e_update_sw_counters() > does is 'memset(s, 0, sizeof(*s))'. For example, if mlx5e_get_stats() > is unlucky at one point, rx_bytes and rx_packets could be 0. One second > later, a normal (and much bigger than 0) value will be reported. > > This patch is to use a 'struct mlx5e_sw_stats temp' to avoid > a direct memset zero on priv->stats.sw. > > mlx5e_update_vport_counters() has a similar race. Hence, addressed > together. > > I am lucky enough to catch this 0-reset in rx multicast: > eth0: 41457665 76804 70 0 0 70 0 47085 15586634 87502 3 0 0 0 3 0 > eth0: 41459860 76815 70 0 0 70 0 47094 15588376 87516 3 0 0 0 3 0 > eth0: 41460577 76822 70 0 0 70 0 0 15589083 87521 3 0 0 0 3 0 > eth0: 41463293 76838 70 0 0 70 0 47108 15595872 87538 3 0 0 0 3 0 > eth0: 41463379 76839 70 0 0 70 0 47116 15596138 87539 3 0 0 0 3 0 > > Cc: Saeed Mahameed <saeedm@mellanox.com> > Suggested-by: Eric Dumazet <eric.dumazet@gmail.com> > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > --- > drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c > index 66c133757a5e..246786bb861b 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c > @@ -174,7 +174,7 @@ static void mlx5e_tx_timeout_work(struct work_struct *work) > > static void mlx5e_update_sw_counters(struct mlx5e_priv *priv) > { > - struct mlx5e_sw_stats *s = &priv->stats.sw; > + struct mlx5e_sw_stats temp, *s = &temp; > struct mlx5e_rq_stats *rq_stats; > struct mlx5e_sq_stats *sq_stats; > u64 tx_offload_none = 0; > @@ -229,12 +229,14 @@ static void mlx5e_update_sw_counters(struct mlx5e_priv *priv) > s->link_down_events_phy = MLX5_GET(ppcnt_reg, > priv->stats.pport.phy_counters, > counter_set.phys_layer_cntrs.link_down_events); > + memcpy(&priv->stats.sw, s, sizeof(*s)); > } > > static void mlx5e_update_vport_counters(struct mlx5e_priv *priv) > { > + struct mlx5e_vport_stats temp; > int outlen = MLX5_ST_SZ_BYTES(query_vport_counter_out); > - u32 *out = (u32 *)priv->stats.vport.query_vport_out; > + u32 *out = (u32 *)temp.query_vport_out; > u32 in[MLX5_ST_SZ_DW(query_vport_counter_in)] = {0}; > struct mlx5_core_dev *mdev = priv->mdev; > > @@ -245,6 +247,7 @@ static void mlx5e_update_vport_counters(struct mlx5e_priv *priv) > > memset(out, 0, outlen); Actually you don't need any temp here, it is safe to just remove this redundant memset and mlx5_cmd_exec will do the copy for you. > mlx5_cmd_exec(mdev, in, sizeof(in), out, outlen); > + memcpy(priv->stats.vport.query_vport_out, out, outlen); > } Anyway we still need a spin lock here, and also for all the counters under priv->stats which are affected by this race as well. If you want I can accept this as a temporary fix for net and Gal will work on a spin lock based mechanism to fix the memcpy race for all the counters. > > static void mlx5e_update_pport_counters(struct mlx5e_priv *priv) > -- > 2.9.3 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v2] net/mlx5e: Fix race in mlx5e_sw_stats and mlx5e_vport_stats 2017-04-20 14:00 ` Saeed Mahameed @ 2017-04-20 14:15 ` Martin KaFai Lau 2017-04-20 14:23 ` Saeed Mahameed 2017-04-20 15:00 ` Eric Dumazet 0 siblings, 2 replies; 6+ messages in thread From: Martin KaFai Lau @ 2017-04-20 14:15 UTC (permalink / raw) To: Saeed Mahameed Cc: Gal Pressman, Linux Netdev List, Saeed Mahameed, Eric Dumazet, Kernel Team On Thu, Apr 20, 2017 at 05:00:13PM +0300, Saeed Mahameed wrote: > On Thu, Apr 20, 2017 at 2:32 AM, Martin KaFai Lau <kafai@fb.com> wrote: > > We have observed a sudden spike in rx/tx_packets and rx/tx_bytes > > reported under /proc/net/dev. There is a race in mlx5e_update_stats() > > and some of the get-stats functions (the one that we hit is the > > mlx5e_get_stats() which is called by ndo_get_stats64()). > > > > In particular, the very first thing mlx5e_update_sw_counters() > > does is 'memset(s, 0, sizeof(*s))'. For example, if mlx5e_get_stats() > > is unlucky at one point, rx_bytes and rx_packets could be 0. One second > > later, a normal (and much bigger than 0) value will be reported. > > > > This patch is to use a 'struct mlx5e_sw_stats temp' to avoid > > a direct memset zero on priv->stats.sw. > > > > mlx5e_update_vport_counters() has a similar race. Hence, addressed > > together. > > > > I am lucky enough to catch this 0-reset in rx multicast: > > eth0: 41457665 76804 70 0 0 70 0 47085 15586634 87502 3 0 0 0 3 0 > > eth0: 41459860 76815 70 0 0 70 0 47094 15588376 87516 3 0 0 0 3 0 > > eth0: 41460577 76822 70 0 0 70 0 0 15589083 87521 3 0 0 0 3 0 > > eth0: 41463293 76838 70 0 0 70 0 47108 15595872 87538 3 0 0 0 3 0 > > eth0: 41463379 76839 70 0 0 70 0 47116 15596138 87539 3 0 0 0 3 0 > > > > Cc: Saeed Mahameed <saeedm@mellanox.com> > > Suggested-by: Eric Dumazet <eric.dumazet@gmail.com> > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > > --- > > drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c > > index 66c133757a5e..246786bb861b 100644 > > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c > > @@ -174,7 +174,7 @@ static void mlx5e_tx_timeout_work(struct work_struct *work) > > > > static void mlx5e_update_sw_counters(struct mlx5e_priv *priv) > > { > > - struct mlx5e_sw_stats *s = &priv->stats.sw; > > + struct mlx5e_sw_stats temp, *s = &temp; > > struct mlx5e_rq_stats *rq_stats; > > struct mlx5e_sq_stats *sq_stats; > > u64 tx_offload_none = 0; > > @@ -229,12 +229,14 @@ static void mlx5e_update_sw_counters(struct mlx5e_priv *priv) > > s->link_down_events_phy = MLX5_GET(ppcnt_reg, > > priv->stats.pport.phy_counters, > > counter_set.phys_layer_cntrs.link_down_events); > > + memcpy(&priv->stats.sw, s, sizeof(*s)); > > } > > > > static void mlx5e_update_vport_counters(struct mlx5e_priv *priv) > > { > > + struct mlx5e_vport_stats temp; > > int outlen = MLX5_ST_SZ_BYTES(query_vport_counter_out); > > - u32 *out = (u32 *)priv->stats.vport.query_vport_out; > > + u32 *out = (u32 *)temp.query_vport_out; > > u32 in[MLX5_ST_SZ_DW(query_vport_counter_in)] = {0}; > > struct mlx5_core_dev *mdev = priv->mdev; > > > > @@ -245,6 +247,7 @@ static void mlx5e_update_vport_counters(struct mlx5e_priv *priv) > > > > memset(out, 0, outlen); > > Actually you don't need any temp here, it is safe to just remove this > redundant memset > and mlx5_cmd_exec will do the copy for you. > > > mlx5_cmd_exec(mdev, in, sizeof(in), out, outlen); > > + memcpy(priv->stats.vport.query_vport_out, out, outlen); > > } > > Anyway we still need a spin lock here, and also for all the counters > under priv->stats which are affected by this race as well. > > If you want I can accept this as a temporary fix for net and Gal will > work on a spin lock based mechanism to fix the memcpy race for all the > counters. A follow-up patch approach by Gal will be nice. > > > > > static void mlx5e_update_pport_counters(struct mlx5e_priv *priv) > > -- > > 2.9.3 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v2] net/mlx5e: Fix race in mlx5e_sw_stats and mlx5e_vport_stats 2017-04-20 14:15 ` Martin KaFai Lau @ 2017-04-20 14:23 ` Saeed Mahameed 2017-04-20 15:00 ` Eric Dumazet 1 sibling, 0 replies; 6+ messages in thread From: Saeed Mahameed @ 2017-04-20 14:23 UTC (permalink / raw) To: Martin KaFai Lau Cc: Gal Pressman, Linux Netdev List, Saeed Mahameed, Eric Dumazet, Kernel Team On Thu, Apr 20, 2017 at 5:15 PM, Martin KaFai Lau <kafai@fb.com> wrote: > On Thu, Apr 20, 2017 at 05:00:13PM +0300, Saeed Mahameed wrote: >> On Thu, Apr 20, 2017 at 2:32 AM, Martin KaFai Lau <kafai@fb.com> wrote: >> > We have observed a sudden spike in rx/tx_packets and rx/tx_bytes >> > reported under /proc/net/dev. There is a race in mlx5e_update_stats() >> > and some of the get-stats functions (the one that we hit is the >> > mlx5e_get_stats() which is called by ndo_get_stats64()). >> > >> > In particular, the very first thing mlx5e_update_sw_counters() >> > does is 'memset(s, 0, sizeof(*s))'. For example, if mlx5e_get_stats() >> > is unlucky at one point, rx_bytes and rx_packets could be 0. One second >> > later, a normal (and much bigger than 0) value will be reported. >> > >> > This patch is to use a 'struct mlx5e_sw_stats temp' to avoid >> > a direct memset zero on priv->stats.sw. >> > >> > mlx5e_update_vport_counters() has a similar race. Hence, addressed >> > together. >> > >> > I am lucky enough to catch this 0-reset in rx multicast: >> > eth0: 41457665 76804 70 0 0 70 0 47085 15586634 87502 3 0 0 0 3 0 >> > eth0: 41459860 76815 70 0 0 70 0 47094 15588376 87516 3 0 0 0 3 0 >> > eth0: 41460577 76822 70 0 0 70 0 0 15589083 87521 3 0 0 0 3 0 >> > eth0: 41463293 76838 70 0 0 70 0 47108 15595872 87538 3 0 0 0 3 0 >> > eth0: 41463379 76839 70 0 0 70 0 47116 15596138 87539 3 0 0 0 3 0 >> > >> > Cc: Saeed Mahameed <saeedm@mellanox.com> >> > Suggested-by: Eric Dumazet <eric.dumazet@gmail.com> >> > Signed-off-by: Martin KaFai Lau <kafai@fb.com> >> > --- >> > drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 7 +++++-- >> > 1 file changed, 5 insertions(+), 2 deletions(-) >> > >> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c >> > index 66c133757a5e..246786bb861b 100644 >> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c >> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c >> > @@ -174,7 +174,7 @@ static void mlx5e_tx_timeout_work(struct work_struct *work) >> > >> > static void mlx5e_update_sw_counters(struct mlx5e_priv *priv) >> > { >> > - struct mlx5e_sw_stats *s = &priv->stats.sw; >> > + struct mlx5e_sw_stats temp, *s = &temp; >> > struct mlx5e_rq_stats *rq_stats; >> > struct mlx5e_sq_stats *sq_stats; >> > u64 tx_offload_none = 0; >> > @@ -229,12 +229,14 @@ static void mlx5e_update_sw_counters(struct mlx5e_priv *priv) >> > s->link_down_events_phy = MLX5_GET(ppcnt_reg, >> > priv->stats.pport.phy_counters, >> > counter_set.phys_layer_cntrs.link_down_events); >> > + memcpy(&priv->stats.sw, s, sizeof(*s)); >> > } >> > >> > static void mlx5e_update_vport_counters(struct mlx5e_priv *priv) >> > { >> > + struct mlx5e_vport_stats temp; >> > int outlen = MLX5_ST_SZ_BYTES(query_vport_counter_out); >> > - u32 *out = (u32 *)priv->stats.vport.query_vport_out; >> > + u32 *out = (u32 *)temp.query_vport_out; >> > u32 in[MLX5_ST_SZ_DW(query_vport_counter_in)] = {0}; >> > struct mlx5_core_dev *mdev = priv->mdev; >> > >> > @@ -245,6 +247,7 @@ static void mlx5e_update_vport_counters(struct mlx5e_priv *priv) >> > >> > memset(out, 0, outlen); >> >> Actually you don't need any temp here, it is safe to just remove this >> redundant memset >> and mlx5_cmd_exec will do the copy for you. >> >> > mlx5_cmd_exec(mdev, in, sizeof(in), out, outlen); >> > + memcpy(priv->stats.vport.query_vport_out, out, outlen); >> > } >> >> Anyway we still need a spin lock here, and also for all the counters >> under priv->stats which are affected by this race as well. >> >> If you want I can accept this as a temporary fix for net and Gal will >> work on a spin lock based mechanism to fix the memcpy race for all the >> counters. > A follow-up patch approach by Gal will be nice. > Ok, I will expect V3 with the removal of the memset from mlx5e_update_vport_counters instead of the memcpy. and Gal will work on the follow up patch ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v2] net/mlx5e: Fix race in mlx5e_sw_stats and mlx5e_vport_stats 2017-04-20 14:15 ` Martin KaFai Lau 2017-04-20 14:23 ` Saeed Mahameed @ 2017-04-20 15:00 ` Eric Dumazet 2017-04-20 16:50 ` Martin KaFai Lau 1 sibling, 1 reply; 6+ messages in thread From: Eric Dumazet @ 2017-04-20 15:00 UTC (permalink / raw) To: Martin KaFai Lau Cc: Saeed Mahameed, Gal Pressman, Linux Netdev List, Saeed Mahameed, Kernel Team On Thu, 2017-04-20 at 07:15 -0700, Martin KaFai Lau wrote: > A follow-up patch approach by Gal will be nice. Note that I had a similar issue on mlx4. Stats are cleared/reset at open time, but in an asynchronous way. Using a bonding device ( and fix in https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=142c6594acbcc32391af9c15f8cd65c6c177698f ) worked around the problem. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v2] net/mlx5e: Fix race in mlx5e_sw_stats and mlx5e_vport_stats 2017-04-20 15:00 ` Eric Dumazet @ 2017-04-20 16:50 ` Martin KaFai Lau 0 siblings, 0 replies; 6+ messages in thread From: Martin KaFai Lau @ 2017-04-20 16:50 UTC (permalink / raw) To: Eric Dumazet Cc: Saeed Mahameed, Gal Pressman, Linux Netdev List, Saeed Mahameed, Kernel Team On Thu, Apr 20, 2017 at 08:00:41AM -0700, Eric Dumazet wrote: > On Thu, 2017-04-20 at 07:15 -0700, Martin KaFai Lau wrote: > > > A follow-up patch approach by Gal will be nice. > > Note that I had a similar issue on mlx4. > > Stats are cleared/reset at open time, but in an asynchronous way. > > Using a bonding device ( and fix in > https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=142c6594acbcc32391af9c15f8cd65c6c177698f ) worked around the problem. We also drop the X->0 delta now but does not drop the 0->Y delta. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-04-20 16:50 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-04-19 23:32 [PATCH net v2] net/mlx5e: Fix race in mlx5e_sw_stats and mlx5e_vport_stats Martin KaFai Lau 2017-04-20 14:00 ` Saeed Mahameed 2017-04-20 14:15 ` Martin KaFai Lau 2017-04-20 14:23 ` Saeed Mahameed 2017-04-20 15:00 ` Eric Dumazet 2017-04-20 16:50 ` Martin KaFai Lau
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox