From: Alexander Lobakin <hidden> Date: 2021-08-03 16:37:04
This series follows the Jakub's work on standard statistics and
unifies XDP statistics across [most of] the drivers.
The only driver left unconverted is mlx5 -- it has rather complex
statistics, so I believe it would be better to leave this up to
its developers.
The stats itself consists of 12 counters:
- packets: number of frames passed to bpf_prog_run_xdp();
- errors: number of general XDP errors, if driver has one unified counter;
- aborted: number of XDP_ABORTED returns;
- drop: number of XDP_DROP returns;
- invalid: number of returns of unallowed values (i.e. not XDP_*);
- pass: number of XDP_PASS returns;
- redirect: number of successfully performed XDP_REDIRECT requests;
- redirect_errors: number of failed XDP_REDIRECT requests;
- tx: number of successfully performed XDP_TX requests;
- tx_errors: number of failed XDP_TX requests;
- xmit: number of xdp_frames successfully transmitted via .ndo_xdp_xmit();
- xmit_drops: number of frames dropped from .ndo_xdp_xmit().
As most drivers stores them on a per-channel basis, Ethtool standard
stats infra has been expanded to support this. A new nested
attribute has been added which indicated that the fields enclosed
in this block are related to one particular channel. If Ethtool
utility is older than the kernel, those blocks will just be skipped
with no errors.
When the stats are not per-channel, Ethtool core treats them as
regular and so does Ethtool utility display them. Otherwise,
the example output looks like:
$ ./ethtool -S enp175s0f0 --all-groups
Standard stats for enp175s0f0:
[ snip ]
channel0-xdp-aborted: 1
channel0-xdp-drop: 2
channel0-xdp-illegal: 3
channel0-xdp-pass: 4
channel0-xdp-redirect: 5
[ snip ]
...and the JSON output looks like:
[ snip ]
"xdp": {
"per-channel": [
"channel0": {
"aborted": 1,
"drop": 2,
"illegal": 3,
"pass": 4,
"redirect": 5,
[ snip ]
} ]
}
[ snip ]
Rouhly half of the commits are present to unify XDP stats logics
across the drivers, and the first two are preparatory/housekeeping.
This set is also available here: [0]
[0] https://github.com/alobakin/linux/tree/xdp_stats
Alexander Lobakin (21):
ethtool, stats: use a shorthand pointer in stats_prepare_data()
ethtool, stats: add compile-time checks for standard stats
ethtool, stats: introduce standard XDP statistics
ethernet, dpaa2: simplify per-channel Ethtool stats counting
ethernet, dpaa2: convert to standard XDP stats
ethernet, ena: constify src and syncp args of ena_safe_update_stat()
ethernet, ena: convert to standard XDP stats
ethernet, enetc: convert to standard XDP stats
ethernet, mvneta: rename xdp_xmit_err to xdp_xmit_drops
ethernet, mvneta: convert to standard XDP stats
ethernet, mvpp2: rename xdp_xmit_err to xdp_xmit_drops
ethernet, mvpp2: convert to standard XDP stats
ethernet, sfc: convert to standard XDP stats
veth: rename rx_drops to xdp_errors
veth: rename xdp_xmit_errors to xdp_xmit_drops
veth: rename drop xdp_ suffix from packets and bytes stats
veth: convert to standard XDP stats
virtio-net: rename xdp_tx{,__drops} SQ stats to xdp_xmit{,__drops}
virtio-net: don't mix error-caused drops with XDP_DROP cases
virtio-net: convert to standard XDP stats
Documentation, ethtool-netlink: update standard statistics
documentation
Documentation/networking/ethtool-netlink.rst | 45 +++--
drivers/net/ethernet/amazon/ena/ena_ethtool.c | 50 +++++-
.../net/ethernet/freescale/dpaa2/dpaa2-eth.h | 7 +-
.../ethernet/freescale/dpaa2/dpaa2-ethtool.c | 38 +++-
.../ethernet/freescale/enetc/enetc_ethtool.c | 58 ++++--
drivers/net/ethernet/marvell/mvneta.c | 112 ++++++------
drivers/net/ethernet/marvell/mvpp2/mvpp2.h | 2 +-
.../net/ethernet/marvell/mvpp2/mvpp2_main.c | 96 +++-------
drivers/net/ethernet/sfc/ef100_ethtool.c | 2 +
drivers/net/ethernet/sfc/ethtool.c | 2 +
drivers/net/ethernet/sfc/ethtool_common.c | 35 +++-
drivers/net/ethernet/sfc/ethtool_common.h | 3 +
drivers/net/veth.c | 167 ++++++++++--------
drivers/net/virtio_net.c | 76 ++++++--
include/linux/ethtool.h | 36 ++++
include/uapi/linux/ethtool.h | 2 +
include/uapi/linux/ethtool_netlink.h | 34 ++++
net/ethtool/netlink.h | 1 +
net/ethtool/stats.c | 163 +++++++++++++++--
net/ethtool/strset.c | 5 +
20 files changed, 659 insertions(+), 275 deletions(-)
--
2.31.1
From: Alexander Lobakin <hidden> Date: 2021-08-03 16:37:13
Make sure that the number of counters inside stats structures is
with the corresponding Ethtool Netlink definitions.
RMON stats is a special case -- don't take histogram fields into
account.
Signed-off-by: Alexander Lobakin <redacted>
Reviewed-by: Jesse Brandeburg <redacted>
---
net/ethtool/stats.c | 9 +++++++++
1 file changed, 9 insertions(+)
@@ -117,6 +117,15 @@ static int stats_prepare_data(const struct ethnl_req_info *req_base,ops=dev->ethtool_ops;+BUILD_BUG_ON(sizeof(data->phy_stats)/sizeof(u64)!=+__ETHTOOL_A_STATS_ETH_PHY_CNT);+BUILD_BUG_ON(sizeof(data->mac_stats)/sizeof(u64)!=+__ETHTOOL_A_STATS_ETH_MAC_CNT);+BUILD_BUG_ON(sizeof(data->ctrl_stats)/sizeof(u64)!=+__ETHTOOL_A_STATS_ETH_CTRL_CNT);+BUILD_BUG_ON(offsetof(typeof(data->rmon_stats),hist)/sizeof(u64)!=+__ETHTOOL_A_STATS_RMON_CNT);+/* Mark all stats as unset (see ETHTOOL_STAT_NOT_SET) to prevent them*frombeingreportedtouserspaceincasedriverdidnotsetthem.*/
From: Alexander Lobakin <hidden> Date: 2021-08-03 16:37:18
Just place dev->ethtool_ops on the stack and use it instead of
dereferencing the former a bunch of times to improve code
readability.
Signed-off-by: Alexander Lobakin <redacted>
Reviewed-by: Jesse Brandeburg <redacted>
---
net/ethtool/stats.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
@@ -108,12 +108,15 @@ static int stats_prepare_data(const struct ethnl_req_info *req_base,conststructstats_req_info*req_info=STATS_REQINFO(req_base);structstats_reply_data*data=STATS_REPDATA(reply_base);structnet_device*dev=reply_base->dev;+conststructethtool_ops*ops;intret;ret=ethnl_ops_begin(dev);if(ret<0)returnret;+ops=dev->ethtool_ops;+/* Mark all stats as unset (see ETHTOOL_STAT_NOT_SET) to prevent them*frombeingreportedtouserspaceincasedriverdidnotsetthem.*/
@@ -123,18 +126,17 @@ static int stats_prepare_data(const struct ethnl_req_info *req_base,memset(&data->rmon_stats,0xff,sizeof(data->rmon_stats));if(test_bit(ETHTOOL_STATS_ETH_PHY,req_info->stat_mask)&&-dev->ethtool_ops->get_eth_phy_stats)-dev->ethtool_ops->get_eth_phy_stats(dev,&data->phy_stats);+ops->get_eth_phy_stats)+ops->get_eth_phy_stats(dev,&data->phy_stats);if(test_bit(ETHTOOL_STATS_ETH_MAC,req_info->stat_mask)&&-dev->ethtool_ops->get_eth_mac_stats)-dev->ethtool_ops->get_eth_mac_stats(dev,&data->mac_stats);+ops->get_eth_mac_stats)+ops->get_eth_mac_stats(dev,&data->mac_stats);if(test_bit(ETHTOOL_STATS_ETH_CTRL,req_info->stat_mask)&&-dev->ethtool_ops->get_eth_ctrl_stats)-dev->ethtool_ops->get_eth_ctrl_stats(dev,&data->ctrl_stats);+ops->get_eth_ctrl_stats)+ops->get_eth_ctrl_stats(dev,&data->ctrl_stats);if(test_bit(ETHTOOL_STATS_RMON,req_info->stat_mask)&&-dev->ethtool_ops->get_rmon_stats)-dev->ethtool_ops->get_rmon_stats(dev,&data->rmon_stats,-&data->rmon_ranges);+ops->get_rmon_stats)+ops->get_rmon_stats(dev,&data->rmon_stats,&data->rmon_ranges);ethnl_ops_complete(dev);return0;
From: Alexander Lobakin <hidden> Date: 2021-08-03 16:37:20
Most of the driver-side XDP enabled drivers provide some statistics
on XDP programs runs and different actions taken (number of passes,
drops, redirects etc.).
Regarding that it's almost pretty the same across all the drivers
(which is obvious), we can implement some sort of "standardized"
statistics using Ethtool standard stats infra to eliminate a lot
of code and stringsets duplication, different approaches to count
these stats and so on.
These new 12 fields provided by the standard XDP stats should cover
most, if not all, stats that might be interesting for collecting and
tracking.
Note that most NIC drivers keep XDP statistics on a per-channel
basis, so this also introduces a new callback for getting a number
of channels which a driver will provide stats for. If it's not
implemented or returns 0, it means stats are global/device-wide.
Signed-off-by: Alexander Lobakin <redacted>
Reviewed-by: Jesse Brandeburg <redacted>
---
include/linux/ethtool.h | 36 +++++++
include/uapi/linux/ethtool.h | 2 +
include/uapi/linux/ethtool_netlink.h | 34 +++++++
net/ethtool/netlink.h | 1 +
net/ethtool/stats.c | 134 ++++++++++++++++++++++++++-
net/ethtool/strset.c | 5 +
6 files changed, 211 insertions(+), 1 deletion(-)
@@ -716,6 +716,7 @@ enum {ETHTOOL_STATS_ETH_MAC,ETHTOOL_STATS_ETH_CTRL,ETHTOOL_STATS_RMON,+ETHTOOL_STATS_XDP,/* add new constants above here */__ETHTOOL_STATS_CNT
@@ -737,6 +738,8 @@ enum {ETHTOOL_A_STATS_GRP_HIST_BKT_HI,/* u32 */ETHTOOL_A_STATS_GRP_HIST_VAL,/* u64 */+ETHTOOL_A_STATS_GRP_STAT_BLOCK,/* nest */+/* add new constants above here */__ETHTOOL_A_STATS_GRP_CNT,ETHTOOL_A_STATS_GRP_MAX=(__ETHTOOL_A_STATS_CNT-1)
@@ -831,6 +834,37 @@ enum {ETHTOOL_A_STATS_RMON_MAX=(__ETHTOOL_A_STATS_RMON_CNT-1)};+enum{+/* Number of frames passed to bpf_prog_run_xdp() */+ETHTOOL_A_STATS_XDP_PACKETS,+/* Number o general XDP errors if driver counts them together */+ETHTOOL_A_STATS_XDP_ERRORS,+/* Number of %XDP_ABORTED returns */+ETHTOOL_A_STATS_XDP_ABORTED,+/* Number of %XDP_DROP returns */+ETHTOOL_A_STATS_XDP_DROP,+/* Number of returns of unallowed values (i.e. not XDP_*) */+ETHTOOL_A_STATS_XDP_INVALID,+/* Number of %XDP_PASS returns */+ETHTOOL_A_STATS_XDP_PASS,+/* Number of successfully performed %XDP_REDIRECT requests */+ETHTOOL_A_STATS_XDP_REDIRECT,+/* Number of failed %XDP_REDIRECT requests */+ETHTOOL_A_STATS_XDP_REDIRECT_ERRORS,+/* Number of successfully performed %XDP_TX requests */+ETHTOOL_A_STATS_XDP_TX,+/* Number of failed %XDP_TX requests */+ETHTOOL_A_STATS_XDP_TX_ERRORS,+/* Number of xdp_frames successfully transmitted via .ndo_xdp_xmit() */+ETHTOOL_A_STATS_XDP_XMIT,+/* Number of frames dropped from .ndo_xdp_xmit() */+ETHTOOL_A_STATS_XDP_XMIT_DROPS,++/* Add new constants above here */+__ETHTOOL_A_STATS_XDP_CNT,+ETHTOOL_A_STATS_XDP_MAX=(__ETHTOOL_A_STATS_XDP_CNT-1)+};+/* generic netlink info */#define ETHTOOL_GENL_NAME "ethtool"#define ETHTOOL_GENL_VERSION 1
@@ -101,6 +119,37 @@ static int stats_parse_request(struct ethnl_req_info *req_base,return0;}+staticintstats_prepare_data_xdp(structnet_device*dev,+structstats_reply_data*data)+{+conststructethtool_ops*ops=dev->ethtool_ops;+size_tsize;+intret;++/* Zero means stats are global/device-wide */+data->xdp_stats_channels=0;++if(ops->get_std_stats_channels){+ret=ops->get_std_stats_channels(dev,ETH_SS_STATS_XDP);+if(ret>0)+data->xdp_stats_channels=ret;+}++size=array_size(min_not_zero(data->xdp_stats_channels,1U),+sizeof(*data->xdp_stats));+if(unlikely(size==SIZE_MAX))+return-EOVERFLOW;++data->xdp_stats=kvmalloc(size,GFP_KERNEL);+if(!data->xdp_stats)+return-ENOMEM;++memset(data->xdp_stats,0xff,size);+ops->get_xdp_stats(dev,data->xdp_stats);++return0;+}+staticintstats_prepare_data(conststructethnl_req_info*req_base,structethnl_reply_data*reply_base,structgenl_info*info)
@@ -125,6 +174,8 @@ static int stats_prepare_data(const struct ethnl_req_info *req_base,__ETHTOOL_A_STATS_ETH_CTRL_CNT);BUILD_BUG_ON(offsetof(typeof(data->rmon_stats),hist)/sizeof(u64)!=__ETHTOOL_A_STATS_RMON_CNT);+BUILD_BUG_ON(sizeof(*data->xdp_stats)/sizeof(u64)!=+__ETHTOOL_A_STATS_XDP_CNT);/* Mark all stats as unset (see ETHTOOL_STAT_NOT_SET) to prevent them*frombeingreportedtouserspaceincasedriverdidnotsetthem.
@@ -146,15 +197,19 @@ static int stats_prepare_data(const struct ethnl_req_info *req_base,if(test_bit(ETHTOOL_STATS_RMON,req_info->stat_mask)&&ops->get_rmon_stats)ops->get_rmon_stats(dev,&data->rmon_stats,&data->rmon_ranges);+if(test_bit(ETHTOOL_STATS_XDP,req_info->stat_mask)&&+ops->get_xdp_stats)+ret=stats_prepare_data_xdp(dev,data);ethnl_ops_complete(dev);-return0;+returnret;}staticintstats_reply_size(conststructethnl_req_info*req_base,conststructethnl_reply_data*reply_base){conststructstats_req_info*req_info=STATS_REQINFO(req_base);+conststructstats_reply_data*data=STATS_REPDATA(reply_base);unsignedintn_grps=0,n_stats=0;intlen=0;
From: Alexander Lobakin <hidden> Date: 2021-08-03 16:37:29
DPAA2 driver has 4 XDP counters which align just fine with the
standard XDP stats. Convert the driver to use the new approach.
Note that those counters are stored per-channel, but originally
were being given to Ethtool as sums across all channels. This
change makes them per-channel in Ethtool as well by providing
a number of channels to the standard stats infra.
Signed-off-by: Alexander Lobakin <redacted>
Reviewed-by: Jesse Brandeburg <redacted>
---
.../net/ethernet/freescale/dpaa2/dpaa2-eth.h | 4 +--
.../ethernet/freescale/dpaa2/dpaa2-ethtool.c | 36 ++++++++++++++++---
2 files changed, 34 insertions(+), 6 deletions(-)
@@ -377,13 +377,13 @@ struct dpaa2_eth_ch_stats {__u64pull_err;/* Number of CDANs; useful to estimate avg NAPI len */__u64cdan;+/* The rest of the structure does not show up in ethtool stats */+struct{}__eth_end;/* XDP counters */__u64xdp_drop;__u64xdp_tx;__u64xdp_tx_err;__u64xdp_redirect;-/* The rest of the structure does not show up in ethtool stats */-struct{}__eth_end;/* Must be last */__u64frames;};
From: Alexander Lobakin <hidden> Date: 2021-08-03 16:37:37
Its 6 XDP per-channel counters align just fine with the standard
stats.
Drop them from the custom Ethtool statistics and expose to the
standard stats infra instead.
Signed-off-by: Alexander Lobakin <redacted>
Reviewed-by: Jesse Brandeburg <redacted>
---
drivers/net/ethernet/amazon/ena/ena_ethtool.c | 46 ++++++++++++++++---
1 file changed, 40 insertions(+), 6 deletions(-)
From: Alexander Lobakin <hidden> Date: 2021-08-03 16:37:41
This driver has 6 per-channel XDP counters. Convert them to 5
standard XDP stats (redirect_sg and redirect_failures go under
the same redirect_errors).
As this driver theoretically can have different numbers of RX
and TX rings, collect statistics separately for each direction.
Signed-off-by: Alexander Lobakin <redacted>
Reviewed-by: Jesse Brandeburg <redacted>
---
.../ethernet/freescale/enetc/enetc_ethtool.c | 58 ++++++++++++++-----
1 file changed, 44 insertions(+), 14 deletions(-)
@@ -192,18 +192,12 @@ static const struct {staticconstcharrx_ring_stats[][ETH_GSTRING_LEN]={"Rx ring %2d frames","Rx ring %2d alloc errors",-"Rx ring %2d XDP drops","Rx ring %2d recycles","Rx ring %2d recycle failures",-"Rx ring %2d redirects",-"Rx ring %2d redirect failures",-"Rx ring %2d redirect S/G",};staticconstchartx_ring_stats[][ETH_GSTRING_LEN]={"Tx ring %2d frames",-"Tx ring %2d XDP frames",-"Tx ring %2d XDP drops",};staticintenetc_get_sset_count(structnet_device*ndev,intsset)
From: Alexander Lobakin <hidden> Date: 2021-08-03 16:37:47
src and syncp pointers are being read only, and thus can be const.
Signed-off-by: Alexander Lobakin <redacted>
Reviewed-by: Jesse Brandeburg <redacted>
---
drivers/net/ethernet/amazon/ena/ena_ethtool.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Alexander Lobakin <hidden> Date: 2021-08-03 16:38:14
Don't hardcode ARRAY_SIZE() - 1, just put a placeholder and take
offsetof() from it to hide unwanted statistics from Ethtool.
Will be handy for switching to standard XDP stats.
Signed-off-by: Alexander Lobakin <redacted>
Reviewed-by: Jesse Brandeburg <redacted>
---
drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h | 7 ++++++-
drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c | 2 +-
2 files changed, 7 insertions(+), 2 deletions(-)
@@ -382,10 +382,15 @@ struct dpaa2_eth_ch_stats {__u64xdp_tx;__u64xdp_tx_err;__u64xdp_redirect;-/* Must be last, does not show up in ethtool stats */+/* The rest of the structure does not show up in ethtool stats */+struct{}__eth_end;+/* Must be last */__u64frames;};+#define DPAA2_ETH_NUM_CH_STATS (offsetof(struct dpaa2_eth_ch_stats, \+__eth_end)/sizeof(u64))+/* Maximum number of queues associated with a DPNI */#define DPAA2_ETH_MAX_TCS 8#define DPAA2_ETH_MAX_RX_QUEUES_PER_TC 16
From: Alexander Lobakin <hidden> Date: 2021-08-03 16:38:42
NETA driver also doesn't separate XDP xmit errors from drops, and in
that case more general "drops" should be used.
Rename the field before converting to standard XDP stats infra.
Signed-off-by: Alexander Lobakin <redacted>
Reviewed-by: Jesse Brandeburg <redacted>
---
drivers/net/ethernet/marvell/mvneta.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
From: Alexander Lobakin <hidden> Date: 2021-08-03 16:38:52
Convert PPv2 driver to provide standard XDP statistics instead of
custom-defined Ethtool stats. This also allows to greatly simplify
stats filling code.
In the same fashion as mvneta, the driver uses global XDP counters.
Signed-off-by: Alexander Lobakin <redacted>
Reviewed-by: Jesse Brandeburg <redacted>
---
.../net/ethernet/marvell/mvpp2/mvpp2_main.c | 90 +++++--------------
1 file changed, 20 insertions(+), 70 deletions(-)
@@ -5735,6 +5684,7 @@ static const struct ethtool_ops mvpp2_eth_tool_ops = {.set_rxfh=mvpp2_ethtool_set_rxfh,.get_rxfh_context=mvpp2_ethtool_get_rxfh_context,.set_rxfh_context=mvpp2_ethtool_set_rxfh_context,+.get_xdp_stats=mvpp2_ethtool_get_xdp_stats,};/* Used for PPv2.1, or PPv2.2 with the old Device Tree binding that
From: Alexander Lobakin <hidden> Date: 2021-08-03 16:39:05
Just like DPAA2 driver, EF{100,X} store XDP stats per-channel, but
present them as the sums across all channels.
Switch to the standard per-channel XDP stats. n_rx_xdp_bad_drops
goes as "general XDP errors", because driver uses just one counter
for all kinds of errors.
Signed-off-by: Alexander Lobakin <redacted>
Reviewed-by: Jesse Brandeburg <redacted>
---
drivers/net/ethernet/sfc/ef100_ethtool.c | 2 ++
drivers/net/ethernet/sfc/ethtool.c | 2 ++
drivers/net/ethernet/sfc/ethtool_common.c | 35 ++++++++++++++++++++---
drivers/net/ethernet/sfc/ethtool_common.h | 3 ++
4 files changed, 38 insertions(+), 4 deletions(-)
@@ -557,6 +553,37 @@ void efx_ethtool_get_stats(struct net_device *net_dev,efx_ptp_update_stats(efx,data);}+intefx_ethtool_get_std_stats_channels(structnet_device*net_dev,u32sset)+{+conststructefx_nic*efx=netdev_priv(net_dev);++switch(sset){+caseETH_SS_STATS_XDP:+returnefx->n_channels;+default:+return-EOPNOTSUPP;+}+}++voidefx_ethtool_get_xdp_stats(structnet_device*net_dev,+structethtool_xdp_stats*xdp_stats)+{+structefx_nic*efx=netdev_priv(net_dev);+conststructefx_channel*channel;++spin_lock_bh(&efx->stats_lock);++efx_for_each_channel(channel,efx){+xdp_stats->drop=channel->n_rx_xdp_drops;+xdp_stats->errors=channel->n_rx_xdp_bad_drops;+xdp_stats->redirect=channel->n_rx_xdp_redirect;+xdp_stats->tx=channel->n_rx_xdp_tx;+xdp_stats++;+}++spin_unlock_bh(&efx->stats_lock);+}+/* This must be called with rtnl_lock held. */intefx_ethtool_get_link_ksettings(structnet_device*net_dev,structethtool_link_ksettings*cmd)
From: Alexander Lobakin <hidden> Date: 2021-08-03 16:39:09
Replace custom Ethtools XDP statistics with the standard infra based
one (7 basic fields).
This driver uses global [per-cpu] stats, no other callbacks needed.
Signed-off-by: Alexander Lobakin <redacted>
Reviewed-by: Jesse Brandeburg <redacted>
---
drivers/net/ethernet/marvell/mvneta.c | 108 +++++++++++++-------------
1 file changed, 52 insertions(+), 56 deletions(-)
From: Alexander Lobakin <hidden> Date: 2021-08-03 16:39:11
veth keeps tracking of numbers of XDP frames being dropped from
inside of .ndo_xdp_xmit() callback. This counter really should
be named after drops, not errors.
Signed-off-by: Alexander Lobakin <redacted>
Reviewed-by: Jesse Brandeburg <redacted>
---
drivers/net/veth.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
From: Alexander Lobakin <hidden> Date: 2021-08-03 16:39:15
These two are present to track the number of frames processed and
dropped by .ndo_xdp_xmit() callback, rename them accordingly to
avoid confusion with xdp_tx RQ field and XDP_TX case in general.
Signed-off-by: Alexander Lobakin <redacted>
Reviewed-by: Jesse Brandeburg <redacted>
---
drivers/net/virtio_net.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
From: Alexander Lobakin <hidden> Date: 2021-08-03 16:39:18
They get updated not only on XDP path. Moreover, packet counter
stores the total number of frames, not only the ones passed to
bpf_prog_run_xdp(), so it's rather confusing.
Drop the xdp_ suffix from both of them to not mix XDP-only stats
with the general ones.
Signed-off-by: Alexander Lobakin <redacted>
Reviewed-by: Jesse Brandeburg <redacted>
---
drivers/net/veth.c | 36 ++++++++++++++++++------------------
1 file changed, 18 insertions(+), 18 deletions(-)
From: Alexander Lobakin <hidden> Date: 2021-08-03 16:39:26
rx_drops field stores the number of errors occurred on XDP path, such
as XDP_TX or XDP_REDIRECT non-zero return codes. There are no stores
of this field outside XDP code.
Give it a more fitting name which also aligns well with the standard
XDP stats.
Signed-off-by: Alexander Lobakin <redacted>
Reviewed-by: Jesse Brandeburg <redacted>
---
drivers/net/veth.c | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
From: Alexander Lobakin <hidden> Date: 2021-08-03 16:39:32
xdp_xmit_err stat is defined as num_frames - nxmit in
.ndo_xdp_xmit() callback implementation, and regarding that the
frames which weren't transmitted are treated as drops by BPF core,
give it a more fitting name in preparation for switching to standard
XDP stats.
Signed-off-by: Alexander Lobakin <redacted>
Reviewed-by: Jesse Brandeburg <redacted>
---
drivers/net/ethernet/marvell/mvpp2/mvpp2.h | 2 +-
drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 16 ++++++++--------
2 files changed, 9 insertions(+), 9 deletions(-)
From: Alexander Lobakin <hidden> Date: 2021-08-03 16:39:35
veth has 7 per-channel XDP counters which could be aligned to the
standard XDP stats. Peer stats are here too as well, with the
original channel numbering logics.
Signed-off-by: Alexander Lobakin <redacted>
Reviewed-by: Jesse Brandeburg <redacted>
---
drivers/net/veth.c | 91 +++++++++++++++++++++++++++++-----------------
1 file changed, 57 insertions(+), 34 deletions(-)
From: Alexander Lobakin <hidden> Date: 2021-08-03 16:42:32
These two are present to track the number of frames processed and
dropped by .ndo_xdp_xmit() callback, rename them accordingly to
avoid confusion with xdp_tx RQ field and XDP_TX case in general.
Signed-off-by: Alexander Lobakin <redacted>
Reviewed-by: Jesse Brandeburg <redacted>
---
drivers/net/virtio_net.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
From: Alexander Lobakin <hidden> Date: 2021-08-03 16:42:43
It's pretty confusing to have just one field for tracking both
XDP_DROP cases and various errors which lead to the frame being
dropped.
Add a new field, xdp_errors, to separate error paths, and leave
xdp_drops purely for counting frames with the XDP_DROP verdict.
Signed-off-by: Alexander Lobakin <redacted>
Reviewed-by: Jesse Brandeburg <redacted>
---
drivers/net/virtio_net.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
From: Alexander Lobakin <hidden> Date: 2021-08-03 16:42:59
Now that we have 1:1 correspondence between the driver XDP stats and
the standard XDP stats, we can go forth and convert the driver to
expose XDP statistics via our standard interface.
Signed-off-by: Alexander Lobakin <redacted>
Reviewed-by: Jesse Brandeburg <redacted>
---
drivers/net/virtio_net.c | 52 ++++++++++++++++++++++++++++++++++------
1 file changed, 45 insertions(+), 7 deletions(-)
From: Alexander Lobakin <hidden> Date: 2021-08-03 16:43:14
Reflect the addition of the new standard XDP stats as well as of
a new NL attribute.
Signed-off-by: Alexander Lobakin <redacted>
Reviewed-by: Jesse Brandeburg <redacted>
---
Documentation/networking/ethtool-netlink.rst | 45 +++++++++++++-------
1 file changed, 30 insertions(+), 15 deletions(-)
@@ -1415,21 +1415,26 @@ Request contents: Kernel response contents:- +-----------------------------------+--------+--------------------------------+-|``ETHTOOL_A_STATS_HEADER`` | nested | reply header |- +-----------------------------------+--------+--------------------------------+-|``ETHTOOL_A_STATS_GRP`` | nested | one or more group of stats |- +-+---------------------------------+--------+--------------------------------+-| | ``ETHTOOL_A_STATS_GRP_ID`` | u32 | group ID - ``ETHTOOL_STATS_*`` |- +-+---------------------------------+--------+--------------------------------+-| | ``ETHTOOL_A_STATS_GRP_SS_ID`` | u32 | string set ID for names |- +-+---------------------------------+--------+--------------------------------+-| | ``ETHTOOL_A_STATS_GRP_STAT`` | nested | nest containing a statistic |- +-+---------------------------------+--------+--------------------------------+-| | ``ETHTOOL_A_STATS_GRP_HIST_RX`` | nested | histogram statistic (Rx) |- +-+---------------------------------+--------+--------------------------------+-| | ``ETHTOOL_A_STATS_GRP_HIST_TX`` | nested | histogram statistic (Tx) |- +-+---------------------------------+--------+--------------------------------++ +--------------------------------------+--------+-----------------------------++|``ETHTOOL_A_STATS_HEADER`` | nested | reply header |+ +--------------------------------------+--------+-----------------------------++|``ETHTOOL_A_STATS_GRP`` | nested | one or more group of stats |+ +-+------------------------------------+--------+-----------------------------++| | ``ETHTOOL_A_STATS_GRP_ID`` | u32 | group ID - |+| | | | ``ETHTOOL_STATS_*`` |+ +-+------------------------------------+--------+-----------------------------++| | ``ETHTOOL_A_STATS_GRP_SS_ID`` | u32 | string set ID for names |+ +-+------------------------------------+--------+-----------------------------++| | ``ETHTOOL_A_STATS_GRP_STAT`` | nested | nest containing a statistic |+ +-+------------------------------------+--------+-----------------------------++| | ``ETHTOOL_A_STATS_GRP_STAT_BLOCK`` | nested | block of stats per channel |+ +-+-+----------------------------------+--------+-----------------------------++| | | ``ETHTOOL_A_STATS_GRP_STAT`` | nested | nest containing a statistic |+ +-+-+----------------------------------+--------+-----------------------------++| | ``ETHTOOL_A_STATS_GRP_HIST_RX`` | nested | histogram statistic (Rx) |+ +-+------------------------------------+--------+-----------------------------++| | ``ETHTOOL_A_STATS_GRP_HIST_TX`` | nested | histogram statistic (Tx) |+ +-+------------------------------------+--------+-----------------------------+ Users specify which groups of statistics they are requesting via the ``ETHTOOL_A_STATS_GROUPS`` bitset. Currently defined values are:
@@ -1439,6 +1444,7 @@ the ``ETHTOOL_A_STATS_GROUPS`` bitset. Currently defined values are: ETHTOOL_STATS_ETH_PHY eth-phy Basic IEEE 802.3 PHY statistics (30.3.2.1.*) ETHTOOL_STATS_ETH_CTRL eth-ctrl Basic IEEE 802.3 MAC Ctrl statistics (30.3.3.*) ETHTOOL_STATS_RMON rmon RMON (RFC 2819) statistics+ ETHTOOL_STATS_XDP xdp XDP statistics ====================== ======== =============================================== Each group should have a corresponding ``ETHTOOL_A_STATS_GRP`` in the reply.
@@ -1451,6 +1457,10 @@ Statistics are added to the ``ETHTOOL_A_STATS_GRP`` nest under single 8 byte (u64) attribute inside - the type of that attribute is the statistic ID and the value is the value of the statistic. Each group has its own interpretation of statistic IDs.+Statistics can be folded into a consistent (non-broken with any other attr)+sequence of blocks ``ETHTOOL_A_STATS_GRP_STAT_BLOCK``. This way they are+treated by Ethtool as per-channel statistics, and are printed with the+"channel%d-" prefix. Attribute IDs correspond to strings from the string set identified by ``ETHTOOL_A_STATS_GRP_SS_ID``. Complex statistics (such as RMON histogram entries) are also listed inside ``ETHTOOL_A_STATS_GRP`` and do not have
@@ -1479,6 +1489,11 @@ Low and high bounds are inclusive, for example: etherStatsPkts512to1023Octets 512 1023 ============================= ==== ====+Drivers which want to export global (per-device) XDP statistics should+only implement ``get_xdp_stats`` callback. An additional one+``get_std_stats_channels`` is needed if the driver exposes per-channel+statistics.+ PHC_VCLOCKS_GET ===============
From: Edward Cree <ecree.xilinx@gmail.com> Date: 2021-08-03 17:59:15
On 03/08/2021 17:36, Alexander Lobakin wrote:
Just like DPAA2 driver, EF{100,X} store XDP stats per-channel, but
present them as the sums across all channels.
Switch to the standard per-channel XDP stats. n_rx_xdp_bad_drops
goes as "general XDP errors", because driver uses just one counter
for all kinds of errors.
Signed-off-by: Alexander Lobakin <redacted>
Reviewed-by: Jesse Brandeburg <redacted>
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-08-03 20:49:11
On Tue, 3 Aug 2021 18:36:23 +0200 Alexander Lobakin wrote:
Most of the driver-side XDP enabled drivers provide some statistics
on XDP programs runs and different actions taken (number of passes,
drops, redirects etc.).
Could you please share the statistics to back that statement up?
Having uAPI for XDP stats is pretty much making the recommendation
that drivers should implement such stats. The recommendation from
Alexei and others back in the day (IIRC) was that XDP programs should
implement stats, not the drivers, to avoid duplication.
Regarding that it's almost pretty the same across all the drivers
(which is obvious), we can implement some sort of "standardized"
statistics using Ethtool standard stats infra to eliminate a lot
of code and stringsets duplication, different approaches to count
these stats and so on.
I'm not 100% sold on the fact that these should be ethtool stats.
Why not rtnl_fill_statsinfo() stats? Current ethtool std stats are
all pretty Ethernet specific, and all HW stats. Mixing HW and SW stats
is what we're trying to get away from.
These new 12 fields provided by the standard XDP stats should cover
most, if not all, stats that might be interesting for collecting and
tracking.
Note that most NIC drivers keep XDP statistics on a per-channel
basis, so this also introduces a new callback for getting a number
of channels which a driver will provide stats for. If it's not
implemented or returns 0, it means stats are global/device-wide.
Per-channel stats via std ethtool stats are not a good idea. Per queue
stats must be via the queue netlink interface we keep talking about for
ever but which doesn't seem to materialize. When stats are reported via
a different interface than objects they pertain to matching stats,
objects and their lifetime becomes very murky.
On Tue, 2021-08-03 at 13:49 -0700, Jakub Kicinski wrote:
On Tue, 3 Aug 2021 18:36:23 +0200 Alexander Lobakin wrote:
quoted
Most of the driver-side XDP enabled drivers provide some statistics
on XDP programs runs and different actions taken (number of passes,
drops, redirects etc.).
Could you please share the statistics to back that statement up?
Having uAPI for XDP stats is pretty much making the recommendation
that drivers should implement such stats. The recommendation from
Alexei and others back in the day (IIRC) was that XDP programs should
implement stats, not the drivers, to avoid duplication.
There are stats "mainly errors*" that are not even visible or reported
to the user prog, for that i had an idea in the past to attach an
exception_bpf_prog provided by the user, where driver/stack will report
errors to this special exception_prog.
quoted
Regarding that it's almost pretty the same across all the drivers
(which is obvious), we can implement some sort of "standardized"
statistics using Ethtool standard stats infra to eliminate a lot
of code and stringsets duplication, different approaches to count
these stats and so on.
I'm not 100% sold on the fact that these should be ethtool stats.
Why not rtnl_fill_statsinfo() stats? Current ethtool std stats are
all pretty Ethernet specific, and all HW stats. Mixing HW and SW
stats
is what we're trying to get away from.
XDP is going to always be eBPF based ! why not just report such stats
to a special BPF_MAP ? BPF stack can collect the stats from the driver
and report them to this special MAP upon user request.
quoted
These new 12 fields provided by the standard XDP stats should cover
most, if not all, stats that might be interesting for collecting
and
tracking.
Note that most NIC drivers keep XDP statistics on a per-channel
basis, so this also introduces a new callback for getting a number
of channels which a driver will provide stats for. If it's not
implemented or returns 0, it means stats are global/device-wide.
Per-channel stats via std ethtool stats are not a good idea. Per
queue
stats must be via the queue netlink interface we keep talking about
for
ever but which doesn't seem to materialize. When stats are reported
via
a different interface than objects they pertain to matching stats,
objects and their lifetime becomes very murky.
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-08-04 12:36:55
On Tue, 03 Aug 2021 16:57:22 -0700 Saeed Mahameed wrote:
On Tue, 2021-08-03 at 13:49 -0700, Jakub Kicinski wrote:
quoted
On Tue, 3 Aug 2021 18:36:23 +0200 Alexander Lobakin wrote:
quoted
Most of the driver-side XDP enabled drivers provide some statistics
on XDP programs runs and different actions taken (number of passes,
drops, redirects etc.).
Could you please share the statistics to back that statement up?
Having uAPI for XDP stats is pretty much making the recommendation
that drivers should implement such stats. The recommendation from
Alexei and others back in the day (IIRC) was that XDP programs should
implement stats, not the drivers, to avoid duplication.
There are stats "mainly errors*" that are not even visible or reported
to the user prog,
Fair point, exceptions should not be performance critical.
for that i had an idea in the past to attach an
exception_bpf_prog provided by the user, where driver/stack will report
errors to this special exception_prog.
Or maybe we should turn trace_xdp_exception() into a call which
unconditionally collects exception stats? I think we can reasonably
expect the exception_bpf_prog to always be attached, right?
quoted
quoted
Regarding that it's almost pretty the same across all the drivers
(which is obvious), we can implement some sort of "standardized"
statistics using Ethtool standard stats infra to eliminate a lot
of code and stringsets duplication, different approaches to count
these stats and so on.
I'm not 100% sold on the fact that these should be ethtool stats.
Why not rtnl_fill_statsinfo() stats? Current ethtool std stats are
all pretty Ethernet specific, and all HW stats. Mixing HW and SW
stats
is what we're trying to get away from.
XDP is going to always be eBPF based ! why not just report such stats
to a special BPF_MAP ? BPF stack can collect the stats from the driver
and report them to this special MAP upon user request.
Do you mean replacing the ethtool-netlink / rtnetlink etc. with
a new BPF_MAP? I don't think adding another category of uAPI thru
which netdevice stats are exposed would do much good :( Plus it
doesn't address the "yet another cacheline" concern.
To my understanding the need for stats recognizes the fact that (in
large organizations) fleet monitoring is done by different teams than
XDP development. So XDP team may have all the stats they need, but the
team doing fleet monitoring has no idea how to get to them.
To bridge the two worlds we need a way for the infra team to ask the
XDP for well-defined stats. Maybe we should take a page from the BPF
iterators book and create a program type for bridging the two worlds?
Called by networking core when duping stats to extract from the
existing BPF maps all the relevant stats and render them into a well
known struct? Users' XDP design can still use a single per-cpu map with
all the stats if they so choose, but there's a way to implement more
optimal designs and still expose well-defined stats.
Maybe that's too complex, IDK.
Its 6 XDP per-channel counters align just fine with the standard
stats.
Drop them from the custom Ethtool statistics and expose to the
standard stats infra instead.
Signed-off-by: Alexander Lobakin <redacted>
Reviewed-by: Jesse Brandeburg <redacted>
---
drivers/net/ethernet/amazon/ena/ena_ethtool.c | 46
++++++++++++++++---
1 file changed, 40 insertions(+), 6 deletions(-)
Hi,
thanks for making this patch. I like the idea of splitting stats
into a per-queue basis
The ena_stats_rx_strings array is (indirectly) accessed through
ena_get_stats() function which is used for both fetching ethtool
stats and
for sharing the stats with the device in case of an error (through
ena_dump_stats_ex() function).
The latter use is broken by removing the XDP specific stats from
ena_stats_rx_strings array.
I can submit an adaptation for the new system later (similar to
mlx5) if you prefer
thanks,
Shay
Its 6 XDP per-channel counters align just fine with the standard
stats.
Drop them from the custom Ethtool statistics and expose to the
standard stats infra instead.
Signed-off-by: Alexander Lobakin <redacted>
Reviewed-by: Jesse Brandeburg <redacted>
---
drivers/net/ethernet/amazon/ena/ena_ethtool.c | 46
++++++++++++++++---
1 file changed, 40 insertions(+), 6 deletions(-)
Hi,
thanks for making this patch. I like the idea of splitting stats
into a per-queue basis
The ena_stats_rx_strings array is (indirectly) accessed through
ena_get_stats() function which is used for both fetching ethtool
stats and
for sharing the stats with the device in case of an error (through
ena_dump_stats_ex() function).
The latter use is broken by removing the XDP specific stats from
ena_stats_rx_strings array.
I can submit an adaptation for the new system later (similar to
mlx5) if you prefer
Feel free to either do that (I'll exclude this patch from that
series then) or you can give me some little tips or examples or
anything on how to improve this one, so ena would stay converted.
Both ways are fine for me.
From: Alexander Lobakin <hidden> Date: 2021-08-04 15:53:52
From: Jakub Kicinski <kuba@kernel.org>
Date: Wed, 4 Aug 2021 05:36:50 -0700
On Tue, 03 Aug 2021 16:57:22 -0700 Saeed Mahameed wrote:
quoted
On Tue, 2021-08-03 at 13:49 -0700, Jakub Kicinski wrote:
quoted
On Tue, 3 Aug 2021 18:36:23 +0200 Alexander Lobakin wrote:
quoted
Most of the driver-side XDP enabled drivers provide some statistics
on XDP programs runs and different actions taken (number of passes,
drops, redirects etc.).
Could you please share the statistics to back that statement up?
Having uAPI for XDP stats is pretty much making the recommendation
that drivers should implement such stats. The recommendation from
Alexei and others back in the day (IIRC) was that XDP programs should
implement stats, not the drivers, to avoid duplication.
Well, 20+ patches in the series with at least half of them is
drivers conversion. Plus mlx5. Plus we'll about to land XDP
statistics for all Intel drivers, just firstly need to get a
common infra for them (the purpose of this series).
Also, introducing IEEE and rmon stats didn't make a statement that
all drivers should really expose them, right?
quoted
There are stats "mainly errors*" that are not even visible or reported
to the user prog,
Not really. Many drivers like to count the number of redirects,
xdp_xmits and stuff (incl. mlx5). Nevertheless, these stats aren't
the same as something you can get from inside an XDP prog, right.
Fair point, exceptions should not be performance critical.
quoted
for that i had an idea in the past to attach an
exception_bpf_prog provided by the user, where driver/stack will report
errors to this special exception_prog.
Or maybe we should turn trace_xdp_exception() into a call which
unconditionally collects exception stats? I think we can reasonably
expect the exception_bpf_prog to always be attached, right?
trace_xdp_exception() is again a error path, and would restrict us
to have only "bad" statistics.
quoted
quoted
quoted
Regarding that it's almost pretty the same across all the drivers
(which is obvious), we can implement some sort of "standardized"
statistics using Ethtool standard stats infra to eliminate a lot
of code and stringsets duplication, different approaches to count
these stats and so on.
I'm not 100% sold on the fact that these should be ethtool stats.
Why not rtnl_fill_statsinfo() stats? Current ethtool std stats are
all pretty Ethernet specific, and all HW stats. Mixing HW and SW
stats
is what we're trying to get away from.
I was trying to introduce as few functional changes as possible,
including that all the current drivers expose XDP stats through
Ethtool.
I don't say it's a 100% optimal way, but lots of different scripts
and monitoring tools are already based on this fact and there can
be some negative impact. There'll be for sure due to that std stats
is a bit different thing and different drivers count and name XDP
stats differently (breh).
BTW, I'm fine with rtnl xstats. A nice reminder, thanks. If there
won't be much cons like "don't touch our Ethtool stats", I would
prefer this one instead of Ethtool standard stats way.
quoted
XDP is going to always be eBPF based ! why not just report such stats
to a special BPF_MAP ? BPF stack can collect the stats from the driver
and report them to this special MAP upon user request.
Do you mean replacing the ethtool-netlink / rtnetlink etc. with
a new BPF_MAP? I don't think adding another category of uAPI thru
which netdevice stats are exposed would do much good :( Plus it
doesn't address the "yet another cacheline" concern.
+ this makes obtaining/tracking the statistics much harder. For now,
all you need is `ethtool -S devname` (mainline) or
`ethtool -S devname --groups xdp` (this series), and obtaining rtnl
xstats is just a different command to invoke. BPF_MAP-based stats
are a completely different story then.
To my understanding the need for stats recognizes the fact that (in
large organizations) fleet monitoring is done by different teams than
XDP development. So XDP team may have all the stats they need, but the
team doing fleet monitoring has no idea how to get to them.
To bridge the two worlds we need a way for the infra team to ask the
XDP for well-defined stats. Maybe we should take a page from the BPF
iterators book and create a program type for bridging the two worlds?
Called by networking core when duping stats to extract from the
existing BPF maps all the relevant stats and render them into a well
known struct? Users' XDP design can still use a single per-cpu map with
all the stats if they so choose, but there's a way to implement more
optimal designs and still expose well-defined stats.
Maybe that's too complex, IDK.
From: David Ahern <hidden> Date: 2021-08-04 16:18:06
On 8/4/21 6:36 AM, Jakub Kicinski wrote:
quoted
XDP is going to always be eBPF based ! why not just report such stats
to a special BPF_MAP ? BPF stack can collect the stats from the driver
and report them to this special MAP upon user request.
Do you mean replacing the ethtool-netlink / rtnetlink etc. with
a new BPF_MAP? I don't think adding another category of uAPI thru
which netdevice stats are exposed would do much good :( Plus it
doesn't address the "yet another cacheline" concern.
To my understanding the need for stats recognizes the fact that (in
large organizations) fleet monitoring is done by different teams than
XDP development. So XDP team may have all the stats they need, but the
team doing fleet monitoring has no idea how to get to them.
To bridge the two worlds we need a way for the infra team to ask the
XDP for well-defined stats. Maybe we should take a page from the BPF
iterators book and create a program type for bridging the two worlds?
Called by networking core when duping stats to extract from the
existing BPF maps all the relevant stats and render them into a well
known struct? Users' XDP design can still use a single per-cpu map with
all the stats if they so choose, but there's a way to implement more
optimal designs and still expose well-defined stats.
Maybe that's too complex, IDK.
I was just explaining to someone internally how to get stats at all of
the different points in the stack to track down reasons for dropped packets:
ethtool -S for h/w and driver
tc -s for drops by the qdisc
/proc/net/softnet_stat for drops at the backlog layer
netstat -s for network and transport layer
yet another command and API just adds to the nightmare of explaining and
understanding these stats.
There is real value in continuing to use ethtool API for XDP stats. Not
saying this reorg of the XDP stats is the right thing to do, only that
the existing API has real user benefits.
Does anyone have data that shows bumping a properly implemented counter
causes a noticeable performance degradation and if so by how much? You
mention 'yet another cacheline' but collecting stats on stack and
incrementing the driver structs at the end of the napi loop should not
have a huge impact versus the value the stats provide.
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-08-04 16:44:38
On Wed, 4 Aug 2021 10:17:56 -0600 David Ahern wrote:
On 8/4/21 6:36 AM, Jakub Kicinski wrote:
quoted
quoted
XDP is going to always be eBPF based ! why not just report such stats
to a special BPF_MAP ? BPF stack can collect the stats from the driver
and report them to this special MAP upon user request.
Do you mean replacing the ethtool-netlink / rtnetlink etc. with
a new BPF_MAP? I don't think adding another category of uAPI thru
which netdevice stats are exposed would do much good :( Plus it
doesn't address the "yet another cacheline" concern.
To my understanding the need for stats recognizes the fact that (in
large organizations) fleet monitoring is done by different teams than
XDP development. So XDP team may have all the stats they need, but the
team doing fleet monitoring has no idea how to get to them.
To bridge the two worlds we need a way for the infra team to ask the
XDP for well-defined stats. Maybe we should take a page from the BPF
iterators book and create a program type for bridging the two worlds?
Called by networking core when duping stats to extract from the
existing BPF maps all the relevant stats and render them into a well
known struct? Users' XDP design can still use a single per-cpu map with
all the stats if they so choose, but there's a way to implement more
optimal designs and still expose well-defined stats.
Maybe that's too complex, IDK.
I was just explaining to someone internally how to get stats at all of
the different points in the stack to track down reasons for dropped packets:
ethtool -S for h/w and driver
tc -s for drops by the qdisc
/proc/net/softnet_stat for drops at the backlog layer
netstat -s for network and transport layer
yet another command and API just adds to the nightmare of explaining and
understanding these stats.
Are you referring to RTM_GETSTATS when you say "yet another command"?
RTM_GETSTATS exists and is used by offloads today.
I'd expect ip -s (-s) to be extended to run GETSTATS and display the xdp
stats. (Not sure why ip -s was left out of your list :))
There is real value in continuing to use ethtool API for XDP stats. Not
saying this reorg of the XDP stats is the right thing to do, only that
the existing API has real user benefits.
RTM_GETSTATS is an existing API. New ethtool stats are intended to be HW
stats. I don't want to go back to ethtool being a dumping ground for all
stats because that's what the old interface encouraged.
Does anyone have data that shows bumping a properly implemented counter
causes a noticeable performance degradation and if so by how much? You
mention 'yet another cacheline' but collecting stats on stack and
incrementing the driver structs at the end of the napi loop should not
have a huge impact versus the value the stats provide.
Not sure, maybe Jesper has some numbers. Maybe Intel folks do?
I'm just allergic to situations when there is a decision made and
then months later patches are posted disregarding the decision,
without analysis on why that decision was wrong. And while the
maintainer who made the decision is on vacation.
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-08-04 16:57:23
On Wed, 4 Aug 2021 17:53:27 +0200 Alexander Lobakin wrote:
From: Jakub Kicinski <kuba@kernel.org>
Date: Wed, 4 Aug 2021 05:36:50 -0700
quoted
On Tue, 03 Aug 2021 16:57:22 -0700 Saeed Mahameed wrote:
quoted
On Tue, 2021-08-03 at 13:49 -0700, Jakub Kicinski wrote:
quoted
On Tue, 3 Aug 2021 18:36:23 +0200 Alexander Lobakin wrote:
quoted
Most of the driver-side XDP enabled drivers provide some statistics
on XDP programs runs and different actions taken (number of passes,
drops, redirects etc.).
Could you please share the statistics to back that statement up?
Having uAPI for XDP stats is pretty much making the recommendation
that drivers should implement such stats. The recommendation from
Alexei and others back in the day (IIRC) was that XDP programs should
implement stats, not the drivers, to avoid duplication.
Well, 20+ patches in the series with at least half of them is
drivers conversion. Plus mlx5. Plus we'll about to land XDP
statistics for all Intel drivers, just firstly need to get a
common infra for them (the purpose of this series).
Great, do you have impact of the stats on Intel drivers?
(Preferably from realistic scenarios where CPU cache is actually
under pressure, not { return XDP_PASS; }). Numbers win arguments.
Also, introducing IEEE and rmon stats didn't make a statement that
all drivers should really expose them, right?
That's not relevant. IEEE and RMON stats are read from HW, they have
no impact on the SW fast path.
quoted
quoted
There are stats "mainly errors*" that are not even visible or reported
to the user prog,
Not really. Many drivers like to count the number of redirects,
xdp_xmits and stuff (incl. mlx5). Nevertheless, these stats aren't
the same as something you can get from inside an XDP prog, right.
quoted
Fair point, exceptions should not be performance critical.
quoted
for that i had an idea in the past to attach an
exception_bpf_prog provided by the user, where driver/stack will report
errors to this special exception_prog.
Or maybe we should turn trace_xdp_exception() into a call which
unconditionally collects exception stats? I think we can reasonably
expect the exception_bpf_prog to always be attached, right?
trace_xdp_exception() is again a error path, and would restrict us
to have only "bad" statistics.
quoted
quoted
quoted
quoted
Regarding that it's almost pretty the same across all the drivers
(which is obvious), we can implement some sort of "standardized"
statistics using Ethtool standard stats infra to eliminate a lot
of code and stringsets duplication, different approaches to count
these stats and so on.
I'm not 100% sold on the fact that these should be ethtool stats.
Why not rtnl_fill_statsinfo() stats? Current ethtool std stats are
all pretty Ethernet specific, and all HW stats. Mixing HW and SW
stats
is what we're trying to get away from.
I was trying to introduce as few functional changes as possible,
including that all the current drivers expose XDP stats through
Ethtool.
You know this, but for the benefit of others - ethtool -S does not
dump standard stats from the netlink API, and ethtool -S --goups does
not dump "old" stats. So users will need to use different commands
to get to the two, anyway.
I don't say it's a 100% optimal way, but lots of different scripts
and monitoring tools are already based on this fact and there can
be some negative impact. There'll be for sure due to that std stats
is a bit different thing and different drivers count and name XDP
stats differently (breh).
That's concerning. I'd much rather you didn't convert all the drivers
than convert them before someone makes 100% sure the meaning of the
stats is equivalent.
BTW, I'm fine with rtnl xstats. A nice reminder, thanks. If there
won't be much cons like "don't touch our Ethtool stats", I would
prefer this one instead of Ethtool standard stats way.
You'll have to leave the ethtool -S ones in place anyway, right?
New drivers would not include them but I don't think there's much
we can (or should) do for the existing ones.
quoted
quoted
XDP is going to always be eBPF based ! why not just report such stats
to a special BPF_MAP ? BPF stack can collect the stats from the driver
and report them to this special MAP upon user request.
Do you mean replacing the ethtool-netlink / rtnetlink etc. with
a new BPF_MAP? I don't think adding another category of uAPI thru
which netdevice stats are exposed would do much good :( Plus it
doesn't address the "yet another cacheline" concern.
+ this makes obtaining/tracking the statistics much harder. For now,
all you need is `ethtool -S devname` (mainline) or
`ethtool -S devname --groups xdp` (this series), and obtaining rtnl
xstats is just a different command to invoke. BPF_MAP-based stats
are a completely different story then.
quoted
To my understanding the need for stats recognizes the fact that (in
large organizations) fleet monitoring is done by different teams than
XDP development. So XDP team may have all the stats they need, but the
team doing fleet monitoring has no idea how to get to them.
To bridge the two worlds we need a way for the infra team to ask the
XDP for well-defined stats. Maybe we should take a page from the BPF
iterators book and create a program type for bridging the two worlds?
Called by networking core when duping stats to extract from the
existing BPF maps all the relevant stats and render them into a well
known struct? Users' XDP design can still use a single per-cpu map with
all the stats if they so choose, but there's a way to implement more
optimal designs and still expose well-defined stats.
Maybe that's too complex, IDK.
From: David Ahern <hidden> Date: 2021-08-04 17:28:44
On 8/4/21 10:44 AM, Jakub Kicinski wrote:
On Wed, 4 Aug 2021 10:17:56 -0600 David Ahern wrote:
quoted
On 8/4/21 6:36 AM, Jakub Kicinski wrote:
quoted
quoted
XDP is going to always be eBPF based ! why not just report such stats
to a special BPF_MAP ? BPF stack can collect the stats from the driver
and report them to this special MAP upon user request.
Do you mean replacing the ethtool-netlink / rtnetlink etc. with
a new BPF_MAP? I don't think adding another category of uAPI thru
which netdevice stats are exposed would do much good :( Plus it
doesn't address the "yet another cacheline" concern.
To my understanding the need for stats recognizes the fact that (in
large organizations) fleet monitoring is done by different teams than
XDP development. So XDP team may have all the stats they need, but the
team doing fleet monitoring has no idea how to get to them.
To bridge the two worlds we need a way for the infra team to ask the
XDP for well-defined stats. Maybe we should take a page from the BPF
iterators book and create a program type for bridging the two worlds?
Called by networking core when duping stats to extract from the
existing BPF maps all the relevant stats and render them into a well
known struct? Users' XDP design can still use a single per-cpu map with
all the stats if they so choose, but there's a way to implement more
optimal designs and still expose well-defined stats.
Maybe that's too complex, IDK.
I was just explaining to someone internally how to get stats at all of
the different points in the stack to track down reasons for dropped packets:
ethtool -S for h/w and driver
tc -s for drops by the qdisc
/proc/net/softnet_stat for drops at the backlog layer
netstat -s for network and transport layer
yet another command and API just adds to the nightmare of explaining and
understanding these stats.
Are you referring to RTM_GETSTATS when you say "yet another command"?
RTM_GETSTATS exists and is used by offloads today.
I'd expect ip -s (-s) to be extended to run GETSTATS and display the xdp
stats. (Not sure why ip -s was left out of your list :))
It's on my diagram, and yes, forgot to add it here.
quoted
There is real value in continuing to use ethtool API for XDP stats. Not
saying this reorg of the XDP stats is the right thing to do, only that
the existing API has real user benefits.
RTM_GETSTATS is an existing API. New ethtool stats are intended to be HW
stats. I don't want to go back to ethtool being a dumping ground for all
stats because that's what the old interface encouraged.
driver stats are important too. e.g., mlx5's cache stats and per-queue
stats.
quoted
Does anyone have data that shows bumping a properly implemented counter
causes a noticeable performance degradation and if so by how much? You
mention 'yet another cacheline' but collecting stats on stack and
incrementing the driver structs at the end of the napi loop should not
have a huge impact versus the value the stats provide.
Not sure, maybe Jesper has some numbers. Maybe Intel folks do?
I just ran some quick tests with my setup and measured about 1.2% worst
case. Certainly not exhaustive. Perhaps Intel or Mellanox can provide
numbers for their high speed nics - e.g. ConnectX-6 and a saturated host.
I'm just allergic to situations when there is a decision made and
then months later patches are posted disregarding the decision,
without analysis on why that decision was wrong. And while the
maintainer who made the decision is on vacation.
stats is one of the many sensitive topics. I have been consistent in
defending the need to use existing APIs and tooling and not relying on
XDP program writers to add the relevant stats and then provide whatever
tool is needed to extract and print them. Standardization for
fundamental analysis tools.
On Wed, 2021-08-04 at 11:28 -0600, David Ahern wrote:
On 8/4/21 10:44 AM, Jakub Kicinski wrote:
quoted
On Wed, 4 Aug 2021 10:17:56 -0600 David Ahern wrote:
quoted
On 8/4/21 6:36 AM, Jakub Kicinski wrote:
quoted
quoted
XDP is going to always be eBPF based ! why not just report
such stats
to a special BPF_MAP ? BPF stack can collect the stats from
the driver
and report them to this special MAP upon user request.
Do you mean replacing the ethtool-netlink / rtnetlink etc. with
a new BPF_MAP? I don't think adding another category of uAPI
thru
which netdevice stats are exposed would do much good :( Plus it
doesn't address the "yet another cacheline" concern.
To my understanding the need for stats recognizes the fact that
(in
large organizations) fleet monitoring is done by different
teams than
XDP development. So XDP team may have all the stats they need,
but the
team doing fleet monitoring has no idea how to get to them.
To bridge the two worlds we need a way for the infra team to
ask the
XDP for well-defined stats. Maybe we should take a page from
the BPF
iterators book and create a program type for bridging the two
worlds?
Called by networking core when duping stats to extract from the
existing BPF maps all the relevant stats and render them into a
well
known struct? Users' XDP design can still use a single per-cpu
map with
all the stats if they so choose, but there's a way to implement
more
optimal designs and still expose well-defined stats.
Maybe that's too complex, IDK.
The main question here, do we want the prog to count or driver ?
and the answer will lead to more questions :) :
1) will the prog/user need to access driver for driver only stats ? or
driver shall report to a special program and all the collection and
reporting is done in XDP/BPF internally ..
2) stats per prog/queue/cpu/interface ?
3) how to eventually report to user ethtool/ip -s/bpftool ?
too complex, IDK too .. :D
quoted
quoted
I was just explaining to someone internally how to get stats at
all of
the different points in the stack to track down reasons for
dropped packets:
ethtool -S for h/w and driver
tc -s for drops by the qdisc
/proc/net/softnet_stat for drops at the backlog layer
netstat -s for network and transport layer
yet another command and API just adds to the nightmare of
explaining and
understanding these stats.
Are you referring to RTM_GETSTATS when you say "yet another
command"?
RTM_GETSTATS exists and is used by offloads today.
I'd expect ip -s (-s) to be extended to run GETSTATS and display
the xdp
stats. (Not sure why ip -s was left out of your list :))
It's on my diagram, and yes, forgot to add it here.
i think ip -s is a good place for "standard" driver based xdp stats.
but as Jakub already explained, adding such driver mechanism is like
making a statement that drivers must implement this.
quoted
quoted
There is real value in continuing to use ethtool API for XDP
stats. Not
saying this reorg of the XDP stats is the right thing to do, only
that
the existing API has real user benefits.
RTM_GETSTATS is an existing API. New ethtool stats are intended to
be HW
stats. I don't want to go back to ethtool being a dumping ground
for all
stats because that's what the old interface encouraged.
driver stats are important too. e.g., mlx5's cache stats and per-
queue
stats.
one could claim that mlx5 cache stats should move to page_pool and
per_queue stats should move to the stack.
quoted
quoted
Does anyone have data that shows bumping a properly implemented
counter
causes a noticeable performance degradation and if so by how
much? You
mention 'yet another cacheline' but collecting stats on stack and
incrementing the driver structs at the end of the napi loop
should not
have a huge impact versus the value the stats provide.
Not sure, maybe Jesper has some numbers. Maybe Intel folks do?
A properly implemented counter that doesn't introduce new cache misses,
will hardly show any measurable difference, the only way to measure is
via instructions per packet.
usually the way we implement counters in mlx5 is that if this is the
fastest flow that we expect then we only increment the good counters
"packet++/drop++/redirect++" any slower path should include counters to
indicate the slower path and the effect of the new "slower" counters
will still be negligible as we already are at a higher instructions per
packet hence the slower path ..
the only time you measure a difference is when you introduce new
counting on a counter-free flow, e.g page_pool ;)
I just ran some quick tests with my setup and measured about 1.2%
worst
1.2% is a lot ! what was the test ? what is the change ?
case. Certainly not exhaustive. Perhaps Intel or Mellanox can provide
numbers for their high speed nics - e.g. ConnectX-6 and a saturated
host.
let's define what are we testing first, there are multiple places we
need to check, Tariq will be exploring transitioning mlx5 cache to
page_pool with all the counters, maybe it is a good place to measure..
quoted
I'm just allergic to situations when there is a decision made and
then months later patches are posted disregarding the decision,
without analysis on why that decision was wrong. And while the
maintainer who made the decision is on vacation.
stats is one of the many sensitive topics. I have been consistent in
defending the need to use existing APIs and tooling and not relying
on
XDP program writers to add the relevant stats and then provide
whatever
tool is needed to extract and print them. Standardization for
fundamental analysis tools.
From: David Ahern <hidden> Date: 2021-08-05 00:43:31
On 8/4/21 12:27 PM, Saeed Mahameed wrote:
quoted
I just ran some quick tests with my setup and measured about 1.2%
worst
1.2% is a lot ! what was the test ? what is the change ?
I did say "quick test ... not exhaustive" and it was definitely
eyeballing a pps change over a small time window.
If multiple counters are bumped 20-25 million times a second (e.g. XDP
drop case), how measurable is it? I was just trying to ballpark the
overhead - 1%, 5%, more? If it is <~ 1% then there is no performance
argument in which case let's do the right thing for users - export via
existing APIs.
On Wed, 2021-08-04 at 18:43 -0600, David Ahern wrote:
On 8/4/21 12:27 PM, Saeed Mahameed wrote:
quoted
quoted
I just ran some quick tests with my setup and measured about 1.2%
worst
1.2% is a lot ! what was the test ? what is the change ?
I did say "quick test ... not exhaustive" and it was definitely
eyeballing a pps change over a small time window.
If multiple counters are bumped 20-25 million times a second (e.g.
XDP
drop case), how measurable is it? I was just trying to ballpark the
overhead - 1%, 5%, more? If it is <~ 1% then there is no performance
argument in which case let's do the right thing for users - export
via
existing APIs.
from experience I don't believe it can be more than 1% and yes on
existing APIs !
From: Alexander Lobakin <hidden> Date: 2021-08-05 11:19:06
From: Jakub Kicinski <kuba@kernel.org>
Date: Wed, 5 Aug 2021 09:57:16 -0700
On Wed, 4 Aug 2021 17:53:27 +0200 Alexander Lobakin wrote:
quoted
From: Jakub Kicinski <kuba@kernel.org>
Date: Wed, 4 Aug 2021 05:36:50 -0700
quoted
On Tue, 03 Aug 2021 16:57:22 -0700 Saeed Mahameed wrote:
quoted
On Tue, 2021-08-03 at 13:49 -0700, Jakub Kicinski wrote:
quoted
On Tue, 3 Aug 2021 18:36:23 +0200 Alexander Lobakin wrote:
quoted
Most of the driver-side XDP enabled drivers provide some statistics
on XDP programs runs and different actions taken (number of passes,
drops, redirects etc.).
Could you please share the statistics to back that statement up?
Having uAPI for XDP stats is pretty much making the recommendation
that drivers should implement such stats. The recommendation from
Alexei and others back in the day (IIRC) was that XDP programs should
implement stats, not the drivers, to avoid duplication.
Well, 20+ patches in the series with at least half of them is
drivers conversion. Plus mlx5. Plus we'll about to land XDP
statistics for all Intel drivers, just firstly need to get a
common infra for them (the purpose of this series).
Great, do you have impact of the stats on Intel drivers?
(Preferably from realistic scenarios where CPU cache is actually
under pressure, not { return XDP_PASS; }). Numbers win arguments.
quoted
Also, introducing IEEE and rmon stats didn't make a statement that
all drivers should really expose them, right?
That's not relevant. IEEE and RMON stats are read from HW, they have
no impact on the SW fast path.
True, I thought about this, but after the mail was sent, sorry.
quoted
quoted
quoted
There are stats "mainly errors*" that are not even visible or reported
to the user prog,
Not really. Many drivers like to count the number of redirects,
xdp_xmits and stuff (incl. mlx5). Nevertheless, these stats aren't
the same as something you can get from inside an XDP prog, right.
quoted
Fair point, exceptions should not be performance critical.
quoted
for that i had an idea in the past to attach an
exception_bpf_prog provided by the user, where driver/stack will report
errors to this special exception_prog.
Or maybe we should turn trace_xdp_exception() into a call which
unconditionally collects exception stats? I think we can reasonably
expect the exception_bpf_prog to always be attached, right?
trace_xdp_exception() is again a error path, and would restrict us
to have only "bad" statistics.
quoted
quoted
quoted
quoted
Regarding that it's almost pretty the same across all the drivers
(which is obvious), we can implement some sort of "standardized"
statistics using Ethtool standard stats infra to eliminate a lot
of code and stringsets duplication, different approaches to count
these stats and so on.
I'm not 100% sold on the fact that these should be ethtool stats.
Why not rtnl_fill_statsinfo() stats? Current ethtool std stats are
all pretty Ethernet specific, and all HW stats. Mixing HW and SW
stats
is what we're trying to get away from.
I was trying to introduce as few functional changes as possible,
including that all the current drivers expose XDP stats through
Ethtool.
You know this, but for the benefit of others - ethtool -S does not
dump standard stats from the netlink API, and ethtool -S --goups does
not dump "old" stats. So users will need to use different commands
to get to the two, anyway.
Ditto.
quoted
I don't say it's a 100% optimal way, but lots of different scripts
and monitoring tools are already based on this fact and there can
be some negative impact. There'll be for sure due to that std stats
is a bit different thing and different drivers count and name XDP
stats differently (breh).
That's concerning. I'd much rather you didn't convert all the drivers
than convert them before someone makes 100% sure the meaning of the
stats is equivalent.
quoted
BTW, I'm fine with rtnl xstats. A nice reminder, thanks. If there
won't be much cons like "don't touch our Ethtool stats", I would
prefer this one instead of Ethtool standard stats way.
You'll have to leave the ethtool -S ones in place anyway, right?
New drivers would not include them but I don't think there's much
we can (or should) do for the existing ones.
That's also a nice and fair one. So I leave a little conclusion
at the end of this message to clarify things.
quoted
quoted
quoted
XDP is going to always be eBPF based ! why not just report such stats
to a special BPF_MAP ? BPF stack can collect the stats from the driver
and report them to this special MAP upon user request.
Do you mean replacing the ethtool-netlink / rtnetlink etc. with
a new BPF_MAP? I don't think adding another category of uAPI thru
which netdevice stats are exposed would do much good :( Plus it
doesn't address the "yet another cacheline" concern.
+ this makes obtaining/tracking the statistics much harder. For now,
all you need is `ethtool -S devname` (mainline) or
`ethtool -S devname --groups xdp` (this series), and obtaining rtnl
xstats is just a different command to invoke. BPF_MAP-based stats
are a completely different story then.
quoted
To my understanding the need for stats recognizes the fact that (in
large organizations) fleet monitoring is done by different teams than
XDP development. So XDP team may have all the stats they need, but the
team doing fleet monitoring has no idea how to get to them.
To bridge the two worlds we need a way for the infra team to ask the
XDP for well-defined stats. Maybe we should take a page from the BPF
iterators book and create a program type for bridging the two worlds?
Called by networking core when duping stats to extract from the
existing BPF maps all the relevant stats and render them into a well
known struct? Users' XDP design can still use a single per-cpu map with
all the stats if they so choose, but there's a way to implement more
optimal designs and still expose well-defined stats.
Maybe that's too complex, IDK.
[ the conclusion I talk about above ]
From the concerns, ideas and thoughts gone throughout this thread,
I see the next steps as:
- don't use Ethtool standard stats infra as it's designed for HW
stats only;
- expose SW XDP stats via rtnl xstats which is currently used to
query SW/CPU-port stats from switchdev-based drivers;
- don't remove the existing Ethtool XDP stats provided by drivers,
just wire them up with the new API too;
- encourage driver developers to use this new API when they want to
provide any XDP stats, not the Ethtool stats which are already
overburdened and mix HW, SW and whatnot in most of the complex
drivers.
That really makes lot more sense for me than the v1 approach.
Anyways, I don't think I'll queue v2 before going on a vacation
which will happen in two days, so we have two more weeks to
discuss all this before I'll start the rework. I'll still be on
the line here and keep on tracking the thread and replying when
needed.
quoted
Thanks,
Al
Thanks everyone for the feedback, lots of precious stuff,
Al
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-08-05 13:31:08
On Thu, 5 Aug 2021 13:18:26 +0200 Alexander Lobakin wrote:
- encourage driver developers to use this new API when they want to
provide any XDP stats, not the Ethtool stats which are already
overburdened and mix HW, SW and whatnot in most of the complex
drivers.
On the question of adding the stats in general I'd still ask for
(a) performance analysis or (b) to start with just the exception
stats which I'd think should be uncontroversial.
On Wed, 4 Aug 2021 10:17:56 -0600 David Ahern wrote:
quoted
On 8/4/21 6:36 AM, Jakub Kicinski wrote:
quoted
Does anyone have data that shows bumping a properly implemented counter
causes a noticeable performance degradation and if so by how much? You
mention 'yet another cacheline' but collecting stats on stack and
incrementing the driver structs at the end of the napi loop should not
have a huge impact versus the value the stats provide.
Not sure, maybe Jesper has some numbers. Maybe Intel folks do?
(sorry, behind on emails after vacation ... just partly answering inside
this thread, not checking if you did a smart counter impl.).
I don't have exact numbers, but I hope Magnus (Intel) would be motivated
to validate performance degradation from this patchset. As I know Intel
is hunting the DPDK numbers with AF_XDP-zc, where every last cycle *do*
count.
My experience is that counters can easily hurt performance, without the
developers noticing the small degradation's. As Ahern sketch out above
(stats on stack + end of napi loop update), I do believe that a smart
counter implementation is possible to hide this overhead (hopefully
completely in the CPUs pipeline slots).
I do highly appreciate the effort to standardize the XDP stats!
So, I do hope this can somehow move forward.
--Jesper
XDP is going to always be eBPF based ! why not just report such stats
to a special BPF_MAP ? BPF stack can collect the stats from the driver
and report them to this special MAP upon user request.
I really dig this idea now. How do you see it?
<ifindex:channel:stat_id> as a key and its value as a value or ...?
[ snip ]
Thanks,
Al
XDP is going to always be eBPF based ! why not just report such stats
to a special BPF_MAP ? BPF stack can collect the stats from the driver
and report them to this special MAP upon user request.
I really dig this idea now. How do you see it?
<ifindex:channel:stat_id> as a key and its value as a value or ...?
XDP is going to always be eBPF based ! why not just report such stats
to a special BPF_MAP ? BPF stack can collect the stats from the driver
and report them to this special MAP upon user request.
I really dig this idea now. How do you see it?
<ifindex:channel:stat_id> as a key and its value as a value or ...?
Ideas, suggestions, anyone?
I don't like the idea of putting statistics in a map instead of the
regular statistics counters. Sure, for bespoke things people want to put
into their XDP programs, use a map, but for regular packet/byte
counters, update the regular counters so XDP isn't "invisible".
As Jesper pointed out, batching the updates so the global counters are
only updated once per NAPI cycle is the way to avoid a huge performance
overhead of this...
-Toke
XDP is going to always be eBPF based ! why not just report such stats
to a special BPF_MAP ? BPF stack can collect the stats from the driver
and report them to this special MAP upon user request.
I really dig this idea now. How do you see it?
<ifindex:channel:stat_id> as a key and its value as a value or ...?
Ideas, suggestions, anyone?
I don't like the idea of putting statistics in a map instead of the
regular statistics counters. Sure, for bespoke things people want to put
into their XDP programs, use a map, but for regular packet/byte
counters, update the regular counters so XDP isn't "invisible".
I wanted to provide an `ip link` command for getting these stats
from maps and printing them in a usual format as well, but seems
like that's an unneeded overcomplication of things since using
maps for "regular"/"generic" XDP stats really has no reason except
for "XDP means eBPF means maps".
As Jesper pointed out, batching the updates so the global counters are
only updated once per NAPI cycle is the way to avoid a huge performance
overhead of this...
That's how I do things currently, seems to work just fine.
XDP is going to always be eBPF based ! why not just report such stats
to a special BPF_MAP ? BPF stack can collect the stats from the driver
and report them to this special MAP upon user request.
I really dig this idea now. How do you see it?
<ifindex:channel:stat_id> as a key and its value as a value or ...?
Ideas, suggestions, anyone?
I don't like the idea of putting statistics in a map instead of the
regular statistics counters. Sure, for bespoke things people want to put
into their XDP programs, use a map, but for regular packet/byte
counters, update the regular counters so XDP isn't "invisible".
I wanted to provide an `ip link` command for getting these stats
from maps and printing them in a usual format as well, but seems
like that's an unneeded overcomplication of things since using
maps for "regular"/"generic" XDP stats really has no reason except
for "XDP means eBPF means maps".
Yeah, don't really see why it would have to: to me, one of the benefits
of XDP is being integrated closely with the kernel so we can have a
"fast path" *without* reinventing everything...
quoted
As Jesper pointed out, batching the updates so the global counters are
only updated once per NAPI cycle is the way to avoid a huge performance
overhead of this...
That's how I do things currently, seems to work just fine.