| Message ID | 54cf35ea0d5c0ec46e0717a6181daaa2419ca91e.1724852597.git.ecree.xilinx@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | sfc: per-queue stats | expand |
On 8/28/2024 6:45 AM, edward.cree@amd.com wrote: > From: Edward Cree <ecree.xilinx@gmail.com> > > Just RX and TX packet counts for now. We do not have per-queue > byte counts, which causes us to fail stats.pkt_byte_sum selftest > with "Drivers should always report basic keys" error. Seems like the self tests here should be fixed for this? > Per-queue counts are since the last time the queue was inited > (typically by efx_start_datapath(), on ifup or reconfiguration); > device-wide total (efx_get_base_stats()) is since driver probe. > This is not the same lifetime as rtnl_link_stats64, which uses > firmware stats which count since FW (re)booted; this can cause a > "Qstats are lower" or "RTNL stats are lower" failure in > stats.pkt_byte_sum selftest. Could we have software somehow manage this so that the actual reported stats correctly survive the lifetime of the FW instead of surviving only from the queue initialization? > Move the increment of rx_queue->rx_packets to match the semantics > specified for netdev per-queue stats, i.e. just before handing > the packet to XDP (if present) or the netstack (through GRO). > This will affect the existing ethtool -S output which also > reports these counters. > Seems reasonable.
On Wed, 28 Aug 2024 14:45:11 +0100 edward.cree@amd.com wrote: > + /* If a TX channel has XDP TXQs, the stats for these will be counted > + * under the channel rather than in base stats. Unclear whether this > + * is correct behaviour, but we can't reliably exclude XDP TXes from > + * these stats anyway because in EFX_XDP_TX_QUEUES_BORROWED we use > + * the same TXQ as the core. > + */ > + efx_for_each_channel_tx_queue(tx_queue, channel) > + stats->packets += tx_queue->tx_packets - tx_queue->old_tx_packets; We haven't had to deal with shared host/XDP queues in the other drivers, yet. But talking to Olek about his stats work it sounds like he's planning to add support for reporting XDP queues. At which point it will be relatively intuitive - if XDP queues are listed - they count XDP packets, if not, and XDP_TX happens - the queues must be shared and so are the counters. IOW let's not count dedicated XDP queues here at all, if we can. XDP traffic on shared queues can get added in.
On Wed, 28 Aug 2024 15:20:53 -0700 Jacob Keller wrote: > On 8/28/2024 6:45 AM, edward.cree@amd.com wrote: > > From: Edward Cree <ecree.xilinx@gmail.com> > > > > Just RX and TX packet counts for now. We do not have per-queue > > byte counts, which causes us to fail stats.pkt_byte_sum selftest > > with "Drivers should always report basic keys" error. > > Seems like the self tests here should be fixed for this? FWIW - I just didn't take lack of byte counters into account :) It's probably fine to remove the requirement, imbalance (which is the main use for the per queue stats) is better tacked using packets, anyway. Not a requirement from my perspective to merge the series tho
From: Jakub Kicinski <kuba@kernel.org> Date: Wed, 28 Aug 2024 17:41:14 -0700 > On Wed, 28 Aug 2024 14:45:11 +0100 edward.cree@amd.com wrote: >> + /* If a TX channel has XDP TXQs, the stats for these will be counted >> + * under the channel rather than in base stats. Unclear whether this >> + * is correct behaviour, but we can't reliably exclude XDP TXes from >> + * these stats anyway because in EFX_XDP_TX_QUEUES_BORROWED we use >> + * the same TXQ as the core. >> + */ >> + efx_for_each_channel_tx_queue(tx_queue, channel) >> + stats->packets += tx_queue->tx_packets - tx_queue->old_tx_packets; > > We haven't had to deal with shared host/XDP queues in the other > drivers, yet. But talking to Olek about his stats work it sounds > like he's planning to add support for reporting XDP queues. > At which point it will be relatively intuitive - if XDP queues > are listed - they count XDP packets, if not, and XDP_TX happens > - the queues must be shared and so are the counters. > > IOW let's not count dedicated XDP queues here at all, if we can. > XDP traffic on shared queues can get added in. Yes, I agree. If a queue is shared between XDP and regular traffic, everything should go to the NL stats. But if the driver allocates dedicated XDP queues not exposed to the stack (i.e. above dev->num_tx_queues), they shouldn't count for now. Thanks, Olek
On 29/08/2024 01:41, Jakub Kicinski wrote:
> IOW let's not count dedicated XDP queues here at all, if we can.
But they should still go in base_stats, like other not-core
queues, right?
On Thu, 5 Sep 2024 11:57:59 +0100 Edward Cree wrote: > On 29/08/2024 01:41, Jakub Kicinski wrote: > > IOW let's not count dedicated XDP queues here at all, if we can. > > But they should still go in base_stats, like other not-core > queues, right? Yeah, I think so. I'm not 100% confident, but I think it makes sense for tx-packets / tx-bytes to be inclusive of all more granular counters (IOW even if we report xdp-tx-packets, the main tx-packets should also count those frames).
diff --git a/drivers/net/ethernet/sfc/ef100_rx.c b/drivers/net/ethernet/sfc/ef100_rx.c index 83d9db71d7d7..992151775cb8 100644 --- a/drivers/net/ethernet/sfc/ef100_rx.c +++ b/drivers/net/ethernet/sfc/ef100_rx.c @@ -134,6 +134,8 @@ void __ef100_rx_packet(struct efx_channel *channel) goto free_rx_buffer; } + ++rx_queue->rx_packets; + efx_rx_packet_gro(channel, rx_buf, channel->rx_pkt_n_frags, eh, csum); goto out; @@ -149,8 +151,6 @@ static void ef100_rx_packet(struct efx_rx_queue *rx_queue, unsigned int index) struct efx_channel *channel = efx_rx_queue_channel(rx_queue); struct efx_nic *efx = rx_queue->efx; - ++rx_queue->rx_packets; - netif_vdbg(efx, rx_status, efx->net_dev, "RX queue %d received id %x\n", efx_rx_queue_index(rx_queue), index); diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c index 6f1a01ded7d4..e4656efce969 100644 --- a/drivers/net/ethernet/sfc/efx.c +++ b/drivers/net/ethernet/sfc/efx.c @@ -22,6 +22,7 @@ #include "net_driver.h" #include <net/gre.h> #include <net/udp_tunnel.h> +#include <net/netdev_queues.h> #include "efx.h" #include "efx_common.h" #include "efx_channels.h" @@ -626,6 +627,76 @@ static const struct net_device_ops efx_netdev_ops = { .ndo_bpf = efx_xdp }; +static void efx_get_queue_stats_rx(struct net_device *net_dev, int idx, + struct netdev_queue_stats_rx *stats) +{ + struct efx_nic *efx = efx_netdev_priv(net_dev); + struct efx_rx_queue *rx_queue; + struct efx_channel *channel; + + channel = efx_get_channel(efx, idx); + rx_queue = efx_channel_get_rx_queue(channel); + /* Count only packets since last time datapath was started */ + stats->packets = rx_queue->rx_packets - rx_queue->old_rx_packets; +} + +static void efx_get_queue_stats_tx(struct net_device *net_dev, int idx, + struct netdev_queue_stats_tx *stats) +{ + struct efx_nic *efx = efx_netdev_priv(net_dev); + struct efx_tx_queue *tx_queue; + struct efx_channel *channel; + + channel = efx_get_tx_channel(efx, idx); + stats->packets = 0; + /* If a TX channel has XDP TXQs, the stats for these will be counted + * under the channel rather than in base stats. Unclear whether this + * is correct behaviour, but we can't reliably exclude XDP TXes from + * these stats anyway because in EFX_XDP_TX_QUEUES_BORROWED we use + * the same TXQ as the core. + */ + efx_for_each_channel_tx_queue(tx_queue, channel) + stats->packets += tx_queue->tx_packets - tx_queue->old_tx_packets; +} + +static void efx_get_base_stats(struct net_device *net_dev, + struct netdev_queue_stats_rx *rx, + struct netdev_queue_stats_tx *tx) +{ + struct efx_nic *efx = efx_netdev_priv(net_dev); + struct efx_tx_queue *tx_queue; + struct efx_rx_queue *rx_queue; + struct efx_channel *channel; + + rx->packets = 0; + tx->packets = 0; + + /* Count all packets on non-core queues, and packets before last + * datapath start on core queues. + */ + efx_for_each_channel(channel, efx) { + rx_queue = efx_channel_get_rx_queue(channel); + if (channel->channel >= net_dev->real_num_rx_queues) + rx->packets += rx_queue->rx_packets; + else + rx->packets += rx_queue->old_rx_packets; + efx_for_each_channel_tx_queue(tx_queue, channel) { + if (channel->channel < efx->tx_channel_offset || + channel->channel >= efx->tx_channel_offset + + net_dev->real_num_tx_queues) + tx->packets += tx_queue->tx_packets; + else + tx->packets += tx_queue->old_tx_packets; + } + } +} + +static const struct netdev_stat_ops efx_stat_ops = { + .get_queue_stats_rx = efx_get_queue_stats_rx, + .get_queue_stats_tx = efx_get_queue_stats_tx, + .get_base_stats = efx_get_base_stats, +}; + static int efx_xdp_setup_prog(struct efx_nic *efx, struct bpf_prog *prog) { struct bpf_prog *old_prog; @@ -716,6 +787,7 @@ static int efx_register_netdev(struct efx_nic *efx) net_dev->watchdog_timeo = 5 * HZ; net_dev->irq = efx->pci_dev->irq; net_dev->netdev_ops = &efx_netdev_ops; + net_dev->stat_ops = &efx_stat_ops; if (efx_nic_rev(efx) >= EFX_REV_HUNT_A0) net_dev->priv_flags |= IFF_UNICAST_FLT; net_dev->ethtool_ops = &efx_ethtool_ops; diff --git a/drivers/net/ethernet/sfc/efx_channels.h b/drivers/net/ethernet/sfc/efx_channels.h index 46b702648721..b3b5e18a69cc 100644 --- a/drivers/net/ethernet/sfc/efx_channels.h +++ b/drivers/net/ethernet/sfc/efx_channels.h @@ -49,5 +49,4 @@ void efx_fini_napi_channel(struct efx_channel *channel); void efx_fini_napi(struct efx_nic *efx); void efx_channel_dummy_op_void(struct efx_channel *channel); - #endif diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h index 4d904e1404d4..cc96716d8dbe 100644 --- a/drivers/net/ethernet/sfc/net_driver.h +++ b/drivers/net/ethernet/sfc/net_driver.h @@ -232,6 +232,8 @@ struct efx_tx_buffer { * @xmit_pending: Are any packets waiting to be pushed to the NIC * @cb_packets: Number of times the TX copybreak feature has been used * @notify_count: Count of notified descriptors to the NIC + * @tx_packets: Number of packets sent since this struct was created + * @old_tx_packets: Value of @tx_packets as of last efx_init_tx_queue() * @empty_read_count: If the completion path has seen the queue as empty * and the transmission path has not yet checked this, the value of * @read_count bitwise-added to %EFX_EMPTY_COUNT_VALID; otherwise 0. @@ -281,6 +283,7 @@ struct efx_tx_queue { unsigned int notify_count; /* Statistics to supplement MAC stats */ unsigned long tx_packets; + unsigned long old_tx_packets; /* Members shared between paths and sometimes updated */ unsigned int empty_read_count ____cacheline_aligned_in_smp; @@ -370,6 +373,8 @@ struct efx_rx_page_state { * @recycle_count: RX buffer recycle counter. * @slow_fill: Timer used to defer efx_nic_generate_fill_event(). * @grant_work: workitem used to grant credits to the MAE if @grant_credits + * @rx_packets: Number of packets received since this struct was created + * @old_rx_packets: Value of @rx_packets as of last efx_init_rx_queue() * @xdp_rxq_info: XDP specific RX queue information. * @xdp_rxq_info_valid: Is xdp_rxq_info valid data?. */ @@ -406,6 +411,7 @@ struct efx_rx_queue { struct work_struct grant_work; /* Statistics to supplement MAC stats */ unsigned long rx_packets; + unsigned long old_rx_packets; struct xdp_rxq_info xdp_rxq_info; bool xdp_rxq_info_valid; }; diff --git a/drivers/net/ethernet/sfc/rx.c b/drivers/net/ethernet/sfc/rx.c index f77a2d3ef37e..f07495582125 100644 --- a/drivers/net/ethernet/sfc/rx.c +++ b/drivers/net/ethernet/sfc/rx.c @@ -125,8 +125,6 @@ void efx_rx_packet(struct efx_rx_queue *rx_queue, unsigned int index, struct efx_channel *channel = efx_rx_queue_channel(rx_queue); struct efx_rx_buffer *rx_buf; - rx_queue->rx_packets++; - rx_buf = efx_rx_buffer(rx_queue, index); rx_buf->flags |= flags; @@ -394,6 +392,8 @@ void __efx_rx_packet(struct efx_channel *channel) goto out; } + rx_queue->rx_packets++; + if (!efx_do_xdp(efx, channel, rx_buf, &eh)) goto out; diff --git a/drivers/net/ethernet/sfc/rx_common.c b/drivers/net/ethernet/sfc/rx_common.c index 0b7dc75c40f9..bdb4325a7c2c 100644 --- a/drivers/net/ethernet/sfc/rx_common.c +++ b/drivers/net/ethernet/sfc/rx_common.c @@ -241,6 +241,8 @@ void efx_init_rx_queue(struct efx_rx_queue *rx_queue) rx_queue->page_recycle_failed = 0; rx_queue->page_recycle_full = 0; + rx_queue->old_rx_packets = rx_queue->rx_packets; + /* Initialise limit fields */ max_fill = efx->rxq_entries - EFX_RXD_HEAD_ROOM; max_trigger = diff --git a/drivers/net/ethernet/sfc/tx_common.c b/drivers/net/ethernet/sfc/tx_common.c index 2adb132b2f7e..f1694900e0f0 100644 --- a/drivers/net/ethernet/sfc/tx_common.c +++ b/drivers/net/ethernet/sfc/tx_common.c @@ -86,6 +86,8 @@ void efx_init_tx_queue(struct efx_tx_queue *tx_queue) tx_queue->completed_timestamp_major = 0; tx_queue->completed_timestamp_minor = 0; + tx_queue->old_tx_packets = tx_queue->tx_packets; + tx_queue->xdp_tx = efx_channel_is_xdp_tx(tx_queue->channel); tx_queue->tso_version = 0;