Re: [PATCH net-next 4/5] veth: introduce more xdp counters

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 7241 bytes --]


[...]

> > > }
> > 
> > actually I have a different idea..what about account tx stats on the peer rx
> > queue as a result of XDP_TX or ndo_xdp_xmit and properly report this info in
> > the ethool stats? In this way we do not have any locking issue and we still use
> > the per-queue stats approach. Could you please take a look to the following patch?
> 
> Thanks I think your idea is nice.
> 
> > diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> > index b6505a6c7102..f2acd2ee6287 100644
> > --- a/drivers/net/veth.c
> > +++ b/drivers/net/veth.c
> > @@ -92,17 +92,22 @@ struct veth_q_stat_desc {
> >   static const struct veth_q_stat_desc veth_rq_stats_desc[] = {
> >   	{ "xdp_packets",	VETH_RQ_STAT(xdp_packets) },
> >   	{ "xdp_bytes",		VETH_RQ_STAT(xdp_bytes) },
> > -	{ "rx_drops",		VETH_RQ_STAT(rx_drops) },
> > -	{ "rx_xdp_redirect",	VETH_RQ_STAT(xdp_redirect) },
> > -	{ "rx_xdp_drops",	VETH_RQ_STAT(xdp_drops) },
> > -	{ "rx_xdp_tx",		VETH_RQ_STAT(xdp_tx) },
> > -	{ "rx_xdp_tx_errors",	VETH_RQ_STAT(xdp_tx_err) },
> > -	{ "tx_xdp_xmit",	VETH_RQ_STAT(xdp_xmit) },
> > -	{ "tx_xdp_xmit_errors",	VETH_RQ_STAT(xdp_xmit_err) },
> > +	{ "drops",		VETH_RQ_STAT(rx_drops) },
> > +	{ "xdp_redirect",	VETH_RQ_STAT(xdp_redirect) },
> > +	{ "xdp_drops",		VETH_RQ_STAT(xdp_drops) },
> >   };
> >   #define VETH_RQ_STATS_LEN	ARRAY_SIZE(veth_rq_stats_desc)
> > +static const struct veth_q_stat_desc veth_tq_stats_desc[] = {
> > +	{ "xdp_tx",		VETH_RQ_STAT(xdp_tx) },
> > +	{ "xdp_tx_errors",	VETH_RQ_STAT(xdp_tx_err) },
> 
> I'm wondering why xdp_tx is not accounted in rx_queue?
> You can count xdp_tx/tx_error somewhere in rx path like veth_xdp_flush_bq().
> xdp_redirect and xdp_drops are similar actions and in rx stats.

Hi,

thanks for the review :)

I moved the accounting of xdp_tx/tx_error in veth_xdp_xmit for two reason:
1- veth_xdp_tx in veth_xdp_rcv_one or veth_xdp_rcv_skb returns an error
   for XDP_TX just if xdp_frame pointer is invalid but the packet can be
   discarded in veth_xdp_xmit if the device is 'under-pressure' (and this can
   be a problem since in XDP there are no queueing mechanisms so far)
2- to be symmetric  with ndo_xdp_xmit

> 
> > +	{ "xdp_xmit",		VETH_RQ_STAT(xdp_xmit) },
> > +	{ "xdp_xmit_errors",	VETH_RQ_STAT(xdp_xmit_err) },
> > +};
> > +
> > +#define VETH_TQ_STATS_LEN	ARRAY_SIZE(veth_tq_stats_desc)
> > +
> >   static struct {
> >   	const char string[ETH_GSTRING_LEN];
> >   } ethtool_stats_keys[] = {
> > @@ -142,6 +147,14 @@ static void veth_get_strings(struct net_device *dev, u32 stringset, u8 *buf)
> >   				p += ETH_GSTRING_LEN;
> >   			}
> >   		}
> > +		for (i = 0; i < dev->real_num_tx_queues; i++) {
> > +			for (j = 0; j < VETH_TQ_STATS_LEN; j++) {
> > +				snprintf(p, ETH_GSTRING_LEN,
> > +					 "tx_queue_%u_%.18s",
> > +					 i, veth_tq_stats_desc[j].desc);
> > +				p += ETH_GSTRING_LEN;
> > +			}
> > +		}
> >   		break;
> >   	}
> >   }
> > @@ -151,7 +164,8 @@ static int veth_get_sset_count(struct net_device *dev, int sset)
> >   	switch (sset) {
> >   	case ETH_SS_STATS:
> >   		return ARRAY_SIZE(ethtool_stats_keys) +
> > -		       VETH_RQ_STATS_LEN * dev->real_num_rx_queues;
> > +		       VETH_RQ_STATS_LEN * dev->real_num_rx_queues +
> > +		       VETH_TQ_STATS_LEN * dev->real_num_tx_queues;
> >   	default:
> >   		return -EOPNOTSUPP;
> >   	}
> > @@ -160,7 +174,7 @@ static int veth_get_sset_count(struct net_device *dev, int sset)
> >   static void veth_get_ethtool_stats(struct net_device *dev,
> >   		struct ethtool_stats *stats, u64 *data)
> >   {
> > -	struct veth_priv *priv = netdev_priv(dev);
> > +	struct veth_priv *rcv_priv, *priv = netdev_priv(dev);
> >   	struct net_device *peer = rtnl_dereference(priv->peer);
> >   	int i, j, idx;
> > @@ -181,6 +195,26 @@ static void veth_get_ethtool_stats(struct net_device *dev,
> >   		} while (u64_stats_fetch_retry_irq(&rq_stats->syncp, start));
> >   		idx += VETH_RQ_STATS_LEN;
> >   	}
> > +
> > +	if (!peer)
> > +		return;
> > +
> > +	rcv_priv = netdev_priv(peer);
> > +	for (i = 0; i < peer->real_num_rx_queues; i++) {
> > +		const struct veth_rq_stats *rq_stats = &rcv_priv->rq[i].stats;
> > +		const void *stats_base = (void *)&rq_stats->vs;
> > +		unsigned int start, tx_idx;
> > +		size_t offset;
> > +
> > +		tx_idx = (i % dev->real_num_tx_queues) * VETH_TQ_STATS_LEN;
> 
> I'm a bit concerned that this can fold multiple rx queue counters into one
> tx counter. But I cannot think of a better idea provided that we want to
> align XDP stats between drivers. So I'm OK with this.

Since peer->real_num_rx_queues can be greater than dev->real_num_tx_queues,
right? IIUC the only guarantee we have is:

peer->real_num_tx_queues < dev->real_num_rx_queues

If you are fine with that approach, I will post a patch before net-next
closure.

Regards,
Lorenzo


> 
> Toshiaki Makita
> 
> > +		do {
> > +			start = u64_stats_fetch_begin_irq(&rq_stats->syncp);
> > +			for (j = 0; j < VETH_TQ_STATS_LEN; j++) {
> > +				offset = veth_tq_stats_desc[j].offset;
> > +				data[tx_idx + idx + j] += *(u64 *)(stats_base + offset);
> > +			}
> > +		} while (u64_stats_fetch_retry_irq(&rq_stats->syncp, start));
> > +	}
> >   }
> >   static const struct ethtool_ops veth_ethtool_ops = {
> > @@ -340,8 +374,7 @@ static void veth_get_stats64(struct net_device *dev,
> >   	tot->tx_packets = packets;
> >   	veth_stats_rx(&rx, dev);
> > -	tot->tx_dropped += rx.xdp_xmit_err + rx.xdp_tx_err;
> > -	tot->rx_dropped = rx.rx_drops;
> > +	tot->rx_dropped = rx.rx_drops + rx.xdp_tx_err + rx.xdp_xmit_err;
> >   	tot->rx_bytes = rx.xdp_bytes;
> >   	tot->rx_packets = rx.xdp_packets;
> > @@ -353,7 +386,7 @@ static void veth_get_stats64(struct net_device *dev,
> >   		tot->rx_packets += packets;
> >   		veth_stats_rx(&rx, peer);
> > -		tot->rx_dropped += rx.xdp_xmit_err + rx.xdp_tx_err;
> > +		tot->tx_dropped += rx.xdp_xmit_err + rx.xdp_tx_err;
> >   		tot->tx_bytes += rx.xdp_bytes;
> >   		tot->tx_packets += rx.xdp_packets;
> >   	}
> > @@ -394,9 +427,9 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
> >   			 u32 flags, bool ndo_xmit)
> >   {
> >   	struct veth_priv *rcv_priv, *priv = netdev_priv(dev);
> > -	unsigned int qidx, max_len;
> >   	struct net_device *rcv;
> >   	int i, ret, drops = n;
> > +	unsigned int max_len;
> >   	struct veth_rq *rq;
> >   	rcu_read_lock();
> > @@ -414,8 +447,7 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
> >   	}
> >   	rcv_priv = netdev_priv(rcv);
> > -	qidx = veth_select_rxq(rcv);
> > -	rq = &rcv_priv->rq[qidx];
> > +	rq = &rcv_priv->rq[veth_select_rxq(rcv)];
> >   	/* Non-NULL xdp_prog ensures that xdp_ring is initialized on receive
> >   	 * side. This means an XDP program is loaded on the peer and the peer
> >   	 * device is up.
> > @@ -446,7 +478,6 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
> >   	ret = n - drops;
> >   drop:
> > -	rq = &priv->rq[qidx];
> >   	u64_stats_update_begin(&rq->stats.syncp);
> >   	if (ndo_xmit) {
> >   		rq->stats.vs.xdp_xmit += n - drops;
> > 
> > > 
> > > Thoughts?
> > > 
> > > Toshiaki Makita

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]