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

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

> On 2020/03/20 22:37, Lorenzo Bianconi wrote:
> > > On 2020/03/20 1:41, Lorenzo Bianconi wrote:
> > > > Introduce xdp_xmit counter in order to distinguish between XDP_TX and
> > > > ndo_xdp_xmit stats. Introduce the following ethtool counters:
> > > > - rx_xdp_tx
> > > > - rx_xdp_tx_errors
> > > > - tx_xdp_xmit
> > > > - tx_xdp_xmit_errors
> > > > - rx_xdp_redirect
> > > 
> > > Thank you for working on this!
> > > 
> > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > ---
> > > ...
> > > > @@ -395,7 +404,8 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
> > > >    	}
> > > >    	rcv_priv = netdev_priv(rcv);
> > > > -	rq = &rcv_priv->rq[veth_select_rxq(rcv)];
> > > > +	qidx = veth_select_rxq(rcv);
> > > > +	rq = &rcv_priv->rq[qidx];
> > > >    	/* 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.
> > > > @@ -424,6 +434,17 @@ static int veth_xdp_xmit(struct net_device *dev, int n,
> > > >    	if (flags & XDP_XMIT_FLUSH)
> > > >    		__veth_xdp_flush(rq);
> > > > +	rq = &priv->rq[qidx];
> > > 
> > > I think there is no guarantee that this rq exists. Qidx is less than
> > > rcv->real_num_rx_queues, but not necessarily less than
> > > dev->real_num_rx_queues.
> > > 
> > > > +	u64_stats_update_begin(&rq->stats.syncp);
> > > 
> > > So this can cuase NULL pointer dereference.
> > 
> > oh right, thanks for spotting this.
> > I think we can recompute qidx for tx netdevice in this case, doing something
> > like:
> > 
> > qidx = veth_select_rxq(dev);
> > rq = &priv->rq[qidx];
> > 
> > what do you think?
> 
> This would not cause NULL pointer deref, but I wonder what counters you've
> added mean.
> 
> - rx_xdp_redirect, rx_xdp_drops, rx_xdp_tx
> 
> These counters names will be rx_queue_[i]_rx_xdp_[redirect|drops|tx].
> "rx_" in their names looks redundant.

yes, we can drop the "rx" prefix in the stats name for them.

> Also it looks like there is not "rx[i]_xdp_tx" counter but there is
> "rx[i]_xdp_tx_xmit" in mlx5 from this page.
> https://community.mellanox.com/s/article/understanding-mlx5-ethtool-counters

rx[i]_xdp_tx_xmit and rx_xdp_tx are the same, we decided to use rx_xdp_tx for
it since it seems more clear

> 
> - tx_xdp_xmit, tx_xdp_xmit_errors
> 
> These counters names will be rx_queue_[i]_tx_xdp_[xmit|xmit_errors].
> Are these rx counters or tx counters?

tx_xdp_xmit[_errors] are used to count ndo_xmit stats so they are tx counters.
I reused veth_stats for it just for convenience. Probably we can show them without
rx suffix so it is clear they are transmitted by the current device.
Another approach would be create per_cput struct to collect all tx stats.
What do you think?

Regards,
Lorenzo

> If tx, currently there is no storage to store these tx counters so adding
> these would not be simple.
> If rx, I guess you should use peer rx queue counters instead of dev rx queue
> counters.
> 
> Toshiaki Makita

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