| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200325145305.GA79426@lore-desk-wlan>
Date: Wed, 25 Mar 2020 15:53:05 +0100
From: Lorenzo Bianconi <lorenzo@...nel.org>
To: Toshiaki Makita <toshiaki.makita1@...il.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, brouer@...hat.com,
dsahern@...il.com, lorenzo.bianconi@...hat.com, toke@...hat.com
Subject: Re: [PATCH net-next 4/5] veth: introduce more xdp counters
> On 2020/03/24 23:36, Lorenzo Bianconi wrote:
[...]
> > 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)
>
> Right, but you can track the discard in veth_xdp_flush_bq().
>
ack, I guess it is just a matter of passing veth_rq to veth_xdp_flush_bq and
account there for XDP_TX. I will post a patch.
Regards,
Lorenzo
> > 2- to be symmetric with ndo_xdp_xmit
>
> I thought consistency between drivers is more important. What about other drivers?
>
> Toshiaki Makita
>
> >
> > >
> > > > + { "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
Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)
Powered by blists - more mailing lists