From: Jiri Pirko <redacted>
Nogah says:
Until now we had stats functions return SW statistics. However, it makes
a lot of sense to return HW stats as default. The existing apps count with
having the defaults stats complete, but that is not true now as the offloaded
forward traffic is not visible there.
If user wants to know real SW stats, this patchset provides way to get
it as well.
---
v3->v4:
- patch1/4:
- fixed "return ()" pointed out by EricD
- patch2/4:
- fixed if_nlmsg_size as pointed out by EricD
v2->v3:
- patch1/4:
- added dev_have_sw_stats helper
- patch2/4:
- avoided memcpy as requerted by DaveM
- patch3/4:
- use new dev_have_sw_stats helper
v1->v2:
- patch3/4:
- fixed NULL initialization
Nogah Frankel (4):
netdevice: add SW statistics ndo
rtnetlink: add HW/SW stats distinction in rtnl_fill_stats
net: core: add SW stats to if_stats_msg
mlxsw: spectrum: Implement SW stats ndo and expose HW stats by default
drivers/net/ethernet/mellanox/mlxsw/spectrum.c | 110 +++++++++++++++++++++++--
drivers/net/ethernet/mellanox/mlxsw/spectrum.h | 5 ++
include/linux/netdevice.h | 13 +++
include/uapi/linux/if_link.h | 2 +
net/core/dev.c | 31 +++++++
net/core/rtnetlink.c | 54 ++++++++++--
6 files changed, 200 insertions(+), 15 deletions(-)
--
2.5.5
From: Nogah Frankel <redacted>
Till now we had a ndo statistics function that returned SW statistics.
We want to change the "basic" statistics to return HW statistics if
available.
In this case we need to expose a new ndo to return the SW statistics.
Add a new ndo declaration to get SW statistics
Add a function that gets SW statistics if a compatible ndo exist
Signed-off-by: Nogah Frankel <redacted>
Reviewed-by: Ido Schimmel <redacted>
Signed-off-by: Jiri Pirko <redacted>
---
include/linux/netdevice.h | 13 +++++++++++++
net/core/dev.c | 31 +++++++++++++++++++++++++++++++
2 files changed, 44 insertions(+)
From: Nogah Frankel <redacted>
Since hardware stats are now returned by default, add a way to
query only software stats.
They are saved in IFLA_SW_STATS64.
(This option is valid only if the driver returns HW stats in the
default ndo stats)
Signed-off-by: Nogah Frankel <redacted>
Reviewed-by: Ido Schimmel <redacted>
Signed-off-by: Jiri Pirko <redacted>
---
include/uapi/linux/if_link.h | 1 +
net/core/rtnetlink.c | 13 ++++++++++++-
2 files changed, 13 insertions(+), 1 deletion(-)
From: Nogah Frankel <redacted>
If there is a dedicated ndo to return SW stats - use
it. Otherwise (indicates that there is no HW stats) use
the default stats ndo.
Return results under IFLA_STATS_LINK_SW_64.
Signed-off-by: Nogah Frankel <redacted>
Reviewed-by: Ido Schimmel <redacted>
Signed-off-by: Jiri Pirko <redacted>
---
include/uapi/linux/if_link.h | 1 +
net/core/rtnetlink.c | 41 +++++++++++++++++++++++++++++++++++------
2 files changed, 36 insertions(+), 6 deletions(-)
@@ -823,6 +823,7 @@ enum {IFLA_STATS_UNSPEC,/* also used as 64bit pad attribute */IFLA_STATS_LINK_64,IFLA_STATS_LINK_XSTATS,+IFLA_STATS_LINK_SW_64,__IFLA_STATS_MAX,};
@@ -3538,6 +3542,28 @@ static int rtnl_fill_statsinfo(struct sk_buff *skb, struct net_device *dev,}}+if(stats_attr_valid(filter_mask,IFLA_STATS_LINK_SW_64,*idxattr)){+attr=nla_reserve_64bit(skb,IFLA_STATS_LINK_SW_64,+sizeof(structrtnl_link_stats64),+IFLA_STATS_UNSPEC);+if(!attr)+gotonla_put_failure;++sp=nla_data(attr);++if(dev_have_sw_stats(dev)){+dev_get_sw_stats(dev,sp);+}else{+/* if SW stats are not available we return default+*stats.Wequeryonlyifwedon'talreadyhavethem.+*/+if(stats64_sp)+copy_rtnl_link_stats64(sp,stats64_sp);+else+dev_get_stats(dev,sp);+}+}+nlmsg_end(skb,nlh);return0;
From: Nogah Frankel <redacted>
Add a function to get the SW statistics with an ndo.
Change the default statistics ndo to return HW statistics
(like the one returned by ethtool_ops)
The HW stats are collected to a cache by delayed work every 1 sec.
Signed-off-by: Nogah Frankel <redacted>
Reviewed-by: Ido Schimmel <redacted>
Signed-off-by: Jiri Pirko <redacted>
---
drivers/net/ethernet/mellanox/mlxsw/spectrum.c | 110 +++++++++++++++++++++++--
drivers/net/ethernet/mellanox/mlxsw/spectrum.h | 5 ++
2 files changed, 107 insertions(+), 8 deletions(-)
@@ -564,6 +564,89 @@ mlxsw_sp_port_get_stats64(struct net_device *dev,returnstats;}+staticintmlxsw_sp_port_get_stats_raw(structnet_device*dev,+char*ppcnt_pl)+{+structmlxsw_sp_port*mlxsw_sp_port=netdev_priv(dev);+structmlxsw_sp*mlxsw_sp=mlxsw_sp_port->mlxsw_sp;++mlxsw_reg_ppcnt_pack(ppcnt_pl,mlxsw_sp_port->local_port,+MLXSW_REG_PPCNT_IEEE_8023_CNT,0)+;+returnmlxsw_reg_query(mlxsw_sp->core,MLXSW_REG(ppcnt),ppcnt_pl);+}++staticintmlxsw_sp_port_get_hw_stats(structnet_device*dev,+structrtnl_link_stats64*stats)+{+charppcnt_pl[MLXSW_REG_PPCNT_LEN];+interr;++err=mlxsw_sp_port_get_stats_raw(dev,ppcnt_pl);+if(err)+gotoout;++stats->tx_packets=+mlxsw_reg_ppcnt_a_frames_transmitted_ok_get(ppcnt_pl);+stats->rx_packets=+mlxsw_reg_ppcnt_a_frames_received_ok_get(ppcnt_pl);+stats->tx_bytes=+mlxsw_reg_ppcnt_a_octets_transmitted_ok_get(ppcnt_pl);+stats->rx_bytes=+mlxsw_reg_ppcnt_a_octets_received_ok_get(ppcnt_pl);+stats->multicast=+mlxsw_reg_ppcnt_a_multicast_frames_received_ok_get(ppcnt_pl);++stats->rx_crc_errors=+mlxsw_reg_ppcnt_a_frame_check_sequence_errors_get(ppcnt_pl);+stats->rx_frame_errors=+mlxsw_reg_ppcnt_a_alignment_errors_get(ppcnt_pl);++stats->rx_length_errors=(+mlxsw_reg_ppcnt_a_in_range_length_errors_get(ppcnt_pl)++mlxsw_reg_ppcnt_a_out_of_range_length_field_get(ppcnt_pl)++mlxsw_reg_ppcnt_a_frame_too_long_errors_get(ppcnt_pl));++stats->rx_errors=(stats->rx_crc_errors++stats->rx_frame_errors+stats->rx_length_errors);++out:+returnerr;+}++staticvoidupdate_stats_cache(structwork_struct*work)+{+structmlxsw_sp_port*mlxsw_sp_port=+container_of(work,structmlxsw_sp_port,+hw_stats.update_dw.work);++if(!netif_carrier_ok(mlxsw_sp_port->dev))+gotoout;++mlxsw_sp_port_get_hw_stats(mlxsw_sp_port->dev,+mlxsw_sp_port->hw_stats.cache);++out:+mlxsw_core_schedule_dw(&mlxsw_sp_port->hw_stats.update_dw,+MLXSW_HW_STATS_UPDATE_TIME);+}++/* HW stats are being read by a delayed work to a cache+*andthisfunctionreturnit.Theuseforcacheisbecause+*readingHWstatsrequireEMADandthatforsleepandwemight+*beaskedtoreturnstatsinsleeplessmode+*/+staticstructrtnl_link_stats64*+mlxsw_sp_port_get_stats64(structnet_device*dev,+structrtnl_link_stats64*stats)+{+structmlxsw_sp_port*mlxsw_sp_port=netdev_priv(dev);++memcpy(stats,mlxsw_sp_port->hw_stats.cache,sizeof(*stats));++returnstats;+}+intmlxsw_sp_port_vlan_set(structmlxsw_sp_port*mlxsw_sp_port,u16vid_begin,u16vid_end,boolis_member,booluntagged){
I don't see any reason why copy_rtnl_link_stats64's first argument should be
"void *". Please make it "struct rtnl_link_stats64 *".
In fact just doing a straight memcpy() inline is probably the best, there are
no special typing nor casting requirements here.
Thanks.
Until now we had stats functions return SW statistics. However, it makes
a lot of sense to return HW stats as default. The existing apps count with
having the defaults stats complete, but that is not true now as the offloaded
forward traffic is not visible there.
If user wants to know real SW stats, this patchset provides way to get
it as well.
I think we haven't been very good about defining good rules nor
guidelines for how to handle HW vs SW stats, and I'm talking
strictly about what we publish via rtnl_link_stats64.
However, if I were working from scratch on a new driver what I would
be inclined to do is use HW stats for everything that the chip
provides direct and accurate support for, and fill in the gaps with SW
stats.
Because to me, stats are stats, the user wants to know (for example)
how many broadcast packets have gone through the port and doesn't care
how you obtain that number.
If the problem being addressed is that drivers aren't reporting
information on all the packets going through the device, then that's a
bug.
But it seems to me like mlxsw is already maintaining the software
statistic counters, so I can't see a performance reason for not
properly providing all of the statistics using HW vs. SW as is
appropriate for each and every value to fix this problem. Why
create an entire new facility just for that? It doesn't seem to
be needed.
Maybe you just need to describe things a bit more completely in this
header posting.
I don't see any reason why copy_rtnl_link_stats64's first argument should be
"void *". Please make it "struct rtnl_link_stats64 *".
In fact just doing a straight memcpy() inline is probably the best, there are
no special typing nor casting requirements here.
Until now we had stats functions return SW statistics. However, it makes
a lot of sense to return HW stats as default. The existing apps count with
having the defaults stats complete, but that is not true now as the offloaded
forward traffic is not visible there.
If user wants to know real SW stats, this patchset provides way to get
it as well.
I think we haven't been very good about defining good rules nor
guidelines for how to handle HW vs SW stats, and I'm talking
strictly about what we publish via rtnl_link_stats64.
However, if I were working from scratch on a new driver what I would
be inclined to do is use HW stats for everything that the chip
provides direct and accurate support for, and fill in the gaps with SW
stats.
Because to me, stats are stats, the user wants to know (for example)
how many broadcast packets have gone through the port and doesn't care
how you obtain that number.
If the problem being addressed is that drivers aren't reporting
information on all the packets going through the device, then that's a
bug.
But it seems to me like mlxsw is already maintaining the software
statistic counters, so I can't see a performance reason for not
properly providing all of the statistics using HW vs. SW as is
appropriate for each and every value to fix this problem. Why
create an entire new facility just for that? It doesn't seem to
be needed.
Maybe you just need to describe things a bit more completely in this
header posting.
The problem we try to handle is different, it's about offloaded
forwarded packets which are not seen by kernel. Let me try to draw it :)
port1 port2 (HW stats are counted here)
\ /
\ /
\ /
--(A)---- ASIC --(B)--
|
(C)
|
CPU (SW stats are counted here)
Now we have couple of flows for TX and RX (direction does not matter here):
1) port1->A->ASIC->C->CPU
For this flow, HW and SW stats are equal.
2) port1->A->ASIC->C->CPU->C->ASIC->B->port2
For this flow, HW and SW stats are equal.
3) port1->A->ASIC->B->port2
For this flow, SW stats are 0.
The purpose of this patchset is to provide facility for user to
find out the difference between flows 1+2 and 3. In other words, user
will be able to see the statistics for his slow-path (through kernel).
Also, as a default the accumulated stats (HW) will be exposed to user
so the userspace apps can react properly.
From: David Ahern <hidden> Date: 2016-06-17 13:48:44
On 6/17/16 2:24 AM, Jiri Pirko wrote:
The problem we try to handle is different, it's about offloaded
forwarded packets which are not seen by kernel. Let me try to draw it :)
port1 port2 (HW stats are counted here)
\ /
\ /
\ /
--(A)---- ASIC --(B)--
|
(C)
|
CPU (SW stats are counted here)
Now we have couple of flows for TX and RX (direction does not matter here):
1) port1->A->ASIC->C->CPU
For this flow, HW and SW stats are equal.
2) port1->A->ASIC->C->CPU->C->ASIC->B->port2
For this flow, HW and SW stats are equal.
3) port1->A->ASIC->B->port2
For this flow, SW stats are 0.
The purpose of this patchset is to provide facility for user to
find out the difference between flows 1+2 and 3. In other words, user
will be able to see the statistics for his slow-path (through kernel).
Also, as a default the accumulated stats (HW) will be exposed to user
so the userspace apps can react properly.
You no longer agree with this discussion?
http://comments.gmane.org/gmane.linux.network/346740
Essentially netdevice stats show counters for packets punted to the cpu
and ethool -S shows h/w stats. This patch set seems to invert that.
Fri, Jun 17, 2016 at 03:48:35PM CEST, dsa@cumulusnetworks.com wrote:
On 6/17/16 2:24 AM, Jiri Pirko wrote:
quoted
The problem we try to handle is different, it's about offloaded
forwarded packets which are not seen by kernel. Let me try to draw it :)
port1 port2 (HW stats are counted here)
\ /
\ /
\ /
--(A)---- ASIC --(B)--
|
(C)
|
CPU (SW stats are counted here)
Now we have couple of flows for TX and RX (direction does not matter here):
1) port1->A->ASIC->C->CPU
For this flow, HW and SW stats are equal.
2) port1->A->ASIC->C->CPU->C->ASIC->B->port2
For this flow, HW and SW stats are equal.
3) port1->A->ASIC->B->port2
For this flow, SW stats are 0.
The purpose of this patchset is to provide facility for user to
find out the difference between flows 1+2 and 3. In other words, user
will be able to see the statistics for his slow-path (through kernel).
Also, as a default the accumulated stats (HW) will be exposed to user
so the userspace apps can react properly.
You no longer agree with this discussion?
http://comments.gmane.org/gmane.linux.network/346740
Essentially netdevice stats show counters for packets punted to the cpu and
ethool -S shows h/w stats. This patch set seems to invert that.
That is problematic. Existing apps depend on rtnetlink stats. But if we
don't count offloaded forwarded packets, the apps don't see anything.
Therefore I believe that this patchset approach is better. The existing
apps continue to work and future apps can use newly introduces sw_stats
to query slowpath traffic. Makes sense to me.
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2016-06-17 14:54:46
On 16-06-17 10:05 AM, Jiri Pirko wrote:
Fri, Jun 17, 2016 at 03:48:35PM CEST, dsa@cumulusnetworks.com wrote:
quoted
On 6/17/16 2:24 AM, Jiri Pirko wrote:
quoted
That is problematic. Existing apps depend on rtnetlink stats. But if we
don't count offloaded forwarded packets, the apps don't see anything.
Therefore I believe that this patchset approach is better. The existing
apps continue to work and future apps can use newly introduces sw_stats
to query slowpath traffic. Makes sense to me.
I agree with Jiri. It is a bad idea to depend on ethtool for any of
this stuff. Is there a way we can tag netlink stats instead
to indicate they are hardware or software?
We already have a use case with the tc where someone could get/set
hardware and/or software.
cheers,
jamal
Fri, Jun 17, 2016 at 04:54:34PM CEST, jhs@mojatatu.com wrote:
On 16-06-17 10:05 AM, Jiri Pirko wrote:
quoted
Fri, Jun 17, 2016 at 03:48:35PM CEST, dsa@cumulusnetworks.com wrote:
quoted
On 6/17/16 2:24 AM, Jiri Pirko wrote:
quoted
quoted
That is problematic. Existing apps depend on rtnetlink stats. But if we
don't count offloaded forwarded packets, the apps don't see anything.
Therefore I believe that this patchset approach is better. The existing
apps continue to work and future apps can use newly introduces sw_stats
to query slowpath traffic. Makes sense to me.
I agree with Jiri. It is a bad idea to depend on ethtool for any of
this stuff. Is there a way we can tag netlink stats instead
to indicate they are hardware or software?
In this patchset, those are 2 nl attrs. And they come kernel->user at once.
So I see no need for any tagging. Also won't be appropriate.
We already have a use case with the tc where someone could get/set
hardware and/or software.
From: David Ahern <hidden> Date: 2016-06-17 15:36:03
On 6/17/16 8:54 AM, Jamal Hadi Salim wrote:
On 16-06-17 10:05 AM, Jiri Pirko wrote:
quoted
Fri, Jun 17, 2016 at 03:48:35PM CEST, dsa@cumulusnetworks.com wrote:
quoted
On 6/17/16 2:24 AM, Jiri Pirko wrote:
quoted
quoted
That is problematic. Existing apps depend on rtnetlink stats. But if we
don't count offloaded forwarded packets, the apps don't see anything.
Therefore I believe that this patchset approach is better. The existing
apps continue to work and future apps can use newly introduces sw_stats
to query slowpath traffic. Makes sense to me.
I agree with Jiri. It is a bad idea to depend on ethtool for any of
this stuff. Is there a way we can tag netlink stats instead
to indicate they are hardware or software?
Right, old API but the key here is that low level h/w stats are returned
by a different API.
By default ip, ifconfig, snmpd, etc all continue to get traditional S/W
stats - counters as seen by the CPU.
Fri, Jun 17, 2016 at 05:35:53PM CEST, dsa@cumulusnetworks.com wrote:
On 6/17/16 8:54 AM, Jamal Hadi Salim wrote:
quoted
On 16-06-17 10:05 AM, Jiri Pirko wrote:
quoted
Fri, Jun 17, 2016 at 03:48:35PM CEST, dsa@cumulusnetworks.com wrote:
quoted
On 6/17/16 2:24 AM, Jiri Pirko wrote:
quoted
quoted
That is problematic. Existing apps depend on rtnetlink stats. But if we
don't count offloaded forwarded packets, the apps don't see anything.
Therefore I believe that this patchset approach is better. The existing
apps continue to work and future apps can use newly introduces sw_stats
to query slowpath traffic. Makes sense to me.
I agree with Jiri. It is a bad idea to depend on ethtool for any of
this stuff. Is there a way we can tag netlink stats instead
to indicate they are hardware or software?
Right, old API but the key here is that low level h/w stats are returned by a
different API.
By default ip, ifconfig, snmpd, etc all continue to get traditional S/W stats
- counters as seen by the CPU.
Yep. And I believe that for offloaded forwarding, this tools should see
hw counters, as they show what is going on in real.
Fri, Jun 17, 2016 at 05:35:53PM CEST, dsa@cumulusnetworks.com wrote:
quoted
On 6/17/16 8:54 AM, Jamal Hadi Salim wrote:
quoted
On 16-06-17 10:05 AM, Jiri Pirko wrote:
quoted
Fri, Jun 17, 2016 at 03:48:35PM CEST, dsa@cumulusnetworks.com wrote:
quoted
On 6/17/16 2:24 AM, Jiri Pirko wrote:
quoted
quoted
That is problematic. Existing apps depend on rtnetlink stats. But if we
don't count offloaded forwarded packets, the apps don't see anything.
Therefore I believe that this patchset approach is better. The existing
apps continue to work and future apps can use newly introduces sw_stats
to query slowpath traffic. Makes sense to me.
I agree with Jiri. It is a bad idea to depend on ethtool for any of
this stuff. Is there a way we can tag netlink stats instead
to indicate they are hardware or software?
Right, old API but the key here is that low level h/w stats are returned by a
different API.
By default ip, ifconfig, snmpd, etc all continue to get traditional S/W stats
- counters as seen by the CPU.
Yep. And I believe that for offloaded forwarding, this tools should see
hw counters, as they show what is going on in real.
If your NIC is offloading packets today, these tools typically won't see
these stats, but ethtool -S likely will report what is going on under
the hood.
Do we actually need to tell apart SW maintained from HW maintained
stats, or at the end all that matters is just, as DaveM pointed out,
getting the information, and in the case of an Ethernet switch, return
HW stats by default and supplement with SW stats whenever we have them,
all in the same namespace?
--
Florian
Fri, Jun 17, 2016 at 07:12:22PM CEST, f.fainelli@gmail.com wrote:
On 06/17/2016 08:42 AM, Jiri Pirko wrote:
quoted
Fri, Jun 17, 2016 at 05:35:53PM CEST, dsa@cumulusnetworks.com wrote:
quoted
On 6/17/16 8:54 AM, Jamal Hadi Salim wrote:
quoted
On 16-06-17 10:05 AM, Jiri Pirko wrote:
quoted
Fri, Jun 17, 2016 at 03:48:35PM CEST, dsa@cumulusnetworks.com wrote:
quoted
On 6/17/16 2:24 AM, Jiri Pirko wrote:
quoted
quoted
That is problematic. Existing apps depend on rtnetlink stats. But if we
don't count offloaded forwarded packets, the apps don't see anything.
Therefore I believe that this patchset approach is better. The existing
apps continue to work and future apps can use newly introduces sw_stats
to query slowpath traffic. Makes sense to me.
I agree with Jiri. It is a bad idea to depend on ethtool for any of
this stuff. Is there a way we can tag netlink stats instead
to indicate they are hardware or software?
Right, old API but the key here is that low level h/w stats are returned by a
different API.
By default ip, ifconfig, snmpd, etc all continue to get traditional S/W stats
- counters as seen by the CPU.
Yep. And I believe that for offloaded forwarding, this tools should see
hw counters, as they show what is going on in real.
If your NIC is offloading packets today, these tools typically won't see
these stats, but ethtool -S likely will report what is going on under
the hood.
Do we actually need to tell apart SW maintained from HW maintained
stats, or at the end all that matters is just, as DaveM pointed out,
getting the information, and in the case of an Ethernet switch, return
HW stats by default and supplement with SW stats whenever we have them,
all in the same namespace?
I believe it is valuable for user to know stats for slow path
(non-forwarded by ASIC). Also, it's just another rtnl attr. Easy.
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2016-06-18 13:59:00
On 16-06-18 04:00 AM, Jiri Pirko wrote:
Fri, Jun 17, 2016 at 07:12:22PM CEST, f.fainelli@gmail.com wrote:
quoted
quoted
Yep. And I believe that for offloaded forwarding, this tools should see
hw counters, as they show what is going on in real.
If your NIC is offloading packets today, these tools typically won't see
these stats, but ethtool -S likely will report what is going on under
the hood.
Do we actually need to tell apart SW maintained from HW maintained
stats, or at the end all that matters is just, as DaveM pointed out,
getting the information, and in the case of an Ethernet switch, return
HW stats by default and supplement with SW stats whenever we have them,
all in the same namespace?
In general it is extremely useful for debugging to be able to see them
separately. One API to unify them (and that API being netlink) is
the way to go. I dont know if you can ever obsolete ethtool if lots
of other utils are using it - but would be nice.
It is also useful to just get the sum of them - but user space can
take care of that. David A., whatever user space tools that depended
on ethtool should now be able to retrieve them via netlink, no?
I believe it is valuable for user to know stats for slow path
(non-forwarded by ASIC). Also, it's just another rtnl attr. Easy.
So Jiri, I see:
IFLA_SW_STATS64 should that be: IFLA_HW_STATS_LINK_64?
I think IFLA_STATS_LINK_64 should continue to send s/ware stats.
cheers,
jamal
Sat, Jun 18, 2016 at 03:58:56PM CEST, jhs@mojatatu.com wrote:
On 16-06-18 04:00 AM, Jiri Pirko wrote:
quoted
Fri, Jun 17, 2016 at 07:12:22PM CEST, f.fainelli@gmail.com wrote:
quoted
quoted
quoted
Yep. And I believe that for offloaded forwarding, this tools should see
hw counters, as they show what is going on in real.
If your NIC is offloading packets today, these tools typically won't see
these stats, but ethtool -S likely will report what is going on under
the hood.
Do we actually need to tell apart SW maintained from HW maintained
stats, or at the end all that matters is just, as DaveM pointed out,
getting the information, and in the case of an Ethernet switch, return
HW stats by default and supplement with SW stats whenever we have them,
all in the same namespace?
In general it is extremely useful for debugging to be able to see them
separately. One API to unify them (and that API being netlink) is
the way to go. I dont know if you can ever obsolete ethtool if lots
of other utils are using it - but would be nice.
It is also useful to just get the sum of them - but user space can
take care of that. David A., whatever user space tools that depended
on ethtool should now be able to retrieve them via netlink, no?
quoted
I believe it is valuable for user to know stats for slow path
(non-forwarded by ASIC). Also, it's just another rtnl attr. Easy.
So Jiri, I see:
IFLA_SW_STATS64 should that be: IFLA_HW_STATS_LINK_64?
I think IFLA_STATS_LINK_64 should continue to send s/ware stats.
Well, we spent a lot of time to think about this. The problem with your
approach is that existing apps don't see "real-stats" - hw stats. For
example snmp daemon takes IFLA_STATS_LINK_64i, so it has to see HW stats
there. In order to not break existing apps, we expose HW stats as default.
On Fri, Jun 17, 2016 at 7:05 AM, Jiri Pirko [off-list ref] wrote:
Fri, Jun 17, 2016 at 03:48:35PM CEST, dsa@cumulusnetworks.com wrote:
quoted
On 6/17/16 2:24 AM, Jiri Pirko wrote:
quoted
The problem we try to handle is different, it's about offloaded
forwarded packets which are not seen by kernel. Let me try to draw it :)
port1 port2 (HW stats are counted here)
\ /
\ /
\ /
--(A)---- ASIC --(B)--
|
(C)
|
CPU (SW stats are counted here)
Now we have couple of flows for TX and RX (direction does not matter here):
1) port1->A->ASIC->C->CPU
For this flow, HW and SW stats are equal.
2) port1->A->ASIC->C->CPU->C->ASIC->B->port2
For this flow, HW and SW stats are equal.
3) port1->A->ASIC->B->port2
For this flow, SW stats are 0.
The purpose of this patchset is to provide facility for user to
find out the difference between flows 1+2 and 3. In other words, user
will be able to see the statistics for his slow-path (through kernel).
Also, as a default the accumulated stats (HW) will be exposed to user
so the userspace apps can react properly.
You no longer agree with this discussion?
http://comments.gmane.org/gmane.linux.network/346740
Essentially netdevice stats show counters for packets punted to the cpu and
ethool -S shows h/w stats. This patch set seems to invert that.
That is problematic. Existing apps depend on rtnetlink stats. But if we
don't count offloaded forwarded packets, the apps don't see anything.
Therefore I believe that this patchset approach is better. The existing
apps continue to work and future apps can use newly introduces sw_stats
to query slowpath traffic. Makes sense to me.
Apps only care about stats. they don't care about sw vs hardware
stats. what apps are these ?.
For debugging, I agree it would be useful, but thats why we have
always had ethtool stats which
the driver can break down and display. And in all my patches about the
new stats api, i have indicated
that we will migrate the existing ethtool stats to a new netlink
attribute in the new stats api.
On Fri, Jun 17, 2016 at 7:54 AM, Jamal Hadi Salim [off-list ref] wrote:
On 16-06-17 10:05 AM, Jiri Pirko wrote:
quoted
Fri, Jun 17, 2016 at 03:48:35PM CEST, dsa@cumulusnetworks.com wrote:
quoted
On 6/17/16 2:24 AM, Jiri Pirko wrote:
quoted
quoted
That is problematic. Existing apps depend on rtnetlink stats. But if we
don't count offloaded forwarded packets, the apps don't see anything.
Therefore I believe that this patchset approach is better. The existing
apps continue to work and future apps can use newly introduces sw_stats
to query slowpath traffic. Makes sense to me.
I agree with Jiri. It is a bad idea to depend on ethtool for any of
this stuff.
The concern should not be that it is an ethtool api.
In all previous discussions on this patchset and also my
stats api patches, i have indicated that we have to move all stats
in one place, so naturally, ethtool stats should move eventually to the
stats api as a new nested netlink attribute. I think i called it
IFLA_STATS_LINK_HW (or something like that)...
and this nested attribute should provide the flexibility and extensibility
of the current ethtool stats api.
Is there a way we can tag netlink stats instead
to indicate they are hardware or software?
We already have a use case with the tc where someone could get/set
hardware and/or software.
cheers,
jamal
On Fri, Jun 17, 2016 at 10:12 AM, Florian Fainelli [off-list ref] wrote:
On 06/17/2016 08:42 AM, Jiri Pirko wrote:
quoted
Fri, Jun 17, 2016 at 05:35:53PM CEST, dsa@cumulusnetworks.com wrote:
quoted
On 6/17/16 8:54 AM, Jamal Hadi Salim wrote:
quoted
On 16-06-17 10:05 AM, Jiri Pirko wrote:
quoted
Fri, Jun 17, 2016 at 03:48:35PM CEST, dsa@cumulusnetworks.com wrote:
quoted
On 6/17/16 2:24 AM, Jiri Pirko wrote:
quoted
quoted
That is problematic. Existing apps depend on rtnetlink stats. But if we
don't count offloaded forwarded packets, the apps don't see anything.
Therefore I believe that this patchset approach is better. The existing
apps continue to work and future apps can use newly introduces sw_stats
to query slowpath traffic. Makes sense to me.
I agree with Jiri. It is a bad idea to depend on ethtool for any of
this stuff. Is there a way we can tag netlink stats instead
to indicate they are hardware or software?
Right, old API but the key here is that low level h/w stats are returned by a
different API.
By default ip, ifconfig, snmpd, etc all continue to get traditional S/W stats
- counters as seen by the CPU.
Yep. And I believe that for offloaded forwarding, this tools should see
hw counters, as they show what is going on in real.
If your NIC is offloading packets today, these tools typically won't see
these stats, but ethtool -S likely will report what is going on under
the hood.
Do we actually need to tell apart SW maintained from HW maintained
stats, or at the end all that matters is just, as DaveM pointed out,
getting the information, and in the case of an Ethernet switch, return
HW stats by default and supplement with SW stats whenever we have them,
all in the same namespace?
--
I have also mentioned this before, the default api must provide
accumulated (hw and sw) stats...,
because this is the api that the user queries on an interface.
For advanced debugging, people do want a break down and thats what
traditionally ethtool has provided
and the new stats api should eventually include support for ethtool like stats.
From: Jamal Hadi Salim <jhs@mojatatu.com> Date: 2016-06-20 12:28:35
On 16-06-19 11:14 PM, Roopa Prabhu wrote:
On Fri, Jun 17, 2016 at 10:12 AM, Florian Fainelli [off-list ref] wrote:
I have also mentioned this before, the default api must provide
accumulated (hw and sw) stats...,
because this is the api that the user queries on an interface.
Sorry - I missed those discussions.
What is current practise? Do people request for one via ip link
stats and the other via ethtool?
What do you guys do in your implementation?
Yes, it would be more accurate to provide aggregated stats but
it may break backward compat if expectation is both are read
separately today.
Maybe it makes sense to have a brand new TLV for these aggregated
stats as Jiri was suggesting.That means two new TLVs not one.
1) TLV for aggregated stats - which cant be current one
2) TLV for h/w stats
The existing stat implies s/ware only.
cheers,
jamal
Mon, Jun 20, 2016 at 02:28:31PM CEST, jhs@mojatatu.com wrote:
On 16-06-19 11:14 PM, Roopa Prabhu wrote:
quoted
On Fri, Jun 17, 2016 at 10:12 AM, Florian Fainelli [off-list ref] wrote:
quoted
I have also mentioned this before, the default api must provide
accumulated (hw and sw) stats...,
because this is the api that the user queries on an interface.
Sorry - I missed those discussions.
What is current practise? Do people request for one via ip link
stats and the other via ethtool?
Yes.
What do you guys do in your implementation?
Currently we do what you described. This patchset changes it.
Yes, it would be more accurate to provide aggregated stats but
it may break backward compat if expectation is both are read
separately today.
Maybe it makes sense to have a brand new TLV for these aggregated
stats as Jiri was suggesting.That means two new TLVs not one.
1) TLV for aggregated stats - which cant be current one
2) TLV for h/w stats
The existing stat implies s/ware only.
What is "aggregated"? if hw counts it, sw counts it as well.
HW stats are in fact aggregated. It includes all.
Apps should see HW stats as they don't care if the forwarding is
offloaded to HW or not. And if someone is aware there is a forward
offload, he can query sw-only stats via iface proposed by this patchset.
On Mon, Jun 20, 2016 at 5:28 AM, Jamal Hadi Salim [off-list ref] wrote:
On 16-06-19 11:14 PM, Roopa Prabhu wrote:
quoted
On Fri, Jun 17, 2016 at 10:12 AM, Florian Fainelli [off-list ref]
wrote:
quoted
I have also mentioned this before, the default api must provide
accumulated (hw and sw) stats...,
because this is the api that the user queries on an interface.
Sorry - I missed those discussions.
What is current practise? Do people request for one via ip link
stats and the other via ethtool?
What do you guys do in your implementation?
for us the standard netlink api that returns netdev stats includes all
stats hw and sw.
When i say hw and sw, I mean some of the error counters can also include errors
counted by sw.
ethtool stats has always provided drivers/users with additional stats
that the hw or driver
can expose.
Yes, it would be more accurate to provide aggregated stats but
it may break backward compat if expectation is both are read
separately today.
I don't think people see netdev stats as sw and ethtool as hw stats.
The latter just provides more granularity for debugging.
Thats the way i have looked at it forever.
Maybe it makes sense to have a brand new TLV for these aggregated
stats as Jiri was suggesting.That means two new TLVs not one.
1) TLV for aggregated stats - which cant be current one
2) TLV for h/w stats
The existing stat implies s/ware only.
I don't think existing stat implies s/ware stats only. so, I think we
should be careful
about changing the meaning of existing stats.
logical devices like bridge stats have always been software only...but
with switchdev
the way we see these or implement these is to also include hardware stats when
they are hw offloaded. For us bridge vlan stats, vxlan stats and so on
will follow the
same model. You cannot introduce separate sw and hw stats for these.
All stats will have to follow a consistent model.
Thanks,
Roopa