| Message ID | 1388619628-3373-5-git-send-email-sd@queasysnail.net |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Sabrina Dubroca <sd@queasysnail.net> Date: Thu, 2 Jan 2014 00:40:27 +0100 > + spin_lock(&alx->stats_lock); > + > + __alx_update_hw_stats(&alx->hw); Please use something like the linux/u64_stats_sync.h stuff rather than spin locking. Thank you. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2014-01-01 at 22:27 -0500, David Miller wrote: > From: Sabrina Dubroca <sd@queasysnail.net> > Date: Thu, 2 Jan 2014 00:40:27 +0100 > > > + spin_lock(&alx->stats_lock); > > + > > + __alx_update_hw_stats(&alx->hw); > > Please use something like the linux/u64_stats_sync.h stuff rather > than spin locking. I don't think that'sa useful, as we can have multiple writers (concurrent calls to get_stats). And there is really no harm in using a spinlock to serialise get_stats calls. The u64_stats API is good for statistics updated from the data path. Ben.
On Thu, 2014-01-02 at 00:40 +0100, Sabrina Dubroca wrote: [...] > --- a/drivers/net/ethernet/atheros/alx/main.c > +++ b/drivers/net/ethernet/atheros/alx/main.c > @@ -1166,10 +1166,54 @@ static void alx_poll_controller(struct net_device *netdev) > } > #endif > > +static struct net_device_stats *alx_get_stats(struct net_device *netdev) [...] > static const struct net_device_ops alx_netdev_ops = { > .ndo_open = alx_open, > .ndo_stop = alx_stop, > .ndo_start_xmit = alx_start_xmit, > + .ndo_get_stats = alx_get_stats, [...] You should implement ndo_get_stats64 rather than ndo_get_stats. Ben.
[2014-01-02, 11:56:53] Ben Hutchings wrote: > On Wed, 2014-01-01 at 22:27 -0500, David Miller wrote: > > From: Sabrina Dubroca <sd@queasysnail.net> > > Date: Thu, 2 Jan 2014 00:40:27 +0100 > > > > > + spin_lock(&alx->stats_lock); > > > + > > > + __alx_update_hw_stats(&alx->hw); > > > > Please use something like the linux/u64_stats_sync.h stuff rather > > than spin locking. > > I don't think that'sa useful, as we can have multiple writers > (concurrent calls to get_stats). And there is really no harm in using a > spinlock to serialise get_stats calls. The u64_stats API is good for > statistics updated from the data path. I've read the comments in linux/u64_stats_sync.h, which mentions the need for exclusive access to the data. I've looked at other drivers (broadcom/b44.c, nvidia/forcedeth.c) that use the u64_stats functions to get stats from hardware, and they use a spin_lock around the update code. The other drivers that I've looked at and that use u64_stats have software stats that they update on rx/tx, so I think the situation is a bit different. For now I've added u64_stats and modified the functions this way: static struct net_device_stats *alx_get_stats(struct net_device *netdev) { struct alx_priv *alx = netdev_priv(netdev); struct net_device_stats *net_stats = &netdev->stats; struct alx_hw_stats *hw_stats = &alx->hw.stats; unsigned int start; spin_lock(&alx->stats_lock); __alx_update_hw_stats(&alx->hw); spin_unlock(&alx->stats_lock); do { start = u64_stats_fetch_begin(&hw_stats->syncp); /* fill net_stat... */ } while (u64_stats_fetch_retry(&hw_stats->syncp, start)); return net_stats; } void __alx_update_hw_stats(struct alx_hw *hw) { u64_stats_update_begin(&hw->stats.syncp); /* fill hw->stats */ u64_stats_update_end(&hw->stats.syncp); } static void alx_get_ethtool_stats(struct net_device *netdev, struct ethtool_stats *estats, u64 *data) { struct alx_priv *alx = netdev_priv(netdev); struct alx_hw *hw = &alx->hw; unsigned int start; spin_lock(&alx->stats_lock); __alx_update_hw_stats(hw); spin_unlock(&alx->stats_lock); do { start = u64_stats_fetch_begin(&hw->stats.syncp); memcpy(data, &hw->stats, sizeof(hw->stats)); } while (u64_stats_fetch_retry(&hw->stats.syncp, start)); } Thanks,
On Thu, 2014-01-02 at 17:05 +0100, Sabrina Dubroca wrote: > [2014-01-02, 11:56:53] Ben Hutchings wrote: > > On Wed, 2014-01-01 at 22:27 -0500, David Miller wrote: > > > From: Sabrina Dubroca <sd@queasysnail.net> > > > Date: Thu, 2 Jan 2014 00:40:27 +0100 > > > > > > > + spin_lock(&alx->stats_lock); > > > > + > > > > + __alx_update_hw_stats(&alx->hw); > > > > > > Please use something like the linux/u64_stats_sync.h stuff rather > > > than spin locking. > > > > I don't think that'sa useful, as we can have multiple writers > > (concurrent calls to get_stats). And there is really no harm in using a > > spinlock to serialise get_stats calls. The u64_stats API is good for > > statistics updated from the data path. > > I've read the comments in linux/u64_stats_sync.h, which mentions the > need for exclusive access to the data. I've looked at other drivers > (broadcom/b44.c, nvidia/forcedeth.c) that use the u64_stats functions > to get stats from hardware, and they use a spin_lock around the update > code. The other drivers that I've looked at and that use u64_stats > have software stats that they update on rx/tx, so I think the > situation is a bit different. > > > For now I've added u64_stats and modified the functions this way: [...] You could do that but I think your original version was fine. Of course it is David's decision in the end. Ben.
From: Ben Hutchings <bhutchings@solarflare.com> Date: Thu, 2 Jan 2014 11:56:53 +0000 > On Wed, 2014-01-01 at 22:27 -0500, David Miller wrote: >> From: Sabrina Dubroca <sd@queasysnail.net> >> Date: Thu, 2 Jan 2014 00:40:27 +0100 >> >> > + spin_lock(&alx->stats_lock); >> > + >> > + __alx_update_hw_stats(&alx->hw); >> >> Please use something like the linux/u64_stats_sync.h stuff rather >> than spin locking. > > I don't think that'sa useful, as we can have multiple writers > (concurrent calls to get_stats). And there is really no harm in using a > spinlock to serialise get_stats calls. The u64_stats API is good for > statistics updated from the data path. Fair enough. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/atheros/alx/alx.h b/drivers/net/ethernet/atheros/alx/alx.h index d71103d..8fc93c5 100644 --- a/drivers/net/ethernet/atheros/alx/alx.h +++ b/drivers/net/ethernet/atheros/alx/alx.h @@ -106,6 +106,9 @@ struct alx_priv { u16 msg_enable; bool msi; + + /* protects hw.stats */ + spinlock_t stats_lock; }; extern const struct ethtool_ops alx_ethtool_ops; diff --git a/drivers/net/ethernet/atheros/alx/main.c b/drivers/net/ethernet/atheros/alx/main.c index c3c4c26..d825e38 100644 --- a/drivers/net/ethernet/atheros/alx/main.c +++ b/drivers/net/ethernet/atheros/alx/main.c @@ -1166,10 +1166,54 @@ static void alx_poll_controller(struct net_device *netdev) } #endif +static struct net_device_stats *alx_get_stats(struct net_device *netdev) +{ + struct alx_priv *alx = netdev_priv(netdev); + struct net_device_stats *net_stats = &netdev->stats; + struct alx_hw_stats *hw_stats = &alx->hw.stats; + + spin_lock(&alx->stats_lock); + + __alx_update_hw_stats(&alx->hw); + + net_stats->tx_packets = hw_stats->tx_ok; + net_stats->tx_bytes = hw_stats->tx_byte_cnt; + net_stats->rx_packets = hw_stats->rx_ok; + net_stats->rx_bytes = hw_stats->rx_byte_cnt; + net_stats->multicast = hw_stats->rx_mcast; + net_stats->collisions = hw_stats->tx_single_col + + hw_stats->tx_multi_col * 2 + + hw_stats->tx_late_col + hw_stats->tx_abort_col; + + net_stats->rx_errors = hw_stats->rx_frag + hw_stats->rx_fcs_err + + hw_stats->rx_len_err + hw_stats->rx_ov_sz + + hw_stats->rx_ov_rrd + hw_stats->rx_align_err; + + net_stats->rx_fifo_errors = hw_stats->rx_ov_rxf; + net_stats->rx_length_errors = hw_stats->rx_len_err; + net_stats->rx_crc_errors = hw_stats->rx_fcs_err; + net_stats->rx_frame_errors = hw_stats->rx_align_err; + net_stats->rx_over_errors = hw_stats->rx_ov_rrd + hw_stats->rx_ov_rxf; + + net_stats->rx_missed_errors = hw_stats->rx_ov_rrd + hw_stats->rx_ov_rxf; + + net_stats->tx_errors = hw_stats->tx_late_col + hw_stats->tx_abort_col + + hw_stats->tx_underrun + hw_stats->tx_trunc; + + net_stats->tx_aborted_errors = hw_stats->tx_abort_col; + net_stats->tx_fifo_errors = hw_stats->tx_underrun; + net_stats->tx_window_errors = hw_stats->tx_late_col; + + spin_unlock(&alx->stats_lock); + + return net_stats; +} + static const struct net_device_ops alx_netdev_ops = { .ndo_open = alx_open, .ndo_stop = alx_stop, .ndo_start_xmit = alx_start_xmit, + .ndo_get_stats = alx_get_stats, .ndo_set_rx_mode = alx_set_rx_mode, .ndo_validate_addr = eth_validate_addr, .ndo_set_mac_address = alx_set_mac_address,
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net> --- drivers/net/ethernet/atheros/alx/alx.h | 3 +++ drivers/net/ethernet/atheros/alx/main.c | 44 +++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+)