| Message ID | 20201017213611.2557565-2-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Generic TX reallocation for DSA | expand |
On 10/17/2020 2:35 PM, Vladimir Oltean wrote: > DSA needs to push a header onto every packet on TX, and this might cause > reallocation under certain scenarios, which might affect, for example, > performance. > > But reallocated packets are not standardized in struct pcpu_sw_netstats, > struct net_device_stats or anywhere else, it seems, so we need to roll > our own extra netdevice statistics and expose them to ethtool. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> +/* Driver statistics, other than those in struct rtnl_link_stats64. > + * These are collected per-CPU and aggregated by ethtool. > + */ > +struct dsa_slave_stats { > + __u64 tx_reallocs; > + struct u64_stats_sync syncp; > +} __aligned(1 * sizeof(u64)); The convention seems to be to use a prefix of pcpu_, e.g. pcpu_sw_netstats, pcpu_lstats. Also, why __u64? Neither pcpu_sw_netstats nor pcpu_lstats use __u64. Andrew
On Sun, Oct 18, 2020 at 01:15:51AM +0200, Andrew Lunn wrote: > > +/* Driver statistics, other than those in struct rtnl_link_stats64. > > + * These are collected per-CPU and aggregated by ethtool. > > + */ > > +struct dsa_slave_stats { > > + __u64 tx_reallocs; > > + struct u64_stats_sync syncp; > > +} __aligned(1 * sizeof(u64)); > > The convention seems to be to use a prefix of pcpu_, > e.g. pcpu_sw_netstats, pcpu_lstats. I find the "pcpu_sw_netstats" to be long and useless. I can easily see it's percpu based on its usage, I don't need to have it in its name. > Also, why __u64? Neither pcpu_sw_netstats nor pcpu_lstats use __u64. Ok, I'll confess I stole the beginning from the dpaa2-eth driver prior to commit d70446ee1f40 ("dpaa2-eth: send a scatter-gather FD instead of realloc-ing"), since I knew it used to implement this counter. Then I combined with what was already there for the standard statistics in DSA. But to be honest, what I dislike a little bit is that we have 2 structures now. For example, netronome nfp has created one struct nfp_repr_pcpu_stats that holds everything, even if it duplicates some counters found elsewhere. But I think that's a bit easier to digest from a maintainability point of view, what do you think?
On 17.10.2020 23:35, Vladimir Oltean wrote: > DSA needs to push a header onto every packet on TX, and this might cause > reallocation under certain scenarios, which might affect, for example, > performance. > > But reallocated packets are not standardized in struct pcpu_sw_netstats, > struct net_device_stats or anywhere else, it seems, so we need to roll > our own extra netdevice statistics and expose them to ethtool. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > net/dsa/dsa_priv.h | 9 +++++++++ > net/dsa/slave.c | 25 ++++++++++++++++++++++--- > 2 files changed, 31 insertions(+), 3 deletions(-) > > diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h > index 12998bf04e55..d39db7500cdd 100644 > --- a/net/dsa/dsa_priv.h > +++ b/net/dsa/dsa_priv.h > @@ -73,12 +73,21 @@ struct dsa_notifier_mtu_info { > int mtu; > }; > > +/* Driver statistics, other than those in struct rtnl_link_stats64. > + * These are collected per-CPU and aggregated by ethtool. > + */ > +struct dsa_slave_stats { > + __u64 tx_reallocs; > + struct u64_stats_sync syncp; > +} __aligned(1 * sizeof(u64)); > + Wouldn't a simple unsigned long (like in struct net_device_stats) be sufficient here? This would make handling the counter much simpler. And as far as I understand we talk about a packet counter that is touched in certain scenarios only. > struct dsa_slave_priv { > /* Copy of CPU port xmit for faster access in slave transmit hot path */ > struct sk_buff * (*xmit)(struct sk_buff *skb, > struct net_device *dev); > > struct pcpu_sw_netstats __percpu *stats64; > + struct dsa_slave_stats __percpu *extra_stats; > > struct gro_cells gcells; > > diff --git a/net/dsa/slave.c b/net/dsa/slave.c > index 3bc5ca40c9fb..d4326940233c 100644 > --- a/net/dsa/slave.c > +++ b/net/dsa/slave.c > @@ -668,9 +668,10 @@ static void dsa_slave_get_strings(struct net_device *dev, > strncpy(data + len, "tx_bytes", len); > strncpy(data + 2 * len, "rx_packets", len); > strncpy(data + 3 * len, "rx_bytes", len); > + strncpy(data + 4 * len, "tx_reallocs", len); > if (ds->ops->get_strings) > ds->ops->get_strings(ds, dp->index, stringset, > - data + 4 * len); > + data + 5 * len); > } > } > > @@ -682,11 +683,13 @@ static void dsa_slave_get_ethtool_stats(struct net_device *dev, > struct dsa_slave_priv *p = netdev_priv(dev); > struct dsa_switch *ds = dp->ds; > struct pcpu_sw_netstats *s; > + struct dsa_slave_stats *e; > unsigned int start; > int i; > > for_each_possible_cpu(i) { > u64 tx_packets, tx_bytes, rx_packets, rx_bytes; > + u64 tx_reallocs; > > s = per_cpu_ptr(p->stats64, i); > do { > @@ -696,13 +699,21 @@ static void dsa_slave_get_ethtool_stats(struct net_device *dev, > rx_packets = s->rx_packets; > rx_bytes = s->rx_bytes; > } while (u64_stats_fetch_retry_irq(&s->syncp, start)); > + > + e = per_cpu_ptr(p->extra_stats, i); > + do { > + start = u64_stats_fetch_begin_irq(&e->syncp); > + tx_reallocs = e->tx_reallocs; > + } while (u64_stats_fetch_retry_irq(&e->syncp, start)); > + > data[0] += tx_packets; > data[1] += tx_bytes; > data[2] += rx_packets; > data[3] += rx_bytes; > + data[4] += tx_reallocs; > } > if (ds->ops->get_ethtool_stats) > - ds->ops->get_ethtool_stats(ds, dp->index, data + 4); > + ds->ops->get_ethtool_stats(ds, dp->index, data + 5); > } > > static int dsa_slave_get_sset_count(struct net_device *dev, int sset) > @@ -713,7 +724,7 @@ static int dsa_slave_get_sset_count(struct net_device *dev, int sset) > if (sset == ETH_SS_STATS) { > int count; > > - count = 4; > + count = 5; > if (ds->ops->get_sset_count) > count += ds->ops->get_sset_count(ds, dp->index, sset); > > @@ -1806,6 +1817,12 @@ int dsa_slave_create(struct dsa_port *port) > free_netdev(slave_dev); > return -ENOMEM; > } > + p->extra_stats = netdev_alloc_pcpu_stats(struct dsa_slave_stats); > + if (!p->extra_stats) { > + free_percpu(p->stats64); > + free_netdev(slave_dev); > + return -ENOMEM; > + } > > ret = gro_cells_init(&p->gcells, slave_dev); > if (ret) > @@ -1864,6 +1881,7 @@ int dsa_slave_create(struct dsa_port *port) > out_gcells: > gro_cells_destroy(&p->gcells); > out_free: > + free_percpu(p->extra_stats); > free_percpu(p->stats64); > free_netdev(slave_dev); > port->slave = NULL; > @@ -1886,6 +1904,7 @@ void dsa_slave_destroy(struct net_device *slave_dev) > dsa_slave_notify(slave_dev, DSA_PORT_UNREGISTER); > phylink_destroy(dp->pl); > gro_cells_destroy(&p->gcells); > + free_percpu(p->extra_stats); > free_percpu(p->stats64); > free_netdev(slave_dev); > } >
On Sun, Oct 18, 2020 at 02:02:46PM +0200, Heiner Kallweit wrote: > Wouldn't a simple unsigned long (like in struct net_device_stats) be > sufficient here? This would make handling the counter much simpler. > And as far as I understand we talk about a packet counter that is > touched in certain scenarios only. I don't understand, in what sense 'sufficient'? This counter is exported to ethtool which works with u64 values, how would an unsigned long, which is u32 on 32-bit systems, help?
On 18.10.2020 14:16, Vladimir Oltean wrote: > On Sun, Oct 18, 2020 at 02:02:46PM +0200, Heiner Kallweit wrote: >> Wouldn't a simple unsigned long (like in struct net_device_stats) be >> sufficient here? This would make handling the counter much simpler. >> And as far as I understand we talk about a packet counter that is >> touched in certain scenarios only. > > I don't understand, in what sense 'sufficient'? This counter is exported > to ethtool which works with u64 values, how would an unsigned long, > which is u32 on 32-bit systems, help? > Sufficient for me means that it's unlikely that a 32 bit counter will overflow. Many drivers use the 32 bit counters (on a 32bit system) in net_device_stats for infrequent events like rx/tx errors, and 64bit counters only for things like rx/tx bytes, which are more likely to overflow.
On Sun, Oct 18, 2020 at 03:09:32PM +0200, Heiner Kallweit wrote: > On 18.10.2020 14:16, Vladimir Oltean wrote: > > On Sun, Oct 18, 2020 at 02:02:46PM +0200, Heiner Kallweit wrote: > >> Wouldn't a simple unsigned long (like in struct net_device_stats) be > >> sufficient here? This would make handling the counter much simpler. > >> And as far as I understand we talk about a packet counter that is > >> touched in certain scenarios only. > > > > I don't understand, in what sense 'sufficient'? This counter is exported > > to ethtool which works with u64 values, how would an unsigned long, > > which is u32 on 32-bit systems, help? > > > Sufficient for me means that it's unlikely that a 32 bit counter will > overflow. Many drivers use the 32 bit counters (on a 32bit system) in > net_device_stats for infrequent events like rx/tx errors, and 64bit > counters only for things like rx/tx bytes, which are more likely to > overflow. 2^32 = 4,294,967,296 = 4 billion packets Considering that every packet that needs TX timestamping must be reallocated, protocols such as IEEE 802.1AS will trigger 5 reallocs per second. So time synchronization alone (background traffic, by all accounts) will make this counter overflow in 27 years. Every packet flooded or multicast by the bridge will also trigger reallocs. In this case it is not difficult to imagine that overflows might occur sooner. Even if the above wasn't true. What becomes easier when I make the counter an unsigned long? I need to make arch-dependent casts between an unsigned long an an u64 when I expose the counter to ethtool, and there it ends up being u64 too, doesn't it?
On 18.10.2020 15:48, Vladimir Oltean wrote: > On Sun, Oct 18, 2020 at 03:09:32PM +0200, Heiner Kallweit wrote: >> On 18.10.2020 14:16, Vladimir Oltean wrote: >>> On Sun, Oct 18, 2020 at 02:02:46PM +0200, Heiner Kallweit wrote: >>>> Wouldn't a simple unsigned long (like in struct net_device_stats) be >>>> sufficient here? This would make handling the counter much simpler. >>>> And as far as I understand we talk about a packet counter that is >>>> touched in certain scenarios only. >>> >>> I don't understand, in what sense 'sufficient'? This counter is exported >>> to ethtool which works with u64 values, how would an unsigned long, >>> which is u32 on 32-bit systems, help? >>> >> Sufficient for me means that it's unlikely that a 32 bit counter will >> overflow. Many drivers use the 32 bit counters (on a 32bit system) in >> net_device_stats for infrequent events like rx/tx errors, and 64bit >> counters only for things like rx/tx bytes, which are more likely to >> overflow. > > 2^32 = 4,294,967,296 = 4 billion packets > Considering that every packet that needs TX timestamping must be > reallocated, protocols such as IEEE 802.1AS will trigger 5 reallocs per > second. So time synchronization alone (background traffic, by all > accounts) will make this counter overflow in 27 years. > Every packet flooded or multicast by the bridge will also trigger > reallocs. In this case it is not difficult to imagine that overflows > might occur sooner. > > Even if the above wasn't true. What becomes easier when I make the > counter an unsigned long? I need to make arch-dependent casts between an > unsigned long an an u64 when I expose the counter to ethtool, and there > it ends up being u64 too, doesn't it? > Access to unsigned long should be atomic, so you could avoid the following and access the counter directly (like other drivers do it with the net_device_stats members). On a side note, because I'm also just dealing with it: this_cpu_ptr() is safe only if preemption is disabled. Is this the case here? Else there's get_cpu_ptr/put_cpu_ptr. Also, if irq's aren't disabled there might be a need to use u64_stats_update_begin_irqsave() et al. See: 2695578b896a ("net: usbnet: fix potential deadlock on 32bit hosts") But I don't know dsa good enough to say whether this is applicable here. + e = this_cpu_ptr(p->extra_stats); + u64_stats_update_begin(&e->syncp); + e->tx_reallocs++; + u64_stats_update_end(&e->syncp); + e = per_cpu_ptr(p->extra_stats, i); + do { + start = u64_stats_fetch_begin_irq(&e->syncp); + tx_reallocs = e->tx_reallocs; + } while (u64_stats_fetch_retry_irq(&e->syncp, start));
On Sun, 18 Oct 2020 00:35:59 +0300 Vladimir Oltean wrote: > DSA needs to push a header onto every packet on TX, and this might cause > reallocation under certain scenarios, which might affect, for example, > performance. > > But reallocated packets are not standardized in struct pcpu_sw_netstats, > struct net_device_stats or anywhere else, it seems, so we need to roll > our own extra netdevice statistics and expose them to ethtool. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Could you consider adding "driver" stats under RTM_GETSTATS, or a similar new structured interface over ethtool? Looks like the statistic in question has pretty clear semantics, and may be more broadly useful. > +/* Driver statistics, other than those in struct rtnl_link_stats64. > + * These are collected per-CPU and aggregated by ethtool. > + */ > +struct dsa_slave_stats { > + __u64 tx_reallocs; s/__u/u/ > + struct u64_stats_sync syncp; > +} __aligned(1 * sizeof(u64)); Why aligned to u64? Compiler should pick a reasonable alignment here by itself.
On Sun, Oct 18, 2020 at 09:49:51AM -0700, Jakub Kicinski wrote: > > + struct u64_stats_sync syncp; > > +} __aligned(1 * sizeof(u64)); > > Why aligned to u64? Compiler should pick a reasonable alignment here > by itself. Hi Jakub I wondered that as well. But: struct gnet_stats_basic_cpu { struct gnet_stats_basic_packed bstats; struct u64_stats_sync syncp; } __aligned(2 * sizeof(u64)); /* often modified stats are per-CPU, other are shared (netdev->stats) */ struct pcpu_sw_netstats { u64 rx_packets; u64 rx_bytes; u64 tx_packets; u64 tx_bytes; struct u64_stats_sync syncp; } __aligned(4 * sizeof(u64)); struct pcpu_lstats { u64_stats_t packets; u64_stats_t bytes; struct u64_stats_sync syncp; } __aligned(2 * sizeof(u64)); Cargo cult or is there a real need? Andrew
On Sun, 18 Oct 2020 19:36:49 +0200 Andrew Lunn wrote: > On Sun, Oct 18, 2020 at 09:49:51AM -0700, Jakub Kicinski wrote: > > > + struct u64_stats_sync syncp; > > > +} __aligned(1 * sizeof(u64)); > > > > Why aligned to u64? Compiler should pick a reasonable alignment here > > by itself. > > Hi Jakub > > I wondered that as well. But: > > struct gnet_stats_basic_cpu { > struct gnet_stats_basic_packed bstats; > struct u64_stats_sync syncp; > } __aligned(2 * sizeof(u64)); > > /* often modified stats are per-CPU, other are shared (netdev->stats) */ > struct pcpu_sw_netstats { > u64 rx_packets; > u64 rx_bytes; > u64 tx_packets; > u64 tx_bytes; > struct u64_stats_sync syncp; > } __aligned(4 * sizeof(u64)); > > struct pcpu_lstats { > u64_stats_t packets; > u64_stats_t bytes; > struct u64_stats_sync syncp; > } __aligned(2 * sizeof(u64)); > > Cargo cult or is there a real need? Hm, looks like the intent is to enforce power of two alignment to prevent the structure from spanning cache lines. Doesn't make any difference for 1 counter, but I guess we can keep the style for consistency.
On Sun, 18 Oct 2020 11:26:53 -0700 Jakub Kicinski wrote: > Hm, looks like the intent is to enforce power of two alignment > to prevent the structure from spanning cache lines. Doesn't make > any difference for 1 counter, but I guess we can keep the style > for consistency. I take that back, looks like seqcount_t is 4B, so it will make a difference, don't mind me :)
On Sun, Oct 18, 2020 at 04:13:28PM +0200, Heiner Kallweit wrote: > On a side note, because I'm also just dealing with it: this_cpu_ptr() > is safe only if preemption is disabled. Is this the case here? Else > there's get_cpu_ptr/put_cpu_ptr. lockdep would shout about using smp_processor_id() in preemptible context? Because it doesn't do that, and never did, but I don't really understand why, coming to think of it. > Also, if irq's aren't disabled there might be a need to use > u64_stats_update_begin_irqsave() et al. See: 2695578b896a ("net: > usbnet: fix potential deadlock on 32bit hosts") But I don't know dsa > good enough to say whether this is applicable here. DSA xmit and receive path do not run from hardirq context, just softirq, so I believe the issue reported to Eric there does not apply here.
On 10/18/2020 3:58 PM, Vladimir Oltean wrote: > On Sun, Oct 18, 2020 at 04:13:28PM +0200, Heiner Kallweit wrote: >> On a side note, because I'm also just dealing with it: this_cpu_ptr() >> is safe only if preemption is disabled. Is this the case here? Else >> there's get_cpu_ptr/put_cpu_ptr. > > lockdep would shout about using smp_processor_id() in preemptible > context? Because it doesn't do that, and never did, but I don't really > understand why, coming to think of it. > >> Also, if irq's aren't disabled there might be a need to use >> u64_stats_update_begin_irqsave() et al. See: 2695578b896a ("net: >> usbnet: fix potential deadlock on 32bit hosts") But I don't know dsa >> good enough to say whether this is applicable here. > > DSA xmit and receive path do not run from hardirq context, just softirq, > so I believe the issue reported to Eric there does not apply here. How about when used as a netconsole? We do support netconsole over DSA interfaces.
On Sun, Oct 18, 2020 at 04:11:14PM -0700, Florian Fainelli wrote: > How about when used as a netconsole? We do support netconsole over DSA > interfaces. How? Who is supposed to bring up the master interface, and when?
On 10/18/2020 5:21 PM, Vladimir Oltean wrote: > On Sun, Oct 18, 2020 at 04:11:14PM -0700, Florian Fainelli wrote: >> How about when used as a netconsole? We do support netconsole over DSA >> interfaces. > > How? Who is supposed to bring up the master interface, and when? > You are right that this appears not to work when configured on the kernel command line: [ 6.836910] netpoll: netconsole: local port 4444 [ 6.841553] netpoll: netconsole: local IPv4 address 192.168.1.10 [ 6.847582] netpoll: netconsole: interface 'gphy' [ 6.852305] netpoll: netconsole: remote port 9353 [ 6.857030] netpoll: netconsole: remote IPv4 address 192.168.1.254 [ 6.863233] netpoll: netconsole: remote ethernet address b8:ac:6f:80:af:7e [ 6.870134] netpoll: netconsole: device gphy not up yet, forcing it [ 6.876428] netpoll: netconsole: failed to open gphy [ 6.881412] netconsole: cleaning up looking at my test notes from 2015 when it was added, I had only tested dynamic netconsole while the network devices have already been brought up which is why I did not catch it. Let me see if I can fix that somehow.
On Sun, Oct 18, 2020 at 08:49:31PM -0700, Florian Fainelli wrote: > > > On 10/18/2020 5:21 PM, Vladimir Oltean wrote: > > On Sun, Oct 18, 2020 at 04:11:14PM -0700, Florian Fainelli wrote: > > > How about when used as a netconsole? We do support netconsole over DSA > > > interfaces. > > > > How? Who is supposed to bring up the master interface, and when? > > > > You are right that this appears not to work when configured on the kernel > command line: > > [ 6.836910] netpoll: netconsole: local port 4444 > [ 6.841553] netpoll: netconsole: local IPv4 address 192.168.1.10 > [ 6.847582] netpoll: netconsole: interface 'gphy' > [ 6.852305] netpoll: netconsole: remote port 9353 > [ 6.857030] netpoll: netconsole: remote IPv4 address 192.168.1.254 > [ 6.863233] netpoll: netconsole: remote ethernet address > b8:ac:6f:80:af:7e > [ 6.870134] netpoll: netconsole: device gphy not up yet, forcing it > [ 6.876428] netpoll: netconsole: failed to open gphy > [ 6.881412] netconsole: cleaning up > > looking at my test notes from 2015 when it was added, I had only tested > dynamic netconsole while the network devices have already been brought up > which is why I did not catch it. Let me see if I can fix that somehow. Hi Florian NFS root used to work, so there must be some code in the kernel to bring the master interface up. Might just need copy/pasting. Andrew
On 10/19/20 5:05 AM, Andrew Lunn wrote: > On Sun, Oct 18, 2020 at 08:49:31PM -0700, Florian Fainelli wrote: >> >> >> On 10/18/2020 5:21 PM, Vladimir Oltean wrote: >>> On Sun, Oct 18, 2020 at 04:11:14PM -0700, Florian Fainelli wrote: >>>> How about when used as a netconsole? We do support netconsole over DSA >>>> interfaces. >>> >>> How? Who is supposed to bring up the master interface, and when? >>> >> >> You are right that this appears not to work when configured on the kernel >> command line: >> >> [ 6.836910] netpoll: netconsole: local port 4444 >> [ 6.841553] netpoll: netconsole: local IPv4 address 192.168.1.10 >> [ 6.847582] netpoll: netconsole: interface 'gphy' >> [ 6.852305] netpoll: netconsole: remote port 9353 >> [ 6.857030] netpoll: netconsole: remote IPv4 address 192.168.1.254 >> [ 6.863233] netpoll: netconsole: remote ethernet address >> b8:ac:6f:80:af:7e >> [ 6.870134] netpoll: netconsole: device gphy not up yet, forcing it >> [ 6.876428] netpoll: netconsole: failed to open gphy >> [ 6.881412] netconsole: cleaning up >> >> looking at my test notes from 2015 when it was added, I had only tested >> dynamic netconsole while the network devices have already been brought up >> which is why I did not catch it. Let me see if I can fix that somehow. > > Hi Florian > > NFS root used to work, so there must be some code in the kernel to > bring the master interface up. Might just need copy/pasting. This is a tiny bit different because netconsole goes through netpoll which is responsible for doing the interface configuration. Unlike root over NFS, this does not utilize net/ipv4/ipconfig.c, so the existing DSA checks in that file are not used. The same "cure" could be applied, but I am not sure if it will be accepted, we shall see.
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h index 12998bf04e55..d39db7500cdd 100644 --- a/net/dsa/dsa_priv.h +++ b/net/dsa/dsa_priv.h @@ -73,12 +73,21 @@ struct dsa_notifier_mtu_info { int mtu; }; +/* Driver statistics, other than those in struct rtnl_link_stats64. + * These are collected per-CPU and aggregated by ethtool. + */ +struct dsa_slave_stats { + __u64 tx_reallocs; + struct u64_stats_sync syncp; +} __aligned(1 * sizeof(u64)); + struct dsa_slave_priv { /* Copy of CPU port xmit for faster access in slave transmit hot path */ struct sk_buff * (*xmit)(struct sk_buff *skb, struct net_device *dev); struct pcpu_sw_netstats __percpu *stats64; + struct dsa_slave_stats __percpu *extra_stats; struct gro_cells gcells; diff --git a/net/dsa/slave.c b/net/dsa/slave.c index 3bc5ca40c9fb..d4326940233c 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -668,9 +668,10 @@ static void dsa_slave_get_strings(struct net_device *dev, strncpy(data + len, "tx_bytes", len); strncpy(data + 2 * len, "rx_packets", len); strncpy(data + 3 * len, "rx_bytes", len); + strncpy(data + 4 * len, "tx_reallocs", len); if (ds->ops->get_strings) ds->ops->get_strings(ds, dp->index, stringset, - data + 4 * len); + data + 5 * len); } } @@ -682,11 +683,13 @@ static void dsa_slave_get_ethtool_stats(struct net_device *dev, struct dsa_slave_priv *p = netdev_priv(dev); struct dsa_switch *ds = dp->ds; struct pcpu_sw_netstats *s; + struct dsa_slave_stats *e; unsigned int start; int i; for_each_possible_cpu(i) { u64 tx_packets, tx_bytes, rx_packets, rx_bytes; + u64 tx_reallocs; s = per_cpu_ptr(p->stats64, i); do { @@ -696,13 +699,21 @@ static void dsa_slave_get_ethtool_stats(struct net_device *dev, rx_packets = s->rx_packets; rx_bytes = s->rx_bytes; } while (u64_stats_fetch_retry_irq(&s->syncp, start)); + + e = per_cpu_ptr(p->extra_stats, i); + do { + start = u64_stats_fetch_begin_irq(&e->syncp); + tx_reallocs = e->tx_reallocs; + } while (u64_stats_fetch_retry_irq(&e->syncp, start)); + data[0] += tx_packets; data[1] += tx_bytes; data[2] += rx_packets; data[3] += rx_bytes; + data[4] += tx_reallocs; } if (ds->ops->get_ethtool_stats) - ds->ops->get_ethtool_stats(ds, dp->index, data + 4); + ds->ops->get_ethtool_stats(ds, dp->index, data + 5); } static int dsa_slave_get_sset_count(struct net_device *dev, int sset) @@ -713,7 +724,7 @@ static int dsa_slave_get_sset_count(struct net_device *dev, int sset) if (sset == ETH_SS_STATS) { int count; - count = 4; + count = 5; if (ds->ops->get_sset_count) count += ds->ops->get_sset_count(ds, dp->index, sset); @@ -1806,6 +1817,12 @@ int dsa_slave_create(struct dsa_port *port) free_netdev(slave_dev); return -ENOMEM; } + p->extra_stats = netdev_alloc_pcpu_stats(struct dsa_slave_stats); + if (!p->extra_stats) { + free_percpu(p->stats64); + free_netdev(slave_dev); + return -ENOMEM; + } ret = gro_cells_init(&p->gcells, slave_dev); if (ret) @@ -1864,6 +1881,7 @@ int dsa_slave_create(struct dsa_port *port) out_gcells: gro_cells_destroy(&p->gcells); out_free: + free_percpu(p->extra_stats); free_percpu(p->stats64); free_netdev(slave_dev); port->slave = NULL; @@ -1886,6 +1904,7 @@ void dsa_slave_destroy(struct net_device *slave_dev) dsa_slave_notify(slave_dev, DSA_PORT_UNREGISTER); phylink_destroy(dp->pl); gro_cells_destroy(&p->gcells); + free_percpu(p->extra_stats); free_percpu(p->stats64); free_netdev(slave_dev); }
DSA needs to push a header onto every packet on TX, and this might cause reallocation under certain scenarios, which might affect, for example, performance. But reallocated packets are not standardized in struct pcpu_sw_netstats, struct net_device_stats or anywhere else, it seems, so we need to roll our own extra netdevice statistics and expose them to ethtool. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- net/dsa/dsa_priv.h | 9 +++++++++ net/dsa/slave.c | 25 ++++++++++++++++++++++--- 2 files changed, 31 insertions(+), 3 deletions(-)