| 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: <20200323173113.GA300262@lore-desk-wlan>
Date: Mon, 23 Mar 2020 18:31:13 +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/21 23:30, Lorenzo Bianconi wrote:
> > > On 2020/03/20 22:37, Lorenzo Bianconi wrote:
> > > > > On 2020/03/20 1:41, Lorenzo Bianconi wrote:
[...]
> As veth_xdp_xmit really does not use tx queue but select peer rxq directly,
> per_cpu sounds more appropriate than per-queue.
> One concern is consistency. Per-queue rx stats and per-cpu tx stats (or only
> sum of them?) looks inconsistent.
> One alternative way is to change the queue selection login in veth_xdp_xmit
> and select txq instead of rxq. Then select peer rxq from txq, like
> veth_xmit. Accounting per queue tx stats is possible only when we can
> determine which txq is used.
>
> Something like this:
>
> static int veth_select_txq(struct net_device *dev)
> {
> return smp_processor_id() % dev->real_num_tx_queues;
> }
>
> static int veth_xdp_xmit(struct net_device *dev, int n,
> struct xdp_frame **frames, u32 flags)
> {
> ...
> txq = veth_select_txq(dev);
> rcv_rxq = txq; // 1-to-1 mapping from txq to peer rxq
> // Note: when XDP is enabled on rcv, this condition is always false
> if (rcv_rxq >= rcv->real_num_rx_queues)
> return -ENXIO;
> rcv_priv = netdev_priv(rcv);
> rq = &rcv_priv->rq[rcv_rxq];
> ...
> // account txq stats in some way here
> }
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?
Regards,
Lorenzo
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) },
+ { "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;
+ 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