* Re: [PATCH net-next] virtio_net: bulk free tx skbs
From: David Miller @ 2019-01-16 23:31 UTC (permalink / raw)
To: mst; +Cc: netdev, linux-kernel, virtualization
In-Reply-To: <20190115012847.14730-1-mst@redhat.com>
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Mon, 14 Jan 2019 20:34:26 -0500
> Use napi_consume_skb() to get bulk free. Note that napi_consume_skb is
> safe to call in a non-napi context as long as the napi_budget flag is
> correct.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> My perf testing setup is down but it works fine on my devel box and
> should be fairly uncontroversial.
It would be uncontroversial if it compiled.
drivers/net/virtio_net.c: In function ‘free_old_xmit_skbs’:
drivers/net/virtio_net.c:1346:25: error: ‘use_napi’ undeclared (first use in this function); did you mean ‘used_math’?
napi_consume_skb(skb, use_napi);
^~~~~~~~
used_math
drivers/net/virtio_net.c:1346:25: note: each undeclared identifier is reported only once for each function it appears in
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH net 0/7] virtio_net: Fix problems around XDP tx and napi_tx
From: Toshiaki Makita @ 2019-01-17 11:20 UTC (permalink / raw)
To: David S. Miller, Michael S. Tsirkin, Jason Wang
Cc: netdev, Willem de Bruijn, virtualization, Jesper Dangaard Brouer
While I'm looking into how to account standard tx counters on XDP tx
processing, I found several bugs around XDP tx and napi_tx.
Patch1: Fix oops on error path. Patch2 depends on this.
Patch2: Fix memory corruption on freeing xdp_frames with napi_tx enabled.
Patch3: Minor fix patch5 depends on.
Patch4: Fix memory corruption on processing xdp_frames when XDP is disabled.
Also patch5 depends on this.
Patch5: Fix memory corruption on processing xdp_frames while XDP is being
disabled.
Patch6: Minor fix patch7 depends on.
Patch7: Fix memory corruption on freeing sk_buff or xdp_frames when a normal
queue is reused for XDP and vise versa.
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
Toshiaki Makita (7):
virtio_net: Don't enable NAPI when interface is down
virtio_net: Don't call free_old_xmit_skbs for xdp_frames
virtio_net: Fix not restoring real_num_rx_queues
virtio_net: Fix out of bounds access of sq
virtio_net: Don't process redirected XDP frames when XDP is disabled
virtio_net: Use xdp_return_frame to free xdp_frames on destroying vqs
virtio_net: Differentiate sk_buff and xdp_frame on freeing
drivers/net/virtio_net.c | 154 +++++++++++++++++++++++++++++++++--------------
1 file changed, 109 insertions(+), 45 deletions(-)
--
1.8.3.1
^ permalink raw reply
* [PATCH net 1/7] virtio_net: Don't enable NAPI when interface is down
From: Toshiaki Makita @ 2019-01-17 11:20 UTC (permalink / raw)
To: David S. Miller, Michael S. Tsirkin, Jason Wang; +Cc: netdev, virtualization
In-Reply-To: <1547724045-2726-1-git-send-email-makita.toshiaki@lab.ntt.co.jp>
Commit 4e09ff536284 ("virtio-net: disable NAPI only when enabled during
XDP set") tried to fix inappropriate NAPI enabling/disabling when
!netif_running(), but was not complete.
On error path virtio_net could enable NAPI even when !netif_running().
This can cause enabling NAPI twice on virtnet_open(), which would
trigger BUG_ON() in napi_enable().
Fixes: 4941d472bf95b ("virtio-net: do not reset during XDP set")
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
drivers/net/virtio_net.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 0237250..a08da9e 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2430,8 +2430,10 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
return 0;
err:
- for (i = 0; i < vi->max_queue_pairs; i++)
- virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
+ if (netif_running(dev)) {
+ for (i = 0; i < vi->max_queue_pairs; i++)
+ virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
+ }
if (prog)
bpf_prog_sub(prog, vi->max_queue_pairs - 1);
return err;
--
1.8.3.1
^ permalink raw reply related
* [PATCH net 2/7] virtio_net: Don't call free_old_xmit_skbs for xdp_frames
From: Toshiaki Makita @ 2019-01-17 11:20 UTC (permalink / raw)
To: David S. Miller, Michael S. Tsirkin, Jason Wang
Cc: netdev, Willem de Bruijn, virtualization
In-Reply-To: <1547724045-2726-1-git-send-email-makita.toshiaki@lab.ntt.co.jp>
When napi_tx is enabled, virtnet_poll_cleantx() called
free_old_xmit_skbs() even for xdp send queue.
This is bogus since the queue has xdp_frames, not sk_buffs, thus mangled
device tx bytes counters because skb->len is meaningless value, and even
triggered oops due to general protection fault on freeing them.
Since xdp send queues do not aquire locks, old xdp_frames should be
freed only in virtnet_xdp_xmit(), so just skip free_old_xmit_skbs() for
xdp send queues.
Similarly virtnet_poll_tx() called free_old_xmit_skbs(). This NAPI
handler is called even without calling start_xmit() because cb for tx is
by default enabled. Once the handler is called, it enabled the cb again,
and then the handler would be called again. We don't need this handler
for XDP, so don't enable cb as well as not calling free_old_xmit_skbs().
Also, we need to disable tx NAPI when disabling XDP, so
virtnet_poll_tx() can safely access curr_queue_pairs and
xdp_queue_pairs, which are not atomically updated while disabling XDP.
Fixes: b92f1e6751a6 ("virtio-net: transmit napi")
Fixes: 7b0411ef4aa6 ("virtio-net: clean tx descriptors from rx napi")
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
drivers/net/virtio_net.c | 49 ++++++++++++++++++++++++++++++++----------------
1 file changed, 33 insertions(+), 16 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index a08da9e..7d35e6d 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1358,6 +1358,16 @@ static void free_old_xmit_skbs(struct send_queue *sq)
u64_stats_update_end(&sq->stats.syncp);
}
+static bool is_xdp_raw_buffer_queue(struct virtnet_info *vi, int q)
+{
+ if (q < (vi->curr_queue_pairs - vi->xdp_queue_pairs))
+ return false;
+ else if (q < vi->curr_queue_pairs)
+ return true;
+ else
+ return false;
+}
+
static void virtnet_poll_cleantx(struct receive_queue *rq)
{
struct virtnet_info *vi = rq->vq->vdev->priv;
@@ -1365,7 +1375,7 @@ static void virtnet_poll_cleantx(struct receive_queue *rq)
struct send_queue *sq = &vi->sq[index];
struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, index);
- if (!sq->napi.weight)
+ if (!sq->napi.weight || is_xdp_raw_buffer_queue(vi, index))
return;
if (__netif_tx_trylock(txq)) {
@@ -1442,8 +1452,16 @@ static int virtnet_poll_tx(struct napi_struct *napi, int budget)
{
struct send_queue *sq = container_of(napi, struct send_queue, napi);
struct virtnet_info *vi = sq->vq->vdev->priv;
- struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, vq2txq(sq->vq));
+ unsigned int index = vq2txq(sq->vq);
+ struct netdev_queue *txq;
+ if (unlikely(is_xdp_raw_buffer_queue(vi, index))) {
+ /* We don't need to enable cb for XDP */
+ napi_complete_done(napi, 0);
+ return 0;
+ }
+
+ txq = netdev_get_tx_queue(vi->dev, index);
__netif_tx_lock(txq, raw_smp_processor_id());
free_old_xmit_skbs(sq);
__netif_tx_unlock(txq);
@@ -2402,9 +2420,12 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
}
/* Make sure NAPI is not using any XDP TX queues for RX. */
- if (netif_running(dev))
- for (i = 0; i < vi->max_queue_pairs; i++)
+ if (netif_running(dev)) {
+ for (i = 0; i < vi->max_queue_pairs; i++) {
napi_disable(&vi->rq[i].napi);
+ virtnet_napi_tx_disable(&vi->sq[i].napi);
+ }
+ }
netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
@@ -2423,16 +2444,22 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
}
if (old_prog)
bpf_prog_put(old_prog);
- if (netif_running(dev))
+ if (netif_running(dev)) {
virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
+ virtnet_napi_tx_enable(vi, vi->sq[i].vq,
+ &vi->sq[i].napi);
+ }
}
return 0;
err:
if (netif_running(dev)) {
- for (i = 0; i < vi->max_queue_pairs; i++)
+ for (i = 0; i < vi->max_queue_pairs; i++) {
virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
+ virtnet_napi_tx_enable(vi, vi->sq[i].vq,
+ &vi->sq[i].napi);
+ }
}
if (prog)
bpf_prog_sub(prog, vi->max_queue_pairs - 1);
@@ -2615,16 +2642,6 @@ static void free_receive_page_frags(struct virtnet_info *vi)
put_page(vi->rq[i].alloc_frag.page);
}
-static bool is_xdp_raw_buffer_queue(struct virtnet_info *vi, int q)
-{
- if (q < (vi->curr_queue_pairs - vi->xdp_queue_pairs))
- return false;
- else if (q < vi->curr_queue_pairs)
- return true;
- else
- return false;
-}
-
static void free_unused_bufs(struct virtnet_info *vi)
{
void *buf;
--
1.8.3.1
^ permalink raw reply related
* [PATCH net 3/7] virtio_net: Fix not restoring real_num_rx_queues
From: Toshiaki Makita @ 2019-01-17 11:20 UTC (permalink / raw)
To: David S. Miller, Michael S. Tsirkin, Jason Wang; +Cc: netdev, virtualization
In-Reply-To: <1547724045-2726-1-git-send-email-makita.toshiaki@lab.ntt.co.jp>
When _virtnet_set_queues() failed we did not restore real_num_rx_queues.
Fix this by placing the change of real_num_rx_queues after
_virtnet_set_queues().
This order is also in line with virtnet_set_channels().
Fixes: 4941d472bf95 ("virtio-net: do not reset during XDP set")
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
drivers/net/virtio_net.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 7d35e6d..670cc15 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2427,10 +2427,10 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
}
}
- netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
if (err)
goto err;
+ netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
vi->xdp_queue_pairs = xdp_qp;
for (i = 0; i < vi->max_queue_pairs; i++) {
--
1.8.3.1
^ permalink raw reply related
* [PATCH net 4/7] virtio_net: Fix out of bounds access of sq
From: Toshiaki Makita @ 2019-01-17 11:20 UTC (permalink / raw)
To: David S. Miller, Michael S. Tsirkin, Jason Wang; +Cc: netdev, virtualization
In-Reply-To: <1547724045-2726-1-git-send-email-makita.toshiaki@lab.ntt.co.jp>
When XDP is disabled, curr_queue_pairs + smp_processor_id() can be
larger than max_queue_pairs.
There is no guarantee that we have enough XDP send queues dedicated for
each cpu when XDP is disabled, so do not count drops on sq in that case.
Fixes: 5b8f3c8d30a6 ("virtio_net: Add XDP related stats")
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
drivers/net/virtio_net.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 670cc15..204eedf 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -491,20 +491,17 @@ static int virtnet_xdp_xmit(struct net_device *dev,
int ret, err;
int i;
- sq = virtnet_xdp_sq(vi);
-
- if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK)) {
- ret = -EINVAL;
- drops = n;
- goto out;
- }
-
/* Only allow ndo_xdp_xmit if XDP is loaded on dev, as this
* indicate XDP resources have been successfully allocated.
*/
xdp_prog = rcu_dereference(rq->xdp_prog);
- if (!xdp_prog) {
- ret = -ENXIO;
+ if (!xdp_prog)
+ return -ENXIO;
+
+ sq = virtnet_xdp_sq(vi);
+
+ if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK)) {
+ ret = -EINVAL;
drops = n;
goto out;
}
--
1.8.3.1
^ permalink raw reply related
* [PATCH net 5/7] virtio_net: Don't process redirected XDP frames when XDP is disabled
From: Toshiaki Makita @ 2019-01-17 11:20 UTC (permalink / raw)
To: David S. Miller, Michael S. Tsirkin, Jason Wang
Cc: netdev, virtualization, Jesper Dangaard Brouer
In-Reply-To: <1547724045-2726-1-git-send-email-makita.toshiaki@lab.ntt.co.jp>
Commit 8dcc5b0ab0ec ("virtio_net: fix ndo_xdp_xmit crash towards dev not
ready for XDP") tried to avoid access to unexpected sq while XDP is
disabled, but was not complete.
There was a small window which causes out of bounds sq access in
virtnet_xdp_xmit() while disabling XDP.
An example case of
- curr_queue_pairs = 6 (2 for SKB and 4 for XDP)
- online_cpu_num = xdp_queue_paris = 4
when XDP is enabled:
CPU 0 CPU 1
(Disabling XDP) (Processing redirected XDP frames)
virtnet_xdp_xmit()
virtnet_xdp_set()
_virtnet_set_queues()
set curr_queue_pairs (2)
check if rq->xdp_prog is not NULL
virtnet_xdp_sq(vi)
qp = curr_queue_pairs -
xdp_queue_pairs +
smp_processor_id()
= 2 - 4 + 1 = -1
sq = &vi->sq[qp] // out of bounds access
set xdp_queue_pairs (0)
rq->xdp_prog = NULL
Basically we should not change curr_queue_pairs and xdp_queue_pairs
while someone can read the values. Thus, when disabling XDP, assign NULL
to rq->xdp_prog first, and wait for RCU grace period, then change
xxx_queue_pairs.
Note that we need to keep the current order when enabling XDP though.
Fixes: 186b3c998c50 ("virtio-net: support XDP_REDIRECT")
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
drivers/net/virtio_net.c | 32 +++++++++++++++++++++++++-------
1 file changed, 25 insertions(+), 7 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 204eedf..ae93f0e 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2424,14 +2424,16 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
}
}
- err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
- if (err)
- goto err;
- netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
- vi->xdp_queue_pairs = xdp_qp;
+ old_prog = rtnl_dereference(vi->rq[0].xdp_prog);
+ if (!old_prog && prog) {
+ err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
+ if (err)
+ goto err_new_prog;
+ netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
+ vi->xdp_queue_pairs = xdp_qp;
+ }
for (i = 0; i < vi->max_queue_pairs; i++) {
- old_prog = rtnl_dereference(vi->rq[i].xdp_prog);
rcu_assign_pointer(vi->rq[i].xdp_prog, prog);
if (i == 0) {
if (!old_prog)
@@ -2439,6 +2441,18 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
if (!prog)
virtnet_restore_guest_offloads(vi);
}
+ }
+
+ if (old_prog && !prog) {
+ synchronize_net();
+ err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
+ if (err)
+ goto err_old_prog;
+ netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
+ vi->xdp_queue_pairs = xdp_qp;
+ }
+
+ for (i = 0; i < vi->max_queue_pairs; i++) {
if (old_prog)
bpf_prog_put(old_prog);
if (netif_running(dev)) {
@@ -2450,7 +2464,11 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
return 0;
-err:
+err_old_prog:
+ virtnet_clear_guest_offloads(vi);
+ for (i = 0; i < vi->max_queue_pairs; i++)
+ rcu_assign_pointer(vi->rq[i].xdp_prog, old_prog);
+err_new_prog:
if (netif_running(dev)) {
for (i = 0; i < vi->max_queue_pairs; i++) {
virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
--
1.8.3.1
^ permalink raw reply related
* [PATCH net 6/7] virtio_net: Use xdp_return_frame to free xdp_frames on destroying vqs
From: Toshiaki Makita @ 2019-01-17 11:20 UTC (permalink / raw)
To: David S. Miller, Michael S. Tsirkin, Jason Wang
Cc: netdev, virtualization, Jesper Dangaard Brouer
In-Reply-To: <1547724045-2726-1-git-send-email-makita.toshiaki@lab.ntt.co.jp>
put_page() can work as a fallback for freeing xdp_frames, but the
appropriate way is to use xdp_return_frame().
Fixes: cac320c850ef ("virtio_net: convert to use generic xdp_frame and xdp_return_frame API")
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
drivers/net/virtio_net.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index ae93f0e..996de69 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2668,7 +2668,7 @@ static void free_unused_bufs(struct virtnet_info *vi)
if (!is_xdp_raw_buffer_queue(vi, i))
dev_kfree_skb(buf);
else
- put_page(virt_to_head_page(buf));
+ xdp_return_frame(buf);
}
}
--
1.8.3.1
^ permalink raw reply related
* [PATCH net 7/7] virtio_net: Differentiate sk_buff and xdp_frame on freeing
From: Toshiaki Makita @ 2019-01-17 11:20 UTC (permalink / raw)
To: David S. Miller, Michael S. Tsirkin, Jason Wang; +Cc: netdev, virtualization
In-Reply-To: <1547724045-2726-1-git-send-email-makita.toshiaki@lab.ntt.co.jp>
We do not reset or free up unused buffers when enabling/disabling XDP,
so it can happen that xdp_frames are freed after disabling XDP or
sk_buffs are freed after enabling XDP on xdp tx queues.
Thus we need to handle both forms (xdp_frames and sk_buffs) regardless
of XDP setting.
One way to trigger this problem is to disable XDP when napi_tx is
enabled. In that case, virtnet_xdp_set() calls virtnet_napi_enable()
which kicks NAPI. The NAPI handler will call virtnet_poll_cleantx()
which invokes free_old_xmit_skbs() for queues which have been used by
XDP.
Note that even with this change we need to keep skipping
free_old_xmit_skbs() from NAPI handlers when XDP is enabled, because XDP
tx queues do not aquire queue locks.
Fixes: 4941d472bf95 ("virtio-net: do not reset during XDP set")
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
drivers/net/virtio_net.c | 54 +++++++++++++++++++++++++++++++++++++-----------
1 file changed, 42 insertions(+), 12 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 996de69..6598c25 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -57,6 +57,8 @@
#define VIRTIO_XDP_TX BIT(0)
#define VIRTIO_XDP_REDIR BIT(1)
+#define VIRTIO_XDP_FLAG BIT(0)
+
/* RX packet size EWMA. The average packet size is used to determine the packet
* buffer size when refilling RX rings. As the entire RX ring may be refilled
* at once, the weight is chosen so that the EWMA will be insensitive to short-
@@ -252,6 +254,21 @@ struct padded_vnet_hdr {
char padding[4];
};
+static bool is_xdp_frame(void *ptr)
+{
+ return (unsigned long)ptr & VIRTIO_XDP_FLAG;
+}
+
+static void *xdp_to_ptr(struct xdp_frame *ptr)
+{
+ return (void *)((unsigned long)ptr | VIRTIO_XDP_FLAG);
+}
+
+static struct xdp_frame *ptr_to_xdp(void *ptr)
+{
+ return (struct xdp_frame *)((unsigned long)ptr & ~VIRTIO_XDP_FLAG);
+}
+
/* Converting between virtqueue no. and kernel tx/rx queue no.
* 0:rx0 1:tx0 2:rx1 3:tx1 ... 2N:rxN 2N+1:txN 2N+2:cvq
*/
@@ -462,7 +479,8 @@ static int __virtnet_xdp_xmit_one(struct virtnet_info *vi,
sg_init_one(sq->sg, xdpf->data, xdpf->len);
- err = virtqueue_add_outbuf(sq->vq, sq->sg, 1, xdpf, GFP_ATOMIC);
+ err = virtqueue_add_outbuf(sq->vq, sq->sg, 1, xdp_to_ptr(xdpf),
+ GFP_ATOMIC);
if (unlikely(err))
return -ENOSPC; /* Caller handle free/refcnt */
@@ -482,13 +500,13 @@ static int virtnet_xdp_xmit(struct net_device *dev,
{
struct virtnet_info *vi = netdev_priv(dev);
struct receive_queue *rq = vi->rq;
- struct xdp_frame *xdpf_sent;
struct bpf_prog *xdp_prog;
struct send_queue *sq;
unsigned int len;
int drops = 0;
int kicks = 0;
int ret, err;
+ void *ptr;
int i;
/* Only allow ndo_xdp_xmit if XDP is loaded on dev, as this
@@ -507,8 +525,12 @@ static int virtnet_xdp_xmit(struct net_device *dev,
}
/* 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);
+ while ((ptr = virtqueue_get_buf(sq->vq, &len)) != NULL) {
+ if (likely(is_xdp_frame(ptr)))
+ xdp_return_frame(ptr_to_xdp(ptr));
+ else
+ dev_consume_skb_any(ptr);
+ }
for (i = 0; i < n; i++) {
struct xdp_frame *xdpf = frames[i];
@@ -1329,18 +1351,26 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
static void free_old_xmit_skbs(struct send_queue *sq)
{
- struct sk_buff *skb;
unsigned int len;
unsigned int packets = 0;
unsigned int bytes = 0;
+ void *ptr;
- while ((skb = virtqueue_get_buf(sq->vq, &len)) != NULL) {
- pr_debug("Sent skb %p\n", skb);
+ while ((ptr = virtqueue_get_buf(sq->vq, &len)) != NULL) {
+ if (likely(!is_xdp_frame(ptr))) {
+ struct sk_buff *skb = ptr;
- bytes += skb->len;
- packets++;
+ pr_debug("Sent skb %p\n", skb);
- dev_consume_skb_any(skb);
+ bytes += skb->len;
+ dev_consume_skb_any(skb);
+ } else {
+ struct xdp_frame *frame = ptr_to_xdp(ptr);
+
+ bytes += frame->len;
+ xdp_return_frame(frame);
+ }
+ packets++;
}
/* Avoid overhead when no packets have been processed
@@ -2665,10 +2695,10 @@ static void free_unused_bufs(struct virtnet_info *vi)
for (i = 0; i < vi->max_queue_pairs; i++) {
struct virtqueue *vq = vi->sq[i].vq;
while ((buf = virtqueue_detach_unused_buf(vq)) != NULL) {
- if (!is_xdp_raw_buffer_queue(vi, i))
+ if (!is_xdp_frame(buf))
dev_kfree_skb(buf);
else
- xdp_return_frame(buf);
+ xdp_return_frame(ptr_to_xdp(buf));
}
}
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH net 1/7] virtio_net: Don't enable NAPI when interface is down
From: Jason Wang @ 2019-01-17 12:38 UTC (permalink / raw)
To: Toshiaki Makita, David S. Miller, Michael S. Tsirkin
Cc: netdev, virtualization
In-Reply-To: <1547724045-2726-2-git-send-email-makita.toshiaki@lab.ntt.co.jp>
On 2019/1/17 下午7:20, Toshiaki Makita wrote:
> Commit 4e09ff536284 ("virtio-net: disable NAPI only when enabled during
> XDP set") tried to fix inappropriate NAPI enabling/disabling when
> !netif_running(), but was not complete.
>
> On error path virtio_net could enable NAPI even when !netif_running().
> This can cause enabling NAPI twice on virtnet_open(), which would
> trigger BUG_ON() in napi_enable().
>
> Fixes: 4941d472bf95b ("virtio-net: do not reset during XDP set")
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> ---
> drivers/net/virtio_net.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 0237250..a08da9e 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2430,8 +2430,10 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> return 0;
>
> err:
> - for (i = 0; i < vi->max_queue_pairs; i++)
> - virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
> + if (netif_running(dev)) {
> + for (i = 0; i < vi->max_queue_pairs; i++)
> + virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
> + }
> if (prog)
> bpf_prog_sub(prog, vi->max_queue_pairs - 1);
> return err;
Acked-by: Jason Wang <jasowang@redhat.com>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net 2/7] virtio_net: Don't call free_old_xmit_skbs for xdp_frames
From: Jason Wang @ 2019-01-17 12:39 UTC (permalink / raw)
To: Toshiaki Makita, David S. Miller, Michael S. Tsirkin
Cc: netdev, Willem de Bruijn, virtualization
In-Reply-To: <1547724045-2726-3-git-send-email-makita.toshiaki@lab.ntt.co.jp>
On 2019/1/17 下午7:20, Toshiaki Makita wrote:
> When napi_tx is enabled, virtnet_poll_cleantx() called
> free_old_xmit_skbs() even for xdp send queue.
> This is bogus since the queue has xdp_frames, not sk_buffs, thus mangled
> device tx bytes counters because skb->len is meaningless value, and even
> triggered oops due to general protection fault on freeing them.
>
> Since xdp send queues do not aquire locks, old xdp_frames should be
> freed only in virtnet_xdp_xmit(), so just skip free_old_xmit_skbs() for
> xdp send queues.
>
> Similarly virtnet_poll_tx() called free_old_xmit_skbs(). This NAPI
> handler is called even without calling start_xmit() because cb for tx is
> by default enabled. Once the handler is called, it enabled the cb again,
> and then the handler would be called again. We don't need this handler
> for XDP, so don't enable cb as well as not calling free_old_xmit_skbs().
>
> Also, we need to disable tx NAPI when disabling XDP, so
> virtnet_poll_tx() can safely access curr_queue_pairs and
> xdp_queue_pairs, which are not atomically updated while disabling XDP.
I suggest to split this into another patch or squash this part to patch 1.
Other looks good.
Thanks
>
> Fixes: b92f1e6751a6 ("virtio-net: transmit napi")
> Fixes: 7b0411ef4aa6 ("virtio-net: clean tx descriptors from rx napi")
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> ---
> drivers/net/virtio_net.c | 49 ++++++++++++++++++++++++++++++++----------------
> 1 file changed, 33 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index a08da9e..7d35e6d 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1358,6 +1358,16 @@ static void free_old_xmit_skbs(struct send_queue *sq)
> u64_stats_update_end(&sq->stats.syncp);
> }
>
> +static bool is_xdp_raw_buffer_queue(struct virtnet_info *vi, int q)
> +{
> + if (q < (vi->curr_queue_pairs - vi->xdp_queue_pairs))
> + return false;
> + else if (q < vi->curr_queue_pairs)
> + return true;
> + else
> + return false;
> +}
> +
> static void virtnet_poll_cleantx(struct receive_queue *rq)
> {
> struct virtnet_info *vi = rq->vq->vdev->priv;
> @@ -1365,7 +1375,7 @@ static void virtnet_poll_cleantx(struct receive_queue *rq)
> struct send_queue *sq = &vi->sq[index];
> struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, index);
>
> - if (!sq->napi.weight)
> + if (!sq->napi.weight || is_xdp_raw_buffer_queue(vi, index))
> return;
>
> if (__netif_tx_trylock(txq)) {
> @@ -1442,8 +1452,16 @@ static int virtnet_poll_tx(struct napi_struct *napi, int budget)
> {
> struct send_queue *sq = container_of(napi, struct send_queue, napi);
> struct virtnet_info *vi = sq->vq->vdev->priv;
> - struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, vq2txq(sq->vq));
> + unsigned int index = vq2txq(sq->vq);
> + struct netdev_queue *txq;
>
> + if (unlikely(is_xdp_raw_buffer_queue(vi, index))) {
> + /* We don't need to enable cb for XDP */
> + napi_complete_done(napi, 0);
> + return 0;
> + }
> +
> + txq = netdev_get_tx_queue(vi->dev, index);
> __netif_tx_lock(txq, raw_smp_processor_id());
> free_old_xmit_skbs(sq);
> __netif_tx_unlock(txq);
> @@ -2402,9 +2420,12 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> }
>
> /* Make sure NAPI is not using any XDP TX queues for RX. */
> - if (netif_running(dev))
> - for (i = 0; i < vi->max_queue_pairs; i++)
> + if (netif_running(dev)) {
> + for (i = 0; i < vi->max_queue_pairs; i++) {
> napi_disable(&vi->rq[i].napi);
> + virtnet_napi_tx_disable(&vi->sq[i].napi);
> + }
> + }
>
> netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
> err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
> @@ -2423,16 +2444,22 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> }
> if (old_prog)
> bpf_prog_put(old_prog);
> - if (netif_running(dev))
> + if (netif_running(dev)) {
> virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
> + virtnet_napi_tx_enable(vi, vi->sq[i].vq,
> + &vi->sq[i].napi);
> + }
> }
>
> return 0;
>
> err:
> if (netif_running(dev)) {
> - for (i = 0; i < vi->max_queue_pairs; i++)
> + for (i = 0; i < vi->max_queue_pairs; i++) {
> virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
> + virtnet_napi_tx_enable(vi, vi->sq[i].vq,
> + &vi->sq[i].napi);
> + }
> }
> if (prog)
> bpf_prog_sub(prog, vi->max_queue_pairs - 1);
> @@ -2615,16 +2642,6 @@ static void free_receive_page_frags(struct virtnet_info *vi)
> put_page(vi->rq[i].alloc_frag.page);
> }
>
> -static bool is_xdp_raw_buffer_queue(struct virtnet_info *vi, int q)
> -{
> - if (q < (vi->curr_queue_pairs - vi->xdp_queue_pairs))
> - return false;
> - else if (q < vi->curr_queue_pairs)
> - return true;
> - else
> - return false;
> -}
> -
> static void free_unused_bufs(struct virtnet_info *vi)
> {
> void *buf;
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net 3/7] virtio_net: Fix not restoring real_num_rx_queues
From: Jason Wang @ 2019-01-17 12:39 UTC (permalink / raw)
To: Toshiaki Makita, David S. Miller, Michael S. Tsirkin
Cc: netdev, virtualization
In-Reply-To: <1547724045-2726-4-git-send-email-makita.toshiaki@lab.ntt.co.jp>
On 2019/1/17 下午7:20, Toshiaki Makita wrote:
> When _virtnet_set_queues() failed we did not restore real_num_rx_queues.
> Fix this by placing the change of real_num_rx_queues after
> _virtnet_set_queues().
> This order is also in line with virtnet_set_channels().
>
> Fixes: 4941d472bf95 ("virtio-net: do not reset during XDP set")
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> ---
> drivers/net/virtio_net.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 7d35e6d..670cc15 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2427,10 +2427,10 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> }
> }
>
> - netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
> err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
> if (err)
> goto err;
> + netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
> vi->xdp_queue_pairs = xdp_qp;
>
> for (i = 0; i < vi->max_queue_pairs; i++) {
Acked-by: Jason Wang <jasowang@redhat.com>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net 4/7] virtio_net: Fix out of bounds access of sq
From: Jason Wang @ 2019-01-17 12:42 UTC (permalink / raw)
To: Toshiaki Makita, David S. Miller, Michael S. Tsirkin
Cc: netdev, virtualization
In-Reply-To: <1547724045-2726-5-git-send-email-makita.toshiaki@lab.ntt.co.jp>
On 2019/1/17 下午7:20, Toshiaki Makita wrote:
> When XDP is disabled, curr_queue_pairs + smp_processor_id() can be
> larger than max_queue_pairs.
> There is no guarantee that we have enough XDP send queues dedicated for
> each cpu when XDP is disabled, so do not count drops on sq in that case.
>
> Fixes: 5b8f3c8d30a6 ("virtio_net: Add XDP related stats")
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> ---
> drivers/net/virtio_net.c | 17 +++++++----------
> 1 file changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 670cc15..204eedf 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -491,20 +491,17 @@ static int virtnet_xdp_xmit(struct net_device *dev,
> int ret, err;
> int i;
>
> - sq = virtnet_xdp_sq(vi);
> -
> - if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK)) {
> - ret = -EINVAL;
> - drops = n;
> - goto out;
> - }
> -
> /* Only allow ndo_xdp_xmit if XDP is loaded on dev, as this
> * indicate XDP resources have been successfully allocated.
> */
> xdp_prog = rcu_dereference(rq->xdp_prog);
> - if (!xdp_prog) {
> - ret = -ENXIO;
> + if (!xdp_prog)
> + return -ENXIO;
> +
> + sq = virtnet_xdp_sq(vi);
> +
> + if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK)) {
> + ret = -EINVAL;
> drops = n;
> goto out;
> }
Acked-by: Jason Wang <jasowang@redhat.com>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net 5/7] virtio_net: Don't process redirected XDP frames when XDP is disabled
From: Jason Wang @ 2019-01-17 12:53 UTC (permalink / raw)
To: Toshiaki Makita, David S. Miller, Michael S. Tsirkin
Cc: netdev, Jesper Dangaard Brouer, virtualization
In-Reply-To: <1547724045-2726-6-git-send-email-makita.toshiaki@lab.ntt.co.jp>
On 2019/1/17 下午7:20, Toshiaki Makita wrote:
> Commit 8dcc5b0ab0ec ("virtio_net: fix ndo_xdp_xmit crash towards dev not
> ready for XDP") tried to avoid access to unexpected sq while XDP is
> disabled, but was not complete.
>
> There was a small window which causes out of bounds sq access in
> virtnet_xdp_xmit() while disabling XDP.
>
> An example case of
> - curr_queue_pairs = 6 (2 for SKB and 4 for XDP)
> - online_cpu_num = xdp_queue_paris = 4
> when XDP is enabled:
>
> CPU 0 CPU 1
> (Disabling XDP) (Processing redirected XDP frames)
>
> virtnet_xdp_xmit()
> virtnet_xdp_set()
> _virtnet_set_queues()
> set curr_queue_pairs (2)
> check if rq->xdp_prog is not NULL
> virtnet_xdp_sq(vi)
> qp = curr_queue_pairs -
> xdp_queue_pairs +
> smp_processor_id()
> = 2 - 4 + 1 = -1
> sq = &vi->sq[qp] // out of bounds access
> set xdp_queue_pairs (0)
> rq->xdp_prog = NULL
>
> Basically we should not change curr_queue_pairs and xdp_queue_pairs
> while someone can read the values. Thus, when disabling XDP, assign NULL
> to rq->xdp_prog first, and wait for RCU grace period, then change
> xxx_queue_pairs.
> Note that we need to keep the current order when enabling XDP though.
>
> Fixes: 186b3c998c50 ("virtio-net: support XDP_REDIRECT")
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
I wonder whether or not we could simply do:
if (prog) {
rcu_assign_pointer()
synchronize_net()
}
set queues
if (!prog) {
rcu_assign_pointer()
}
Thanks
> ---
> drivers/net/virtio_net.c | 32 +++++++++++++++++++++++++-------
> 1 file changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 204eedf..ae93f0e 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2424,14 +2424,16 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> }
> }
>
> - err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
> - if (err)
> - goto err;
> - netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
> - vi->xdp_queue_pairs = xdp_qp;
> + old_prog = rtnl_dereference(vi->rq[0].xdp_prog);
> + if (!old_prog && prog) {
> + err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
> + if (err)
> + goto err_new_prog;
> + netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
> + vi->xdp_queue_pairs = xdp_qp;
> + }
>
> for (i = 0; i < vi->max_queue_pairs; i++) {
> - old_prog = rtnl_dereference(vi->rq[i].xdp_prog);
> rcu_assign_pointer(vi->rq[i].xdp_prog, prog);
> if (i == 0) {
> if (!old_prog)
> @@ -2439,6 +2441,18 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> if (!prog)
> virtnet_restore_guest_offloads(vi);
> }
> + }
> +
> + if (old_prog && !prog) {
> + synchronize_net();
> + err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
> + if (err)
> + goto err_old_prog;
> + netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
> + vi->xdp_queue_pairs = xdp_qp;
> + }
> +
> + for (i = 0; i < vi->max_queue_pairs; i++) {
> if (old_prog)
> bpf_prog_put(old_prog);
> if (netif_running(dev)) {
> @@ -2450,7 +2464,11 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>
> return 0;
>
> -err:
> +err_old_prog:
> + virtnet_clear_guest_offloads(vi);
> + for (i = 0; i < vi->max_queue_pairs; i++)
> + rcu_assign_pointer(vi->rq[i].xdp_prog, old_prog);
> +err_new_prog:
> if (netif_running(dev)) {
> for (i = 0; i < vi->max_queue_pairs; i++) {
> virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net 6/7] virtio_net: Use xdp_return_frame to free xdp_frames on destroying vqs
From: Jason Wang @ 2019-01-17 12:56 UTC (permalink / raw)
To: Toshiaki Makita, David S. Miller, Michael S. Tsirkin
Cc: netdev, Jesper Dangaard Brouer, virtualization
In-Reply-To: <1547724045-2726-7-git-send-email-makita.toshiaki@lab.ntt.co.jp>
On 2019/1/17 下午7:20, Toshiaki Makita wrote:
> put_page() can work as a fallback for freeing xdp_frames, but the
> appropriate way is to use xdp_return_frame().
>
> Fixes: cac320c850ef ("virtio_net: convert to use generic xdp_frame and xdp_return_frame API")
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> ---
> drivers/net/virtio_net.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index ae93f0e..996de69 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2668,7 +2668,7 @@ static void free_unused_bufs(struct virtnet_info *vi)
> if (!is_xdp_raw_buffer_queue(vi, i))
> dev_kfree_skb(buf);
> else
> - put_page(virt_to_head_page(buf));
> + xdp_return_frame(buf);
> }
> }
>
Acked-by: Jason Wang <jasowang@redhat.com>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net 7/7] virtio_net: Differentiate sk_buff and xdp_frame on freeing
From: Jason Wang @ 2019-01-17 13:04 UTC (permalink / raw)
To: Toshiaki Makita, David S. Miller, Michael S. Tsirkin
Cc: netdev, virtualization
In-Reply-To: <1547724045-2726-8-git-send-email-makita.toshiaki@lab.ntt.co.jp>
On 2019/1/17 7:20, Toshiaki Makita wrote:
> We do not reset or free up unused buffers when enabling/disabling XDP,
> so it can happen that xdp_frames are freed after disabling XDP or
> sk_buffs are freed after enabling XDP on xdp tx queues.
> Thus we need to handle both forms (xdp_frames and sk_buffs) regardless
> of XDP setting.
> One way to trigger this problem is to disable XDP when napi_tx is
> enabled. In that case, virtnet_xdp_set() calls virtnet_napi_enable()
> which kicks NAPI. The NAPI handler will call virtnet_poll_cleantx()
> which invokes free_old_xmit_skbs() for queues which have been used by
> XDP.
>
> Note that even with this change we need to keep skipping
> free_old_xmit_skbs() from NAPI handlers when XDP is enabled, because XDP
> tx queues do not aquire queue locks.
>
> Fixes: 4941d472bf95 ("virtio-net: do not reset during XDP set")
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> ---
> drivers/net/virtio_net.c | 54 +++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 42 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 996de69..6598c25 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -57,6 +57,8 @@
> #define VIRTIO_XDP_TX BIT(0)
> #define VIRTIO_XDP_REDIR BIT(1)
>
> +#define VIRTIO_XDP_FLAG BIT(0)
> +
> /* RX packet size EWMA. The average packet size is used to determine the packet
> * buffer size when refilling RX rings. As the entire RX ring may be refilled
> * at once, the weight is chosen so that the EWMA will be insensitive to short-
> @@ -252,6 +254,21 @@ struct padded_vnet_hdr {
> char padding[4];
> };
>
> +static bool is_xdp_frame(void *ptr)
> +{
> + return (unsigned long)ptr & VIRTIO_XDP_FLAG;
> +}
> +
> +static void *xdp_to_ptr(struct xdp_frame *ptr)
> +{
> + return (void *)((unsigned long)ptr | VIRTIO_XDP_FLAG);
> +}
> +
> +static struct xdp_frame *ptr_to_xdp(void *ptr)
> +{
> + return (struct xdp_frame *)((unsigned long)ptr & ~VIRTIO_XDP_FLAG);
> +}
> +
> /* Converting between virtqueue no. and kernel tx/rx queue no.
> * 0:rx0 1:tx0 2:rx1 3:tx1 ... 2N:rxN 2N+1:txN 2N+2:cvq
> */
> @@ -462,7 +479,8 @@ static int __virtnet_xdp_xmit_one(struct virtnet_info *vi,
>
> sg_init_one(sq->sg, xdpf->data, xdpf->len);
>
> - err = virtqueue_add_outbuf(sq->vq, sq->sg, 1, xdpf, GFP_ATOMIC);
> + err = virtqueue_add_outbuf(sq->vq, sq->sg, 1, xdp_to_ptr(xdpf),
> + GFP_ATOMIC);
> if (unlikely(err))
> return -ENOSPC; /* Caller handle free/refcnt */
>
> @@ -482,13 +500,13 @@ static int virtnet_xdp_xmit(struct net_device *dev,
> {
> struct virtnet_info *vi = netdev_priv(dev);
> struct receive_queue *rq = vi->rq;
> - struct xdp_frame *xdpf_sent;
> struct bpf_prog *xdp_prog;
> struct send_queue *sq;
> unsigned int len;
> int drops = 0;
> int kicks = 0;
> int ret, err;
> + void *ptr;
> int i;
>
> /* Only allow ndo_xdp_xmit if XDP is loaded on dev, as this
> @@ -507,8 +525,12 @@ static int virtnet_xdp_xmit(struct net_device *dev,
> }
>
> /* 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);
> + while ((ptr = virtqueue_get_buf(sq->vq, &len)) != NULL) {
> + if (likely(is_xdp_frame(ptr)))
> + xdp_return_frame(ptr_to_xdp(ptr));
> + else
> + dev_consume_skb_any(ptr);
> + }
>
> for (i = 0; i < n; i++) {
> struct xdp_frame *xdpf = frames[i];
> @@ -1329,18 +1351,26 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
>
> static void free_old_xmit_skbs(struct send_queue *sq)
> {
> - struct sk_buff *skb;
> unsigned int len;
> unsigned int packets = 0;
> unsigned int bytes = 0;
> + void *ptr;
>
> - while ((skb = virtqueue_get_buf(sq->vq, &len)) != NULL) {
> - pr_debug("Sent skb %p\n", skb);
> + while ((ptr = virtqueue_get_buf(sq->vq, &len)) != NULL) {
> + if (likely(!is_xdp_frame(ptr))) {
> + struct sk_buff *skb = ptr;
>
> - bytes += skb->len;
> - packets++;
> + pr_debug("Sent skb %p\n", skb);
>
> - dev_consume_skb_any(skb);
> + bytes += skb->len;
> + dev_consume_skb_any(skb);
> + } else {
> + struct xdp_frame *frame = ptr_to_xdp(ptr);
> +
> + bytes += frame->len;
> + xdp_return_frame(frame);
> + }
> + packets++;
> }
>
> /* Avoid overhead when no packets have been processed
> @@ -2665,10 +2695,10 @@ static void free_unused_bufs(struct virtnet_info *vi)
> for (i = 0; i < vi->max_queue_pairs; i++) {
> struct virtqueue *vq = vi->sq[i].vq;
> while ((buf = virtqueue_detach_unused_buf(vq)) != NULL) {
> - if (!is_xdp_raw_buffer_queue(vi, i))
> + if (!is_xdp_frame(buf))
> dev_kfree_skb(buf);
> else
> - xdp_return_frame(buf);
> + xdp_return_frame(ptr_to_xdp(buf));
> }
> }
>
Acked-by: Jason Wang <jasowang@redhat.com>
Thanks!
^ permalink raw reply
* Re: [PATCH net 5/7] virtio_net: Don't process redirected XDP frames when XDP is disabled
From: Jason Wang @ 2019-01-17 13:05 UTC (permalink / raw)
To: Toshiaki Makita, David S. Miller, Michael S. Tsirkin
Cc: netdev, Jesper Dangaard Brouer, virtualization
In-Reply-To: <b6a4ba3f-98f4-ba4d-a318-309780be74ae@redhat.com>
On 2019/1/17 下午8:53, Jason Wang wrote:
>
> On 2019/1/17 下午7:20, Toshiaki Makita wrote:
>> Commit 8dcc5b0ab0ec ("virtio_net: fix ndo_xdp_xmit crash towards dev not
>> ready for XDP") tried to avoid access to unexpected sq while XDP is
>> disabled, but was not complete.
>>
>> There was a small window which causes out of bounds sq access in
>> virtnet_xdp_xmit() while disabling XDP.
>>
>> An example case of
>> - curr_queue_pairs = 6 (2 for SKB and 4 for XDP)
>> - online_cpu_num = xdp_queue_paris = 4
>> when XDP is enabled:
>>
>> CPU 0 CPU 1
>> (Disabling XDP) (Processing redirected XDP frames)
>>
>> virtnet_xdp_xmit()
>> virtnet_xdp_set()
>> _virtnet_set_queues()
>> set curr_queue_pairs (2)
>> check if rq->xdp_prog is not NULL
>> virtnet_xdp_sq(vi)
>> qp = curr_queue_pairs -
>> xdp_queue_pairs +
>> smp_processor_id()
>> = 2 - 4 + 1 = -1
>> sq = &vi->sq[qp] // out of bounds
>> access
>> set xdp_queue_pairs (0)
>> rq->xdp_prog = NULL
>>
>> Basically we should not change curr_queue_pairs and xdp_queue_pairs
>> while someone can read the values. Thus, when disabling XDP, assign NULL
>> to rq->xdp_prog first, and wait for RCU grace period, then change
>> xxx_queue_pairs.
>> Note that we need to keep the current order when enabling XDP though.
>>
>> Fixes: 186b3c998c50 ("virtio-net: support XDP_REDIRECT")
>> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>
>
> I wonder whether or not we could simply do:
>
>
> if (prog) {
Should be !prog
>
> rcu_assign_pointer()
>
> synchronize_net()
>
> }
>
> set queues
>
> if (!prog) {
Should be prog.
Thanks
>
> rcu_assign_pointer()
>
> }
>
> Thanks
>
>
>> ---
>> drivers/net/virtio_net.c | 32 +++++++++++++++++++++++++-------
>> 1 file changed, 25 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 204eedf..ae93f0e 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -2424,14 +2424,16 @@ static int virtnet_xdp_set(struct net_device
>> *dev, struct bpf_prog *prog,
>> }
>> }
>> - err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
>> - if (err)
>> - goto err;
>> - netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
>> - vi->xdp_queue_pairs = xdp_qp;
>> + old_prog = rtnl_dereference(vi->rq[0].xdp_prog);
>> + if (!old_prog && prog) {
>> + err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
>> + if (err)
>> + goto err_new_prog;
>> + netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
>> + vi->xdp_queue_pairs = xdp_qp;
>> + }
>> for (i = 0; i < vi->max_queue_pairs; i++) {
>> - old_prog = rtnl_dereference(vi->rq[i].xdp_prog);
>> rcu_assign_pointer(vi->rq[i].xdp_prog, prog);
>> if (i == 0) {
>> if (!old_prog)
>> @@ -2439,6 +2441,18 @@ static int virtnet_xdp_set(struct net_device
>> *dev, struct bpf_prog *prog,
>> if (!prog)
>> virtnet_restore_guest_offloads(vi);
>> }
>> + }
>> +
>> + if (old_prog && !prog) {
>> + synchronize_net();
>> + err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
>> + if (err)
>> + goto err_old_prog;
>> + netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
>> + vi->xdp_queue_pairs = xdp_qp;
>> + }
>> +
>> + for (i = 0; i < vi->max_queue_pairs; i++) {
>> if (old_prog)
>> bpf_prog_put(old_prog);
>> if (netif_running(dev)) {
>> @@ -2450,7 +2464,11 @@ static int virtnet_xdp_set(struct net_device
>> *dev, struct bpf_prog *prog,
>> return 0;
>> -err:
>> +err_old_prog:
>> + virtnet_clear_guest_offloads(vi);
>> + for (i = 0; i < vi->max_queue_pairs; i++)
>> + rcu_assign_pointer(vi->rq[i].xdp_prog, old_prog);
>> +err_new_prog:
>> if (netif_running(dev)) {
>> for (i = 0; i < vi->max_queue_pairs; i++) {
>> virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH RFC 1/2] virtio-net: bql support
From: Jason Wang @ 2019-01-17 13:09 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: netdev, linux-kernel, virtualization, maxime.coquelin, wexu,
David S. Miller
In-Reply-To: <20190102083202-mutt-send-email-mst@kernel.org>
On 2019/1/2 下午9:54, Michael S. Tsirkin wrote:
> On Wed, Jan 02, 2019 at 11:30:11AM +0800, Jason Wang wrote:
>> On 2018/12/31 上午2:48, Michael S. Tsirkin wrote:
>>> On Thu, Dec 27, 2018 at 06:04:53PM +0800, Jason Wang wrote:
>>>> On 2018/12/26 下午11:22, Michael S. Tsirkin wrote:
>>>>> On Thu, Dec 06, 2018 at 04:17:36PM +0800, Jason Wang wrote:
>>>>>> On 2018/12/6 上午6:54, Michael S. Tsirkin wrote:
>>>>>>> When use_napi is set, let's enable BQLs. Note: some of the issues are
>>>>>>> similar to wifi. It's worth considering whether something similar to
>>>>>>> commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be
>>>>>>> benefitial.
>>>>>> I've played a similar patch several days before. The tricky part is the mode
>>>>>> switching between napi and no napi. We should make sure when the packet is
>>>>>> sent and trakced by BQL, it should be consumed by BQL as well.
>>>>> I just went over the patch again and I don't understand this comment.
>>>>> This patch only enabled BQL with tx napi.
>>>>>
>>>>> Thus there's no mode switching.
>>>>>
>>>>> What did I miss?
>>>> Consider the case:
>>>>
>>>>
>>>> TX NAPI is disabled:
>>>>
>>>> send N packets
>>>>
>>>> turn TX NAPI on:
>>>>
>>>> get tx interrupt
>>>>
>>>> BQL try to consume those packets when lead WARN for dql.
>>>>
>>>>
>>>> Thanks
>>> Can one really switch tx napi on and off? How?
>>> While root can change the napi_tx module parameter, I don't think
>>> that has any effect outside device probe time. What did I miss?
>>>
>>>
>>>
>> We support switch the mode through ethtool recently. See
>>
>> commit 0c465be183c7c57a26446df6ea96d8676b865f92
>> Author: Jason Wang <jasowang@redhat.com>
>> Date: Tue Oct 9 10:06:26 2018 +0800
>>
>> virtio_net: ethtool tx napi configuration
>>
>> Implement ethtool .set_coalesce (-C) and .get_coalesce (-c) handlers.
>> Interrupt moderation is currently not supported, so these accept and
>> display the default settings of 0 usec and 1 frame.
>>
>> Toggle tx napi through setting tx-frames. So as to not interfere
>> with possible future interrupt moderation, value 1 means tx napi while
>> value 0 means not.
>>
>> Only allow the switching when device is down for simplicity.
>>
>> Link: https://patchwork.ozlabs.org/patch/948149/
>> Suggested-by: Jason Wang <jasowang@redhat.com>
>> Signed-off-by: Willem de Bruijn <willemb@google.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> Signed-off-by: David S. Miller <davem@davemloft.net>
>>
>> Thanks
>
> It's disabled when device is up - isn't that enough?
Consider the case:
1) tx napi is disabled
2) send packets
3) tx napi is enabled through ethtool
4) get tx interrupt
5) BQL may start to consume packet that was sent when tx napi is
disabled which will trigger BUG or WARN in dql
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next] virtio_net: bulk free tx skbs
From: Michael S. Tsirkin @ 2019-01-17 13:34 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-kernel, virtualization
In-Reply-To: <20190116.153158.1210819126964530944.davem@davemloft.net>
On Wed, Jan 16, 2019 at 03:31:58PM -0800, David Miller wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
> Date: Mon, 14 Jan 2019 20:34:26 -0500
>
> > Use napi_consume_skb() to get bulk free. Note that napi_consume_skb is
> > safe to call in a non-napi context as long as the napi_budget flag is
> > correct.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >
> > My perf testing setup is down but it works fine on my devel box and
> > should be fairly uncontroversial.
>
> It would be uncontroversial if it compiled.
>
> drivers/net/virtio_net.c: In function ‘free_old_xmit_skbs’:
> drivers/net/virtio_net.c:1346:25: error: ‘use_napi’ undeclared (first use in this function); did you mean ‘used_math’?
> napi_consume_skb(skb, use_napi);
> ^~~~~~~~
> used_math
> drivers/net/virtio_net.c:1346:25: note: each undeclared identifier is reported only once for each function it appears in
Yes my bad it was on top of other patches.
I'm rebasing and will repost.
--
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net 6/7] virtio_net: Use xdp_return_frame to free xdp_frames on destroying vqs
From: Jesper Dangaard Brouer @ 2019-01-17 13:39 UTC (permalink / raw)
To: Jason Wang
Cc: Michael S. Tsirkin, netdev, virtualization, brouer,
David S. Miller
In-Reply-To: <fce18995-52c8-dadd-bab5-4683686e46a8@redhat.com>
On Thu, 17 Jan 2019 20:56:39 +0800
Jason Wang <jasowang@redhat.com> wrote:
> On 2019/1/17 下午7:20, Toshiaki Makita wrote:
> > put_page() can work as a fallback for freeing xdp_frames, but the
> > appropriate way is to use xdp_return_frame().
> >
> > Fixes: cac320c850ef ("virtio_net: convert to use generic xdp_frame and xdp_return_frame API")
> > Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> > ---
> > drivers/net/virtio_net.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index ae93f0e..996de69 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -2668,7 +2668,7 @@ static void free_unused_bufs(struct virtnet_info *vi)
> > if (!is_xdp_raw_buffer_queue(vi, i))
> > dev_kfree_skb(buf);
> > else
> > - put_page(virt_to_head_page(buf));
> > + xdp_return_frame(buf);
I assume the is_xdp_raw_buffer_queue() test assures that this void *buf
is a struct xdp_frame.
> > }
> > }
> >
>
>
> Acked-by: Jason Wang <jasowang@redhat.com>
Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net 0/7] virtio_net: Fix problems around XDP tx and napi_tx
From: Michael S. Tsirkin @ 2019-01-17 14:55 UTC (permalink / raw)
To: Toshiaki Makita
Cc: Willem de Bruijn, netdev, virtualization, Jesper Dangaard Brouer,
David S. Miller
In-Reply-To: <1547724045-2726-1-git-send-email-makita.toshiaki@lab.ntt.co.jp>
On Thu, Jan 17, 2019 at 08:20:38PM +0900, Toshiaki Makita wrote:
> While I'm looking into how to account standard tx counters on XDP tx
> processing, I found several bugs around XDP tx and napi_tx.
>
> Patch1: Fix oops on error path. Patch2 depends on this.
> Patch2: Fix memory corruption on freeing xdp_frames with napi_tx enabled.
> Patch3: Minor fix patch5 depends on.
> Patch4: Fix memory corruption on processing xdp_frames when XDP is disabled.
> Also patch5 depends on this.
> Patch5: Fix memory corruption on processing xdp_frames while XDP is being
> disabled.
> Patch6: Minor fix patch7 depends on.
> Patch7: Fix memory corruption on freeing sk_buff or xdp_frames when a normal
> queue is reused for XDP and vise versa.
>
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
Series:
Acked-by: Michael S. Tsirkin <mst@redhat.com>
I guess we need this stuff on stable?
I'm especially happy with Patch7 as it makes my BQL
work a bit easier.
> Toshiaki Makita (7):
> virtio_net: Don't enable NAPI when interface is down
> virtio_net: Don't call free_old_xmit_skbs for xdp_frames
> virtio_net: Fix not restoring real_num_rx_queues
> virtio_net: Fix out of bounds access of sq
> virtio_net: Don't process redirected XDP frames when XDP is disabled
> virtio_net: Use xdp_return_frame to free xdp_frames on destroying vqs
> virtio_net: Differentiate sk_buff and xdp_frame on freeing
>
> drivers/net/virtio_net.c | 154 +++++++++++++++++++++++++++++++++--------------
> 1 file changed, 109 insertions(+), 45 deletions(-)
>
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH] drm: Split out drm_probe_helper.h
From: Daniel Vetter @ 2019-01-17 16:45 UTC (permalink / raw)
To: Sam Ravnborg
Cc: Neil Armstrong, Daniel Vetter, Liviu Dudau, DRI Development,
virtualization, Laurent Pinchart, Benjamin Gaignard,
Daniel Vetter, linux-stm32, linux-samsung-soc,
Oleksandr Andrushchenko, amd-gfx, linux-rockchip, nouveau, CK Hu,
spice-devel, Jani Nikula, linux-arm-msm, intel-gfx, etnaviv,
Jani Nikula, linux-mediatek
In-Reply-To: <20190116181018.GA27364@ravnborg.org>
On Wed, Jan 16, 2019 at 07:10:18PM +0100, Sam Ravnborg wrote:
> Hi Daniel.
>
> > v5: Actually try to sort them, and while at it, sort all the ones I
> > touch.
>
> Applied this variant on top of drm-misc and did a build test.
> Looked good for ia64, x86 and alpha.
>
> Took a closer look at the changes to atmel_hlcd - and they looked OK.
>
> But I noticed that atmel_hlcdc uses only drm_kms_helper_poll_init() and
> drm_kms_helper_poll_fini().
> But there are no hits on DRM_CONNECTOR_POLL - so I think we maybe
> have a driver here where we have plugged the drm_poll infrastructure,
> but it is not in use.
>
> > include/drm/drm_crtc_helper.h | 16 -----------
>
> The list of include files in this file could be dropped and replaced by:
> struct drm_connector;
> struct drm_device;
> struct drm_display_mode;
> struct drm_encoder;
> struct drm_framebuffer;
> struct drm_mode_set;
> struct drm_modeset_acquire_ctx;
>
> I tried to do so on top of your patch.
> But there were too many build errros and I somehow lost the motivation.
Yeah the drm_crtc_helper.h header is a bit the miniature drmP.h for legacy
kms drivers. Just removing it from all the atomic drivers caused lots of
fallout, I expect even more if you entirely remove the includes it has.
Maybe a todo, care to pls create that patch since it's your idea?
-Daniel
>
>
> > include/drm/drm_probe_helper.h | 27 +++++++++++++++++++
> This on the other hand is fine - as expected as this is a new file.
>
> But the above is just some random comments so:
>
> Acked-by: Sam Ravnborg <sam@ravnborg.org>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply
* Re: [PATCH] drm: Split out drm_probe_helper.h
From: Sam Ravnborg @ 2019-01-17 17:45 UTC (permalink / raw)
To: Daniel Vetter
Cc: Neil Armstrong, Daniel Vetter, Liviu Dudau, DRI Development,
virtualization, Laurent Pinchart, Daniel Vetter, linux-stm32,
linux-samsung-soc, Oleksandr Andrushchenko, amd-gfx,
linux-rockchip, nouveau, spice-devel, Jani Nikula, linux-arm-msm,
intel-gfx, etnaviv, linux-mediatek, Rodrigo Vivi, linux-tegra,
linux-amlogic, linux-arm-kernel
In-Reply-To: <20190117164541.GE3271@phenom.ffwll.local>
On Thu, Jan 17, 2019 at 05:45:41PM +0100, Daniel Vetter wrote:
> On Wed, Jan 16, 2019 at 07:10:18PM +0100, Sam Ravnborg wrote:
> > Hi Daniel.
> >
> > > v5: Actually try to sort them, and while at it, sort all the ones I
> > > touch.
> >
> > Applied this variant on top of drm-misc and did a build test.
> > Looked good for ia64, x86 and alpha.
> >
> > Took a closer look at the changes to atmel_hlcd - and they looked OK.
> >
> > But I noticed that atmel_hlcdc uses only drm_kms_helper_poll_init() and
> > drm_kms_helper_poll_fini().
> > But there are no hits on DRM_CONNECTOR_POLL - so I think we maybe
> > have a driver here where we have plugged the drm_poll infrastructure,
> > but it is not in use.
> >
> > > include/drm/drm_crtc_helper.h | 16 -----------
> >
> > The list of include files in this file could be dropped and replaced by:
> > struct drm_connector;
> > struct drm_device;
> > struct drm_display_mode;
> > struct drm_encoder;
> > struct drm_framebuffer;
> > struct drm_mode_set;
> > struct drm_modeset_acquire_ctx;
> >
> > I tried to do so on top of your patch.
> > But there were too many build errros and I somehow lost the motivation.
>
> Yeah the drm_crtc_helper.h header is a bit the miniature drmP.h for legacy
> kms drivers. Just removing it from all the atomic drivers caused lots of
> fallout, I expect even more if you entirely remove the includes it has.
> Maybe a todo, care to pls create that patch since it's your idea?
The main reason I bailed out initially was that this would create
small changes to several otherwise seldomly touched files.
And then we would later come and remove drmP.h - so lots of
small but incremental changes to the same otherwise seldomly
edited files.
And the job was only partially done.
I will try to experiment with an approach where I clean up the
include/drm/*.h files a little (like suggested above, +delete drmP.h
and maybe a bit more).
Then to try on a driver by driver basis to make it build with a
cleaned set of include files.
I hope that the cleaned up driver can still build without the
cleaned header files so the changes can be submitted piecemal.
Will do so with an eye on the lesser maintained drivers to try it
out to avoid creating too much chrunch for others.
And if it works out I expect the active drivers to follow the
example.
todo.rst item will wait until I run out of energy.
Sam
^ permalink raw reply
* Re: [PATCH net V4] vhost: log dirty page correctly
From: David Miller @ 2019-01-17 19:58 UTC (permalink / raw)
To: jasowang; +Cc: kvm, mst, netdev, linux-kernel, virtualization, jintack
In-Reply-To: <20190116085442.3836-1-jasowang@redhat.com>
From: Jason Wang <jasowang@redhat.com>
Date: Wed, 16 Jan 2019 16:54:42 +0800
> Vhost dirty page logging API is designed to sync through GPA. But we
> try to log GIOVA when device IOTLB is enabled. This is wrong and may
> lead to missing data after migration.
>
> To solve this issue, when logging with device IOTLB enabled, we will:
>
> 1) reuse the device IOTLB translation result of GIOVA->HVA mapping to
> get HVA, for writable descriptor, get HVA through iovec. For used
> ring update, translate its GIOVA to HVA
> 2) traverse the GPA->HVA mapping to get the possible GPA and log
> through GPA. Pay attention this reverse mapping is not guaranteed
> to be unique, so we should log each possible GPA in this case.
>
> This fix the failure of scp to guest during migration. In -next, we
> will probably support passing GIOVA->GPA instead of GIOVA->HVA.
>
> Fixes: 6b1e6cc7855b ("vhost: new device IOTLB API")
> Reported-by: Jintack Lim <jintack@cs.columbia.edu>
> Cc: Jintack Lim <jintack@cs.columbia.edu>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
Michaell, can I get a review for this please?
Thank you.
^ permalink raw reply
* Re: [PATCH net V4] vhost: log dirty page correctly
From: Michael S. Tsirkin @ 2019-01-17 20:42 UTC (permalink / raw)
To: Jason Wang; +Cc: Jintack Lim, netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20190116085442.3836-1-jasowang@redhat.com>
On Wed, Jan 16, 2019 at 04:54:42PM +0800, Jason Wang wrote:
> Vhost dirty page logging API is designed to sync through GPA. But we
> try to log GIOVA when device IOTLB is enabled. This is wrong and may
> lead to missing data after migration.
>
> To solve this issue, when logging with device IOTLB enabled, we will:
>
> 1) reuse the device IOTLB translation result of GIOVA->HVA mapping to
> get HVA, for writable descriptor, get HVA through iovec. For used
> ring update, translate its GIOVA to HVA
> 2) traverse the GPA->HVA mapping to get the possible GPA and log
> through GPA. Pay attention this reverse mapping is not guaranteed
> to be unique, so we should log each possible GPA in this case.
>
> This fix the failure of scp to guest during migration. In -next, we
> will probably support passing GIOVA->GPA instead of GIOVA->HVA.
>
> Fixes: 6b1e6cc7855b ("vhost: new device IOTLB API")
> Reported-by: Jintack Lim <jintack@cs.columbia.edu>
> Cc: Jintack Lim <jintack@cs.columbia.edu>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
This one looks good to me
Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> Changes from V3:
> - make sure each part of the hva was logged when crossing the boundary
> of memory regions
> Changes from V2:
> - check and log the case of range overlap
> - remove unnecessary u64 cast
> - use smp_wmb() for the case of device IOTLB as well
> Changes from V1:
> - return error instead of warn
> ---
> drivers/vhost/net.c | 3 +-
> drivers/vhost/vhost.c | 97 ++++++++++++++++++++++++++++++++++++-------
> drivers/vhost/vhost.h | 3 +-
> 3 files changed, 87 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 36f3d0f49e60..bca86bf7189f 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -1236,7 +1236,8 @@ static void handle_rx(struct vhost_net *net)
> if (nvq->done_idx > VHOST_NET_BATCH)
> vhost_net_signal_used(nvq);
> if (unlikely(vq_log))
> - vhost_log_write(vq, vq_log, log, vhost_len);
> + vhost_log_write(vq, vq_log, log, vhost_len,
> + vq->iov, in);
> total_len += vhost_len;
> if (unlikely(vhost_exceeds_weight(++recv_pkts, total_len))) {
> vhost_poll_queue(&vq->poll);
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 9f7942cbcbb2..babbb32b9bf0 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -1733,13 +1733,87 @@ static int log_write(void __user *log_base,
> return r;
> }
>
> +static int log_write_hva(struct vhost_virtqueue *vq, u64 hva, u64 len)
> +{
> + struct vhost_umem *umem = vq->umem;
> + struct vhost_umem_node *u;
> + u64 start, end, l, min;
> + int r;
> + bool hit = false;
> +
> + while (len) {
> + min = len;
> + /* More than one GPAs can be mapped into a single HVA. So
> + * iterate all possible umems here to be safe.
> + */
> + list_for_each_entry(u, &umem->umem_list, link) {
> + if (u->userspace_addr > hva - 1 + len ||
> + u->userspace_addr - 1 + u->size < hva)
> + continue;
> + start = max(u->userspace_addr, hva);
> + end = min(u->userspace_addr - 1 + u->size,
> + hva - 1 + len);
> + l = end - start + 1;
> + r = log_write(vq->log_base,
> + u->start + start - u->userspace_addr,
> + l);
> + if (r < 0)
> + return r;
> + hit = true;
> + min = min(l, min);
> + }
> +
> + if (!hit)
> + return -EFAULT;
> +
> + len -= min;
> + hva += min;
> + }
> +
> + return 0;
> +}
> +
> +static int log_used(struct vhost_virtqueue *vq, u64 used_offset, u64 len)
> +{
> + struct iovec iov[64];
> + int i, ret;
> +
> + if (!vq->iotlb)
> + return log_write(vq->log_base, vq->log_addr + used_offset, len);
> +
> + ret = translate_desc(vq, (uintptr_t)vq->used + used_offset,
> + len, iov, 64, VHOST_ACCESS_WO);
> + if (ret)
> + return ret;
> +
> + for (i = 0; i < ret; i++) {
> + ret = log_write_hva(vq, (uintptr_t)iov[i].iov_base,
> + iov[i].iov_len);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
> - unsigned int log_num, u64 len)
> + unsigned int log_num, u64 len, struct iovec *iov, int count)
> {
> int i, r;
>
> /* Make sure data written is seen before log. */
> smp_wmb();
> +
> + if (vq->iotlb) {
> + for (i = 0; i < count; i++) {
> + r = log_write_hva(vq, (uintptr_t)iov[i].iov_base,
> + iov[i].iov_len);
> + if (r < 0)
> + return r;
> + }
> + return 0;
> + }
> +
> for (i = 0; i < log_num; ++i) {
> u64 l = min(log[i].len, len);
> r = log_write(vq->log_base, log[i].addr, l);
> @@ -1769,9 +1843,8 @@ static int vhost_update_used_flags(struct vhost_virtqueue *vq)
> smp_wmb();
> /* Log used flag write. */
> used = &vq->used->flags;
> - log_write(vq->log_base, vq->log_addr +
> - (used - (void __user *)vq->used),
> - sizeof vq->used->flags);
> + log_used(vq, (used - (void __user *)vq->used),
> + sizeof vq->used->flags);
> if (vq->log_ctx)
> eventfd_signal(vq->log_ctx, 1);
> }
> @@ -1789,9 +1862,8 @@ static int vhost_update_avail_event(struct vhost_virtqueue *vq, u16 avail_event)
> smp_wmb();
> /* Log avail event write */
> used = vhost_avail_event(vq);
> - log_write(vq->log_base, vq->log_addr +
> - (used - (void __user *)vq->used),
> - sizeof *vhost_avail_event(vq));
> + log_used(vq, (used - (void __user *)vq->used),
> + sizeof *vhost_avail_event(vq));
> if (vq->log_ctx)
> eventfd_signal(vq->log_ctx, 1);
> }
> @@ -2191,10 +2263,8 @@ static int __vhost_add_used_n(struct vhost_virtqueue *vq,
> /* Make sure data is seen before log. */
> smp_wmb();
> /* Log used ring entry write. */
> - log_write(vq->log_base,
> - vq->log_addr +
> - ((void __user *)used - (void __user *)vq->used),
> - count * sizeof *used);
> + log_used(vq, ((void __user *)used - (void __user *)vq->used),
> + count * sizeof *used);
> }
> old = vq->last_used_idx;
> new = (vq->last_used_idx += count);
> @@ -2236,9 +2306,8 @@ int vhost_add_used_n(struct vhost_virtqueue *vq, struct vring_used_elem *heads,
> /* Make sure used idx is seen before log. */
> smp_wmb();
> /* Log used index update. */
> - log_write(vq->log_base,
> - vq->log_addr + offsetof(struct vring_used, idx),
> - sizeof vq->used->idx);
> + log_used(vq, offsetof(struct vring_used, idx),
> + sizeof vq->used->idx);
> if (vq->log_ctx)
> eventfd_signal(vq->log_ctx, 1);
> }
> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index 466ef7542291..1b675dad5e05 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -205,7 +205,8 @@ bool vhost_vq_avail_empty(struct vhost_dev *, struct vhost_virtqueue *);
> bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *);
>
> int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
> - unsigned int log_num, u64 len);
> + unsigned int log_num, u64 len,
> + struct iovec *iov, int count);
> int vq_iotlb_prefetch(struct vhost_virtqueue *vq);
>
> struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type);
> --
> 2.17.1
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox