Commit 5b8f3c8d30a6 ("virtio_net: Add XDP related stats") tries to
count TX XDP stats in virtnet_receive(). This will cause several
issues:
- virtnet_xdp_sq() was called without checking whether or not XDP is
set. This may cause out of bound access when there's no enough txq
for XDP.
- Stats were updated even if there's no XDP/XDP_TX.
Fixing this by reusing virtnet_xdp_xmit() for XDP_TX which can counts
TX XDP counter itself and remove the unnecessary tx stats embedded in
rx stats.
Reported-by: [email protected]
Fixes: 5b8f3c8d30a6 ("virtio_net: Add XDP related stats")
Cc: Toshiaki Makita <[email protected]>
Signed-off-by: Jason Wang <[email protected]>
---
drivers/net/virtio_net.c | 39 ++++-----------------------------------
1 file changed, 4 insertions(+), 35 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 1880c86..72d3f68 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -105,10 +105,6 @@ struct virtnet_rq_stats {
struct virtnet_rx_stats {
struct virtnet_rq_stat_items rx;
- struct {
- unsigned int xdp_tx;
- unsigned int xdp_tx_drops;
- } tx;
};
#define VIRTNET_SQ_STAT(m) offsetof(struct virtnet_sq_stats, m)
@@ -485,22 +481,6 @@ static struct send_queue *virtnet_xdp_sq(struct virtnet_info *vi)
return &vi->sq[qp];
}
-static int __virtnet_xdp_tx_xmit(struct virtnet_info *vi,
- struct xdp_frame *xdpf)
-{
- struct xdp_frame *xdpf_sent;
- struct send_queue *sq;
- unsigned int len;
-
- sq = virtnet_xdp_sq(vi);
-
- /* Free up any pending old buffers before queueing new ones. */
- while ((xdpf_sent = virtqueue_get_buf(sq->vq, &len)) != NULL)
- xdp_return_frame(xdpf_sent);
-
- return __virtnet_xdp_xmit_one(vi, sq, xdpf);
-}
-
static int virtnet_xdp_xmit(struct net_device *dev,
int n, struct xdp_frame **frames, u32 flags)
{
@@ -707,10 +687,8 @@ static struct sk_buff *receive_small(struct net_device *dev,
xdpf = convert_to_xdp_frame(&xdp);
if (unlikely(!xdpf))
goto err_xdp;
- stats->tx.xdp_tx++;
- err = __virtnet_xdp_tx_xmit(vi, xdpf);
- if (unlikely(err)) {
- stats->tx.xdp_tx_drops++;
+ err = virtnet_xdp_xmit(dev, 1, &xdpf, 0);
+ if (unlikely(err < 0)) {
trace_xdp_exception(vi->dev, xdp_prog, act);
goto err_xdp;
}
@@ -879,10 +857,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
xdpf = convert_to_xdp_frame(&xdp);
if (unlikely(!xdpf))
goto err_xdp;
- stats->tx.xdp_tx++;
- err = __virtnet_xdp_tx_xmit(vi, xdpf);
- if (unlikely(err)) {
- stats->tx.xdp_tx_drops++;
+ err = virtnet_xdp_xmit(dev, 1, &xdpf, 0);
+ if (unlikely(err < 0)) {
trace_xdp_exception(vi->dev, xdp_prog, act);
if (unlikely(xdp_page != page))
put_page(xdp_page);
@@ -1315,7 +1291,6 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
{
struct virtnet_info *vi = rq->vq->vdev->priv;
struct virtnet_rx_stats stats = {};
- struct send_queue *sq;
unsigned int len;
void *buf;
int i;
@@ -1351,12 +1326,6 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
}
u64_stats_update_end(&rq->stats.syncp);
- sq = virtnet_xdp_sq(vi);
- u64_stats_update_begin(&sq->stats.syncp);
- sq->stats.xdp_tx += stats.tx.xdp_tx;
- sq->stats.xdp_tx_drops += stats.tx.xdp_tx_drops;
- u64_stats_update_end(&sq->stats.syncp);
-
return stats.rx.packets;
}
--
2.7.4
We don't maintain tx counters in rx stats any more. There's no need
for an extra container of rq stats.
Cc: Toshiaki Makita <[email protected]>
Signed-off-by: Jason Wang <[email protected]>
---
drivers/net/virtio_net.c | 80 ++++++++++++++++++++++--------------------------
1 file changed, 36 insertions(+), 44 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 72d3f68..14f661c 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -87,7 +87,8 @@ struct virtnet_sq_stats {
u64 kicks;
};
-struct virtnet_rq_stat_items {
+struct virtnet_rq_stats {
+ struct u64_stats_sync syncp;
u64 packets;
u64 bytes;
u64 drops;
@@ -98,17 +99,8 @@ struct virtnet_rq_stat_items {
u64 kicks;
};
-struct virtnet_rq_stats {
- struct u64_stats_sync syncp;
- struct virtnet_rq_stat_items items;
-};
-
-struct virtnet_rx_stats {
- struct virtnet_rq_stat_items rx;
-};
-
#define VIRTNET_SQ_STAT(m) offsetof(struct virtnet_sq_stats, m)
-#define VIRTNET_RQ_STAT(m) offsetof(struct virtnet_rq_stat_items, m)
+#define VIRTNET_RQ_STAT(m) offsetof(struct virtnet_rq_stats, m)
static const struct virtnet_stat_desc virtnet_sq_stats_desc[] = {
{ "packets", VIRTNET_SQ_STAT(packets) },
@@ -617,7 +609,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
void *buf, void *ctx,
unsigned int len,
unsigned int *xdp_xmit,
- struct virtnet_rx_stats *stats)
+ struct virtnet_rq_stats *stats)
{
struct sk_buff *skb;
struct bpf_prog *xdp_prog;
@@ -632,7 +624,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
int err;
len -= vi->hdr_len;
- stats->rx.bytes += len;
+ stats->bytes += len;
rcu_read_lock();
xdp_prog = rcu_dereference(rq->xdp_prog);
@@ -674,7 +666,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
xdp.rxq = &rq->xdp_rxq;
orig_data = xdp.data;
act = bpf_prog_run_xdp(xdp_prog, &xdp);
- stats->rx.xdp_packets++;
+ stats->xdp_packets++;
switch (act) {
case XDP_PASS:
@@ -683,7 +675,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
len = xdp.data_end - xdp.data;
break;
case XDP_TX:
- stats->rx.xdp_tx++;
+ stats->xdp_tx++;
xdpf = convert_to_xdp_frame(&xdp);
if (unlikely(!xdpf))
goto err_xdp;
@@ -696,7 +688,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
rcu_read_unlock();
goto xdp_xmit;
case XDP_REDIRECT:
- stats->rx.xdp_redirects++;
+ stats->xdp_redirects++;
err = xdp_do_redirect(dev, &xdp, xdp_prog);
if (err)
goto err_xdp;
@@ -730,8 +722,8 @@ static struct sk_buff *receive_small(struct net_device *dev,
err_xdp:
rcu_read_unlock();
- stats->rx.xdp_drops++;
- stats->rx.drops++;
+ stats->xdp_drops++;
+ stats->drops++;
put_page(page);
xdp_xmit:
return NULL;
@@ -742,19 +734,19 @@ static struct sk_buff *receive_big(struct net_device *dev,
struct receive_queue *rq,
void *buf,
unsigned int len,
- struct virtnet_rx_stats *stats)
+ struct virtnet_rq_stats *stats)
{
struct page *page = buf;
struct sk_buff *skb = page_to_skb(vi, rq, page, 0, len, PAGE_SIZE);
- stats->rx.bytes += len - vi->hdr_len;
+ stats->bytes += len - vi->hdr_len;
if (unlikely(!skb))
goto err;
return skb;
err:
- stats->rx.drops++;
+ stats->drops++;
give_pages(rq, page);
return NULL;
}
@@ -766,7 +758,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
void *ctx,
unsigned int len,
unsigned int *xdp_xmit,
- struct virtnet_rx_stats *stats)
+ struct virtnet_rq_stats *stats)
{
struct virtio_net_hdr_mrg_rxbuf *hdr = buf;
u16 num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers);
@@ -779,7 +771,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
int err;
head_skb = NULL;
- stats->rx.bytes += len - vi->hdr_len;
+ stats->bytes += len - vi->hdr_len;
rcu_read_lock();
xdp_prog = rcu_dereference(rq->xdp_prog);
@@ -828,7 +820,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
xdp.rxq = &rq->xdp_rxq;
act = bpf_prog_run_xdp(xdp_prog, &xdp);
- stats->rx.xdp_packets++;
+ stats->xdp_packets++;
switch (act) {
case XDP_PASS:
@@ -853,7 +845,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
}
break;
case XDP_TX:
- stats->rx.xdp_tx++;
+ stats->xdp_tx++;
xdpf = convert_to_xdp_frame(&xdp);
if (unlikely(!xdpf))
goto err_xdp;
@@ -870,7 +862,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
rcu_read_unlock();
goto xdp_xmit;
case XDP_REDIRECT:
- stats->rx.xdp_redirects++;
+ stats->xdp_redirects++;
err = xdp_do_redirect(dev, &xdp, xdp_prog);
if (err) {
if (unlikely(xdp_page != page))
@@ -920,7 +912,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
goto err_buf;
}
- stats->rx.bytes += len;
+ stats->bytes += len;
page = virt_to_head_page(buf);
truesize = mergeable_ctx_to_truesize(ctx);
@@ -966,7 +958,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
err_xdp:
rcu_read_unlock();
- stats->rx.xdp_drops++;
+ stats->xdp_drops++;
err_skb:
put_page(page);
while (num_buf-- > 1) {
@@ -977,12 +969,12 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
dev->stats.rx_length_errors++;
break;
}
- stats->rx.bytes += len;
+ stats->bytes += len;
page = virt_to_head_page(buf);
put_page(page);
}
err_buf:
- stats->rx.drops++;
+ stats->drops++;
dev_kfree_skb(head_skb);
xdp_xmit:
return NULL;
@@ -991,7 +983,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
void *buf, unsigned int len, void **ctx,
unsigned int *xdp_xmit,
- struct virtnet_rx_stats *stats)
+ struct virtnet_rq_stats *stats)
{
struct net_device *dev = vi->dev;
struct sk_buff *skb;
@@ -1212,7 +1204,7 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
} while (rq->vq->num_free);
if (virtqueue_kick_prepare(rq->vq) && virtqueue_notify(rq->vq)) {
u64_stats_update_begin(&rq->stats.syncp);
- rq->stats.items.kicks++;
+ rq->stats.kicks++;
u64_stats_update_end(&rq->stats.syncp);
}
@@ -1290,7 +1282,7 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
unsigned int *xdp_xmit)
{
struct virtnet_info *vi = rq->vq->vdev->priv;
- struct virtnet_rx_stats stats = {};
+ struct virtnet_rq_stats stats = {};
unsigned int len;
void *buf;
int i;
@@ -1298,16 +1290,16 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
if (!vi->big_packets || vi->mergeable_rx_bufs) {
void *ctx;
- while (stats.rx.packets < budget &&
+ while (stats.packets < budget &&
(buf = virtqueue_get_buf_ctx(rq->vq, &len, &ctx))) {
receive_buf(vi, rq, buf, len, ctx, xdp_xmit, &stats);
- stats.rx.packets++;
+ stats.packets++;
}
} else {
- while (stats.rx.packets < budget &&
+ while (stats.packets < budget &&
(buf = virtqueue_get_buf(rq->vq, &len)) != NULL) {
receive_buf(vi, rq, buf, len, NULL, xdp_xmit, &stats);
- stats.rx.packets++;
+ stats.packets++;
}
}
@@ -1321,12 +1313,12 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
size_t offset = virtnet_rq_stats_desc[i].offset;
u64 *item;
- item = (u64 *)((u8 *)&rq->stats.items + offset);
- *item += *(u64 *)((u8 *)&stats.rx + offset);
+ item = (u64 *)((u8 *)&rq->stats + offset);
+ *item += *(u64 *)((u8 *)&stats + offset);
}
u64_stats_update_end(&rq->stats.syncp);
- return stats.rx.packets;
+ return stats.packets;
}
static void free_old_xmit_skbs(struct send_queue *sq)
@@ -1686,9 +1678,9 @@ static void virtnet_stats(struct net_device *dev,
do {
start = u64_stats_fetch_begin_irq(&rq->stats.syncp);
- rpackets = rq->stats.items.packets;
- rbytes = rq->stats.items.bytes;
- rdrops = rq->stats.items.drops;
+ rpackets = rq->stats.packets;
+ rbytes = rq->stats.bytes;
+ rdrops = rq->stats.drops;
} while (u64_stats_fetch_retry_irq(&rq->stats.syncp, start));
tot->rx_packets += rpackets;
@@ -2078,7 +2070,7 @@ static void virtnet_get_ethtool_stats(struct net_device *dev,
for (i = 0; i < vi->curr_queue_pairs; i++) {
struct receive_queue *rq = &vi->rq[i];
- stats_base = (u8 *)&rq->stats.items;
+ stats_base = (u8 *)&rq->stats;
do {
start = u64_stats_fetch_begin_irq(&rq->stats.syncp);
for (j = 0; j < VIRTNET_RQ_STATS_LEN; j++) {
--
2.7.4
On 2018/07/31 18:43, Jason Wang wrote:
> Commit 5b8f3c8d30a6 ("virtio_net: Add XDP related stats") tries to
> count TX XDP stats in virtnet_receive(). This will cause several
> issues:
>
> - virtnet_xdp_sq() was called without checking whether or not XDP is
> set. This may cause out of bound access when there's no enough txq
> for XDP.
> - Stats were updated even if there's no XDP/XDP_TX.>
> Fixing this by reusing virtnet_xdp_xmit() for XDP_TX which can counts
> TX XDP counter itself and remove the unnecessary tx stats embedded in
> rx stats.
Thanks for fixing this.
I wanted to avoid calling u64_stats_update_begin() (i.e. smp_wmb() in 32
bit systems) for every packet. So I'd like to keep sq stats in
virtnet_rx_stats.
--
Toshiaki Makita
On 2018/07/31 18:43, Jason Wang wrote:
> We don't maintain tx counters in rx stats any more. There's no need
> for an extra container of rq stats.
>
> Cc: Toshiaki Makita <[email protected]>
> Signed-off-by: Jason Wang <[email protected]>
> ---
> drivers/net/virtio_net.c | 80 ++++++++++++++++++++++--------------------------
> 1 file changed, 36 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 72d3f68..14f661c 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -87,7 +87,8 @@ struct virtnet_sq_stats {
> u64 kicks;
> };
>
> -struct virtnet_rq_stat_items {
> +struct virtnet_rq_stats {
> + struct u64_stats_sync syncp;
> u64 packets;
> u64 bytes;
> u64 drops;
> @@ -98,17 +99,8 @@ struct virtnet_rq_stat_items {
> u64 kicks;
> };
>
> -struct virtnet_rq_stats {
> - struct u64_stats_sync syncp;
> - struct virtnet_rq_stat_items items;
> -};
I'm not thinking removing sq stat is needed but even if it is I want to
keep virtnet_rq_stats to avoid allocating unnecessary u64_stats_syncp on
stack in virtnet_receive. I would just remove virtnet_rx_stats if necessary.
> -
> -struct virtnet_rx_stats {
> - struct virtnet_rq_stat_items rx;
> -};
> -
--
Toshiaki Makita
On Tue, Jul 31, 2018 at 05:43:38PM +0800, Jason Wang wrote:
> Commit 5b8f3c8d30a6 ("virtio_net: Add XDP related stats") tries to
> count TX XDP stats in virtnet_receive(). This will cause several
> issues:
>
> - virtnet_xdp_sq() was called without checking whether or not XDP is
> set. This may cause out of bound access when there's no enough txq
> for XDP.
> - Stats were updated even if there's no XDP/XDP_TX.
>
> Fixing this by reusing virtnet_xdp_xmit() for XDP_TX which can counts
> TX XDP counter itself and remove the unnecessary tx stats embedded in
> rx stats.
>
> Reported-by: [email protected]
> Fixes: 5b8f3c8d30a6 ("virtio_net: Add XDP related stats")
> Cc: Toshiaki Makita <[email protected]>
> Signed-off-by: Jason Wang <[email protected]>
Acked-by: Michael S. Tsirkin <[email protected]>
> ---
> drivers/net/virtio_net.c | 39 ++++-----------------------------------
> 1 file changed, 4 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 1880c86..72d3f68 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -105,10 +105,6 @@ struct virtnet_rq_stats {
>
> struct virtnet_rx_stats {
> struct virtnet_rq_stat_items rx;
> - struct {
> - unsigned int xdp_tx;
> - unsigned int xdp_tx_drops;
> - } tx;
> };
>
> #define VIRTNET_SQ_STAT(m) offsetof(struct virtnet_sq_stats, m)
> @@ -485,22 +481,6 @@ static struct send_queue *virtnet_xdp_sq(struct virtnet_info *vi)
> return &vi->sq[qp];
> }
>
> -static int __virtnet_xdp_tx_xmit(struct virtnet_info *vi,
> - struct xdp_frame *xdpf)
> -{
> - struct xdp_frame *xdpf_sent;
> - struct send_queue *sq;
> - unsigned int len;
> -
> - sq = virtnet_xdp_sq(vi);
> -
> - /* Free up any pending old buffers before queueing new ones. */
> - while ((xdpf_sent = virtqueue_get_buf(sq->vq, &len)) != NULL)
> - xdp_return_frame(xdpf_sent);
> -
> - return __virtnet_xdp_xmit_one(vi, sq, xdpf);
> -}
> -
> static int virtnet_xdp_xmit(struct net_device *dev,
> int n, struct xdp_frame **frames, u32 flags)
> {
> @@ -707,10 +687,8 @@ static struct sk_buff *receive_small(struct net_device *dev,
> xdpf = convert_to_xdp_frame(&xdp);
> if (unlikely(!xdpf))
> goto err_xdp;
> - stats->tx.xdp_tx++;
> - err = __virtnet_xdp_tx_xmit(vi, xdpf);
> - if (unlikely(err)) {
> - stats->tx.xdp_tx_drops++;
> + err = virtnet_xdp_xmit(dev, 1, &xdpf, 0);
> + if (unlikely(err < 0)) {
> trace_xdp_exception(vi->dev, xdp_prog, act);
> goto err_xdp;
> }
> @@ -879,10 +857,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> xdpf = convert_to_xdp_frame(&xdp);
> if (unlikely(!xdpf))
> goto err_xdp;
> - stats->tx.xdp_tx++;
> - err = __virtnet_xdp_tx_xmit(vi, xdpf);
> - if (unlikely(err)) {
> - stats->tx.xdp_tx_drops++;
> + err = virtnet_xdp_xmit(dev, 1, &xdpf, 0);
> + if (unlikely(err < 0)) {
> trace_xdp_exception(vi->dev, xdp_prog, act);
> if (unlikely(xdp_page != page))
> put_page(xdp_page);
> @@ -1315,7 +1291,6 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
> {
> struct virtnet_info *vi = rq->vq->vdev->priv;
> struct virtnet_rx_stats stats = {};
> - struct send_queue *sq;
> unsigned int len;
> void *buf;
> int i;
> @@ -1351,12 +1326,6 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
> }
> u64_stats_update_end(&rq->stats.syncp);
>
> - sq = virtnet_xdp_sq(vi);
> - u64_stats_update_begin(&sq->stats.syncp);
> - sq->stats.xdp_tx += stats.tx.xdp_tx;
> - sq->stats.xdp_tx_drops += stats.tx.xdp_tx_drops;
> - u64_stats_update_end(&sq->stats.syncp);
> -
> return stats.rx.packets;
> }
>
> --
> 2.7.4
On Tue, Jul 31, 2018 at 05:43:39PM +0800, Jason Wang wrote:
> We don't maintain tx counters in rx stats any more. There's no need
> for an extra container of rq stats.
>
> Cc: Toshiaki Makita <[email protected]>
> Signed-off-by: Jason Wang <[email protected]>
Acked-by: Michael S. Tsirkin <[email protected]>
> ---
> drivers/net/virtio_net.c | 80 ++++++++++++++++++++++--------------------------
> 1 file changed, 36 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 72d3f68..14f661c 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -87,7 +87,8 @@ struct virtnet_sq_stats {
> u64 kicks;
> };
>
> -struct virtnet_rq_stat_items {
> +struct virtnet_rq_stats {
> + struct u64_stats_sync syncp;
> u64 packets;
> u64 bytes;
> u64 drops;
> @@ -98,17 +99,8 @@ struct virtnet_rq_stat_items {
> u64 kicks;
> };
>
> -struct virtnet_rq_stats {
> - struct u64_stats_sync syncp;
> - struct virtnet_rq_stat_items items;
> -};
> -
> -struct virtnet_rx_stats {
> - struct virtnet_rq_stat_items rx;
> -};
> -
> #define VIRTNET_SQ_STAT(m) offsetof(struct virtnet_sq_stats, m)
> -#define VIRTNET_RQ_STAT(m) offsetof(struct virtnet_rq_stat_items, m)
> +#define VIRTNET_RQ_STAT(m) offsetof(struct virtnet_rq_stats, m)
>
> static const struct virtnet_stat_desc virtnet_sq_stats_desc[] = {
> { "packets", VIRTNET_SQ_STAT(packets) },
> @@ -617,7 +609,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
> void *buf, void *ctx,
> unsigned int len,
> unsigned int *xdp_xmit,
> - struct virtnet_rx_stats *stats)
> + struct virtnet_rq_stats *stats)
> {
> struct sk_buff *skb;
> struct bpf_prog *xdp_prog;
> @@ -632,7 +624,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
> int err;
>
> len -= vi->hdr_len;
> - stats->rx.bytes += len;
> + stats->bytes += len;
>
> rcu_read_lock();
> xdp_prog = rcu_dereference(rq->xdp_prog);
> @@ -674,7 +666,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
> xdp.rxq = &rq->xdp_rxq;
> orig_data = xdp.data;
> act = bpf_prog_run_xdp(xdp_prog, &xdp);
> - stats->rx.xdp_packets++;
> + stats->xdp_packets++;
>
> switch (act) {
> case XDP_PASS:
> @@ -683,7 +675,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
> len = xdp.data_end - xdp.data;
> break;
> case XDP_TX:
> - stats->rx.xdp_tx++;
> + stats->xdp_tx++;
> xdpf = convert_to_xdp_frame(&xdp);
> if (unlikely(!xdpf))
> goto err_xdp;
> @@ -696,7 +688,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
> rcu_read_unlock();
> goto xdp_xmit;
> case XDP_REDIRECT:
> - stats->rx.xdp_redirects++;
> + stats->xdp_redirects++;
> err = xdp_do_redirect(dev, &xdp, xdp_prog);
> if (err)
> goto err_xdp;
> @@ -730,8 +722,8 @@ static struct sk_buff *receive_small(struct net_device *dev,
>
> err_xdp:
> rcu_read_unlock();
> - stats->rx.xdp_drops++;
> - stats->rx.drops++;
> + stats->xdp_drops++;
> + stats->drops++;
> put_page(page);
> xdp_xmit:
> return NULL;
> @@ -742,19 +734,19 @@ static struct sk_buff *receive_big(struct net_device *dev,
> struct receive_queue *rq,
> void *buf,
> unsigned int len,
> - struct virtnet_rx_stats *stats)
> + struct virtnet_rq_stats *stats)
> {
> struct page *page = buf;
> struct sk_buff *skb = page_to_skb(vi, rq, page, 0, len, PAGE_SIZE);
>
> - stats->rx.bytes += len - vi->hdr_len;
> + stats->bytes += len - vi->hdr_len;
> if (unlikely(!skb))
> goto err;
>
> return skb;
>
> err:
> - stats->rx.drops++;
> + stats->drops++;
> give_pages(rq, page);
> return NULL;
> }
> @@ -766,7 +758,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> void *ctx,
> unsigned int len,
> unsigned int *xdp_xmit,
> - struct virtnet_rx_stats *stats)
> + struct virtnet_rq_stats *stats)
> {
> struct virtio_net_hdr_mrg_rxbuf *hdr = buf;
> u16 num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers);
> @@ -779,7 +771,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> int err;
>
> head_skb = NULL;
> - stats->rx.bytes += len - vi->hdr_len;
> + stats->bytes += len - vi->hdr_len;
>
> rcu_read_lock();
> xdp_prog = rcu_dereference(rq->xdp_prog);
> @@ -828,7 +820,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> xdp.rxq = &rq->xdp_rxq;
>
> act = bpf_prog_run_xdp(xdp_prog, &xdp);
> - stats->rx.xdp_packets++;
> + stats->xdp_packets++;
>
> switch (act) {
> case XDP_PASS:
> @@ -853,7 +845,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> }
> break;
> case XDP_TX:
> - stats->rx.xdp_tx++;
> + stats->xdp_tx++;
> xdpf = convert_to_xdp_frame(&xdp);
> if (unlikely(!xdpf))
> goto err_xdp;
> @@ -870,7 +862,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> rcu_read_unlock();
> goto xdp_xmit;
> case XDP_REDIRECT:
> - stats->rx.xdp_redirects++;
> + stats->xdp_redirects++;
> err = xdp_do_redirect(dev, &xdp, xdp_prog);
> if (err) {
> if (unlikely(xdp_page != page))
> @@ -920,7 +912,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> goto err_buf;
> }
>
> - stats->rx.bytes += len;
> + stats->bytes += len;
> page = virt_to_head_page(buf);
>
> truesize = mergeable_ctx_to_truesize(ctx);
> @@ -966,7 +958,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>
> err_xdp:
> rcu_read_unlock();
> - stats->rx.xdp_drops++;
> + stats->xdp_drops++;
> err_skb:
> put_page(page);
> while (num_buf-- > 1) {
> @@ -977,12 +969,12 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> dev->stats.rx_length_errors++;
> break;
> }
> - stats->rx.bytes += len;
> + stats->bytes += len;
> page = virt_to_head_page(buf);
> put_page(page);
> }
> err_buf:
> - stats->rx.drops++;
> + stats->drops++;
> dev_kfree_skb(head_skb);
> xdp_xmit:
> return NULL;
> @@ -991,7 +983,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
> void *buf, unsigned int len, void **ctx,
> unsigned int *xdp_xmit,
> - struct virtnet_rx_stats *stats)
> + struct virtnet_rq_stats *stats)
> {
> struct net_device *dev = vi->dev;
> struct sk_buff *skb;
> @@ -1212,7 +1204,7 @@ static bool try_fill_recv(struct virtnet_info *vi, struct receive_queue *rq,
> } while (rq->vq->num_free);
> if (virtqueue_kick_prepare(rq->vq) && virtqueue_notify(rq->vq)) {
> u64_stats_update_begin(&rq->stats.syncp);
> - rq->stats.items.kicks++;
> + rq->stats.kicks++;
> u64_stats_update_end(&rq->stats.syncp);
> }
>
> @@ -1290,7 +1282,7 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
> unsigned int *xdp_xmit)
> {
> struct virtnet_info *vi = rq->vq->vdev->priv;
> - struct virtnet_rx_stats stats = {};
> + struct virtnet_rq_stats stats = {};
> unsigned int len;
> void *buf;
> int i;
> @@ -1298,16 +1290,16 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
> if (!vi->big_packets || vi->mergeable_rx_bufs) {
> void *ctx;
>
> - while (stats.rx.packets < budget &&
> + while (stats.packets < budget &&
> (buf = virtqueue_get_buf_ctx(rq->vq, &len, &ctx))) {
> receive_buf(vi, rq, buf, len, ctx, xdp_xmit, &stats);
> - stats.rx.packets++;
> + stats.packets++;
> }
> } else {
> - while (stats.rx.packets < budget &&
> + while (stats.packets < budget &&
> (buf = virtqueue_get_buf(rq->vq, &len)) != NULL) {
> receive_buf(vi, rq, buf, len, NULL, xdp_xmit, &stats);
> - stats.rx.packets++;
> + stats.packets++;
> }
> }
>
> @@ -1321,12 +1313,12 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
> size_t offset = virtnet_rq_stats_desc[i].offset;
> u64 *item;
>
> - item = (u64 *)((u8 *)&rq->stats.items + offset);
> - *item += *(u64 *)((u8 *)&stats.rx + offset);
> + item = (u64 *)((u8 *)&rq->stats + offset);
> + *item += *(u64 *)((u8 *)&stats + offset);
> }
> u64_stats_update_end(&rq->stats.syncp);
>
> - return stats.rx.packets;
> + return stats.packets;
> }
>
> static void free_old_xmit_skbs(struct send_queue *sq)
> @@ -1686,9 +1678,9 @@ static void virtnet_stats(struct net_device *dev,
>
> do {
> start = u64_stats_fetch_begin_irq(&rq->stats.syncp);
> - rpackets = rq->stats.items.packets;
> - rbytes = rq->stats.items.bytes;
> - rdrops = rq->stats.items.drops;
> + rpackets = rq->stats.packets;
> + rbytes = rq->stats.bytes;
> + rdrops = rq->stats.drops;
> } while (u64_stats_fetch_retry_irq(&rq->stats.syncp, start));
>
> tot->rx_packets += rpackets;
> @@ -2078,7 +2070,7 @@ static void virtnet_get_ethtool_stats(struct net_device *dev,
> for (i = 0; i < vi->curr_queue_pairs; i++) {
> struct receive_queue *rq = &vi->rq[i];
>
> - stats_base = (u8 *)&rq->stats.items;
> + stats_base = (u8 *)&rq->stats;
> do {
> start = u64_stats_fetch_begin_irq(&rq->stats.syncp);
> for (j = 0; j < VIRTNET_RQ_STATS_LEN; j++) {
> --
> 2.7.4
From: Jason Wang <[email protected]>
Date: Tue, 31 Jul 2018 17:43:38 +0800
> Commit 5b8f3c8d30a6 ("virtio_net: Add XDP related stats") tries to
> count TX XDP stats in virtnet_receive(). This will cause several
> issues:
>
> - virtnet_xdp_sq() was called without checking whether or not XDP is
> set. This may cause out of bound access when there's no enough txq
> for XDP.
> - Stats were updated even if there's no XDP/XDP_TX.
>
> Fixing this by reusing virtnet_xdp_xmit() for XDP_TX which can counts
> TX XDP counter itself and remove the unnecessary tx stats embedded in
> rx stats.
>
> Reported-by: [email protected]
> Fixes: 5b8f3c8d30a6 ("virtio_net: Add XDP related stats")
> Cc: Toshiaki Makita <[email protected]>
> Signed-off-by: Jason Wang <[email protected]>
Applied.
From: Jason Wang <[email protected]>
Date: Tue, 31 Jul 2018 17:43:39 +0800
> We don't maintain tx counters in rx stats any more. There's no need
> for an extra container of rq stats.
>
> Cc: Toshiaki Makita <[email protected]>
> Signed-off-by: Jason Wang <[email protected]>
Applied.
On 2018年07月31日 17:57, Toshiaki Makita wrote:
> On 2018/07/31 18:43, Jason Wang wrote:
>> Commit 5b8f3c8d30a6 ("virtio_net: Add XDP related stats") tries to
>> count TX XDP stats in virtnet_receive(). This will cause several
>> issues:
>>
>> - virtnet_xdp_sq() was called without checking whether or not XDP is
>> set. This may cause out of bound access when there's no enough txq
>> for XDP.
>> - Stats were updated even if there's no XDP/XDP_TX.>
>> Fixing this by reusing virtnet_xdp_xmit() for XDP_TX which can counts
>> TX XDP counter itself and remove the unnecessary tx stats embedded in
>> rx stats.
> Thanks for fixing this.
> I wanted to avoid calling u64_stats_update_begin() (i.e. smp_wmb() in 32
> bit systems) for every packet. So I'd like to keep sq stats in
> virtnet_rx_stats.
>
We can optimize this by adding batching on top. (virtnet_xdp_xmit()
accepts an array of xdp frames). If you like, please send a patch for this.
Thanks
On 2018年07月31日 18:02, Toshiaki Makita wrote:
> On 2018/07/31 18:43, Jason Wang wrote:
>> We don't maintain tx counters in rx stats any more. There's no need
>> for an extra container of rq stats.
>>
>> Cc: Toshiaki Makita <[email protected]>
>> Signed-off-by: Jason Wang <[email protected]>
>> ---
>> drivers/net/virtio_net.c | 80 ++++++++++++++++++++++--------------------------
>> 1 file changed, 36 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 72d3f68..14f661c 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -87,7 +87,8 @@ struct virtnet_sq_stats {
>> u64 kicks;
>> };
>>
>> -struct virtnet_rq_stat_items {
>> +struct virtnet_rq_stats {
>> + struct u64_stats_sync syncp;
>> u64 packets;
>> u64 bytes;
>> u64 drops;
>> @@ -98,17 +99,8 @@ struct virtnet_rq_stat_items {
>> u64 kicks;
>> };
>>
>> -struct virtnet_rq_stats {
>> - struct u64_stats_sync syncp;
>> - struct virtnet_rq_stat_items items;
>> -};
> I'm not thinking removing sq stat is needed but even if it is I want to
> keep virtnet_rq_stats to avoid allocating unnecessary u64_stats_syncp on
> stack in virtnet_receive. I would just remove virtnet_rx_stats if necessary.
It's a nop on 64bit machines. And an unsigned on 32bit. So it's overhead
could be ignored I think.
Thanks
>> -
>> -struct virtnet_rx_stats {
>> - struct virtnet_rq_stat_items rx;
>> -};
>> -
On 2018/08/01 10:31, Jason Wang wrote:
> On 2018年07月31日 17:57, Toshiaki Makita wrote:
>> On 2018/07/31 18:43, Jason Wang wrote:
>>> Commit 5b8f3c8d30a6 ("virtio_net: Add XDP related stats") tries to
>>> count TX XDP stats in virtnet_receive(). This will cause several
>>> issues:
>>>
>>> - virtnet_xdp_sq() was called without checking whether or not XDP is
>>> set. This may cause out of bound access when there's no enough txq
>>> for XDP.
>>> - Stats were updated even if there's no XDP/XDP_TX.>
>>> Fixing this by reusing virtnet_xdp_xmit() for XDP_TX which can counts
>>> TX XDP counter itself and remove the unnecessary tx stats embedded in
>>> rx stats.
>> Thanks for fixing this.
>> I wanted to avoid calling u64_stats_update_begin() (i.e. smp_wmb() in 32
>> bit systems) for every packet. So I'd like to keep sq stats in
>> virtnet_rx_stats.
>>
>
> We can optimize this by adding batching on top. (virtnet_xdp_xmit()
> accepts an array of xdp frames). If you like, please send a patch for this.
Yes, that sounds like a better optimization. will think about it...
Thanks,
Toshiaki Makita
On 2018/08/01 10:39, Jason Wang wrote:
> On 2018年07月31日 18:02, Toshiaki Makita wrote:
>> On 2018/07/31 18:43, Jason Wang wrote:
>>> We don't maintain tx counters in rx stats any more. There's no need
>>> for an extra container of rq stats.
>>>
>>> Cc: Toshiaki Makita <[email protected]>
>>> Signed-off-by: Jason Wang <[email protected]>
>>> ---
>>> drivers/net/virtio_net.c | 80
>>> ++++++++++++++++++++++--------------------------
>>> 1 file changed, 36 insertions(+), 44 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>> index 72d3f68..14f661c 100644
>>> --- a/drivers/net/virtio_net.c
>>> +++ b/drivers/net/virtio_net.c
>>> @@ -87,7 +87,8 @@ struct virtnet_sq_stats {
>>> u64 kicks;
>>> };
>>> -struct virtnet_rq_stat_items {
>>> +struct virtnet_rq_stats {
>>> + struct u64_stats_sync syncp;
>>> u64 packets;
>>> u64 bytes;
>>> u64 drops;
>>> @@ -98,17 +99,8 @@ struct virtnet_rq_stat_items {
>>> u64 kicks;
>>> };
>>> -struct virtnet_rq_stats {
>>> - struct u64_stats_sync syncp;
>>> - struct virtnet_rq_stat_items items;
>>> -};
>> I'm not thinking removing sq stat is needed but even if it is I want to
>> keep virtnet_rq_stats to avoid allocating unnecessary u64_stats_syncp on
>> stack in virtnet_receive. I would just remove virtnet_rx_stats if
>> necessary.
>
> It's a nop on 64bit machines. And an unsigned on 32bit. So it's overhead
> could be ignored I think.
It's not a big problem so that's OK. I just felt like you reverted
unnecessarily too much. Anyway it is already applied and I'm not
thinking of changing this any more.
--
Toshiaki Makita