* [RFC PATCH net] net/mlx5e: Race between mlx5e_update_stats() and getting the stats
@ 2017-04-19 18:29 Martin KaFai Lau
2017-04-19 20:24 ` Eric Dumazet
0 siblings, 1 reply; 5+ messages in thread
From: Martin KaFai Lau @ 2017-04-19 18:29 UTC (permalink / raw)
To: netdev; +Cc: Saeed Mahameed, kernel-team
We have observed a sudden spike in rx/tx_packets and rx/tx_bytes
reported under /proc/net/dev. It seems 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 not meant to be a proper fix. It merely tries
to show what I have suspected and start the discussion.
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
Cc: Saeed Mahameed <saeedm@mellanox.com>
---
drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c | 7 +++++--
drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 3 +++
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
index a004a5a1a4c2..d24916f720bb 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
@@ -313,7 +313,6 @@ static void mlx5e_get_ethtool_stats(struct net_device *dev,
mutex_lock(&priv->state_lock);
if (test_bit(MLX5E_STATE_OPENED, &priv->state))
mlx5e_update_stats(priv);
- mutex_unlock(&priv->state_lock);
for (i = 0; i < NUM_SW_COUNTERS; i++)
data[idx++] = MLX5E_READ_CTR64_CPU(&priv->stats.sw,
@@ -378,8 +377,10 @@ static void mlx5e_get_ethtool_stats(struct net_device *dev,
data[idx++] = MLX5E_READ_CTR64_CPU(mlx5_priv->pme_stats.error_counters,
mlx5e_pme_error_desc, i);
- if (!test_bit(MLX5E_STATE_OPENED, &priv->state))
+ if (!test_bit(MLX5E_STATE_OPENED, &priv->state)) {
+ mutex_unlock(&priv->state_lock);
return;
+ }
/* per channel counters */
for (i = 0; i < priv->params.num_channels; i++)
@@ -393,6 +394,8 @@ static void mlx5e_get_ethtool_stats(struct net_device *dev,
for (j = 0; j < NUM_SQ_STATS; j++)
data[idx++] = MLX5E_READ_CTR64_CPU(&priv->channel[i]->sq[tc].stats,
sq_stats_desc, j);
+
+ mutex_unlock(&priv->state_lock);
}
static u32 mlx5e_rx_wqes_to_packets(struct mlx5e_priv *priv, int rq_wq_type,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index 66c133757a5e..a4c100bea541 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -2748,6 +2748,8 @@ mlx5e_get_stats(struct net_device *dev, struct rtnl_link_stats64 *stats)
struct mlx5e_vport_stats *vstats = &priv->stats.vport;
struct mlx5e_pport_stats *pstats = &priv->stats.pport;
+ mutex_lock(&priv->state_lock);
+
if (mlx5e_is_uplink_rep(priv)) {
stats->rx_packets = PPORT_802_3_GET(pstats, a_frames_received_ok);
stats->rx_bytes = PPORT_802_3_GET(pstats, a_octets_received_ok);
@@ -2783,6 +2785,7 @@ mlx5e_get_stats(struct net_device *dev, struct rtnl_link_stats64 *stats)
stats->multicast =
VPORT_COUNTER_GET(vstats, received_eth_multicast.packets);
+ mutex_unlock(&priv->state_lock);
}
static void mlx5e_set_rx_mode(struct net_device *dev)
--
2.9.3
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [RFC PATCH net] net/mlx5e: Race between mlx5e_update_stats() and getting the stats
2017-04-19 18:29 [RFC PATCH net] net/mlx5e: Race between mlx5e_update_stats() and getting the stats Martin KaFai Lau
@ 2017-04-19 20:24 ` Eric Dumazet
2017-04-19 21:53 ` Martin KaFai Lau
0 siblings, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2017-04-19 20:24 UTC (permalink / raw)
To: Martin KaFai Lau; +Cc: netdev, Saeed Mahameed, kernel-team
On Wed, 2017-04-19 at 11:29 -0700, Martin KaFai Lau wrote:
> We have observed a sudden spike in rx/tx_packets and rx/tx_bytes
> reported under /proc/net/dev. It seems 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 not meant to be a proper fix. It merely tries
> to show what I have suspected and start the discussion.
>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> Cc: Saeed Mahameed <saeedm@mellanox.com>
> ---
> drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c | 7 +++++--
> drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 3 +++
> 2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> index a004a5a1a4c2..d24916f720bb 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> @@ -313,7 +313,6 @@ static void mlx5e_get_ethtool_stats(struct net_device *dev,
> mutex_lock(&priv->state_lock);
> if (test_bit(MLX5E_STATE_OPENED, &priv->state))
> mlx5e_update_stats(priv);
> - mutex_unlock(&priv->state_lock);
>
> for (i = 0; i < NUM_SW_COUNTERS; i++)
> data[idx++] = MLX5E_READ_CTR64_CPU(&priv->stats.sw,
> @@ -378,8 +377,10 @@ static void mlx5e_get_ethtool_stats(struct net_device *dev,
> data[idx++] = MLX5E_READ_CTR64_CPU(mlx5_priv->pme_stats.error_counters,
> mlx5e_pme_error_desc, i);
>
> - if (!test_bit(MLX5E_STATE_OPENED, &priv->state))
> + if (!test_bit(MLX5E_STATE_OPENED, &priv->state)) {
> + mutex_unlock(&priv->state_lock);
> return;
> + }
>
> /* per channel counters */
> for (i = 0; i < priv->params.num_channels; i++)
> @@ -393,6 +394,8 @@ static void mlx5e_get_ethtool_stats(struct net_device *dev,
> for (j = 0; j < NUM_SQ_STATS; j++)
> data[idx++] = MLX5E_READ_CTR64_CPU(&priv->channel[i]->sq[tc].stats,
> sq_stats_desc, j);
> +
> + mutex_unlock(&priv->state_lock);
> }
>
> static u32 mlx5e_rx_wqes_to_packets(struct mlx5e_priv *priv, int rq_wq_type,
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index 66c133757a5e..a4c100bea541 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> @@ -2748,6 +2748,8 @@ mlx5e_get_stats(struct net_device *dev, struct rtnl_link_stats64 *stats)
> struct mlx5e_vport_stats *vstats = &priv->stats.vport;
> struct mlx5e_pport_stats *pstats = &priv->stats.pport;
>
> + mutex_lock(&priv->state_lock);
> +
We can not sleep from ndo_get_stats() ( look at bonding driver )
What about the following ?
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index 66c133757a5ee8daae122e93322306b1c5c44336..b9fea146a0ca18498a8dfa5698dca7dea06e3c5e 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,6 +229,8 @@ 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);
+ /* A bit racy (depending on memcpy() sanity...) , we probably should use a spinlock */
+ memcpy(&priv->stats.sw, s, sizeof(*s));
}
static void mlx5e_update_vport_counters(struct mlx5e_priv *priv)
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [RFC PATCH net] net/mlx5e: Race between mlx5e_update_stats() and getting the stats
2017-04-19 20:24 ` Eric Dumazet
@ 2017-04-19 21:53 ` Martin KaFai Lau
2017-04-19 23:35 ` Eric Dumazet
0 siblings, 1 reply; 5+ messages in thread
From: Martin KaFai Lau @ 2017-04-19 21:53 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, Saeed Mahameed, kernel-team
On Wed, Apr 19, 2017 at 01:24:38PM -0700, Eric Dumazet wrote:
> On Wed, 2017-04-19 at 11:29 -0700, Martin KaFai Lau wrote:
> > We have observed a sudden spike in rx/tx_packets and rx/tx_bytes
> > reported under /proc/net/dev. It seems 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 not meant to be a proper fix. It merely tries
> > to show what I have suspected and start the discussion.
> >
> > Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> > Cc: Saeed Mahameed <saeedm@mellanox.com>
> > ---
> > drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c | 7 +++++--
> > drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 3 +++
> > 2 files changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> > index a004a5a1a4c2..d24916f720bb 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> > @@ -313,7 +313,6 @@ static void mlx5e_get_ethtool_stats(struct net_device *dev,
> > mutex_lock(&priv->state_lock);
> > if (test_bit(MLX5E_STATE_OPENED, &priv->state))
> > mlx5e_update_stats(priv);
> > - mutex_unlock(&priv->state_lock);
> >
> > for (i = 0; i < NUM_SW_COUNTERS; i++)
> > data[idx++] = MLX5E_READ_CTR64_CPU(&priv->stats.sw,
> > @@ -378,8 +377,10 @@ static void mlx5e_get_ethtool_stats(struct net_device *dev,
> > data[idx++] = MLX5E_READ_CTR64_CPU(mlx5_priv->pme_stats.error_counters,
> > mlx5e_pme_error_desc, i);
> >
> > - if (!test_bit(MLX5E_STATE_OPENED, &priv->state))
> > + if (!test_bit(MLX5E_STATE_OPENED, &priv->state)) {
> > + mutex_unlock(&priv->state_lock);
> > return;
> > + }
> >
> > /* per channel counters */
> > for (i = 0; i < priv->params.num_channels; i++)
> > @@ -393,6 +394,8 @@ static void mlx5e_get_ethtool_stats(struct net_device *dev,
> > for (j = 0; j < NUM_SQ_STATS; j++)
> > data[idx++] = MLX5E_READ_CTR64_CPU(&priv->channel[i]->sq[tc].stats,
> > sq_stats_desc, j);
> > +
> > + mutex_unlock(&priv->state_lock);
> > }
> >
> > static u32 mlx5e_rx_wqes_to_packets(struct mlx5e_priv *priv, int rq_wq_type,
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > index 66c133757a5e..a4c100bea541 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > @@ -2748,6 +2748,8 @@ mlx5e_get_stats(struct net_device *dev, struct rtnl_link_stats64 *stats)
> > struct mlx5e_vport_stats *vstats = &priv->stats.vport;
> > struct mlx5e_pport_stats *pstats = &priv->stats.pport;
> >
> > + mutex_lock(&priv->state_lock);
> > +
>
> We can not sleep from ndo_get_stats() ( look at bonding driver )
Thanks for pointing out the bond_get_stats().
>
> What about the following ?
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index 66c133757a5ee8daae122e93322306b1c5c44336..b9fea146a0ca18498a8dfa5698dca7dea06e3c5e 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,6 +229,8 @@ 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);
> + /* A bit racy (depending on memcpy() sanity...) , we probably should use a spinlock */
> + memcpy(&priv->stats.sw, s, sizeof(*s));
Right, a temp and a memcpy should be enough to solve our spike problem.
It may be the right fix for net.
Agree that using a spinlock is better (likely changing state_lock
to spinlock). A quick grep shows 80 line changes. Saeed, thoughts?
> }
>
> static void mlx5e_update_vport_counters(struct mlx5e_priv *priv)
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [RFC PATCH net] net/mlx5e: Race between mlx5e_update_stats() and getting the stats
2017-04-19 21:53 ` Martin KaFai Lau
@ 2017-04-19 23:35 ` Eric Dumazet
2017-04-20 13:22 ` Saeed Mahameed
0 siblings, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2017-04-19 23:35 UTC (permalink / raw)
To: Martin KaFai Lau; +Cc: netdev, Saeed Mahameed, kernel-team
On Wed, 2017-04-19 at 14:53 -0700, Martin KaFai Lau wrote:
> Right, a temp and a memcpy should be enough to solve our spike problem.
> It may be the right fix for net.
>
> Agree that using a spinlock is better (likely changing state_lock
> to spinlock). A quick grep shows 80 line changes. Saeed, thoughts?
I was not advising replacing the mutex (maybe it is a mutex for good
reason), I simply suggested to use another spinlock only for this very
specific section.
Something like :
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en.h b/drivers/net/ethernet/mellanox/mlx5/core/en.h
index dc52053128bc752ccd398449330c24c0bdf8b3a1..9b2e1b79fded22d55e9409cb572308190679cfdd 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en.h
@@ -722,6 +722,7 @@ struct mlx5e_priv {
struct mlx5_core_dev *mdev;
struct net_device *netdev;
struct mlx5e_stats stats;
+ spinlock_t stats_lock;
struct mlx5e_tstamp tstamp;
u16 q_counter;
#ifdef CONFIG_MLX5_CORE_EN_DCB
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
index a004a5a1a4c22a742ef3f9939769c6b5c9445f46..b4b7d43bf899cadca2c2a17151d35acac9773859 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
@@ -315,9 +315,11 @@ static void mlx5e_get_ethtool_stats(struct net_device *dev,
mlx5e_update_stats(priv);
mutex_unlock(&priv->state_lock);
+ spin_lock(&priv->stats_lock);
for (i = 0; i < NUM_SW_COUNTERS; i++)
data[idx++] = MLX5E_READ_CTR64_CPU(&priv->stats.sw,
sw_stats_desc, i);
+ spin_unlock(&priv->stats_lock);
for (i = 0; i < MLX5E_NUM_Q_CNTRS(priv); i++)
data[idx++] = MLX5E_READ_CTR32_CPU(&priv->stats.qcnt,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index 66c133757a5ee8daae122e93322306b1c5c44336..4d6672045b1126a8bab4d6f2035e6a9b830560d2 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,6 +229,9 @@ 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);
+ spin_lock(&priv->stats_lock);
+ memcpy(&priv->stats.sw, s, sizeof(*s));
+ spin_unlock(&priv->stats_lock);
}
static void mlx5e_update_vport_counters(struct mlx5e_priv *priv)
@@ -2754,11 +2757,13 @@ mlx5e_get_stats(struct net_device *dev, struct rtnl_link_stats64 *stats)
stats->tx_packets = PPORT_802_3_GET(pstats, a_frames_transmitted_ok);
stats->tx_bytes = PPORT_802_3_GET(pstats, a_octets_transmitted_ok);
} else {
+ spin_lock(&priv->stats_lock);
stats->rx_packets = sstats->rx_packets;
stats->rx_bytes = sstats->rx_bytes;
stats->tx_packets = sstats->tx_packets;
stats->tx_bytes = sstats->tx_bytes;
stats->tx_dropped = sstats->tx_queue_dropped;
+ spin_unlock(&priv->stats_lock);
}
stats->rx_dropped = priv->stats.qcnt.rx_out_of_buffer;
@@ -3561,6 +3566,8 @@ static void mlx5e_build_nic_netdev_priv(struct mlx5_core_dev *mdev,
mutex_init(&priv->state_lock);
+ spin_lock_init(&priv->stats_lock);
+
INIT_WORK(&priv->update_carrier_work, mlx5e_update_carrier_work);
INIT_WORK(&priv->set_rx_mode_work, mlx5e_set_rx_mode_work);
INIT_WORK(&priv->tx_timeout_work, mlx5e_tx_timeout_work);
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [RFC PATCH net] net/mlx5e: Race between mlx5e_update_stats() and getting the stats
2017-04-19 23:35 ` Eric Dumazet
@ 2017-04-20 13:22 ` Saeed Mahameed
0 siblings, 0 replies; 5+ messages in thread
From: Saeed Mahameed @ 2017-04-20 13:22 UTC (permalink / raw)
To: Eric Dumazet
Cc: Martin KaFai Lau, Linux Netdev List, Saeed Mahameed, Kernel Team
On Thu, Apr 20, 2017 at 2:35 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Wed, 2017-04-19 at 14:53 -0700, Martin KaFai Lau wrote:
>
>> Right, a temp and a memcpy should be enough to solve our spike problem.
>> It may be the right fix for net.
>>
>> Agree that using a spinlock is better (likely changing state_lock
>> to spinlock). A quick grep shows 80 line changes. Saeed, thoughts?
No, changing the current state_lock is a bad idea, as Eric said it is
for a reason,
to synchronize between arbitrary ring/device state changes which might sleep.
memcpy is a good idea to better hide or delay the issue :),
new dedicated spin lock is the right way to go, As Eric suggested below.
BTW, very nice catch Martin, I just got back from vacation to work on this bug,
and you already root caused it, Thanks !!
>
> I was not advising replacing the mutex (maybe it is a mutex for good
> reason), I simply suggested to use another spinlock only for this very
> specific section.
>
> Something like :
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en.h b/drivers/net/ethernet/mellanox/mlx5/core/en.h
> index dc52053128bc752ccd398449330c24c0bdf8b3a1..9b2e1b79fded22d55e9409cb572308190679cfdd 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en.h
> @@ -722,6 +722,7 @@ struct mlx5e_priv {
> struct mlx5_core_dev *mdev;
> struct net_device *netdev;
> struct mlx5e_stats stats;
> + spinlock_t stats_lock;
> struct mlx5e_tstamp tstamp;
> u16 q_counter;
> #ifdef CONFIG_MLX5_CORE_EN_DCB
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> index a004a5a1a4c22a742ef3f9939769c6b5c9445f46..b4b7d43bf899cadca2c2a17151d35acac9773859 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> @@ -315,9 +315,11 @@ static void mlx5e_get_ethtool_stats(struct net_device *dev,
> mlx5e_update_stats(priv);
> mutex_unlock(&priv->state_lock);
>
> + spin_lock(&priv->stats_lock);
> for (i = 0; i < NUM_SW_COUNTERS; i++)
> data[idx++] = MLX5E_READ_CTR64_CPU(&priv->stats.sw,
> sw_stats_desc, i);
> + spin_unlock(&priv->stats_lock);
>
> for (i = 0; i < MLX5E_NUM_Q_CNTRS(priv); i++)
> data[idx++] = MLX5E_READ_CTR32_CPU(&priv->stats.qcnt,
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index 66c133757a5ee8daae122e93322306b1c5c44336..4d6672045b1126a8bab4d6f2035e6a9b830560d2 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,6 +229,9 @@ 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);
> + spin_lock(&priv->stats_lock);
> + memcpy(&priv->stats.sw, s, sizeof(*s));
> + spin_unlock(&priv->stats_lock);
I like this ! minimized the critical section with a temp buffer and a
memcpy .. perfect.
> }
>
> static void mlx5e_update_vport_counters(struct mlx5e_priv *priv)
> @@ -2754,11 +2757,13 @@ mlx5e_get_stats(struct net_device *dev, struct rtnl_link_stats64 *stats)
> stats->tx_packets = PPORT_802_3_GET(pstats, a_frames_transmitted_ok);
> stats->tx_bytes = PPORT_802_3_GET(pstats, a_octets_transmitted_ok);
> } else {
> + spin_lock(&priv->stats_lock);
> stats->rx_packets = sstats->rx_packets;
> stats->rx_bytes = sstats->rx_bytes;
> stats->tx_packets = sstats->tx_packets;
> stats->tx_bytes = sstats->tx_bytes;
> stats->tx_dropped = sstats->tx_queue_dropped;
> + spin_unlock(&priv->stats_lock);
> }
>
> stats->rx_dropped = priv->stats.qcnt.rx_out_of_buffer;
> @@ -3561,6 +3566,8 @@ static void mlx5e_build_nic_netdev_priv(struct mlx5_core_dev *mdev,
>
> mutex_init(&priv->state_lock);
>
> + spin_lock_init(&priv->stats_lock);
> +
> INIT_WORK(&priv->update_carrier_work, mlx5e_update_carrier_work);
> INIT_WORK(&priv->set_rx_mode_work, mlx5e_set_rx_mode_work);
> INIT_WORK(&priv->tx_timeout_work, mlx5e_tx_timeout_work);
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-04-20 13:22 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-04-19 18:29 [RFC PATCH net] net/mlx5e: Race between mlx5e_update_stats() and getting the stats Martin KaFai Lau
2017-04-19 20:24 ` Eric Dumazet
2017-04-19 21:53 ` Martin KaFai Lau
2017-04-19 23:35 ` Eric Dumazet
2017-04-20 13:22 ` Saeed Mahameed
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox