* Re: [PATCH net-next RFC 3/5] vhost: introduce vhost_add_used_idx()
From: Stefan Hajnoczi @ 2017-09-22 9:07 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, virtualization, linux-kernel, kvm, mst
In-Reply-To: <1506067355-5771-4-git-send-email-jasowang@redhat.com>
On Fri, Sep 22, 2017 at 04:02:33PM +0800, Jason Wang wrote:
> This patch introduces a helper which just increase the used idx. This
> will be used in pair with vhost_prefetch_desc_indices() by batching
> code.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> drivers/vhost/vhost.c | 33 +++++++++++++++++++++++++++++++++
> drivers/vhost/vhost.h | 1 +
> 2 files changed, 34 insertions(+)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
^ permalink raw reply
* Re: [PATCH net-next RFC 2/5] vhost: introduce helper to prefetch desc index
From: Stefan Hajnoczi @ 2017-09-22 9:02 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, virtualization, linux-kernel, kvm, mst
In-Reply-To: <1506067355-5771-3-git-send-email-jasowang@redhat.com>
On Fri, Sep 22, 2017 at 04:02:32PM +0800, Jason Wang wrote:
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index f87ec75..8424166d 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -2437,6 +2437,61 @@ struct vhost_msg_node *vhost_dequeue_msg(struct vhost_dev *dev,
> }
> EXPORT_SYMBOL_GPL(vhost_dequeue_msg);
>
> +int vhost_prefetch_desc_indices(struct vhost_virtqueue *vq,
> + struct vring_used_elem *heads,
> + u16 num, bool used_update)
Missing doc comment.
> +{
> + int ret, ret2;
> + u16 last_avail_idx, last_used_idx, total, copied;
> + __virtio16 avail_idx;
> + struct vring_used_elem __user *used;
> + int i;
The following variable names are a little confusing:
last_avail_idx vs vq->last_avail_idx. last_avail_idx is a wrapped
avail->ring[] index, vq->last_avail_idx is a free-running counter. The
same for last_used_idx vs vq->last_used_idx.
num argument vs vq->num. The argument could be called nheads instead to
make it clear that this is heads[] and not the virtqueue size.
Not a bug but it took me a while to figure out what was going on.
^ permalink raw reply
* Re: [PATCH net-next RFC 1/5] vhost: split out ring head fetching logic
From: Stefan Hajnoczi @ 2017-09-22 8:31 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, virtualization, linux-kernel, kvm, mst
In-Reply-To: <1506067355-5771-2-git-send-email-jasowang@redhat.com>
On Fri, Sep 22, 2017 at 04:02:31PM +0800, Jason Wang wrote:
> +/* This looks in the virtqueue and for the first available buffer, and converts
> + * it to an iovec for convenient access. Since descriptors consist of some
> + * number of output then some number of input descriptors, it's actually two
> + * iovecs, but we pack them into one and note how many of each there were.
> + *
> + * This function returns the descriptor number found, or vq->num (which is
> + * never a valid descriptor number) if none was found. A negative code is
> + * returned on error. */
> +int __vhost_get_vq_desc(struct vhost_virtqueue *vq,
> + struct iovec iov[], unsigned int iov_size,
> + unsigned int *out_num, unsigned int *in_num,
> + struct vhost_log *log, unsigned int *log_num,
> + __virtio16 head)
[...]
> +int vhost_get_vq_desc(struct vhost_virtqueue *vq,
> + struct iovec iov[], unsigned int iov_size,
> + unsigned int *out_num, unsigned int *in_num,
> + struct vhost_log *log, unsigned int *log_num)
Please document vhost_get_vq_desc().
Please also explain the difference between __vhost_get_vq_desc() and
vhost_get_vq_desc() in the documentation.
^ permalink raw reply
* [PATCH net-next RFC 5/5] vhost_net: basic tx virtqueue batched processing
From: Jason Wang @ 2017-09-22 8:02 UTC (permalink / raw)
To: mst, jasowang, virtualization, netdev, linux-kernel; +Cc: kvm
In-Reply-To: <1506067355-5771-1-git-send-email-jasowang@redhat.com>
This patch implements basic batched processing of tx virtqueue by
prefetching desc indices and updating used ring in a batch. For
non-zerocopy case, vq->heads were used for storing the prefetched
indices and updating used ring. It is also a requirement for doing
more batching on top. For zerocopy case and for simplicity, batched
processing were simply disabled by only fetching and processing one
descriptor at a time, this could be optimized in the future.
XDP_DROP (without touching skb) on tun (with Moongen in guest) with
zercopy disabled:
Intel(R) Xeon(R) CPU E5-2650 0 @ 2.00GHz:
Before: 3.20Mpps
After: 3.90Mpps (+22%)
No differences were seen with zerocopy enabled.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/net.c | 215 ++++++++++++++++++++++++++++----------------------
drivers/vhost/vhost.c | 2 +-
2 files changed, 121 insertions(+), 96 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index c89640e..c439892 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -408,27 +408,25 @@ static int vhost_net_enable_vq(struct vhost_net *n,
return vhost_poll_start(poll, sock->file);
}
-static int vhost_net_tx_get_vq_desc(struct vhost_net *net,
- struct vhost_virtqueue *vq,
- struct iovec iov[], unsigned int iov_size,
- unsigned int *out_num, unsigned int *in_num)
+static bool vhost_net_tx_avail(struct vhost_net *net,
+ struct vhost_virtqueue *vq)
{
unsigned long uninitialized_var(endtime);
- int r = vhost_get_vq_desc(vq, vq->iov, ARRAY_SIZE(vq->iov),
- out_num, in_num, NULL, NULL);
- if (r == vq->num && vq->busyloop_timeout) {
- preempt_disable();
- endtime = busy_clock() + vq->busyloop_timeout;
- while (vhost_can_busy_poll(vq->dev, endtime) &&
- vhost_vq_avail_empty(vq->dev, vq))
- cpu_relax();
- preempt_enable();
- r = vhost_get_vq_desc(vq, vq->iov, ARRAY_SIZE(vq->iov),
- out_num, in_num, NULL, NULL);
- }
+ if (!vq->busyloop_timeout)
+ return false;
- return r;
+ if (!vhost_vq_avail_empty(vq->dev, vq))
+ return true;
+
+ preempt_disable();
+ endtime = busy_clock() + vq->busyloop_timeout;
+ while (vhost_can_busy_poll(vq->dev, endtime) &&
+ vhost_vq_avail_empty(vq->dev, vq))
+ cpu_relax();
+ preempt_enable();
+
+ return !vhost_vq_avail_empty(vq->dev, vq);
}
static bool vhost_exceeds_maxpend(struct vhost_net *net)
@@ -446,8 +444,9 @@ static void handle_tx(struct vhost_net *net)
{
struct vhost_net_virtqueue *nvq = &net->vqs[VHOST_NET_VQ_TX];
struct vhost_virtqueue *vq = &nvq->vq;
+ struct vring_used_elem used, *heads = vq->heads;
unsigned out, in;
- int head;
+ int avails, head;
struct msghdr msg = {
.msg_name = NULL,
.msg_namelen = 0,
@@ -461,6 +460,7 @@ static void handle_tx(struct vhost_net *net)
struct socket *sock;
struct vhost_net_ubuf_ref *uninitialized_var(ubufs);
bool zcopy, zcopy_used;
+ int i, batched = VHOST_NET_BATCH;
mutex_lock(&vq->mutex);
sock = vq->private_data;
@@ -475,6 +475,12 @@ static void handle_tx(struct vhost_net *net)
hdr_size = nvq->vhost_hlen;
zcopy = nvq->ubufs;
+ /* Disable zerocopy batched fetching for simplicity */
+ if (zcopy) {
+ heads = &used;
+ batched = 1;
+ }
+
for (;;) {
/* Release DMAs done buffers first */
if (zcopy)
@@ -486,95 +492,114 @@ static void handle_tx(struct vhost_net *net)
if (unlikely(vhost_exceeds_maxpend(net)))
break;
- head = vhost_net_tx_get_vq_desc(net, vq, vq->iov,
- ARRAY_SIZE(vq->iov),
- &out, &in);
+ avails = vhost_prefetch_desc_indices(vq, heads, batched, !zcopy);
/* On error, stop handling until the next kick. */
- if (unlikely(head < 0))
+ if (unlikely(avails < 0))
break;
- /* Nothing new? Wait for eventfd to tell us they refilled. */
- if (head == vq->num) {
+ /* Nothing new? Busy poll for a while or wait for
+ * eventfd to tell us they refilled. */
+ if (!avails) {
+ if (vhost_net_tx_avail(net, vq))
+ continue;
if (unlikely(vhost_enable_notify(&net->dev, vq))) {
vhost_disable_notify(&net->dev, vq);
continue;
}
break;
}
- if (in) {
- vq_err(vq, "Unexpected descriptor format for TX: "
- "out %d, int %d\n", out, in);
- break;
- }
- /* Skip header. TODO: support TSO. */
- len = iov_length(vq->iov, out);
- iov_iter_init(&msg.msg_iter, WRITE, vq->iov, out, len);
- iov_iter_advance(&msg.msg_iter, hdr_size);
- /* Sanity check */
- if (!msg_data_left(&msg)) {
- vq_err(vq, "Unexpected header len for TX: "
- "%zd expected %zd\n",
- len, hdr_size);
- break;
- }
- len = msg_data_left(&msg);
-
- zcopy_used = zcopy && len >= VHOST_GOODCOPY_LEN
- && (nvq->upend_idx + 1) % UIO_MAXIOV !=
- nvq->done_idx
- && vhost_net_tx_select_zcopy(net);
-
- /* use msg_control to pass vhost zerocopy ubuf info to skb */
- if (zcopy_used) {
- struct ubuf_info *ubuf;
- ubuf = nvq->ubuf_info + nvq->upend_idx;
-
- vq->heads[nvq->upend_idx].id = cpu_to_vhost32(vq, head);
- vq->heads[nvq->upend_idx].len = VHOST_DMA_IN_PROGRESS;
- ubuf->callback = vhost_zerocopy_callback;
- ubuf->ctx = nvq->ubufs;
- ubuf->desc = nvq->upend_idx;
- refcount_set(&ubuf->refcnt, 1);
- msg.msg_control = ubuf;
- msg.msg_controllen = sizeof(ubuf);
- ubufs = nvq->ubufs;
- atomic_inc(&ubufs->refcount);
- nvq->upend_idx = (nvq->upend_idx + 1) % UIO_MAXIOV;
- } else {
- msg.msg_control = NULL;
- ubufs = NULL;
- }
+ for (i = 0; i < avails; i++) {
+ head = __vhost_get_vq_desc(vq, vq->iov,
+ ARRAY_SIZE(vq->iov),
+ &out, &in, NULL, NULL,
+ vhost16_to_cpu(vq, heads[i].id));
+ if (in) {
+ vq_err(vq, "Unexpected descriptor format for "
+ "TX: out %d, int %d\n", out, in);
+ goto out;
+ }
- total_len += len;
- if (total_len < VHOST_NET_WEIGHT &&
- !vhost_vq_avail_empty(&net->dev, vq) &&
- likely(!vhost_exceeds_maxpend(net))) {
- msg.msg_flags |= MSG_MORE;
- } else {
- msg.msg_flags &= ~MSG_MORE;
- }
+ /* Skip header. TODO: support TSO. */
+ len = iov_length(vq->iov, out);
+ iov_iter_init(&msg.msg_iter, WRITE, vq->iov, out, len);
+ iov_iter_advance(&msg.msg_iter, hdr_size);
+ /* Sanity check */
+ if (!msg_data_left(&msg)) {
+ vq_err(vq, "Unexpected header len for TX: "
+ "%zd expected %zd\n",
+ len, hdr_size);
+ goto out;
+ }
+ len = msg_data_left(&msg);
- /* TODO: Check specific error and bomb out unless ENOBUFS? */
- err = sock->ops->sendmsg(sock, &msg, len);
- if (unlikely(err < 0)) {
+ zcopy_used = zcopy && len >= VHOST_GOODCOPY_LEN
+ && (nvq->upend_idx + 1) % UIO_MAXIOV !=
+ nvq->done_idx
+ && vhost_net_tx_select_zcopy(net);
+
+ /* use msg_control to pass vhost zerocopy ubuf
+ * info to skb
+ */
if (zcopy_used) {
- vhost_net_ubuf_put(ubufs);
- nvq->upend_idx = ((unsigned)nvq->upend_idx - 1)
- % UIO_MAXIOV;
+ struct ubuf_info *ubuf;
+ ubuf = nvq->ubuf_info + nvq->upend_idx;
+
+ vq->heads[nvq->upend_idx].id =
+ cpu_to_vhost32(vq, head);
+ vq->heads[nvq->upend_idx].len =
+ VHOST_DMA_IN_PROGRESS;
+ ubuf->callback = vhost_zerocopy_callback;
+ ubuf->ctx = nvq->ubufs;
+ ubuf->desc = nvq->upend_idx;
+ refcount_set(&ubuf->refcnt, 1);
+ msg.msg_control = ubuf;
+ msg.msg_controllen = sizeof(ubuf);
+ ubufs = nvq->ubufs;
+ atomic_inc(&ubufs->refcount);
+ nvq->upend_idx =
+ (nvq->upend_idx + 1) % UIO_MAXIOV;
+ } else {
+ msg.msg_control = NULL;
+ ubufs = NULL;
+ }
+
+ total_len += len;
+ if (total_len < VHOST_NET_WEIGHT &&
+ !vhost_vq_avail_empty(&net->dev, vq) &&
+ likely(!vhost_exceeds_maxpend(net))) {
+ msg.msg_flags |= MSG_MORE;
+ } else {
+ msg.msg_flags &= ~MSG_MORE;
+ }
+
+ /* TODO: Check specific error and bomb out
+ * unless ENOBUFS?
+ */
+ err = sock->ops->sendmsg(sock, &msg, len);
+ if (unlikely(err < 0)) {
+ if (zcopy_used) {
+ vhost_net_ubuf_put(ubufs);
+ nvq->upend_idx =
+ ((unsigned)nvq->upend_idx - 1) % UIO_MAXIOV;
+ }
+ vhost_discard_vq_desc(vq, 1);
+ goto out;
+ }
+ if (err != len)
+ pr_debug("Truncated TX packet: "
+ " len %d != %zd\n", err, len);
+ if (!zcopy) {
+ vhost_add_used_idx(vq, 1);
+ vhost_signal(&net->dev, vq);
+ } else if (!zcopy_used) {
+ vhost_add_used_and_signal(&net->dev,
+ vq, head, 0);
+ } else
+ vhost_zerocopy_signal_used(net, vq);
+ vhost_net_tx_packet(net);
+ if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
+ vhost_poll_queue(&vq->poll);
+ goto out;
}
- vhost_discard_vq_desc(vq, 1);
- break;
- }
- if (err != len)
- pr_debug("Truncated TX packet: "
- " len %d != %zd\n", err, len);
- if (!zcopy_used)
- vhost_add_used_and_signal(&net->dev, vq, head, 0);
- else
- vhost_zerocopy_signal_used(net, vq);
- vhost_net_tx_packet(net);
- if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
- vhost_poll_queue(&vq->poll);
- break;
}
}
out:
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 6532cda..8764df5 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -392,7 +392,7 @@ static long vhost_dev_alloc_iovecs(struct vhost_dev *dev)
vq->indirect = kmalloc(sizeof *vq->indirect * UIO_MAXIOV,
GFP_KERNEL);
vq->log = kmalloc(sizeof *vq->log * UIO_MAXIOV, GFP_KERNEL);
- vq->heads = kmalloc(sizeof *vq->heads * UIO_MAXIOV, GFP_KERNEL);
+ vq->heads = kzalloc(sizeof *vq->heads * UIO_MAXIOV, GFP_KERNEL);
if (!vq->indirect || !vq->log || !vq->heads)
goto err_nomem;
}
--
2.7.4
^ permalink raw reply related
* [PATCH net-next RFC 4/5] vhost_net: rename VHOST_RX_BATCH to VHOST_NET_BATCH
From: Jason Wang @ 2017-09-22 8:02 UTC (permalink / raw)
To: mst, jasowang, virtualization, netdev, linux-kernel; +Cc: kvm
In-Reply-To: <1506067355-5771-1-git-send-email-jasowang@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/net.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 58585ec..c89640e 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -87,7 +87,7 @@ struct vhost_net_ubuf_ref {
struct vhost_virtqueue *vq;
};
-#define VHOST_RX_BATCH 64
+#define VHOST_NET_BATCH 64
struct vhost_net_buf {
struct sk_buff **queue;
int tail;
@@ -159,7 +159,7 @@ static int vhost_net_buf_produce(struct vhost_net_virtqueue *nvq)
rxq->head = 0;
rxq->tail = skb_array_consume_batched(nvq->rx_array, rxq->queue,
- VHOST_RX_BATCH);
+ VHOST_NET_BATCH);
return rxq->tail;
}
@@ -912,7 +912,7 @@ static int vhost_net_open(struct inode *inode, struct file *f)
return -ENOMEM;
}
- queue = kmalloc_array(VHOST_RX_BATCH, sizeof(struct sk_buff *),
+ queue = kmalloc_array(VHOST_NET_BATCH, sizeof(struct sk_buff *),
GFP_KERNEL);
if (!queue) {
kfree(vqs);
--
2.7.4
^ permalink raw reply related
* [PATCH net-next RFC 3/5] vhost: introduce vhost_add_used_idx()
From: Jason Wang @ 2017-09-22 8:02 UTC (permalink / raw)
To: mst, jasowang, virtualization, netdev, linux-kernel; +Cc: kvm
In-Reply-To: <1506067355-5771-1-git-send-email-jasowang@redhat.com>
This patch introduces a helper which just increase the used idx. This
will be used in pair with vhost_prefetch_desc_indices() by batching
code.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/vhost.c | 33 +++++++++++++++++++++++++++++++++
drivers/vhost/vhost.h | 1 +
2 files changed, 34 insertions(+)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 8424166d..6532cda 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -2178,6 +2178,39 @@ int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head, int len)
}
EXPORT_SYMBOL_GPL(vhost_add_used);
+int vhost_add_used_idx(struct vhost_virtqueue *vq, int n)
+{
+ u16 old, new;
+
+ old = vq->last_used_idx;
+ new = (vq->last_used_idx += n);
+ /* If the driver never bothers to signal in a very long while,
+ * used index might wrap around. If that happens, invalidate
+ * signalled_used index we stored. TODO: make sure driver
+ * signals at least once in 2^16 and remove this.
+ */
+ if (unlikely((u16)(new - vq->signalled_used) < (u16)(new - old)))
+ vq->signalled_used_valid = false;
+
+ /* Make sure buffer is written before we update index. */
+ smp_wmb();
+ if (vhost_put_user(vq, cpu_to_vhost16(vq, vq->last_used_idx),
+ &vq->used->idx)) {
+ vq_err(vq, "Failed to increment used idx");
+ return -EFAULT;
+ }
+ if (unlikely(vq->log_used)) {
+ /* Log used index update. */
+ log_write(vq->log_base,
+ vq->log_addr + offsetof(struct vring_used, idx),
+ sizeof(vq->used->idx));
+ if (vq->log_ctx)
+ eventfd_signal(vq->log_ctx, 1);
+ }
+ return 0;
+}
+EXPORT_SYMBOL_GPL(vhost_add_used_idx);
+
static int __vhost_add_used_n(struct vhost_virtqueue *vq,
struct vring_used_elem *heads,
unsigned count)
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 16c2cb6..5dd6c05 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -199,6 +199,7 @@ int __vhost_get_vq_desc(struct vhost_virtqueue *vq,
void vhost_discard_vq_desc(struct vhost_virtqueue *, int n);
int vhost_vq_init_access(struct vhost_virtqueue *);
+int vhost_add_used_idx(struct vhost_virtqueue *vq, int n);
int vhost_add_used(struct vhost_virtqueue *, unsigned int head, int len);
int vhost_add_used_n(struct vhost_virtqueue *, struct vring_used_elem *heads,
unsigned count);
--
2.7.4
^ permalink raw reply related
* [PATCH net-next RFC 2/5] vhost: introduce helper to prefetch desc index
From: Jason Wang @ 2017-09-22 8:02 UTC (permalink / raw)
To: mst, jasowang, virtualization, netdev, linux-kernel; +Cc: kvm
In-Reply-To: <1506067355-5771-1-git-send-email-jasowang@redhat.com>
This patch introduces vhost_prefetch_desc_indices() which could batch
descriptor indices fetching and used ring updating. This intends to
reduce the cache misses of indices fetching and updating and reduce
cache line bounce when virtqueue is almost full. copy_to_user() was
used in order to benefit from modern cpus that support fast string
copy. Batched virtqueue processing will be the first user.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/vhost.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++
drivers/vhost/vhost.h | 3 +++
2 files changed, 58 insertions(+)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index f87ec75..8424166d 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -2437,6 +2437,61 @@ struct vhost_msg_node *vhost_dequeue_msg(struct vhost_dev *dev,
}
EXPORT_SYMBOL_GPL(vhost_dequeue_msg);
+int vhost_prefetch_desc_indices(struct vhost_virtqueue *vq,
+ struct vring_used_elem *heads,
+ u16 num, bool used_update)
+{
+ int ret, ret2;
+ u16 last_avail_idx, last_used_idx, total, copied;
+ __virtio16 avail_idx;
+ struct vring_used_elem __user *used;
+ int i;
+
+ if (unlikely(vhost_get_avail(vq, avail_idx, &vq->avail->idx))) {
+ vq_err(vq, "Failed to access avail idx at %p\n",
+ &vq->avail->idx);
+ return -EFAULT;
+ }
+ last_avail_idx = vq->last_avail_idx & (vq->num - 1);
+ vq->avail_idx = vhost16_to_cpu(vq, avail_idx);
+ total = vq->avail_idx - vq->last_avail_idx;
+ ret = total = min(total, num);
+
+ for (i = 0; i < ret; i++) {
+ ret2 = vhost_get_avail(vq, heads[i].id,
+ &vq->avail->ring[last_avail_idx]);
+ if (unlikely(ret2)) {
+ vq_err(vq, "Failed to get descriptors\n");
+ return -EFAULT;
+ }
+ last_avail_idx = (last_avail_idx + 1) & (vq->num - 1);
+ }
+
+ if (!used_update)
+ return ret;
+
+ last_used_idx = vq->last_used_idx & (vq->num - 1);
+ while (total) {
+ copied = min((u16)(vq->num - last_used_idx), total);
+ ret2 = vhost_copy_to_user(vq,
+ &vq->used->ring[last_used_idx],
+ &heads[ret - total],
+ copied * sizeof(*used));
+
+ if (unlikely(ret2)) {
+ vq_err(vq, "Failed to update used ring!\n");
+ return -EFAULT;
+ }
+
+ last_used_idx = 0;
+ total -= copied;
+ }
+
+ /* Only get avail ring entries after they have been exposed by guest. */
+ smp_rmb();
+ return ret;
+}
+EXPORT_SYMBOL(vhost_prefetch_desc_indices);
static int __init vhost_init(void)
{
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 39ff897..16c2cb6 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -228,6 +228,9 @@ ssize_t vhost_chr_read_iter(struct vhost_dev *dev, struct iov_iter *to,
ssize_t vhost_chr_write_iter(struct vhost_dev *dev,
struct iov_iter *from);
int vhost_init_device_iotlb(struct vhost_dev *d, bool enabled);
+int vhost_prefetch_desc_indices(struct vhost_virtqueue *vq,
+ struct vring_used_elem *heads,
+ u16 num, bool used_update);
#define vq_err(vq, fmt, ...) do { \
pr_debug(pr_fmt(fmt), ##__VA_ARGS__); \
--
2.7.4
^ permalink raw reply related
* [PATCH net-next RFC 1/5] vhost: split out ring head fetching logic
From: Jason Wang @ 2017-09-22 8:02 UTC (permalink / raw)
To: mst, jasowang, virtualization, netdev, linux-kernel; +Cc: kvm
In-Reply-To: <1506067355-5771-1-git-send-email-jasowang@redhat.com>
This patch splits out ring head fetching logic and leave the
descriptor fetching and translation logic. This makes it is possible
to batch fetching the descriptor indices.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/vhost.c | 75 +++++++++++++++++++++++++++++++++------------------
drivers/vhost/vhost.h | 5 ++++
2 files changed, 54 insertions(+), 26 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index d6dbb28..f87ec75 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1984,41 +1984,23 @@ static int get_indirect(struct vhost_virtqueue *vq,
return 0;
}
-/* This looks in the virtqueue and for the first available buffer, and converts
- * it to an iovec for convenient access. Since descriptors consist of some
- * number of output then some number of input descriptors, it's actually two
- * iovecs, but we pack them into one and note how many of each there were.
- *
- * This function returns the descriptor number found, or vq->num (which is
- * never a valid descriptor number) if none was found. A negative code is
- * returned on error. */
-int vhost_get_vq_desc(struct vhost_virtqueue *vq,
- struct iovec iov[], unsigned int iov_size,
- unsigned int *out_num, unsigned int *in_num,
- struct vhost_log *log, unsigned int *log_num)
+static unsigned int vhost_get_vq_head(struct vhost_virtqueue *vq, int *err)
{
- struct vring_desc desc;
- unsigned int i, head, found = 0;
- u16 last_avail_idx;
- __virtio16 avail_idx;
- __virtio16 ring_head;
- int ret, access;
-
- /* Check it isn't doing very strange things with descriptor numbers. */
- last_avail_idx = vq->last_avail_idx;
+ u16 last_avail_idx = vq->last_avail_idx;
+ __virtio16 avail_idx, ring_head;
if (vq->avail_idx == vq->last_avail_idx) {
if (unlikely(vhost_get_avail(vq, avail_idx, &vq->avail->idx))) {
vq_err(vq, "Failed to access avail idx at %p\n",
&vq->avail->idx);
- return -EFAULT;
+ goto err;
}
vq->avail_idx = vhost16_to_cpu(vq, avail_idx);
if (unlikely((u16)(vq->avail_idx - last_avail_idx) > vq->num)) {
vq_err(vq, "Guest moved used index from %u to %u",
last_avail_idx, vq->avail_idx);
- return -EFAULT;
+ goto err;
}
/* If there's nothing new since last we looked, return
@@ -2040,13 +2022,35 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
vq_err(vq, "Failed to read head: idx %d address %p\n",
last_avail_idx,
&vq->avail->ring[last_avail_idx % vq->num]);
- return -EFAULT;
+ goto err;
}
- head = vhost16_to_cpu(vq, ring_head);
+ return vhost16_to_cpu(vq, ring_head);
+err:
+ *err = -EFAULT;
+ return 0;
+}
+
+/* This looks in the virtqueue and for the first available buffer, and converts
+ * it to an iovec for convenient access. Since descriptors consist of some
+ * number of output then some number of input descriptors, it's actually two
+ * iovecs, but we pack them into one and note how many of each there were.
+ *
+ * This function returns the descriptor number found, or vq->num (which is
+ * never a valid descriptor number) if none was found. A negative code is
+ * returned on error. */
+int __vhost_get_vq_desc(struct vhost_virtqueue *vq,
+ struct iovec iov[], unsigned int iov_size,
+ unsigned int *out_num, unsigned int *in_num,
+ struct vhost_log *log, unsigned int *log_num,
+ __virtio16 head)
+{
+ struct vring_desc desc;
+ unsigned int i, found = 0;
+ int ret = 0, access;
/* If their number is silly, that's an error. */
- if (unlikely(head >= vq->num)) {
+ if (unlikely(head > vq->num)) {
vq_err(vq, "Guest says index %u > %u is available",
head, vq->num);
return -EINVAL;
@@ -2133,6 +2137,25 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
BUG_ON(!(vq->used_flags & VRING_USED_F_NO_NOTIFY));
return head;
}
+EXPORT_SYMBOL_GPL(__vhost_get_vq_desc);
+
+int vhost_get_vq_desc(struct vhost_virtqueue *vq,
+ struct iovec iov[], unsigned int iov_size,
+ unsigned int *out_num, unsigned int *in_num,
+ struct vhost_log *log, unsigned int *log_num)
+{
+ int ret = 0;
+ unsigned int head = vhost_get_vq_head(vq, &ret);
+
+ if (ret)
+ return ret;
+
+ if (head == vq->num)
+ return head;
+
+ return __vhost_get_vq_desc(vq, iov, iov_size, out_num, in_num,
+ log, log_num, head);
+}
EXPORT_SYMBOL_GPL(vhost_get_vq_desc);
/* Reverse the effect of vhost_get_vq_desc. Useful for error handling. */
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index d59a9cc..39ff897 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -191,6 +191,11 @@ int vhost_get_vq_desc(struct vhost_virtqueue *,
struct iovec iov[], unsigned int iov_count,
unsigned int *out_num, unsigned int *in_num,
struct vhost_log *log, unsigned int *log_num);
+int __vhost_get_vq_desc(struct vhost_virtqueue *vq,
+ struct iovec iov[], unsigned int iov_count,
+ unsigned int *out_num, unsigned int *in_num,
+ struct vhost_log *log, unsigned int *log_num,
+ __virtio16 ring_head);
void vhost_discard_vq_desc(struct vhost_virtqueue *, int n);
int vhost_vq_init_access(struct vhost_virtqueue *);
--
2.7.4
^ permalink raw reply related
* [PATCH net-next RFC 0/5] batched tx processing in vhost_net
From: Jason Wang @ 2017-09-22 8:02 UTC (permalink / raw)
To: mst, jasowang, virtualization, netdev, linux-kernel; +Cc: kvm
Hi:
This series tries to implement basic tx batched processing. This is
done by prefetching descriptor indices and update used ring in a
batch. This intends to speed up used ring updating and improve the
cache utilization. Test shows about ~22% improvement in tx pss.
Please review.
Jason Wang (5):
vhost: split out ring head fetching logic
vhost: introduce helper to prefetch desc index
vhost: introduce vhost_add_used_idx()
vhost_net: rename VHOST_RX_BATCH to VHOST_NET_BATCH
vhost_net: basic tx virtqueue batched processing
drivers/vhost/net.c | 221 ++++++++++++++++++++++++++++----------------------
drivers/vhost/vhost.c | 165 +++++++++++++++++++++++++++++++------
drivers/vhost/vhost.h | 9 ++
3 files changed, 270 insertions(+), 125 deletions(-)
--
2.7.4
^ permalink raw reply
* [PATCH net-next] virtio-net: correctly set xdp_xmit for mergeable buffer
From: Jason Wang @ 2017-09-22 6:38 UTC (permalink / raw)
To: mst, jasowang, virtualization, netdev, linux-kernel; +Cc: John Fastabend
We should set xdp_xmit only when xdp_do_redirect() succeed.
Cc: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
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 f6c1f13..dd14a45 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -721,7 +721,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
goto xdp_xmit;
case XDP_REDIRECT:
err = xdp_do_redirect(dev, &xdp, xdp_prog);
- if (err)
+ if (!err)
*xdp_xmit = true;
rcu_read_unlock();
goto xdp_xmit;
--
2.7.4
^ permalink raw reply related
* Re: [PATCH net-next 3/3] virtio-net: support XDP_REDIRECT
From: Jason Wang @ 2017-09-22 4:06 UTC (permalink / raw)
To: John Fastabend, mst, virtualization, netdev, linux-kernel
In-Reply-To: <59C2E588.6060203@gmail.com>
On 2017年09月21日 06:02, John Fastabend wrote:
> On 09/19/2017 02:42 AM, Jason Wang wrote:
>> This patch tries to add XDP_REDIRECT for virtio-net. The changes are
>> not complex as we could use exist XDP_TX helpers for most of the
>> work. The rest is passing the XDP_TX to NAPI handler for implementing
>> batching.
>>
>> Cc: John Fastabend <john.fastabend@gmail.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
> [...]
>
>> @@ -678,12 +711,20 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> }
>> break;
>> case XDP_TX:
>> - if (unlikely(!virtnet_xdp_xmit(vi, &xdp)))
>> + if (unlikely(!__virtnet_xdp_xmit(vi, &xdp)))
>> trace_xdp_exception(vi->dev, xdp_prog, act);
>> + else
>> + *xdp_xmit = true;
>> if (unlikely(xdp_page != page))
>> goto err_xdp;
>> rcu_read_unlock();
>> goto xdp_xmit;
>> + case XDP_REDIRECT:
>> + err = xdp_do_redirect(dev, &xdp, xdp_prog);
>> + if (err)
>> + *xdp_xmit = true;
> Is this a typo? Should above be
>
> if (!err)
> *xdp_xmit = true;
>
> this would match the pattern in receive_small and also make more sense.
Right, will post a fix.
>
>> + rcu_read_unlock();
>> + goto xdp_xmit;
>> default:
>> bpf_warn_invalid_xdp_action(act);
>> case XDP_ABORTED:
>> @@ -788,7 +829,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> }
>>
> [...]
>
> Otherwise looks good to me thanks for doing this.
>
> .John
>
Thanks for the reviewing.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* RE: [virtio-dev] packed ring layout proposal v3
From: Liang, Cunming @ 2017-09-21 13:36 UTC (permalink / raw)
To: Michael S. Tsirkin, virtio-dev@lists.oasis-open.org
Cc: virtualization@lists.linux-foundation.org
In-Reply-To: <20160915223915.qjlnlvf2w7u37bu3@redhat.com>
Hi,
> -----Original Message-----
> From: virtio-dev@lists.oasis-open.org [mailto:virtio-dev@lists.oasis-open.org] On
> Behalf Of Michael S. Tsirkin
> Sent: Sunday, September 10, 2017 1:06 PM
> To: virtio-dev@lists.oasis-open.org
> Cc: virtualization@lists.linux-foundation.org
> Subject: [virtio-dev] packed ring layout proposal v3
>
[...]
> * Batching descriptors:
>
> virtio 1.0 allows passing a batch of descriptors in both directions, by incrementing
> the used/avail index by values > 1.
> At the moment only batching of used descriptors is used.
>
> We can support this by chaining a list of device descriptors through
> VRING_DESC_F_MORE flag. Device sets this bit to signal driver that this is part of
> a batch of used descriptors which are all part of a single transaction.
It supposes each s/g chain represents for a packet, while each descriptor among batching chain represents for a packet. There're a few thoughts of batching chain(by VRING_DESC_F_MORE) and s/g chain(by VRING_DESC_F_NEXT).
- batching chain: It's up to device to coalesce the write-out of a batched used descriptors. As the batching size is variable, driver has to detect validity of each descriptor unless the number of incoming valid descriptor is predictable, being curious on the benefits of driver from VRING_DESC_F_MORE to take batching descriptors as a single transaction. On device perspective, it's great to write out one descriptor for the whole chain. However, it assumes no other useful fields in each descriptor of chain needs to write. TX buffer reclaiming can be the candidate, while RX side has to update 'len' at least. Even for this purpose, instead of writing out VRING_DESC_F_MORE on a few descriptors to suppress device writing back, it's cheaper to set flag (e.g. VRING_DESC_F_WB) on single descriptor
of chain to hint the expected position for device to write back.
- s/g chain: It makes sense to indicate the packet boundary. Considering in-order descriptor ring without VRING_DESC_F_INDIRECT, the next descriptor always belongs to the same s/g chain until end of packet indicators occur. So one alternative approach is only to set a flag (e.g. VRING_DESC_F_EOP) on the last descriptor of the chain.
>
> Driver might detect a partial descriptor chain (VRING_DESC_F_MORE set but next
> descriptor not valid). In that case it must not use any parts of the chain - it will
> later be completed by device, but driver is allowed to store the valid parts of the
> chain as device is not allowed to change them anymore.
As each descriptor represent for a whole packet(otherwise it's s/g chain), wondering why it must not use any parts of the chain.
>
> Descriptor should not have both VRING_DESC_F_MORE and
> VRING_DESC_F_NEXT set.
>
[...]
>
> * Selective use of descriptors
>
> As described above, descriptors with NEXT bit set are part of a scatter/gather
> chain and so do not have to cause device to write a used descriptor out.
>
> Similarly, driver can set a flag VRING_DESC_F_MORE in the descriptor to signal to
> device that it does not have to write out the used descriptor as it is part of a batch
> of descriptors. Device has two options (similar to VRING_DESC_F_NEXT):
>
> Device can write out the same number of descriptors for the batch, setting
> VRING_DESC_F_MORE for all but the last descriptor.
> Driver will ignore all used descriptors with VRING_DESC_F_MORE bit set.
It will write out last descriptor without VRING_DESC_F_MORE anyway, so the following statement seems not like another option.
>
> Device only writes out a single descriptor for the whole batch.
> However, to keep device and driver in sync, it then skips a number of descriptors
> corresponding to the length of the batch before writing out the next used
> descriptor.
> After detecting a used descriptor driver must find out the length of the batch that
> it built in order to know where to look for the next device descriptor.
It would be good to keep it simple on device side, and to have the driver control the expectation.
>
>
> TODO (blocker): skipping descriptors for selective and scatter/gather seem to be
> only requested with in-order right now. Let's require in-order for this skipping?
> This will simplify the accounting by driver.
>
>
Thanks,
Steve
^ permalink raw reply
* [PATCH v2 1/1] virtio_balloon: include buffers and cached memory statistics
From: Tomáš Golembiovský @ 2017-09-21 12:55 UTC (permalink / raw)
To: linux-mm, virtualization, qemu-devel, kvm, virtio-dev
Cc: Michael S. Tsirkin, Tomáš Golembiovský, Shaohua Li,
Huang Ying
In-Reply-To: <cover.1505998455.git.tgolembi@redhat.com>
Add a new fields, VIRTIO_BALLOON_S_BUFFERS and VIRTIO_BALLOON_S_CACHED,
to virtio_balloon memory statistics protocol. The values correspond to
'Buffers' and 'Cached' in /proc/meminfo.
To be able to compute the value of 'Cached' memory it is necessary to
export total_swapcache_pages() to modules.
Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
---
drivers/virtio/virtio_balloon.c | 11 +++++++++++
include/uapi/linux/virtio_balloon.h | 4 +++-
mm/swap_state.c | 1 +
3 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index f0b3a0b9d42f..c2558ec47a62 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -244,12 +244,19 @@ static unsigned int update_balloon_stats(struct virtio_balloon *vb)
struct sysinfo i;
unsigned int idx = 0;
long available;
+ long cached;
all_vm_events(events);
si_meminfo(&i);
available = si_mem_available();
+ cached = global_node_page_state(NR_FILE_PAGES) -
+ total_swapcache_pages() - i.bufferram;
+ if (cached < 0)
+ cached = 0;
+
+
#ifdef CONFIG_VM_EVENT_COUNTERS
update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
pages_to_bytes(events[PSWPIN]));
@@ -264,6 +271,10 @@ static unsigned int update_balloon_stats(struct virtio_balloon *vb)
pages_to_bytes(i.totalram));
update_stat(vb, idx++, VIRTIO_BALLOON_S_AVAIL,
pages_to_bytes(available));
+ update_stat(vb, idx++, VIRTIO_BALLOON_S_BUFFERS,
+ pages_to_bytes(i.bufferram));
+ update_stat(vb, idx++, VIRTIO_BALLOON_S_CACHED,
+ pages_to_bytes(cached));
return idx;
}
diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
index 343d7ddefe04..d5dc8a56a497 100644
--- a/include/uapi/linux/virtio_balloon.h
+++ b/include/uapi/linux/virtio_balloon.h
@@ -52,7 +52,9 @@ struct virtio_balloon_config {
#define VIRTIO_BALLOON_S_MEMFREE 4 /* Total amount of free memory */
#define VIRTIO_BALLOON_S_MEMTOT 5 /* Total amount of memory */
#define VIRTIO_BALLOON_S_AVAIL 6 /* Available memory as in /proc */
-#define VIRTIO_BALLOON_S_NR 7
+#define VIRTIO_BALLOON_S_BUFFERS 7 /* Buffers memory as in /proc */
+#define VIRTIO_BALLOON_S_CACHED 8 /* Cached memory as in /proc */
+#define VIRTIO_BALLOON_S_NR 9
/*
* Memory statistics structure.
diff --git a/mm/swap_state.c b/mm/swap_state.c
index 71ce2d1ccbf7..f3a4ff7d6c52 100644
--- a/mm/swap_state.c
+++ b/mm/swap_state.c
@@ -95,6 +95,7 @@ unsigned long total_swapcache_pages(void)
rcu_read_unlock();
return ret;
}
+EXPORT_SYMBOL_GPL(total_swapcache_pages);
static atomic_t swapin_readahead_hits = ATOMIC_INIT(4);
--
2.14.1
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related
* [PATCH v2 0/1] linux: Buffers/caches in VirtIO Balloon driver stats
From: Tomáš Golembiovský @ 2017-09-21 12:55 UTC (permalink / raw)
To: linux-mm, virtualization, qemu-devel, kvm, virtio-dev
Cc: Michael S. Tsirkin, Tomáš Golembiovský, Shaohua Li,
Huang Ying
Linux driver part
v2:
- fixed typos
Tomáš Golembiovský (1):
virtio_balloon: include buffers and cached memory statistics
drivers/virtio/virtio_balloon.c | 11 +++++++++++
include/uapi/linux/virtio_balloon.h | 4 +++-
mm/swap_state.c | 1 +
3 files changed, 15 insertions(+), 1 deletion(-)
--
2.14.1
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC] virtio-iommu version 0.4
From: Jean-Philippe Brucker @ 2017-09-21 10:29 UTC (permalink / raw)
To: Auger Eric, iommu@lists.linux-foundation.org, kvm@vger.kernel.org,
virtualization@lists.linux-foundation.org,
virtio-dev@lists.oasis-open.org
Cc: Lorenzo Pieralisi, mst@redhat.com, Marc Zyngier, Will Deacon,
Robin Murphy, eric.auger.pro@gmail.com
In-Reply-To: <6d9f91df-ebb0-2ae0-a098-60a6bcd0e09b@redhat.com>
On 20/09/17 10:37, Auger Eric wrote:
> Hi Jean,
> On 19/09/2017 12:47, Jean-Philippe Brucker wrote:
>> Hi Eric,
>>
>> On 12/09/17 18:13, Auger Eric wrote:
>>> 2.6.7
>>> - As I am currently integrating v0.4 in QEMU here are some other comments:
>>> At the moment struct virtio_iommu_req_probe flags is missing in your
>>> header. As such I understood the ACK protocol was not implemented by the
>>> driver in your branch.
>>
>> Uh indeed. And yet I could swear I've written that code... somewhere. I
>> will add it to the batch of v0.5 changes, it shouldn't be too invasive.
>>
>>> - VIRTIO_IOMMU_PROBE_PROPERTY_TYPE_MASK is VIRTIO_IOMMU_T_MASK in your
>>> header too.
>>
>> Yes, keeping VIRTIO_IOMMU_PROBE_PROPERTY_TYPE_MASK is probably best
>> (though it is a mouthful).
>>
>>> 2.6.8.2:
>>> - I am really confused about what the device should report as resv
>>> regions depending on the PE nature (VFIO or not VFIO)
>>>
>>> In other iommu drivers, the resv regions are populated by the iommu
>>> driver through its get_resv_regions callback. They are usually composed
>>> of an iommu specific MSI region (mapped or bypassed) and non IOMMU
>>> specific (device specific) reserved regions:
>>> iommu_dma_get_resv_regions(). In the case of virtio-iommu driver, those
>>> are the guest reserved regions.
>>>
>>> First in the current virtio-iommu driver I don't see the
>>> iommu_dma_get_resv_regions call. Do you agree that the virtio-iommu
>>> driver should compute the non IOMMU specific MSI regions ie. this is
>>> not the responsability of the virtio-iommu device.
>>
>> For SW_MSI, certainly. The driver allocates a fixed IOVA region for
>> mapping the MSI doorbell. But the driver has to know whether the doorbell
>> region is translated or bypassed.
> Sorry I was talking about *non* IOMMU specific MSI regions, typically
> the regions corresponding to guest PCI host bridge windows. This is
> normally computed in the iommu driver and I didn't that that in the
> existing virtio-iommu driver.
Ah right, I don't think the virtio-iommu device has to report the windows
of the emulated host bridge. RESV is useful for things that are not
obvious to the guest, for instance the physical PCI bridges that are
hidden from the guest.
It's an interesting point though, I can imagine non-Linux guest that are
not as well-equipped with dealing with things like PCI bridge windows, and
could benefit from the device reporting them.
In the end it is up to the device implementation to decide what regions to
report, and make sure that the guest is aware of the various traps in IOVA
space. For things like emulated bridges, the device can expect the guest
to find out about them. For physical bridges/SW_MSI of the host, it should
report the region and make sure that the guest doesn't map them. I'll add
a few more examples to the Implementation Notes, but I suspect reading
your Qemu source code will always be more helpful to people.
>>> Then why is it more the job of the device to return the guest iommu
>>> specific region rather than the driver itself?
>>
>> The MSI region is architectural on x86 IOMMUs, but
>> implementation-defined on virtio-iommu. It depends which platform the host
>> is emulating. In Linux, x86 IOMMU drivers register the bypass region
>> because there always is an IOAPIC on the other end, with a fixed MSI
>> address. But virtio-iommu may be either behind a GIC, an APIC or some
>> other IRQ chip.
>>
>> The driver *could* go over all the irqchips/platforms it knows and try to
>> guess if there is a fixed doorbell or if it needs to reserve an IOVA for
>> them, but it would look horrible. I much prefer having a well-defined way
>> of doing this, so a description from the device.
>
> This means I must have target specific code in the virtio-iommu device
> which is unusual, right? I was initially thinking you could handle that
> on the driver side using a config set for ARM|ARM64. But on the other
> hand you should communicate the info to the device ...
But the device has to know that it has a region that DMA transactions
bypass, right? If you want to implement MSI bypass, then you already have
to add a special case to your device code, and reporting it in a probe
shouldn't require a lot more work. For example, amdvi_translate() has a
special case for amdvi_is_interrupt_addr().
>>> Then I understand this is the responsability of the virtio-iommu device
>>> to gather information about the host resv regions in case of VFIO EP.
>>> Typically the host PCIe host bridge windows cannot be used for IOVA.
>>> Also the host MSI reserved IOVA window cannot be used. Do you agree.
>>
>> Yes, all regions reported in sysfs reserved_regions in the host would be
>> reported as RESV_T_RESERVED by virtio-iommu.
> So to summarize if the probe request is sent to an emulated device, we
> should return the target specific MSI window. We can't and don't return
> the non IOMMU specific guest reserved windows.
>
> For a VFIO device, we would return all reserved regions of the group the
> device belongs to. Is that correct?
Yes
>>
>>> I really think the spec should clarify what exact resv regions the
>>> device should return in case of VFIO device and non VFIO device.
>>
>> Agreed. I will add something about RESV_T_RESERVED with the PCI bridge
>> example in Implementation Notes. Do you think the MSI examples at the end
>> need improvement as well? I can try to explain that RESV_MSI regions in
>> virtio-iommu are only those of the emulated platform, not the HW or SW MSI
>> regions from the host.
>
> I think I would expect an explanation detailing returned reserved
> regions for pure emulated devices and HW/VFIO devices.
>
> Another unrelated remark:
> - you should add a permission violation error.
Do you mean reporting errors from device to driver? It's on the top of my
list, I might add it to v0.5 (without recoverable/PRI mode at the moment)
> - wrt the probe request ACK protocol, this looks pretty heavy as both
> the driver and the device need to parse the req_probe buffer. The device
> need to fill in the output buffer and then read the same info on the
> input buffer. Couldn't we imagine something simpler?
I can't, but I can get rid of the ack protocol and leave space if someone
needs to re-introduce it. I quite like it but don't think it'll be useful
to the few features I'm planning to add. A future re-introduction will
simply have to add a global feature bit VIRTIO_IOMMU_F_PROBE_ACK.
Note that the device isn't required to implement the ACK phase, we simply
give it a chance to verify that the driver understood its information. It
could simply check if the ACK bit is present and ignore the request in
this case.
Thanks,
Jean
^ permalink raw reply
* RE: [virtio-dev] [RFC] virtio-iommu version 0.4
From: Tian, Kevin @ 2017-09-21 6:41 UTC (permalink / raw)
To: Jean-Philippe Brucker, Auger Eric,
iommu@lists.linux-foundation.org, kvm@vger.kernel.org,
virtualization@lists.linux-foundation.org,
virtio-dev@lists.oasis-open.org
Cc: lorenzo.pieralisi@arm.com, mst@redhat.com, marc.zyngier@arm.com,
will.deacon@arm.com, robin.murphy@arm.com,
eric.auger.pro@gmail.com
In-Reply-To: <6b56fd96-fee5-d105-5261-2e7274b4aa15@arm.com>
> From: Jean-Philippe Brucker [mailto:jean-philippe.brucker@arm.com]
> Sent: Wednesday, September 6, 2017 7:49 PM
>
>
> > 2.6.8.2.1
> > Multiple overlapping RESV_MEM properties are merged together. Device
> > requirement? if same types I assume?
>
> Combination rules apply to device and driver. When the driver puts
> multiple endpoints in the same domain, combination rules apply. They will
> become important once the guest attempts to do things like combining
> endpoints with different PASID capabilities in the same domain. I'm
> considering these endpoints to be behind different physical IOMMUs.
>
> For reserved regions, we specify what the driver should do and what the
> device should enforce with regard to mapping IOVAs of overlapping regions.
> For example, if a device has some virtual address RESV_T_MSI and an other
> device has the same virtual address RESV_T_IDENTITY, what should the
> driver do?
>
> I think it should apply the RESV_T_IDENTITY. RESV_T_MSI is just a special
> case of RESV_T_RESERVED, it's a hint for the IRQ subsystem and doesn't
> have a meaning within a domain. From DMA mappings point of view, it is
> effectively the same as RESV_T_RESERVED. When merging
> RESV_T_RESERVED and
> RESV_T_IDENTITY, we should make it RESV_T_IDENTITY. Because it is
> required
> for one endpoint to work (the one with IDENTITY) and is not accessed by
> the other (the driver must not allocate this IOVA range for DMA).
>
> More generally, I'd like to add the following combination table to the
> spec, that shows the resulting reserved region type after merging regions
> from two endpoints. N: no reserved region, R: RESV_T_RESERVED, M:
> RESV_T_MSI, I: RESV_T_IDENTITY. It is symmetric so I didn't fill the
> bottom half.
>
> | N | R | M | I
> ------------------
> N | N | R | M | ?
> ------------------
> R | | R | R | I
> ------------------
> M | | | M | I
> ------------------
> I | | | | I
>
> The 'I' column is problematic. If one endpoint has an IDENTITY region and
> the other doesn't have anything at that virtual address, then making that
> region an identity mapping will result in the second endpoint being able
> to access firmware memory. If using nested translation, stage-2 can help
> us here. But in general we should allow the device to reject an attach
> that would result in a N+I combination if the host considers it too dodgy.
> I think the other combinations are safe enough.
>
will overlap happen in real? Can we simplify the spec to have device
not reporting overlapping regions?
Thanks
Kevin
^ permalink raw reply
* RE: [RFC] virtio-iommu version 0.4
From: Tian, Kevin @ 2017-09-21 6:27 UTC (permalink / raw)
To: Jean-Philippe Brucker, iommu@lists.linux-foundation.org,
kvm@vger.kernel.org, virtualization@lists.linux-foundation.org,
virtio-dev@lists.oasis-open.org
Cc: lorenzo.pieralisi@arm.com, mst@redhat.com, marc.zyngier@arm.com,
will.deacon@arm.com, eric.auger@redhat.com, robin.murphy@arm.com,
eric.auger.pro@gmail.com
In-Reply-To: <013a2fdd-86f7-a7ee-7e71-11bbb6261f33@arm.com>
> From: Jean-Philippe Brucker
> Sent: Wednesday, September 6, 2017 7:55 PM
>
> Hi Kevin,
>
> On 28/08/17 08:39, Tian, Kevin wrote:
> > Here comes some comments:
> >
> > 1.1 Motivation
> >
> > You describe I/O page faults handling as future work. Seems you
> considered
> > only recoverable fault (since "aka. PCI PRI" being used). What about other
> > unrecoverable faults e.g. what to do if a virtual DMA request doesn't find
> > a valid mapping? Even when there is no PRI support, we need some basic
> > form of fault reporting mechanism to indicate such errors to guest.
>
> I am considering recoverable faults as the end goal, but reporting
> unrecoverable faults should use the same queue, with slightly different
> fields and no need for the driver to reply to the device.
what about adding a placeholder for now? Though same mechanism
can be reused, it's an essential part to make virtio-iommu architecture
complete even before talking support for recoverable faults. :-)
>
> > 2.6.8.2 Property RESV_MEM
> >
> > I'm not immediately clear when
> VIRTIO_IOMMU_PROBE_RESV_MEM_T_ABORT
> > should be explicitly reported. Is there any real example on bare metal
> IOMMU?
> > usually reserved memory is reported to CPU through other method (e.g.
> e820
> > on x86 platform). Of course MSI is a special case which is covered by
> BYPASS
> > and MSI flag... If yes, maybe you can also include an example in
> implementation
> > notes.
>
> The RESV_MEM regions only describes IOVA space for the moment, not
> guest-physical, so I guess it provides different information than e820.
>
> I think a useful example is the PCI bridge windows reported by the Linux
> host to userspace using RESV_RESERVED regions (see
> iommu_dma_get_resv_regions). If I understand correctly, they represent
> DMA
> addresses that shouldn't be accessed by endpoints because they won't
> reach
> the IOMMU. These are specific to the physical topology: a device will have
> different reserved regions depending on the PCI slot it occupies.
>
> When handled properly, PCI bridge windows quickly become a nuisance.
> With
> kvmtool we observed that carving out their addresses globally removes a
> lot of useful GPA space from the guest. Without a virtual IOMMU we can
> either ignore them and hope everything will be fine, or remove all
> reserved regions from the GPA space (which currently means editing by
> hand
> the static guest-physical map...)
>
> That's where RESV_MEM_T_ABORT comes handy with virtio-iommu. It
> describes
> reserved IOVAs for a specific endpoint, and therefore removes the need to
> carve the window out of the whole guest.
Understand and thanks for elaboration.
>
> > Another thing I want to ask your opinion, about whether there is value of
> > adding another subtype (MEM_T_IDENTITY), asking for identity mapping
> > in the address space. It's similar to Reserved Memory Region Reporting
> > (RMRR) structure defined in VT-d, to indicate BIOS allocated reserved
> > memory ranges which may be DMA target and has to be identity mapped
> > when DMA remapping is enabled. I'm not sure whether ARM has similar
> > capability and whether there might be a general usage beyond VT-d. For
> > now the only usage in my mind is to assign a device with RMRR associated
> > on VT-d (Intel GPU, or some USB controllers) where the RMRR info needs
> > propagated to the guest (since identity mapping also means reservation
> > of virtual address space).
>
> Yes I think adding MEM_T_IDENTITY will be necessary. I can see they are
> used for both iGPU and USB controllers on my x86 machines. Do you know
> more precisely what they are used for by the firmware?
VTd spec has a clear description:
3.14 Handling Requests to Reserved System Memory
Reserved system memory regions are typically allocated by BIOS at boot
time and reported to OS as reserved address ranges in the system memory
map. Requests-without-PASID to these reserved regions may either occur
as a result of operations performed by the system software driver (for
example in the case of DMA from unified memory access (UMA) graphics
controllers to graphics reserved memory), or may be initiated by non
system software (for example in case of DMA performed by a USB
controller under BIOS SMM control for legacy keyboard emulation).
For proper functioning of these legacy reserved memory usages, when
system software enables DMA remapping, the second-level translation
structures for the respective devices are expected to be set up to provide
identity mapping for the specified reserved memory regions with read
and write permissions.
(one specific example for GPU happens in legacy VGA usage in early
boot time before actual graphics driver is loaded)
>
> It's not necessary with the base virtio-iommu device though (v0.4),
> because the device can create the identity mappings itself and report them
> to the guest as MEM_T_BYPASS. However, when we start handing page
when you say "the device can create ...", I think you really meant
"host iommu driver can create identity mapping for assigned device",
correct?
Then yes, I think above works.
> table
> control over to the guest, the host won't be in control of IOVA->GPA
> mappings and will need to gracefully ask the guest to do it.
>
> I'm not aware of any firmware description resembling Intel RMRR or AMD
> IVMD on ARM platforms. I do think ARM platforms could need
> MEM_T_IDENTITY
> for requesting the guest to map MSI windows when page-table handover is
> in
> use (MSI addresses are translated by the physical SMMU, so a IOVA->GPA
> mapping must be installed by the guest). But since a vSMMU would need a
> solution as well, I think I'll try to implement something more generic.
curious do you need identity mapping full IOVA->GPA->HPA translation,
or just in GPA->HPA stage sufficient for above MSI scenario?
>
> > 2.6.8.2.3 Device Requirements: Property RESV_MEM
> >
> > --citation start--
> > If an endpoint is attached to an address space, the device SHOULD leave
> > any access targeting one of its
> VIRTIO_IOMMU_PROBE_RESV_MEM_T_BYPASS
> > regions pass through untranslated. In other words, the device SHOULD
> > handle such a region as if it was identity-mapped (virtual address equal to
> > physical address). If the endpoint is not attached to any address space,
> > then the device MAY abort the transaction.
> > --citation end
> >
> > I have a question for the last sentence. From definition of BYPASS, it's
> > orthogonal to whether there is an address space attached, then should
> > we still allow "May abort" behavior?
>
> The behavior is left as an implementation choice, and I'm not sure it's
> worth enforcing in the architecture. If the endpoint isn't attached to any
> domain then (unless VIRTIO_IOMMU_F_BYPASS is negotiated), it isn't
> necessarily able to do DMA at all. The virtio-iommu device may setup DMA
> mastering lazily, in which case any DMA transaction would abort, or have
> setup DMA already, in which case the endpoint can access MEM_T_BYPASS
> regions.
>
fair enough. thanks
Kevin
^ permalink raw reply
* Re: [PATCH net-next 3/3] virtio-net: support XDP_REDIRECT
From: John Fastabend @ 2017-09-20 22:02 UTC (permalink / raw)
To: Jason Wang, mst, virtualization, netdev, linux-kernel
In-Reply-To: <1505814163-7315-3-git-send-email-jasowang@redhat.com>
On 09/19/2017 02:42 AM, Jason Wang wrote:
> This patch tries to add XDP_REDIRECT for virtio-net. The changes are
> not complex as we could use exist XDP_TX helpers for most of the
> work. The rest is passing the XDP_TX to NAPI handler for implementing
> batching.
>
> Cc: John Fastabend <john.fastabend@gmail.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
[...]
> @@ -678,12 +711,20 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> }
> break;
> case XDP_TX:
> - if (unlikely(!virtnet_xdp_xmit(vi, &xdp)))
> + if (unlikely(!__virtnet_xdp_xmit(vi, &xdp)))
> trace_xdp_exception(vi->dev, xdp_prog, act);
> + else
> + *xdp_xmit = true;
> if (unlikely(xdp_page != page))
> goto err_xdp;
> rcu_read_unlock();
> goto xdp_xmit;
> + case XDP_REDIRECT:
> + err = xdp_do_redirect(dev, &xdp, xdp_prog);
> + if (err)
> + *xdp_xmit = true;
Is this a typo? Should above be
if (!err)
*xdp_xmit = true;
this would match the pattern in receive_small and also make more sense.
> + rcu_read_unlock();
> + goto xdp_xmit;
> default:
> bpf_warn_invalid_xdp_action(act);
> case XDP_ABORTED:
> @@ -788,7 +829,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> }
>
[...]
Otherwise looks good to me thanks for doing this.
.John
^ permalink raw reply
* Re: [PATCH net-next 3/3] virtio-net: support XDP_REDIRECT
From: David Miller @ 2017-09-20 21:28 UTC (permalink / raw)
To: jasowang; +Cc: netdev, virtualization, john.fastabend, linux-kernel, mst
In-Reply-To: <1505814163-7315-3-git-send-email-jasowang@redhat.com>
From: Jason Wang <jasowang@redhat.com>
Date: Tue, 19 Sep 2017 17:42:43 +0800
> This patch tries to add XDP_REDIRECT for virtio-net. The changes are
> not complex as we could use exist XDP_TX helpers for most of the
> work. The rest is passing the XDP_TX to NAPI handler for implementing
> batching.
>
> Cc: John Fastabend <john.fastabend@gmail.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next 2/3] virtio-net: add packet len average only when needed during XDP
From: David Miller @ 2017-09-20 21:28 UTC (permalink / raw)
To: jasowang; +Cc: netdev, virtualization, john.fastabend, linux-kernel, mst
In-Reply-To: <1505814163-7315-2-git-send-email-jasowang@redhat.com>
From: Jason Wang <jasowang@redhat.com>
Date: Tue, 19 Sep 2017 17:42:42 +0800
> There's no need to add packet len average in the case of XDP_PASS
> since it will be done soon after skb is created.
>
> Cc: John Fastabend <john.fastabend@gmail.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next 1/3] virtio-net: remove unnecessary parameter of virtnet_xdp_xmit()
From: David Miller @ 2017-09-20 21:28 UTC (permalink / raw)
To: jasowang; +Cc: netdev, virtualization, john.fastabend, linux-kernel, mst
In-Reply-To: <1505814163-7315-1-git-send-email-jasowang@redhat.com>
From: Jason Wang <jasowang@redhat.com>
Date: Tue, 19 Sep 2017 17:42:41 +0800
> CC: John Fastabend <john.fastabend@gmail.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
Applied.
^ permalink raw reply
* Re: [PATCH] virtio_balloon: include buffers and chached memory statistics
From: Tomáš Golembiovský @ 2017-09-20 16:01 UTC (permalink / raw)
To: linux-mm, virtio-dev, qemu-devel, kvm, virtualization
Cc: Michael S. Tsirkin, Shaohua Li, Huang Ying
In-Reply-To: <0bc0c49663fafdf3b03844fe048cac3216d88c5b.1505922364.git.tgolembi@redhat.com>
On Wed, 20 Sep 2017 17:48:36 +0200
Tomáš Golembiovský <tgolembi@redhat.com> wrote:
> Add a new fields, VIRTIO_BALLOON_S_BUFFERS and VIRTIO_BALLOON_S_CACHED,
> to virtio_balloon memory statistics protocol. The values correspond to
> 'Buffers' and 'Cached' in /proc/meminfo.
>
> To be able to compute the value of 'Cached' memory it is necessary to
> export total_swapcache_pages() to modules.
>
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
> drivers/virtio/virtio_balloon.c | 11 +++++++++++
> include/uapi/linux/virtio_balloon.h | 4 +++-
> mm/swap_state.c | 1 +
> 3 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index f0b3a0b9d42f..c2558ec47a62 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -244,12 +244,19 @@ static unsigned int update_balloon_stats(struct virtio_balloon *vb)
> struct sysinfo i;
> unsigned int idx = 0;
> long available;
> + long cached;
>
> all_vm_events(events);
> si_meminfo(&i);
>
> available = si_mem_available();
>
> + cached = global_node_page_state(NR_FILE_PAGES) -
> + total_swapcache_pages() - i.bufferram;
> + if (cached < 0)
> + cached = 0;
> +
> +
> #ifdef CONFIG_VM_EVENT_COUNTERS
> update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
> pages_to_bytes(events[PSWPIN]));
> @@ -264,6 +271,10 @@ static unsigned int update_balloon_stats(struct virtio_balloon *vb)
> pages_to_bytes(i.totalram));
> update_stat(vb, idx++, VIRTIO_BALLOON_S_AVAIL,
> pages_to_bytes(available));
> + update_stat(vb, idx++, VIRTIO_BALLOON_S_BUFFERS,
> + pages_to_bytes(i.bufferram));
> + update_stat(vb, idx++, VIRTIO_BALLOON_S_CACHED,
> + pages_to_bytes(cached));
>
> return idx;
> }
> diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
> index 343d7ddefe04..119224c34389 100644
> --- a/include/uapi/linux/virtio_balloon.h
> +++ b/include/uapi/linux/virtio_balloon.h
> @@ -52,7 +52,9 @@ struct virtio_balloon_config {
> #define VIRTIO_BALLOON_S_MEMFREE 4 /* Total amount of free memory */
> #define VIRTIO_BALLOON_S_MEMTOT 5 /* Total amount of memory */
> #define VIRTIO_BALLOON_S_AVAIL 6 /* Available memory as in /proc */
> -#define VIRTIO_BALLOON_S_NR 7
> +#define VIRTIO_BALLOON_S_BUFFERS 7 /* Bufferes memory as in /proc */
I've just noticed I have a typo in the comment: 'Bufferes' should be
'Buffers'.
> +#define VIRTIO_BALLOON_S_CACHED 8 /* Cached memory as in /proc */
> +#define VIRTIO_BALLOON_S_NR 9
>
> /*
> * Memory statistics structure.
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index 71ce2d1ccbf7..f3a4ff7d6c52 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -95,6 +95,7 @@ unsigned long total_swapcache_pages(void)
> rcu_read_unlock();
> return ret;
> }
> +EXPORT_SYMBOL_GPL(total_swapcache_pages);
>
> static atomic_t swapin_readahead_hits = ATOMIC_INIT(4);
>
> --
> 2.14.1
>
--
Tomáš Golembiovský <tgolembi@redhat.com>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [Qemu-devel] [PATCH] virtio_balloon: include buffers and chached memory statistics
From: Eric Blake @ 2017-09-20 15:55 UTC (permalink / raw)
To: Tomáš Golembiovský, linux-mm, virtio-dev,
qemu-devel, kvm, virtualization
Cc: Huang Ying, Shaohua Li, Michael S. Tsirkin
In-Reply-To: <0bc0c49663fafdf3b03844fe048cac3216d88c5b.1505922364.git.tgolembi@redhat.com>
[-- Attachment #1.1.1: Type: text/plain, Size: 827 bytes --]
On 09/20/2017 10:48 AM, Tomáš Golembiovský wrote:
In the subject: s/chached/cached/
> Add a new fields, VIRTIO_BALLOON_S_BUFFERS and VIRTIO_BALLOON_S_CACHED,
> to virtio_balloon memory statistics protocol. The values correspond to
> 'Buffers' and 'Cached' in /proc/meminfo.
>
> To be able to compute the value of 'Cached' memory it is necessary to
> export total_swapcache_pages() to modules.
>
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
> drivers/virtio/virtio_balloon.c | 11 +++++++++++
> include/uapi/linux/virtio_balloon.h | 4 +++-
> mm/swap_state.c | 1 +
> 3 files changed, 15 insertions(+), 1 deletion(-)
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH] virtio_balloon: include buffers and chached memory statistics
From: Tomáš Golembiovský @ 2017-09-20 15:48 UTC (permalink / raw)
To: linux-mm, virtio-dev, qemu-devel, kvm, virtualization
Cc: Michael S. Tsirkin, Tomáš Golembiovský, Shaohua Li,
Huang Ying
Add a new fields, VIRTIO_BALLOON_S_BUFFERS and VIRTIO_BALLOON_S_CACHED,
to virtio_balloon memory statistics protocol. The values correspond to
'Buffers' and 'Cached' in /proc/meminfo.
To be able to compute the value of 'Cached' memory it is necessary to
export total_swapcache_pages() to modules.
Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
---
drivers/virtio/virtio_balloon.c | 11 +++++++++++
include/uapi/linux/virtio_balloon.h | 4 +++-
mm/swap_state.c | 1 +
3 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index f0b3a0b9d42f..c2558ec47a62 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -244,12 +244,19 @@ static unsigned int update_balloon_stats(struct virtio_balloon *vb)
struct sysinfo i;
unsigned int idx = 0;
long available;
+ long cached;
all_vm_events(events);
si_meminfo(&i);
available = si_mem_available();
+ cached = global_node_page_state(NR_FILE_PAGES) -
+ total_swapcache_pages() - i.bufferram;
+ if (cached < 0)
+ cached = 0;
+
+
#ifdef CONFIG_VM_EVENT_COUNTERS
update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
pages_to_bytes(events[PSWPIN]));
@@ -264,6 +271,10 @@ static unsigned int update_balloon_stats(struct virtio_balloon *vb)
pages_to_bytes(i.totalram));
update_stat(vb, idx++, VIRTIO_BALLOON_S_AVAIL,
pages_to_bytes(available));
+ update_stat(vb, idx++, VIRTIO_BALLOON_S_BUFFERS,
+ pages_to_bytes(i.bufferram));
+ update_stat(vb, idx++, VIRTIO_BALLOON_S_CACHED,
+ pages_to_bytes(cached));
return idx;
}
diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
index 343d7ddefe04..119224c34389 100644
--- a/include/uapi/linux/virtio_balloon.h
+++ b/include/uapi/linux/virtio_balloon.h
@@ -52,7 +52,9 @@ struct virtio_balloon_config {
#define VIRTIO_BALLOON_S_MEMFREE 4 /* Total amount of free memory */
#define VIRTIO_BALLOON_S_MEMTOT 5 /* Total amount of memory */
#define VIRTIO_BALLOON_S_AVAIL 6 /* Available memory as in /proc */
-#define VIRTIO_BALLOON_S_NR 7
+#define VIRTIO_BALLOON_S_BUFFERS 7 /* Bufferes memory as in /proc */
+#define VIRTIO_BALLOON_S_CACHED 8 /* Cached memory as in /proc */
+#define VIRTIO_BALLOON_S_NR 9
/*
* Memory statistics structure.
diff --git a/mm/swap_state.c b/mm/swap_state.c
index 71ce2d1ccbf7..f3a4ff7d6c52 100644
--- a/mm/swap_state.c
+++ b/mm/swap_state.c
@@ -95,6 +95,7 @@ unsigned long total_swapcache_pages(void)
rcu_read_unlock();
return ret;
}
+EXPORT_SYMBOL_GPL(total_swapcache_pages);
static atomic_t swapin_readahead_hits = ATOMIC_INIT(4);
--
2.14.1
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related
* Re: [RFC] virtio-iommu version 0.4
From: Auger Eric @ 2017-09-20 9:37 UTC (permalink / raw)
To: Jean-Philippe Brucker, iommu@lists.linux-foundation.org,
kvm@vger.kernel.org, virtualization@lists.linux-foundation.org,
virtio-dev@lists.oasis-open.org
Cc: Lorenzo Pieralisi, mst@redhat.com, Marc Zyngier, Will Deacon,
Robin Murphy, eric.auger.pro@gmail.com
In-Reply-To: <4543e6dc-0f36-11ad-784a-d7c07399d10f@arm.com>
Hi Jean,
On 19/09/2017 12:47, Jean-Philippe Brucker wrote:
> Hi Eric,
>
> On 12/09/17 18:13, Auger Eric wrote:
>> 2.6.7
>> - As I am currently integrating v0.4 in QEMU here are some other comments:
>> At the moment struct virtio_iommu_req_probe flags is missing in your
>> header. As such I understood the ACK protocol was not implemented by the
>> driver in your branch.
>
> Uh indeed. And yet I could swear I've written that code... somewhere. I
> will add it to the batch of v0.5 changes, it shouldn't be too invasive.
>
>> - VIRTIO_IOMMU_PROBE_PROPERTY_TYPE_MASK is VIRTIO_IOMMU_T_MASK in your
>> header too.
>
> Yes, keeping VIRTIO_IOMMU_PROBE_PROPERTY_TYPE_MASK is probably best
> (though it is a mouthful).
>
>> 2.6.8.2:
>> - I am really confused about what the device should report as resv
>> regions depending on the PE nature (VFIO or not VFIO)
>>
>> In other iommu drivers, the resv regions are populated by the iommu
>> driver through its get_resv_regions callback. They are usually composed
>> of an iommu specific MSI region (mapped or bypassed) and non IOMMU
>> specific (device specific) reserved regions:
>> iommu_dma_get_resv_regions(). In the case of virtio-iommu driver, those
>> are the guest reserved regions.
>>
>> First in the current virtio-iommu driver I don't see the
>> iommu_dma_get_resv_regions call. Do you agree that the virtio-iommu
>> driver should compute the non IOMMU specific MSI regions ie. this is
>> not the responsability of the virtio-iommu device.
>
> For SW_MSI, certainly. The driver allocates a fixed IOVA region for
> mapping the MSI doorbell. But the driver has to know whether the doorbell
> region is translated or bypassed.
Sorry I was talking about *non* IOMMU specific MSI regions, typically
the regions corresponding to guest PCI host bridge windows. This is
normally computed in the iommu driver and I didn't that that in the
existing virtio-iommu driver.
>
>> Then why is it more the job of the device to return the guest iommu
>> specific region rather than the driver itself?
>
> The MSI region is architectural on x86 IOMMUs, but
> implementation-defined on virtio-iommu. It depends which platform the host
> is emulating. In Linux, x86 IOMMU drivers register the bypass region
> because there always is an IOAPIC on the other end, with a fixed MSI
> address. But virtio-iommu may be either behind a GIC, an APIC or some
> other IRQ chip.
>
> The driver *could* go over all the irqchips/platforms it knows and try to
> guess if there is a fixed doorbell or if it needs to reserve an IOVA for
> them, but it would look horrible. I much prefer having a well-defined way
> of doing this, so a description from the device.
This means I must have target specific code in the virtio-iommu device
which is unusual, right? I was initially thinking you could handle that
on the driver side using a config set for ARM|ARM64. But on the other
hand you should communicate the info to the device ...
>
>> Then I understand this is the responsability of the virtio-iommu device
>> to gather information about the host resv regions in case of VFIO EP.
>> Typically the host PCIe host bridge windows cannot be used for IOVA.
>> Also the host MSI reserved IOVA window cannot be used. Do you agree.
>
> Yes, all regions reported in sysfs reserved_regions in the host would be
> reported as RESV_T_RESERVED by virtio-iommu.
So to summarize if the probe request is sent to an emulated device, we
should return the target specific MSI window. We can't and don't return
the non IOMMU specific guest reserved windows.
For a VFIO device, we would return all reserved regions of the group the
device belongs to. Is that correct?
>
>> I really think the spec should clarify what exact resv regions the
>> device should return in case of VFIO device and non VFIO device.
>
> Agreed. I will add something about RESV_T_RESERVED with the PCI bridge
> example in Implementation Notes. Do you think the MSI examples at the end
> need improvement as well? I can try to explain that RESV_MSI regions in
> virtio-iommu are only those of the emulated platform, not the HW or SW MSI
> regions from the host.
I think I would expect an explanation detailing returned reserved
regions for pure emulated devices and HW/VFIO devices.
Another unrelated remark:
- you should add a permission violation error.
- wrt the probe request ACK protocol, this looks pretty heavy as both
the driver and the device need to parse the req_probe buffer. The device
need to fill in the output buffer and then read the same info on the
input buffer. Couldn't we imagine something simpler?
Thanks
Eric
>
> Thanks,
> Jean
>
^ 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