netxen driver has implemented netxen_nic_get_ethtool_stats() interface,
but it doesn't collect stats.rxdropped in driver, so we will get
different rx_dropped statistic information while using ifconfig and ethtool.
this patch fills stats.rxdropped field with data from net core
with dev_get_stats() just as ixgbe driver did for some of its stats.
Tested with last netxen 4.0.82
Compiled with stable 3.15.6
Signed-off-by: Ethan Zhao <[email protected]>
Tested-by: Sriharsha Yadagudde <[email protected]>
---
-v2 only fix rx_dropped field, not all.
-v3 workaround checkpatch.pl bug according to suggestion from Joe Perches <[email protected]>
.../ethernet/qlogic/netxen/netxen_nic_ethtool.c | 59 +++++++++++++++-----
1 files changed, 45 insertions(+), 14 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/netxen/netxen_nic_ethtool.c b/drivers/net/ethernet/qlogic/netxen/netxen_nic_ethtool.c
index 87e073c..2753f00 100644
--- a/drivers/net/ethernet/qlogic/netxen/netxen_nic_ethtool.c
+++ b/drivers/net/ethernet/qlogic/netxen/netxen_nic_ethtool.c
@@ -31,28 +31,43 @@
#include "netxen_nic.h"
#include "netxen_nic_hw.h"
+enum {NETDEV_STATS, NETXEN_STATS};
+
struct netxen_nic_stats {
char stat_string[ETH_GSTRING_LEN];
+ int type;
int sizeof_stat;
int stat_offset;
};
-#define NETXEN_NIC_STAT(m) sizeof(((struct netxen_adapter *)0)->m), \
- offsetof(struct netxen_adapter, m)
+#define NETXEN_NIC_STAT(name, m) \
+{ \
+ .stat_string = name, \
+ .type = NETXEN_STATS, \
+ .sizeof_stat = FIELD_SIZEOF(struct netxen_adapter, m), \
+ .stat_offset = offsetof(struct netxen_adapter, m) \
+}
+
+#define NETXEN_NETDEV_STAT(name, m) \
+{ .stat_string = name, \
+ .type = NETDEV_STATS, \
+ .sizeof_stat = FIELD_SIZEOF(struct rtnl_link_stats64, m), \
+ .stat_offset = offsetof(struct rtnl_link_stats64, m) \
+}
#define NETXEN_NIC_PORT_WINDOW 0x10000
#define NETXEN_NIC_INVALID_DATA 0xDEADBEEF
static const struct netxen_nic_stats netxen_nic_gstrings_stats[] = {
- {"xmit_called", NETXEN_NIC_STAT(stats.xmitcalled)},
- {"xmit_finished", NETXEN_NIC_STAT(stats.xmitfinished)},
- {"rx_dropped", NETXEN_NIC_STAT(stats.rxdropped)},
- {"tx_dropped", NETXEN_NIC_STAT(stats.txdropped)},
- {"csummed", NETXEN_NIC_STAT(stats.csummed)},
- {"rx_pkts", NETXEN_NIC_STAT(stats.rx_pkts)},
- {"lro_pkts", NETXEN_NIC_STAT(stats.lro_pkts)},
- {"rx_bytes", NETXEN_NIC_STAT(stats.rxbytes)},
- {"tx_bytes", NETXEN_NIC_STAT(stats.txbytes)},
+ NETXEN_NIC_STAT("xmit_called", stats.xmitcalled),
+ NETXEN_NIC_STAT("xmit_finished", stats.xmitfinished),
+ NETXEN_NETDEV_STAT("rx_dropped", rx_dropped),
+ NETXEN_NIC_STAT("tx_dropped", stats.txdropped),
+ NETXEN_NIC_STAT("csummed", stats.csummed),
+ NETXEN_NIC_STAT("rx_pkts", stats.rx_pkts),
+ NETXEN_NIC_STAT("lro_pkts", stats.lro_pkts),
+ NETXEN_NIC_STAT("rx_bytes", stats.rxbytes),
+ NETXEN_NIC_STAT("tx_bytes", stats.txbytes),
};
#define NETXEN_NIC_STATS_LEN ARRAY_SIZE(netxen_nic_gstrings_stats)
@@ -677,11 +692,27 @@ netxen_nic_get_ethtool_stats(struct net_device *dev,
{
struct netxen_adapter *adapter = netdev_priv(dev);
int index;
+ struct rtnl_link_stats64 temp;
+ const struct rtnl_link_stats64 *net_stats;
+ char *p = NULL;
+ net_stats = dev_get_stats(dev, &temp);
for (index = 0; index < NETXEN_NIC_STATS_LEN; index++) {
- char *p =
- (char *)adapter +
- netxen_nic_gstrings_stats[index].stat_offset;
+
+ switch (netxen_nic_gstrings_stats[index].type) {
+ case NETDEV_STATS:
+ p = (char *)net_stats +
+ netxen_nic_gstrings_stats[index].stat_offset;
+ break;
+ case NETXEN_STATS:
+ p = (char *)adapter +
+ netxen_nic_gstrings_stats[index].stat_offset;
+ break;
+ default:
+ data[index] = 0;
+ continue;
+ }
+
data[index] =
(netxen_nic_gstrings_stats[index].sizeof_stat ==
sizeof(u64)) ? *(u64 *) p : *(u32 *) p;
--
1.7.1
On 07/18/2014 09:13 AM, Ethan Zhao wrote:
> netxen driver has implemented netxen_nic_get_ethtool_stats() interface,
> but it doesn't collect stats.rxdropped in driver, so we will get
> different rx_dropped statistic information while using ifconfig and ethtool.
> this patch fills stats.rxdropped field with data from net core
> with dev_get_stats() just as ixgbe driver did for some of its stats.
>
> Tested with last netxen 4.0.82
> Compiled with stable 3.15.6
>
> Signed-off-by: Ethan Zhao <[email protected]>
> Tested-by: Sriharsha Yadagudde <[email protected]>
> ---
> -v2 only fix rx_dropped field, not all.
> -v3 workaround checkpatch.pl bug according to suggestion from Joe Perches <[email protected]>
>
> .../ethernet/qlogic/netxen/netxen_nic_ethtool.c | 59 +++++++++++++++-----
> 1 files changed, 45 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/net/ethernet/qlogic/netxen/netxen_nic_ethtool.c b/drivers/net/ethernet/qlogic/netxen/netxen_nic_ethtool.c
> index 87e073c..2753f00 100644
> --- a/drivers/net/ethernet/qlogic/netxen/netxen_nic_ethtool.c
> +++ b/drivers/net/ethernet/qlogic/netxen/netxen_nic_ethtool.c
> @@ -31,28 +31,43 @@
> #include "netxen_nic.h"
> #include "netxen_nic_hw.h"
>
> +enum {NETDEV_STATS, NETXEN_STATS};
> +
> struct netxen_nic_stats {
> char stat_string[ETH_GSTRING_LEN];
> + int type;
> int sizeof_stat;
> int stat_offset;
> };
>
> -#define NETXEN_NIC_STAT(m) sizeof(((struct netxen_adapter *)0)->m), \
> - offsetof(struct netxen_adapter, m)
> +#define NETXEN_NIC_STAT(name, m) \
> +{ \
> + .stat_string = name, \
> + .type = NETXEN_STATS, \
> + .sizeof_stat = FIELD_SIZEOF(struct netxen_adapter, m), \
> + .stat_offset = offsetof(struct netxen_adapter, m) \
> +}
> +
> +#define NETXEN_NETDEV_STAT(name, m) \
> +{ .stat_string = name, \
> + .type = NETDEV_STATS, \
> + .sizeof_stat = FIELD_SIZEOF(struct rtnl_link_stats64, m), \
> + .stat_offset = offsetof(struct rtnl_link_stats64, m) \
> +}
>
> #define NETXEN_NIC_PORT_WINDOW 0x10000
> #define NETXEN_NIC_INVALID_DATA 0xDEADBEEF
>
> static const struct netxen_nic_stats netxen_nic_gstrings_stats[] = {
> - {"xmit_called", NETXEN_NIC_STAT(stats.xmitcalled)},
> - {"xmit_finished", NETXEN_NIC_STAT(stats.xmitfinished)},
> - {"rx_dropped", NETXEN_NIC_STAT(stats.rxdropped)},
> - {"tx_dropped", NETXEN_NIC_STAT(stats.txdropped)},
> - {"csummed", NETXEN_NIC_STAT(stats.csummed)},
> - {"rx_pkts", NETXEN_NIC_STAT(stats.rx_pkts)},
> - {"lro_pkts", NETXEN_NIC_STAT(stats.lro_pkts)},
> - {"rx_bytes", NETXEN_NIC_STAT(stats.rxbytes)},
> - {"tx_bytes", NETXEN_NIC_STAT(stats.txbytes)},
> + NETXEN_NIC_STAT("xmit_called", stats.xmitcalled),
> + NETXEN_NIC_STAT("xmit_finished", stats.xmitfinished),
> + NETXEN_NETDEV_STAT("rx_dropped", rx_dropped),
> + NETXEN_NIC_STAT("tx_dropped", stats.txdropped),
> + NETXEN_NIC_STAT("csummed", stats.csummed),
> + NETXEN_NIC_STAT("rx_pkts", stats.rx_pkts),
> + NETXEN_NIC_STAT("lro_pkts", stats.lro_pkts),
> + NETXEN_NIC_STAT("rx_bytes", stats.rxbytes),
> + NETXEN_NIC_STAT("tx_bytes", stats.txbytes),
> };
>
> #define NETXEN_NIC_STATS_LEN ARRAY_SIZE(netxen_nic_gstrings_stats)
> @@ -677,11 +692,27 @@ netxen_nic_get_ethtool_stats(struct net_device *dev,
> {
> struct netxen_adapter *adapter = netdev_priv(dev);
> int index;
> + struct rtnl_link_stats64 temp;
> + const struct rtnl_link_stats64 *net_stats;
> + char *p = NULL;
>
> + net_stats = dev_get_stats(dev, &temp);
> for (index = 0; index < NETXEN_NIC_STATS_LEN; index++) {
> - char *p =
> - (char *)adapter +
> - netxen_nic_gstrings_stats[index].stat_offset;
> +
> + switch (netxen_nic_gstrings_stats[index].type) {
> + case NETDEV_STATS:
> + p = (char *)net_stats +
> + netxen_nic_gstrings_stats[index].stat_offset;
> + break;
> + case NETXEN_STATS:
> + p = (char *)adapter +
> + netxen_nic_gstrings_stats[index].stat_offset;
> + break;
> + default:
> + data[index] = 0;
> + continue;
If there is a chance of default case, then it will be always in switch case ...?
> + }
> +
> data[index] =
> (netxen_nic_gstrings_stats[index].sizeof_stat ==
> sizeof(u64)) ? *(u64 *) p : *(u32 *) p;
--
Regards,
Varka Bhadram.
On Fri, Jul 18, 2014 at 11:47 AM, Varka Bhadram <[email protected]> wrote:
> On 07/18/2014 09:13 AM, Ethan Zhao wrote:
>>
>> netxen driver has implemented netxen_nic_get_ethtool_stats() interface,
>> but it doesn't collect stats.rxdropped in driver, so we will get
>> different rx_dropped statistic information while using ifconfig and
>> ethtool.
>> this patch fills stats.rxdropped field with data from net core
>> with dev_get_stats() just as ixgbe driver did for some of its stats.
>>
>> Tested with last netxen 4.0.82
>> Compiled with stable 3.15.6
>>
>> Signed-off-by: Ethan Zhao <[email protected]>
>> Tested-by: Sriharsha Yadagudde <[email protected]>
>> ---
>> -v2 only fix rx_dropped field, not all.
>> -v3 workaround checkpatch.pl bug according to suggestion from Joe Perches
>> <[email protected]>
>> .../ethernet/qlogic/netxen/netxen_nic_ethtool.c | 59
>> +++++++++++++++-----
>> 1 files changed, 45 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/qlogic/netxen/netxen_nic_ethtool.c
>> b/drivers/net/ethernet/qlogic/netxen/netxen_nic_ethtool.c
>> index 87e073c..2753f00 100644
>> --- a/drivers/net/ethernet/qlogic/netxen/netxen_nic_ethtool.c
>> +++ b/drivers/net/ethernet/qlogic/netxen/netxen_nic_ethtool.c
>> @@ -31,28 +31,43 @@
>> #include "netxen_nic.h"
>> #include "netxen_nic_hw.h"
>> +enum {NETDEV_STATS, NETXEN_STATS};
>> +
>> struct netxen_nic_stats {
>> char stat_string[ETH_GSTRING_LEN];
>> + int type;
>> int sizeof_stat;
>> int stat_offset;
>> };
>> -#define NETXEN_NIC_STAT(m) sizeof(((struct netxen_adapter *)0)->m), \
>> - offsetof(struct netxen_adapter, m)
>> +#define NETXEN_NIC_STAT(name, m) \
>> +{ \
>> + .stat_string = name, \
>> + .type = NETXEN_STATS, \
>> + .sizeof_stat = FIELD_SIZEOF(struct netxen_adapter, m), \
>> + .stat_offset = offsetof(struct netxen_adapter, m) \
>> +}
>> +
>> +#define NETXEN_NETDEV_STAT(name, m) \
>> +{ .stat_string = name, \
>> + .type = NETDEV_STATS, \
>> + .sizeof_stat = FIELD_SIZEOF(struct rtnl_link_stats64, m), \
>> + .stat_offset = offsetof(struct rtnl_link_stats64, m) \
>> +}
>> #define NETXEN_NIC_PORT_WINDOW 0x10000
>> #define NETXEN_NIC_INVALID_DATA 0xDEADBEEF
>> static const struct netxen_nic_stats netxen_nic_gstrings_stats[] = {
>> - {"xmit_called", NETXEN_NIC_STAT(stats.xmitcalled)},
>> - {"xmit_finished", NETXEN_NIC_STAT(stats.xmitfinished)},
>> - {"rx_dropped", NETXEN_NIC_STAT(stats.rxdropped)},
>> - {"tx_dropped", NETXEN_NIC_STAT(stats.txdropped)},
>> - {"csummed", NETXEN_NIC_STAT(stats.csummed)},
>> - {"rx_pkts", NETXEN_NIC_STAT(stats.rx_pkts)},
>> - {"lro_pkts", NETXEN_NIC_STAT(stats.lro_pkts)},
>> - {"rx_bytes", NETXEN_NIC_STAT(stats.rxbytes)},
>> - {"tx_bytes", NETXEN_NIC_STAT(stats.txbytes)},
>> + NETXEN_NIC_STAT("xmit_called", stats.xmitcalled),
>> + NETXEN_NIC_STAT("xmit_finished", stats.xmitfinished),
>> + NETXEN_NETDEV_STAT("rx_dropped", rx_dropped),
>> + NETXEN_NIC_STAT("tx_dropped", stats.txdropped),
>> + NETXEN_NIC_STAT("csummed", stats.csummed),
>> + NETXEN_NIC_STAT("rx_pkts", stats.rx_pkts),
>> + NETXEN_NIC_STAT("lro_pkts", stats.lro_pkts),
>> + NETXEN_NIC_STAT("rx_bytes", stats.rxbytes),
>> + NETXEN_NIC_STAT("tx_bytes", stats.txbytes),
>> };
>> #define NETXEN_NIC_STATS_LEN
>> ARRAY_SIZE(netxen_nic_gstrings_stats)
>> @@ -677,11 +692,27 @@ netxen_nic_get_ethtool_stats(struct net_device *dev,
>> {
>> struct netxen_adapter *adapter = netdev_priv(dev);
>> int index;
>> + struct rtnl_link_stats64 temp;
>> + const struct rtnl_link_stats64 *net_stats;
>> + char *p = NULL;
>> + net_stats = dev_get_stats(dev, &temp);
>> for (index = 0; index < NETXEN_NIC_STATS_LEN; index++) {
>> - char *p =
>> - (char *)adapter +
>> - netxen_nic_gstrings_stats[index].stat_offset;
>> +
>> + switch (netxen_nic_gstrings_stats[index].type) {
>> + case NETDEV_STATS:
>> + p = (char *)net_stats +
>> +
>> netxen_nic_gstrings_stats[index].stat_offset;
>> + break;
>> + case NETXEN_STATS:
>> + p = (char *)adapter +
>> +
>> netxen_nic_gstrings_stats[index].stat_offset;
>> + break;
>> + default:
>> + data[index] = 0;
>> + continue;
>
>
> If there is a chance of default case, then it will be always in switch case
> ...?
It is possible that hit the default case, but will not stay in swich case.
try this little case
#include <stdio.h>
#include <stdlib.h>
main()
{
int i,j;
for (i=0; i<16; i++) {
j=i%3;
switch(j) {
case 1:
printf("i=%d\n",i);
break;
case 2:
printf("i=%d\n",i);
break;
default:
printf("i=%d,j=%d\n",i,j);
continue;
}
}
}
Thanks,
Ethan
>
>
>> + }
>> +
>> data[index] =
>> (netxen_nic_gstrings_stats[index].sizeof_stat ==
>> sizeof(u64)) ? *(u64 *) p : *(u32 *) p;
>
>
>
> --
> Regards,
> Varka Bhadram.
>
On 07/18/2014 11:35 AM, Ethan Zhao wrote:
> On Fri, Jul 18, 2014 at 11:47 AM, Varka Bhadram <[email protected]> wrote:
>> On 07/18/2014 09:13 AM, Ethan Zhao wrote:
>>> netxen driver has implemented netxen_nic_get_ethtool_stats() interface,
>>> but it doesn't collect stats.rxdropped in driver, so we will get
>>> different rx_dropped statistic information while using ifconfig and
>>> ethtool.
>>> this patch fills stats.rxdropped field with data from net core
>>> with dev_get_stats() just as ixgbe driver did for some of its stats.
>>>
>>> Tested with last netxen 4.0.82
>>> Compiled with stable 3.15.6
>>>
>>> Signed-off-by: Ethan Zhao <[email protected]>
>>> Tested-by: Sriharsha Yadagudde <[email protected]>
>>> ---
>>> -v2 only fix rx_dropped field, not all.
>>> -v3 workaround checkpatch.pl bug according to suggestion from Joe Perches
>>> <[email protected]>
>>> .../ethernet/qlogic/netxen/netxen_nic_ethtool.c | 59
>>> +++++++++++++++-----
>>> 1 files changed, 45 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/qlogic/netxen/netxen_nic_ethtool.c
>>> b/drivers/net/ethernet/qlogic/netxen/netxen_nic_ethtool.c
>>> index 87e073c..2753f00 100644
>>> --- a/drivers/net/ethernet/qlogic/netxen/netxen_nic_ethtool.c
>>> +++ b/drivers/net/ethernet/qlogic/netxen/netxen_nic_ethtool.c
>>> @@ -31,28 +31,43 @@
>>> #include "netxen_nic.h"
>>> #include "netxen_nic_hw.h"
>>> +enum {NETDEV_STATS, NETXEN_STATS};
>>> +
>>> struct netxen_nic_stats {
>>> char stat_string[ETH_GSTRING_LEN];
>>> + int type;
>>> int sizeof_stat;
>>> int stat_offset;
>>> };
>>> -#define NETXEN_NIC_STAT(m) sizeof(((struct netxen_adapter *)0)->m), \
>>> - offsetof(struct netxen_adapter, m)
>>> +#define NETXEN_NIC_STAT(name, m) \
>>> +{ \
>>> + .stat_string = name, \
>>> + .type = NETXEN_STATS, \
>>> + .sizeof_stat = FIELD_SIZEOF(struct netxen_adapter, m), \
>>> + .stat_offset = offsetof(struct netxen_adapter, m) \
>>> +}
>>> +
>>> +#define NETXEN_NETDEV_STAT(name, m) \
>>> +{ .stat_string = name, \
>>> + .type = NETDEV_STATS, \
>>> + .sizeof_stat = FIELD_SIZEOF(struct rtnl_link_stats64, m), \
>>> + .stat_offset = offsetof(struct rtnl_link_stats64, m) \
>>> +}
>>> #define NETXEN_NIC_PORT_WINDOW 0x10000
>>> #define NETXEN_NIC_INVALID_DATA 0xDEADBEEF
>>> static const struct netxen_nic_stats netxen_nic_gstrings_stats[] = {
>>> - {"xmit_called", NETXEN_NIC_STAT(stats.xmitcalled)},
>>> - {"xmit_finished", NETXEN_NIC_STAT(stats.xmitfinished)},
>>> - {"rx_dropped", NETXEN_NIC_STAT(stats.rxdropped)},
>>> - {"tx_dropped", NETXEN_NIC_STAT(stats.txdropped)},
>>> - {"csummed", NETXEN_NIC_STAT(stats.csummed)},
>>> - {"rx_pkts", NETXEN_NIC_STAT(stats.rx_pkts)},
>>> - {"lro_pkts", NETXEN_NIC_STAT(stats.lro_pkts)},
>>> - {"rx_bytes", NETXEN_NIC_STAT(stats.rxbytes)},
>>> - {"tx_bytes", NETXEN_NIC_STAT(stats.txbytes)},
>>> + NETXEN_NIC_STAT("xmit_called", stats.xmitcalled),
>>> + NETXEN_NIC_STAT("xmit_finished", stats.xmitfinished),
>>> + NETXEN_NETDEV_STAT("rx_dropped", rx_dropped),
>>> + NETXEN_NIC_STAT("tx_dropped", stats.txdropped),
>>> + NETXEN_NIC_STAT("csummed", stats.csummed),
>>> + NETXEN_NIC_STAT("rx_pkts", stats.rx_pkts),
>>> + NETXEN_NIC_STAT("lro_pkts", stats.lro_pkts),
>>> + NETXEN_NIC_STAT("rx_bytes", stats.rxbytes),
>>> + NETXEN_NIC_STAT("tx_bytes", stats.txbytes),
>>> };
>>> #define NETXEN_NIC_STATS_LEN
>>> ARRAY_SIZE(netxen_nic_gstrings_stats)
>>> @@ -677,11 +692,27 @@ netxen_nic_get_ethtool_stats(struct net_device *dev,
>>> {
>>> struct netxen_adapter *adapter = netdev_priv(dev);
>>> int index;
>>> + struct rtnl_link_stats64 temp;
>>> + const struct rtnl_link_stats64 *net_stats;
>>> + char *p = NULL;
>>> + net_stats = dev_get_stats(dev, &temp);
>>> for (index = 0; index < NETXEN_NIC_STATS_LEN; index++) {
>>> - char *p =
>>> - (char *)adapter +
>>> - netxen_nic_gstrings_stats[index].stat_offset;
>>> +
>>> + switch (netxen_nic_gstrings_stats[index].type) {
>>> + case NETDEV_STATS:
>>> + p = (char *)net_stats +
>>> +
>>> netxen_nic_gstrings_stats[index].stat_offset;
>>> + break;
>>> + case NETXEN_STATS:
>>> + p = (char *)adapter +
>>> +
>>> netxen_nic_gstrings_stats[index].stat_offset;
>>> + break;
>>> + default:
>>> + data[index] = 0;
>>> + continue;
>>
>> If there is a chance of default case, then it will be always in switch case
>> ...?
> It is possible that hit the default case, but will not stay in swich case.
> try this little case
>
> #include <stdio.h>
> #include <stdlib.h>
>
> main()
> {
> int i,j;
> for (i=0; i<16; i++) {
> j=i%3;
>
> switch(j) {
> case 1:
> printf("i=%d\n",i);
> break;
> case 2:
> printf("i=%d\n",i);
> break;
> default:
> printf("i=%d,j=%d\n",i,j);
> continue;
> }
> }
> }
>
Ohhh.. You are right . Thanks for the clarity. :-)
--
Regards,
Varka Bhadram.
> -----Original Message-----
> From: Ethan Zhao [mailto:[email protected]]
> Sent: Friday, July 18, 2014 9:13 AM
> To: Manish Chopra; Sony Chacko; Rajesh Borundia; netdev
> Cc: linux-kernel; [email protected]; Ethan Zhao
> Subject: [PATCH V3] netxen: fix ethtool rx_dropped information in ethtool
> get_ethtool_stats()
>
> netxen driver has implemented netxen_nic_get_ethtool_stats() interface,
> but it doesn't collect stats.rxdropped in driver, so we will get different
> rx_dropped statistic information while using ifconfig and ethtool.
> this patch fills stats.rxdropped field with data from net core with
> dev_get_stats() just as ixgbe driver did for some of its stats.
I think ethtool stats are adapter specific. Driver does not maintain
rx_dropped stats and we also don't get it from adapter. I am not
sure if ethtool stats should match ifconfig stats as ifconfig also
adds net core stats to adapter stats.
If ethtool stats should report same values as ifconfig stats then
we may also need to fix tx_dropped statistics.
Thanks,
Rajesh
?? 2014??7??19?գ?????2:21??Rajesh Borundia <[email protected]> д????
>> -----Original Message-----
>> From: Ethan Zhao [mailto:[email protected]]
>> Sent: Friday, July 18, 2014 9:13 AM
>> To: Manish Chopra; Sony Chacko; Rajesh Borundia; netdev
>> Cc: linux-kernel; [email protected]; Ethan Zhao
>> Subject: [PATCH V3] netxen: fix ethtool rx_dropped information in ethtool
>> get_ethtool_stats()
>>
>> netxen driver has implemented netxen_nic_get_ethtool_stats() interface,
>> but it doesn't collect stats.rxdropped in driver, so we will get different
>> rx_dropped statistic information while using ifconfig and ethtool.
>> this patch fills stats.rxdropped field with data from net core with
>> dev_get_stats() just as ixgbe driver did for some of its stats.
>
> I think ethtool stats are adapter specific. Driver does not maintain
> rx_dropped stats and we also don't get it from adapter. I am not
> sure if ethtool stats should match ifconfig stats as ifconfig also
> adds net core stats to adapter stats.
I held the same point as yours before I read more drivers implementation under
Ethernet dir, in fact there is no specification/convention that ethtool info is adapter
Specific. All information output by ethtool is based on the information name itself,
One fact we want this patch is netxen defined 'rx dropped' field but output nothing.
>
> If ethtool stats should report same values as ifconfig stats then
> we may also need to fix tx_dropped statistics.
>
at least, tx dropped output information...
Thanks,
Ethan
> Thanks,
> Rajesh
Ethan Zhao <[email protected]> :
> ?? 2014??7??19????????2:21??Rajesh Borundia <[email protected]> ??????
[...]
> > I think ethtool stats are adapter specific. Driver does not maintain
> > rx_dropped stats and we also don't get it from adapter. I am not
> > sure if ethtool stats should match ifconfig stats as ifconfig also
> > adds net core stats to adapter stats.
> I held the same point as yours before I read more drivers implementation
> under Ethernet dir, in fact there is no specification/convention that
> ethtool info is adapter specific. All information output by ethtool is
> based on the information name itself, one fact we want this patch is
> netxen defined 'rx dropped' field but output nothing.
It may be fine as a requirement of the day but it does not bode well
for the future as ethtool stats API won't get less messy.
I'd rather see ethtool stats provides one of those:
1. hardware stats only
2. driver dependent stats - anything you want - as long as the hardware
only part of the stats is clearly identified
2. needs works. 1. hurts any requirement where it would priviously had
been so easy to add a few lines of code in the driver and push the mess
straight from kernel to the end user application.
--
Ueimor
On Sat, Jul 19, 2014 at 6:19 PM, Francois Romieu <[email protected]> wrote:
> Ethan Zhao <[email protected]> :
>> ?? 2014??7??19????????2:21??Rajesh Borundia <[email protected]> ??????
> [...]
>> > I think ethtool stats are adapter specific. Driver does not maintain
>> > rx_dropped stats and we also don't get it from adapter. I am not
>> > sure if ethtool stats should match ifconfig stats as ifconfig also
>> > adds net core stats to adapter stats.
>> I held the same point as yours before I read more drivers implementation
>> under Ethernet dir, in fact there is no specification/convention that
>> ethtool info is adapter specific. All information output by ethtool is
>> based on the information name itself, one fact we want this patch is
>> netxen defined 'rx dropped' field but output nothing.
>
> It may be fine as a requirement of the day but it does not bode well
> for the future as ethtool stats API won't get less messy.
It is the fact.
>
> I'd rather see ethtool stats provides one of those:
> 1. hardware stats only
Sounds very clear, is it done clear, if so we need other tools ?
Then why net core increase dev->dropped like
atomic_long_inc(&skb->dev->rx_dropped);
and mess some of the info to dev. just let ifconfig report it without detail ?
> 2. driver dependent stats - anything you want - as long as the hardware
> only part of the stats is clearly identified
I agree with this point, there is no other interface let the driver
customizes the output of ethtool or ifconfig, ethtool is the only
interface to export information we want that is driver / device
related, even some of the information from net core where stack
information output not covered.
Regard to dropped packets, some packets are to be dropped in net core without
error types reported. maybe it is why some drivers get rx dropped
stats from net core to avoid losing some of them.
>
> 2. needs works. 1. hurts any requirement where it would priviously had
> been so easy to add a few lines of code in the driver and push the mess
> straight from kernel to the end user application.
Maybe some guys think that is enough to mess them together while not
identify the information from net core or driver. that is better than
lose info /no info.
My case is user could be confused when run 'ifconfig' and 'ethtool'
to this netxen device. ifconfig reports packets dropped. but ethtool
says zero. does that mean it is not issue of driver forever ? who
could tell where the packets are dropped, and for what ?
So I would like the name of the data fields that ethtool output is be
better defined.
Instead of saying is not covered by ethtool.
Thanks,
Ethan
>
> --
> Ueimor
Ethan Zhao <[email protected]> :
> On Sat, Jul 19, 2014 at 6:19 PM, Francois Romieu <[email protected]> wrote:
> > Ethan Zhao <[email protected]> :
[...]
> > I'd rather see ethtool stats provides one of those:
> > 1. hardware stats only
> Sounds very clear, is it done clear, if so we need other tools ?
> Then why net core increase dev->dropped like
>
> atomic_long_inc(&skb->dev->rx_dropped);
>
> and mess some of the info to dev. just let ifconfig report it without detail ?
(side note: ifconfig is a bit 90ish)
Regarding the "without details" part, I don't expect ifconfig to tell users
that packets are dropped at net/core/dev.c:3279 because of empty softnet_data
queue in enqueue_to_backlog. More to the point below.
[...]
> > 2. needs works. 1. hurts any requirement where it would priviously had
> > been so easy to add a few lines of code in the driver and push the mess
> > straight from kernel to the end user application.
>
> Maybe some guys think that is enough to mess them together while not
> identify the information from net core or driver. that is better than
> lose info /no info.
Probably.
> My case is user could be confused when run 'ifconfig' and 'ethtool'
> to this netxen device. ifconfig reports packets dropped. but ethtool
> says zero. does that mean it is not issue of driver forever ?
s/driver/hardware/
Hardware correctly delivers packets if ethtool says zero [*]
"correctly" must be understood in the context of said hardware abilities.
[*] Of course we don't live in a world of unicorns and it's almost
unavoidable to check drivers's code to figure which stats are _really_
hardware only in ethtool output.
A part of ethtool if close to bare metal abilities - including protocol
offloading related ones - whereas ifconfig provides a software oriented
vision of layer 2 operations.
<digressio>
People at QLogic have not included *any* hardware counter in their
netxen ethtool stats code. Nada, keud. It's a bit deceptive for a gigabit
ethernet card and http://www.qlogic.com/Search/Pages/Search.aspx?k=NX2031
gives no hint.
</digressio>
I won't claim it's intuitive. However:
$ LANG=C man ifconfig
[...]
NOTE
This program is obsolete! For replacement check ip addr and ip link.
For statistics use ip -s link.
$ LANG=C man ip
[...]
-f, -family <FAMILY>
Specifies the protocol family to use. The protocol family iden-
tifier can be one of inet, inet6, bridge, ipx, dnet or link. If
this option is not present, the protocol family is guessed from
other arguments. If the rest of the command line does not give
enough information to guess the family, ip falls back to the
default one, usually inet or any. link is a special family
^^^^^^^^^^^^^^^^^^^^^^^^
identifier meaning that no networking protocol is involved.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
(huh, "special"...)
$ LANG=C man ethtool
[...]
-S --statistics
Queries the specified network device for NIC- and driver-specif-
ic statistics.
Some code stands between drivers and the upper part of the topmost
no-networking-procol layer, whence the differing stats: they are not
the same thing. Users can understand it.
> who could tell where the packets are dropped, and for what ?
One can't always answer beyond some coarse implementation level. There is
no such thing as a kernel MIB for the net core internal details.
Would you disagree that some of those - rx_dropped locations for instance -
are better kept in the scope of debug and trace utilities ?
> So I would like the name of the data fields that ethtool output is be
> better defined. Instead of saying is not covered by ethtool.
I respectfully disagree.
ethtool and ip are two different tools.
Regarding netxen driver, I do not want more ip / ifconfig stats in
ethtool code. It's quite the opposite: netxen ethtool code has no
business duplicating those and it should instead focus on real hardware
stats.
--
Ueimor
> ?? 2014??7??20?գ?????12:28??Francois Romieu <[email protected]> д????
>
> Ethan Zhao <[email protected]> :
>>> On Sat, Jul 19, 2014 at 6:19 PM, Francois Romieu <[email protected]> wrote:
>>> Ethan Zhao <[email protected]> :
> [...]
>>> I'd rather see ethtool stats provides one of those:
>>> 1. hardware stats only
>> Sounds very clear, is it done clear, if so we need other tools ?
>> Then why net core increase dev->dropped like
>>
>> atomic_long_inc(&skb->dev->rx_dropped);
>>
>> and mess some of the info to dev. just let ifconfig report it without detail ?
>
> (side note: ifconfig is a bit 90ish)
>
> Regarding the "without details" part, I don't expect ifconfig to tell users
> that packets are dropped at net/core/dev.c:3279 because of empty softnet_data
> queue in enqueue_to_backlog. More to the point below.
>
> [...]
>>> 2. needs works. 1. hurts any requirement where it would priviously had
>>> been so easy to add a few lines of code in the driver and push the mess
>>> straight from kernel to the end user application.
>>
>> Maybe some guys think that is enough to mess them together while not
>> identify the information from net core or driver. that is better than
>> lose info /no info.
>
> Probably.
>
>> My case is user could be confused when run 'ifconfig' and 'ethtool'
>> to this netxen device. ifconfig reports packets dropped. but ethtool
>> says zero. does that mean it is not issue of driver forever ?
>
> s/driver/hardware/
>
> Hardware correctly delivers packets if ethtool says zero [*]
>
> "correctly" must be understood in the context of said hardware abilities.
>
> [*] Of course we don't live in a world of unicorns and it's almost
> unavoidable to check drivers's code to figure which stats are _really_
> hardware only in ethtool output.
>
> A part of ethtool if close to bare metal abilities - including protocol
> offloading related ones - whereas ifconfig provides a software oriented
> vision of layer 2 operations.
>
> <digressio>
> People at QLogic have not included *any* hardware counter in their
> netxen ethtool stats code. Nada, keud. It's a bit deceptive for a gigabit
> ethernet card and http://www.qlogic.com/Search/Pages/Search.aspx?k=NX2031
> gives no hint.
> </digressio>
>
> I won't claim it's intuitive. However:
>
> $ LANG=C man ifconfig
> [...]
> NOTE
> This program is obsolete! For replacement check ip addr and ip link.
> For statistics use ip -s link.
>
> $ LANG=C man ip
> [...]
> -f, -family <FAMILY>
> Specifies the protocol family to use. The protocol family iden-
> tifier can be one of inet, inet6, bridge, ipx, dnet or link. If
> this option is not present, the protocol family is guessed from
> other arguments. If the rest of the command line does not give
> enough information to guess the family, ip falls back to the
> default one, usually inet or any. link is a special family
> ^^^^^^^^^^^^^^^^^^^^^^^^
> identifier meaning that no networking protocol is involved.
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> (huh, "special"...)
>
> $ LANG=C man ethtool
> [...]
> -S --statistics
> Queries the specified network device for NIC- and driver-specif-
> ic statistics.
>
> Some code stands between drivers and the upper part of the topmost
> no-networking-procol layer, whence the differing stats: they are not
> the same thing. Users can understand it.
>
>> who could tell where the packets are dropped, and for what ?
>
> One can't always answer beyond some coarse implementation level. There is
> no such thing as a kernel MIB for the net core internal details.
>
> Would you disagree that some of those - rx_dropped locations for instance -
> are better kept in the scope of debug and trace utilities ?
>
Of course I disagree it is the scope of debug and trace utilities. That is not fair or the original purpose the dropped info was collected into dev - >stat, it is dev specific
>> So I would like the name of the data fields that ethtool output is be
>> better defined. Instead of saying is not covered by ethtool.
>
> I respectfully disagree.
>
> ethtool and ip are two different tools.
you leave us without a tool to track the net core part. Would you like to hack the kernel code and rebuild just for peek the dropping packets ?
>
> Regarding netxen driver, I do not want more ip / ifconfig stats in
> ethtool code. It's quite the opposite: netxen ethtool code has no
> business duplicating those and it should instead focus on real hardware
You are ignoring the implementation fact of many drivers' ethtool part. You should persuade the authors of those drivers to delete those unrelated part in their ethtool code,
More radically, and maybe you should write patches to fix them to keep their focus. That is to prove you have principle and always fight for it.
Thanks,
Ethan
> stats.
>
> --
> Ueimor
hi,
Regarding the r8169 chip, there is no other data fields exported to
user via ethtool
except for those dumped from hardware statistic counters in 8169 driver.
But for Intel 82598 NIC and its driver ixgbe, it exports not only
those fields dumped from hardware statistic counters, like:
{"rx_missed_errors", IXGB_NETDEV_STAT(stats.rx_missed_errors)},
also some info like following, these stats are collected from net core.
{"rx_packets", IXGB_NETDEV_STAT(stats.rx_packets)},
{"tx_packets", IXGB_NETDEV_STAT(stats.tx_packets)},
{"rx_bytes", IXGB_NETDEV_STAT(stats.rx_bytes)},
{"tx_bytes", IXGB_NETDEV_STAT(stats.tx_bytes)},
{"rx_errors", IXGB_NETDEV_STAT(stats.rx_errors)},
{"tx_errors", IXGB_NETDEV_STAT(stats.tx_errors)},
{"rx_dropped", IXGB_NETDEV_STAT(stats.rx_dropped)},
{"tx_dropped", IXGB_NETDEV_STAT(stats.tx_dropped)},
As to netxen and its hardware. does it have hardware statistic
counter ? I don't care, As I know follow fields exported via ethtool
should be collected from software(net core / driver ):
{"xmit_called", NETXEN_NIC_STAT(stats.xmitcalled)},
{"xmit_finished", NETXEN_NIC_STAT(stats.xmitfinished)},
{"rx_dropped", NETXEN_NIC_STAT(stats.rxdropped)},
{"tx_dropped", NETXEN_NIC_STAT(stats.txdropped)},
{"csummed", NETXEN_NIC_STAT(stats.csummed)},
So fix the rx_dropped field with my patch is the right way. the
patch is not intended to dump hardware statistic counters.
Thanks,
Ethan
On Sun, Jul 20, 2014 at 8:43 AM, Ethan Zhao <[email protected]> wrote:
>
>
>> 在 2014年7月20日，上午12:28，Francois Romieu <[email protected]> 写道：
>>
>> Ethan Zhao <[email protected]> :
>>>> On Sat, Jul 19, 2014 at 6:19 PM, Francois Romieu <[email protected]> wrote:
>>>> Ethan Zhao <[email protected]> :
>> [...]
>>>> I'd rather see ethtool stats provides one of those:
>>>> 1. hardware stats only
>>> Sounds very clear, is it done clear, if so we need other tools ?
>>> Then why net core increase dev->dropped like
>>>
>>> atomic_long_inc(&skb->dev->rx_dropped);
>>>
>>> and mess some of the info to dev. just let ifconfig report it without detail ?
>>
>> (side note: ifconfig is a bit 90ish)
>>
>> Regarding the "without details" part, I don't expect ifconfig to tell users
>> that packets are dropped at net/core/dev.c:3279 because of empty softnet_data
>> queue in enqueue_to_backlog. More to the point below.
>>
>> [...]
>>>> 2. needs works. 1. hurts any requirement where it would priviously had
>>>> been so easy to add a few lines of code in the driver and push the mess
>>>> straight from kernel to the end user application.
>>>
>>> Maybe some guys think that is enough to mess them together while not
>>> identify the information from net core or driver. that is better than
>>> lose info /no info.
>>
>> Probably.
>>
>>> My case is user could be confused when run 'ifconfig' and 'ethtool'
>>> to this netxen device. ifconfig reports packets dropped. but ethtool
>>> says zero. does that mean it is not issue of driver forever ?
>>
>> s/driver/hardware/
>>
>> Hardware correctly delivers packets if ethtool says zero [*]
>>
>> "correctly" must be understood in the context of said hardware abilities.
>>
>> [*] Of course we don't live in a world of unicorns and it's almost
>> unavoidable to check drivers's code to figure which stats are _really_
>> hardware only in ethtool output.
>>
>> A part of ethtool if close to bare metal abilities - including protocol
>> offloading related ones - whereas ifconfig provides a software oriented
>> vision of layer 2 operations.
>>
>> <digressio>
>> People at QLogic have not included *any* hardware counter in their
>> netxen ethtool stats code. Nada, keud. It's a bit deceptive for a gigabit
>> ethernet card and http://www.qlogic.com/Search/Pages/Search.aspx?k=NX2031
>> gives no hint.
>> </digressio>
>>
>> I won't claim it's intuitive. However:
>>
>> $ LANG=C man ifconfig
>> [...]
>> NOTE
>> This program is obsolete! For replacement check ip addr and ip link.
>> For statistics use ip -s link.
>>
>> $ LANG=C man ip
>> [...]
>> -f, -family <FAMILY>
>> Specifies the protocol family to use. The protocol family iden-
>> tifier can be one of inet, inet6, bridge, ipx, dnet or link. If
>> this option is not present, the protocol family is guessed from
>> other arguments. If the rest of the command line does not give
>> enough information to guess the family, ip falls back to the
>> default one, usually inet or any. link is a special family
>> ^^^^^^^^^^^^^^^^^^^^^^^^
>> identifier meaning that no networking protocol is involved.
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>
>> (huh, "special"...)
>>
>> $ LANG=C man ethtool
>> [...]
>> -S --statistics
>> Queries the specified network device for NIC- and driver-specif-
>> ic statistics.
>>
>> Some code stands between drivers and the upper part of the topmost
>> no-networking-procol layer, whence the differing stats: they are not
>> the same thing. Users can understand it.
>>
>>> who could tell where the packets are dropped, and for what ?
>>
>> One can't always answer beyond some coarse implementation level. There is
>> no such thing as a kernel MIB for the net core internal details.
>>
>> Would you disagree that some of those - rx_dropped locations for instance -
>> are better kept in the scope of debug and trace utilities ?
>>
> Of course I disagree it is the scope of debug and trace utilities. That is not fair or the original purpose the dropped info was collected into dev - >stat, it is dev specific
>
>
>>> So I would like the name of the data fields that ethtool output is be
>>> better defined. Instead of saying is not covered by ethtool.
>>
>> I respectfully disagree.
>>
>> ethtool and ip are two different tools.
>
> you leave us without a tool to track the net core part. Would you like to hack the kernel code and rebuild just for peek the dropping packets ?
>>
>> Regarding netxen driver, I do not want more ip / ifconfig stats in
>> ethtool code. It's quite the opposite: netxen ethtool code has no
>> business duplicating those and it should instead focus on real hardware
> You are ignoring the implementation fact of many drivers' ethtool part. You should persuade the authors of those drivers to delete those unrelated part in their ethtool code,
> More radically, and maybe you should write patches to fix them to keep their focus. That is to prove you have principle and always fight for it.
>
>
>
> Thanks,
> Ethan
>> stats.
>>
>> --
>> Ueimor
Ethan Zhao <[email protected]> :
[...]
> you leave us without a tool to track the net core part. Would you like to
> hack the kernel code and rebuild just for peek the dropping packets ?
Search 'dropwatch' on your favorite engine search.
[...]
> More radically, and maybe you should write patches to fix them to keep
> their focus. That is to prove you have principle and always fight for it.
It's 15 years late but it's still a good advice. :o)
--
Ueimor
---------- Forwarded message ----------
From: Francois Romieu <[email protected]>
Date: Mon, Jul 21, 2014 at 2:01 AM
Subject: Re: [PATCH V3] netxen: fix ethtool rx_dropped information in
ethtool get_ethtool_stats()
To: Ethan Zhao <[email protected]>
Cc: Rajesh Borundia <[email protected]>, Ethan Zhao
<[email protected]>, Manish Chopra <[email protected]>,
Sony Chacko <[email protected]>, netdev <[email protected]>,
linux-kernel <[email protected]>
Ethan Zhao <[email protected]> :
[...]
> you leave us without a tool to track the net core part. Would you like to
> hack the kernel code and rebuild just for peek the dropping packets ?
Search 'dropwatch' on your favorite engine search.
That is a pretty good tool to trace and debug protocal stack when
packet dropping happens. but it is more professional and specific to
kernel kallsyms,
has some bugs to work well on my box. yet another different tool to
output debug info while not statistic info.
I think drivers output rx dropped has their own reason.
[...]
> More radically, and maybe you should write patches to fix them to keep
> their focus. That is to prove you have principle and always fight for it.
It's 15 years late but it's still a good advice. :o)
Does it work ?
Thanks,
Ethan
--
Ueimor
From: Ethan Zhao <[email protected]>
Date: Fri, 18 Jul 2014 11:43:29 +0800
> netxen driver has implemented netxen_nic_get_ethtool_stats() interface,
> but it doesn't collect stats.rxdropped in driver, so we will get
> different rx_dropped statistic information while using ifconfig and ethtool.
> this patch fills stats.rxdropped field with data from net core
> with dev_get_stats() just as ixgbe driver did for some of its stats.
>
> Tested with last netxen 4.0.82
> Compiled with stable 3.15.6
>
> Signed-off-by: Ethan Zhao <[email protected]>
> Tested-by: Sriharsha Yadagudde <[email protected]>
ethtool stats are _NOT_ the place to duplicate normal netdev stats,
PERIOD.
The only exception is when you maintainer per-queue instaces of these
stats, and want to export that information to the user.
I'm not applying this, sorry.