| Message ID | 1278321242.2877.18.camel@edumazet-laptop |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
Le lundi 05 juillet 2010 à 11:14 +0200, Eric Dumazet a écrit : > After commit be1f3c2c027c (net: Enable 64-bit net device statistics on > 32-bit architectures), we can now provide 64bit stats, even on 32bit > arches. > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> > --- Please disregard this patch. There is small possibility a reader might read a 64bit value while another writer makes a change to it, changing high 32bit value. A change in core network would be needed to make this 100% safe, possibly using a seqlock to protect dev->stats64 -- 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 Mon, 2010-07-05 at 18:03 +0200, Eric Dumazet wrote: > Le lundi 05 juillet 2010 à 11:14 +0200, Eric Dumazet a écrit : > > After commit be1f3c2c027c (net: Enable 64-bit net device statistics on > > 32-bit architectures), we can now provide 64bit stats, even on 32bit > > arches. > > > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> > > --- > > Please disregard this patch. > > There is small possibility a reader might read a 64bit value while > another writer makes a change to it, changing high 32bit value. > > A change in core network would be needed to make this 100% safe, > possibly using a seqlock to protect dev->stats64 I really didn't want to add this overhead and complication to readers when only some drivers need it. Ben.
Le lundi 05 juillet 2010 à 18:31 +0100, Ben Hutchings a écrit : > On Mon, 2010-07-05 at 18:03 +0200, Eric Dumazet wrote: > > Le lundi 05 juillet 2010 à 11:14 +0200, Eric Dumazet a écrit : > > > After commit be1f3c2c027c (net: Enable 64-bit net device statistics on > > > 32-bit architectures), we can now provide 64bit stats, even on 32bit > > > arches. > > > > > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> > > > --- > > > > Please disregard this patch. > > > > There is small possibility a reader might read a 64bit value while > > another writer makes a change to it, changing high 32bit value. > > > > A change in core network would be needed to make this 100% safe, > > possibly using a seqlock to protect dev->stats64 > > I really didn't want to add this overhead and complication to readers > when only some drivers need it. > Overhead would be minimal, only in get_stats() method and only for drivers that want to provide 64 bit stats... Clearly, rx/tx path must not have any overhead. Right now, even RTNL 64bit stats are not safe. Should we revert prior patches or try to make progress ? -- 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 Mon, 2010-07-05 at 20:07 +0200, Eric Dumazet wrote: > Le lundi 05 juillet 2010 à 18:31 +0100, Ben Hutchings a écrit : > > On Mon, 2010-07-05 at 18:03 +0200, Eric Dumazet wrote: > > > Le lundi 05 juillet 2010 à 11:14 +0200, Eric Dumazet a écrit : > > > > After commit be1f3c2c027c (net: Enable 64-bit net device statistics on > > > > 32-bit architectures), we can now provide 64bit stats, even on 32bit > > > > arches. > > > > > > > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> > > > > --- > > > > > > Please disregard this patch. > > > > > > There is small possibility a reader might read a 64bit value while > > > another writer makes a change to it, changing high 32bit value. > > > > > > A change in core network would be needed to make this 100% safe, > > > possibly using a seqlock to protect dev->stats64 > > > > I really didn't want to add this overhead and complication to readers > > when only some drivers need it. > > > > Overhead would be minimal, only in get_stats() method and only for > drivers that want to provide 64 bit stats... > > Clearly, rx/tx path must not have any overhead. > > Right now, even RTNL 64bit stats are not safe. > > Should we revert prior patches or try to make progress ? I think you should use a similar approach here as you did in the loopback driver, i.e. update private variables in the RX and TX path and then copy/aggregate them in the implementation ndo_get_stats64 (only without the need for percpu stats). If you want to include a seqlock in the driver stats interface, you can do that but it's not going to be pretty and we're still going to need additional seqlocks for per-queue (or percpy) stats in some drivers. Ben.
Le lundi 05 juillet 2010 à 19:30 +0100, Ben Hutchings a écrit : > I think you should use a similar approach here as you did in the > loopback driver, i.e. update private variables in the RX and TX path and > then copy/aggregate them in the implementation ndo_get_stats64 (only > without the need for percpu stats). > > If you want to include a seqlock in the driver stats interface, you can > do that but it's not going to be pretty and we're still going to need > additional seqlocks for per-queue (or percpy) stats in some drivers. Yes, I provided one patch but am working on a different one, requiring a new rtnl_link_stats64 param to ndo_get_stats64() methods and dev_get_stats() as well. dev->stats64 should not be overwritten without some synchronization, so just disallow it for the moment... -- 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 Mon, 2010-07-05 at 20:07 +0200, Eric Dumazet wrote: > Le lundi 05 juillet 2010 à 18:31 +0100, Ben Hutchings a écrit : > > On Mon, 2010-07-05 at 18:03 +0200, Eric Dumazet wrote: > > > Le lundi 05 juillet 2010 à 11:14 +0200, Eric Dumazet a écrit : > > > > After commit be1f3c2c027c (net: Enable 64-bit net device statistics on > > > > 32-bit architectures), we can now provide 64bit stats, even on 32bit > > > > arches. > > > > > > > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> > > > > --- > > > > > > Please disregard this patch. > > > > > > There is small possibility a reader might read a 64bit value while > > > another writer makes a change to it, changing high 32bit value. > > > > > > A change in core network would be needed to make this 100% safe, > > > possibly using a seqlock to protect dev->stats64 > > > > I really didn't want to add this overhead and complication to readers > > when only some drivers need it. > > > > Overhead would be minimal, only in get_stats() method and only for > drivers that want to provide 64 bit stats... > > Clearly, rx/tx path must not have any overhead. > > Right now, even RTNL 64bit stats are not safe. Ugh, I was assuming that callers of dev_get_stats() would hold dev_base_lock. However only netstat_show() seems to do so currently. Ben. > Should we revert prior patches or try to make progress ? > > >
Le lundi 05 juillet 2010 à 19:41 +0100, Ben Hutchings a écrit : > Ugh, I was assuming that callers of dev_get_stats() would hold > dev_base_lock. However only netstat_show() seems to do so currently. Right, but a read_lock() would not be enough, even if all users would use it. -- 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/tg3.c b/drivers/net/tg3.c index 289cdc5..d1430f9 100644 --- a/drivers/net/tg3.c +++ b/drivers/net/tg3.c @@ -9021,7 +9021,7 @@ err_out1: return err; } -static struct net_device_stats *tg3_get_stats(struct net_device *); +static struct rtnl_link_stats64 *tg3_get_stats64(struct net_device *); static struct tg3_ethtool_stats *tg3_get_estats(struct tg3 *); static int tg3_close(struct net_device *dev) @@ -9055,7 +9055,7 @@ static int tg3_close(struct net_device *dev) tg3_ints_fini(tp); - memcpy(&tp->net_stats_prev, tg3_get_stats(tp->dev), + memcpy(&tp->net_stats_prev, tg3_get_stats64(tp->dev), sizeof(tp->net_stats_prev)); memcpy(&tp->estats_prev, tg3_get_estats(tp), sizeof(tp->estats_prev)); @@ -9069,24 +9069,12 @@ static int tg3_close(struct net_device *dev) return 0; } -static inline unsigned long get_stat64(tg3_stat64_t *val) -{ - unsigned long ret; - -#if (BITS_PER_LONG == 32) - ret = val->low; -#else - ret = ((u64)val->high << 32) | ((u64)val->low); -#endif - return ret; -} - -static inline u64 get_estat64(tg3_stat64_t *val) +static inline u64 get_stat64(tg3_stat64_t *val) { return ((u64)val->high << 32) | ((u64)val->low); } -static unsigned long calc_crc_errors(struct tg3 *tp) +static u64 calc_crc_errors(struct tg3 *tp) { struct tg3_hw_stats *hw_stats = tp->hw_stats; @@ -9114,7 +9102,7 @@ static unsigned long calc_crc_errors(struct tg3 *tp) #define ESTAT_ADD(member) \ estats->member = old_estats->member + \ - get_estat64(&hw_stats->member) + get_stat64(&hw_stats->member) static struct tg3_ethtool_stats *tg3_get_estats(struct tg3 *tp) { @@ -9204,11 +9192,11 @@ static struct tg3_ethtool_stats *tg3_get_estats(struct tg3 *tp) return estats; } -static struct net_device_stats *tg3_get_stats(struct net_device *dev) +static struct rtnl_link_stats64 *tg3_get_stats64(struct net_device *dev) { struct tg3 *tp = netdev_priv(dev); - struct net_device_stats *stats = &tp->net_stats; - struct net_device_stats *old_stats = &tp->net_stats_prev; + struct rtnl_link_stats64 *stats = &tp->net_stats; + struct rtnl_link_stats64 *old_stats = &tp->net_stats_prev; struct tg3_hw_stats *hw_stats = tp->hw_stats; if (!hw_stats) @@ -14317,7 +14305,7 @@ static const struct net_device_ops tg3_netdev_ops = { .ndo_open = tg3_open, .ndo_stop = tg3_close, .ndo_start_xmit = tg3_start_xmit, - .ndo_get_stats = tg3_get_stats, + .ndo_get_stats64 = tg3_get_stats64, .ndo_validate_addr = eth_validate_addr, .ndo_set_multicast_list = tg3_set_rx_mode, .ndo_set_mac_address = tg3_set_mac_addr, @@ -14336,7 +14324,7 @@ static const struct net_device_ops tg3_netdev_ops_dma_bug = { .ndo_open = tg3_open, .ndo_stop = tg3_close, .ndo_start_xmit = tg3_start_xmit_dma_bug, - .ndo_get_stats = tg3_get_stats, + .ndo_get_stats64 = tg3_get_stats64, .ndo_validate_addr = eth_validate_addr, .ndo_set_multicast_list = tg3_set_rx_mode, .ndo_set_mac_address = tg3_set_mac_addr, diff --git a/drivers/net/tg3.h b/drivers/net/tg3.h index 6b6af76..b72ac52 100644 --- a/drivers/net/tg3.h +++ b/drivers/net/tg3.h @@ -2772,8 +2772,8 @@ struct tg3 { /* begin "everything else" cacheline(s) section */ - struct net_device_stats net_stats; - struct net_device_stats net_stats_prev; + struct rtnl_link_stats64 net_stats; + struct rtnl_link_stats64 net_stats_prev; struct tg3_ethtool_stats estats; struct tg3_ethtool_stats estats_prev;
After commit be1f3c2c027c (net: Enable 64-bit net device statistics on 32-bit architectures), we can now provide 64bit stats, even on 32bit arches. Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> --- drivers/net/tg3.c | 32 ++++++++++---------------------- drivers/net/tg3.h | 4 ++-- 2 files changed, 12 insertions(+), 24 deletions(-) -- 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