* Re: [PATCH net-next RFC 2/5] vhost: introduce helper to prefetch desc index
From: Jason Wang @ 2017-09-28 7:18 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20170928012906-mutt-send-email-mst@kernel.org>
On 2017年09月28日 06:57, Michael S. Tsirkin wrote:
> On Wed, Sep 27, 2017 at 08:35:47AM +0800, Jason Wang wrote:
>>
>> On 2017年09月27日 03:19, Michael S. Tsirkin wrote:
>>> On Fri, Sep 22, 2017 at 04:02:32PM +0800, Jason Wang wrote:
>>>> 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)
>>> why do you need to combine used update with prefetch?
>> For better performance
>
> Why is sticking a branch in there better than requesting the update
> conditionally from the caller?
Ok, I get your point, I can split the two functions.
>
>
>
>> and I believe we don't care about the overhead when
>> we meet errors in tx.
> That's a separate question, I do not really understand how
> you can fetch a descriptor and update the used ring at the same
> time. This allows the guest to overwrite the buffer.
> I might be misunderstanding what is going on here though.
We don't update used idx, so guest can't overwrite the buffer I think?
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next RFC 0/5] batched tx processing in vhost_net
From: Jason Wang @ 2017-09-28 7:16 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20170928012009-mutt-send-email-mst@kernel.org>
On 2017年09月28日 06:28, Michael S. Tsirkin wrote:
> On Wed, Sep 27, 2017 at 08:27:37AM +0800, Jason Wang wrote:
>>
>> On 2017年09月26日 21:45, Michael S. Tsirkin wrote:
>>> On Fri, Sep 22, 2017 at 04:02:30PM +0800, Jason Wang wrote:
>>>> 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.
>>> Interesting, thanks for the patches. So IIUC most of the gain is really
>>> overcoming some of the shortcomings of virtio 1.0 wrt cache utilization?
>> Yes.
>>
>> Actually, looks like batching in 1.1 is not as easy as in 1.0.
>>
>> In 1.0, we could do something like:
>>
>> batch update used ring by user copy_to_user()
>> smp_wmb()
>> update used_idx
>> In 1.1, we need more memory barriers, can't benefit from fast copy helpers?
>>
>> for () {
>> update desc.addr
>> smp_wmb()
>> update desc.flag
>> }
> Yes but smp_wmb is a NOP on e.g. x86. We can switch to other types of
> barriers as well.
We need consider non x86 platforms as well. And we meet similar things
on batch reading.
> We do need to do the updates in order, so we might
> need new APIs for that to avoid re-doing the translation all the time.
>
> In 1.0 the last update is a cache miss always. You need batching to get
> less misses. In 1.1 you don't have it so fundamentally there is less
> need for batching. But batching does not always work. DPDK guys (which
> batch things aggressively) already tried 1.1 and saw performance gains
> so we do not need to argue theoretically.
Do they test on non-x86 CPUs? And the prototype should be reviewed
carefully since a bug can boost the performance in this case.
>
>
>
>>> Which is fair enough (1.0 is already deployed) but I would like to avoid
>>> making 1.1 support harder, and this patchset does this unfortunately,
>> I think the new APIs do not expose more internal data structure of virtio
>> than before? (vq->heads has already been used by vhost_net for years).
> For sure we might need to change vring_used_elem.
>
>> Consider the layout is re-designed completely, I don't see an easy method to
>> reuse current 1.0 API for 1.1.
> Current API just says you get buffers then you use them. It is not tied
> to actual separate used ring.
So do this patch I think? It introduces:
int vhost_prefetch_desc_indices(struct vhost_virtqueue *vq,
struct vring_used_elem *heads,
u16 num);
int vhost_add_used_idx(struct vhost_virtqueue *vq, int n);
There's nothing new exposed to vhost_net. (vring_used_elem has been used
by net.c at many places without this patch).
>
>
>>> see comments on individual patches. I'm sure it can be addressed though.
>>>
>>>> Test shows about ~22% improvement in tx pss.
>>> Is this with or without tx napi in guest?
>> MoonGen is used in guest for better numbers.
>>
>> Thanks
> Not sure I understand. Did you set napi_tx to true or false?
MoonGen uses dpdk, so virtio-net pmd is used in guest. (See
http://conferences.sigcomm.org/imc/2015/papers/p275.pdf)
Thanks
>
>>>> 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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next RFC 5/5] vhost_net: basic tx virtqueue batched processing
From: Jason Wang @ 2017-09-28 7:02 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20170928011724-mutt-send-email-mst@kernel.org>
On 2017年09月28日 06:19, Michael S. Tsirkin wrote:
> On Wed, Sep 27, 2017 at 10:04:18AM +0800, Jason Wang wrote:
>>
>> On 2017年09月27日 03:25, Michael S. Tsirkin wrote:
>>> On Fri, Sep 22, 2017 at 04:02:35PM +0800, Jason Wang wrote:
>>>> 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>
>>> So where is the speedup coming from? I'd guess the ring is
>>> hot in cache, it's faster to access it in one go, then
>>> pass many packets to net stack. Is that right?
>>>
>>> Another possibility is better code cache locality.
>> Yes, I think the speed up comes from:
>>
>> - less cache misses
>> - less cache line bounce when virtqueue is about to be full (guest is faster
>> than host which is the case of MoonGen)
>> - less memory barriers
>> - possible faster copy speed by using copy_to_user() on modern CPUs
>>
>>> So how about this patchset is refactored:
>>>
>>> 1. use existing APIs just first get packets then
>>> transmit them all then use them all
>> Looks like current API can not get packets first, it only support get packet
>> one by one (if you mean vhost_get_vq_desc()). And used ring updating may get
>> more misses in this case.
> Right. So if you do
>
> for (...)
> vhost_get_vq_desc
>
>
> then later
>
> for (...)
> vhost_add_used
>
>
> then you get most of benefits except maybe code cache misses
> and copy_to_user.
>
If you mean vhost_add_used_n(), then more barriers and userspace memory
access as well. And you still need to cache vring_used_elem in vq->heads
or elsewhere. Since you do not batch reading avail ring indexes, more
write miss will happen if the ring is about to be full. So it looks
slower than what is done in this patch?
And if we want to indo more batching on top (do we want this?), we still
need new APIs anyway.
Thanks
>
>
>
>
>
>>> 2. add new APIs and move the loop into vhost core
>>> for more speedups
>> I don't see any advantages, looks like just need some e.g callbacks in this
>> case.
>>
>> Thanks
> IUC callbacks pretty much destroy the code cache locality advantages,
> IP is jumping around too much.
>
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next RFC 3/5] vhost: introduce vhost_add_used_idx()
From: Willem de Bruijn @ 2017-09-28 0:59 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Network Development, LKML, kvm, virtualization
In-Reply-To: <20170928015749-mutt-send-email-mst@kernel.org>
>> > > @@ -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);
>> > Please change the API to hide the fact that there's an index that needs
>> > to be updated.
>>
>> In fact, an interesting optimization on top is just call
>> vhost_add_used_idx(vq, n) instead of n vhost_add_used_idx(vq, 1). That's the
>> reason I leave n in the API.
>>
>> Thanks
>
> Right but you could increment some internal counter in the vq
> structure then update the used index using some api
> with a generic name, e.g. add_used_complete or something like this.
That adds a layer of information hiding. If the same variable can be
kept close to the computation in a local variable and passed directly
to vhost_add_used_idx_n that is easier to follow.
^ permalink raw reply
* Re: [PATCH net-next RFC 5/5] vhost_net: basic tx virtqueue batched processing
From: Willem de Bruijn @ 2017-09-28 0:55 UTC (permalink / raw)
To: Jason Wang
Cc: Network Development, virtualization, LKML, kvm,
Michael S. Tsirkin
In-Reply-To: <1506067355-5771-6-git-send-email-jasowang@redhat.com>
> @@ -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 */
This special case can perhaps be avoided if we no longer block
on vhost_exceeds_maxpend, but revert to copying.
> + if (zcopy) {
> + heads = &used;
Can this special case of batchsize 1 not use vq->heads?
> + 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;
> + /* 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);
While batching, perhaps can also move this producer index update
out of the loop and using vhost_add_used_and_signal_n.
> + } 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;
This patch touches many lines just for indentation. If having to touch
these lines anyway (dirtying git blame), it may be a good time to move
the processing of a single descriptor code into a separate helper function.
And while breaking up, perhaps another helper for setting up ubuf_info.
If you agree, preferably in a separate noop refactor patch that precedes
the functional changes.
^ permalink raw reply
* Re: [PATCH net-next RFC 2/5] vhost: introduce helper to prefetch desc index
From: Willem de Bruijn @ 2017-09-28 0:47 UTC (permalink / raw)
To: Jason Wang
Cc: Network Development, virtualization, LKML, kvm,
Michael S. Tsirkin
In-Reply-To: <1506067355-5771-3-git-send-email-jasowang@redhat.com>
On Fri, Sep 22, 2017 at 4:02 AM, Jason Wang <jasowang@redhat.com> wrote:
> 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);
> + }
This is understandably very similar to the existing logic in vhost_get_vq_desc.
Can that be extracted to a helper to avoid code duplication?
Perhaps one helper to update vq->avail_idx and return num, and
another to call vhost_get_avail one or more times.
> +
> + 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;
> + }
This second part seems unrelated and could be a separate function?
Also, no need for ret2 and double assignment "ret = total =" if not
modifying total
in the the second loop:
for (i = 0; i < total; ) {
...
i += copied;
}
^ permalink raw reply
* Re: [PATCH net-next] vhost_net: do not stall on zerocopy depletion
From: Willem de Bruijn @ 2017-09-28 0:33 UTC (permalink / raw)
To: Network Development
Cc: Willem de Bruijn, Michael S. Tsirkin, Koichiro Den,
virtualization, David Miller
In-Reply-To: <20170928002556.41240-1-willemdebruijn.kernel@gmail.com>
On Wed, Sep 27, 2017 at 8:25 PM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> From: Willem de Bruijn <willemb@google.com>
>
> Vhost-net has a hard limit on the number of zerocopy skbs in flight.
> When reached, transmission stalls. Stalls cause latency, as well as
> head-of-line blocking of other flows that do not use zerocopy.
>
> Instead of stalling, revert to copy-based transmission.
>
> Tested by sending two udp flows from guest to host, one with payload
> of VHOST_GOODCOPY_LEN, the other too small for zerocopy (1B). The
> large flow is redirected to a netem instance with 1MBps rate limit
> and deep 1000 entry queue.
>
> modprobe ifb
> ip link set dev ifb0 up
> tc qdisc add dev ifb0 root netem limit 1000 rate 1MBit
>
> tc qdisc add dev tap0 ingress
> tc filter add dev tap0 parent ffff: protocol ip \
> u32 match ip dport 8000 0xffff \
> action mirred egress redirect dev ifb0
>
> Before the delay, both flows process around 80K pps. With the delay,
> before this patch, both process around 400. After this patch, the
> large flow is still rate limited, while the small reverts to its
> original rate. See also discussion in the first link, below.
>
> The limit in vhost_exceeds_maxpend must be carefully chosen. When
> vq->num >> 1, the flows remain correlated. This value happens to
> correspond to VHOST_MAX_PENDING for vq->num == 256. Allow smaller
> fractions and ensure correctness also for much smaller values of
> vq->num, by testing the min() of both explicitly. See also the
> discussion in the second link below.
>
> Link:http://lkml.kernel.org/r/CAF=yD-+Wk9sc9dXMUq1+x_hh=3ThTXa6BnZkygP3tgVpjbp93g@mail.gmail.com
From the same discussion thread: it would be good to expose stats
on the number of zerocopy skb sent and number completed without
copy.
To test this patch, I also added ethtool stats to tun and extended them
with two zerocopy counters. Then had tun override the uarg->callback
with its own and update the counters before calling the original callback.
The one useful datapoint I did not get out of that is why skbs would
revert to non-zerocopy: because of size, vhost_exceeds_maxpend
or vhost_net_tx_select_zcopy. The simplistic implementation with an
extra indirect function call and without percpu counters is also not
suitable for submission as is.
^ permalink raw reply
* [PATCH net-next] vhost_net: do not stall on zerocopy depletion
From: Willem de Bruijn @ 2017-09-28 0:25 UTC (permalink / raw)
To: netdev; +Cc: Willem de Bruijn, mst, den, virtualization, davem
From: Willem de Bruijn <willemb@google.com>
Vhost-net has a hard limit on the number of zerocopy skbs in flight.
When reached, transmission stalls. Stalls cause latency, as well as
head-of-line blocking of other flows that do not use zerocopy.
Instead of stalling, revert to copy-based transmission.
Tested by sending two udp flows from guest to host, one with payload
of VHOST_GOODCOPY_LEN, the other too small for zerocopy (1B). The
large flow is redirected to a netem instance with 1MBps rate limit
and deep 1000 entry queue.
modprobe ifb
ip link set dev ifb0 up
tc qdisc add dev ifb0 root netem limit 1000 rate 1MBit
tc qdisc add dev tap0 ingress
tc filter add dev tap0 parent ffff: protocol ip \
u32 match ip dport 8000 0xffff \
action mirred egress redirect dev ifb0
Before the delay, both flows process around 80K pps. With the delay,
before this patch, both process around 400. After this patch, the
large flow is still rate limited, while the small reverts to its
original rate. See also discussion in the first link, below.
The limit in vhost_exceeds_maxpend must be carefully chosen. When
vq->num >> 1, the flows remain correlated. This value happens to
correspond to VHOST_MAX_PENDING for vq->num == 256. Allow smaller
fractions and ensure correctness also for much smaller values of
vq->num, by testing the min() of both explicitly. See also the
discussion in the second link below.
Link:http://lkml.kernel.org/r/CAF=yD-+Wk9sc9dXMUq1+x_hh=3ThTXa6BnZkygP3tgVpjbp93g@mail.gmail.com
Link:http://lkml.kernel.org/r/20170819064129.27272-1-den@klaipeden.com
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
drivers/vhost/net.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 58585ec8699e..50758602ae9d 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -436,8 +436,8 @@ static bool vhost_exceeds_maxpend(struct vhost_net *net)
struct vhost_net_virtqueue *nvq = &net->vqs[VHOST_NET_VQ_TX];
struct vhost_virtqueue *vq = &nvq->vq;
- return (nvq->upend_idx + vq->num - VHOST_MAX_PEND) % UIO_MAXIOV
- == nvq->done_idx;
+ return (nvq->upend_idx + UIO_MAXIOV - nvq->done_idx) % UIO_MAXIOV >
+ min(VHOST_MAX_PEND, vq->num >> 2);
}
/* Expects to be always run from workqueue - which acts as
@@ -480,12 +480,6 @@ static void handle_tx(struct vhost_net *net)
if (zcopy)
vhost_zerocopy_signal_used(net, vq);
- /* If more outstanding DMAs, queue the work.
- * Handle upend_idx wrap around
- */
- if (unlikely(vhost_exceeds_maxpend(net)))
- break;
-
head = vhost_net_tx_get_vq_desc(net, vq, vq->iov,
ARRAY_SIZE(vq->iov),
&out, &in);
@@ -509,6 +503,7 @@ static void handle_tx(struct vhost_net *net)
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: "
@@ -519,8 +514,7 @@ static void handle_tx(struct vhost_net *net)
len = msg_data_left(&msg);
zcopy_used = zcopy && len >= VHOST_GOODCOPY_LEN
- && (nvq->upend_idx + 1) % UIO_MAXIOV !=
- nvq->done_idx
+ && !vhost_exceeds_maxpend(net)
&& vhost_net_tx_select_zcopy(net);
/* use msg_control to pass vhost zerocopy ubuf info to skb */
--
2.14.2.822.g60be5d43e6-goog
^ permalink raw reply related
* Re: [virtio-dev] packed ring layout proposal v3
From: Michael S. Tsirkin @ 2017-09-27 23:49 UTC (permalink / raw)
To: Steven Luong (sluong)
Cc: virtio-dev@lists.oasis-open.org,
virtualization@lists.linux-foundation.org
In-Reply-To: <7A0DC0C9-F148-4161-B2D1-8D8D14D8B9A1@cisco.com>
On Tue, Sep 26, 2017 at 11:38:18PM +0000, Steven Luong (sluong) wrote:
> Michael,
>
> Would you please give an example or two how these two flags DESC_DRIVER and DESC_WRAP are used together? Like others, I am confused by the description and still don’t quite grok it.
>
> Steven
My bad, I will need to work on it. Here is an example:
Let's assume device promised to consume packets in order
ring size = 2
Ring is 0 initialized.
Device initially polls DESC[0].flags for WRAP bit to change.
driver adds:
DESC[0].addr = 1234
DESC[0].id = 0
DESC[0].flags = DESC_DRIVER | DESC_NEXT | DESC_WRAP
and
DESC[0].addr = 5678
DESC[1].id = 1
DESC[1].flags = DESC_DRIVER | DESC_WRAP
it now starts polling DESC[0] flags.
Device reads 1234, executes it, does not use it.
Device reads 5678, executes it, and uses it:
DESC[0].id = 1
DESC[0].flags = 0
Device now polls DESC[0].flags for WRAP bit to change.
Now driver sees that DRIVER bit has been cleared, so it nows that id is
valid. I sees id 1, therefore id 0 and 1 has been read and are safe to
overwrite.
So it writes it out. It wrapped around to beginning of ring,
so it flips the WRAP bit to 0 on all descriptors now:
DESC[0].addr = 9ABC
DESC[0].id = 0
DESC[0].flags = DESC_DRIVER | DESC_NEXT
DESC[0].addr = DEF0
DESC[0].id = 1
DESC[0].flags = DESC_DRIVER
Next round wrap will be 1 again.
To summarise:
DRIVER bit is used by driver to detect device has used one or more
descriptors. WRAP is is used by device to detect driver has made a
new descriptor available.
--
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next RFC 3/5] vhost: introduce vhost_add_used_idx()
From: Michael S. Tsirkin @ 2017-09-27 22:58 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <c42b7244-6d43-2ce5-46a3-ea70ec9d957f@redhat.com>
On Wed, Sep 27, 2017 at 08:38:24AM +0800, Jason Wang wrote:
>
>
> On 2017年09月27日 03:13, Michael S. Tsirkin wrote:
> > 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(+)
> > >
> > > 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);
> > Please change the API to hide the fact that there's an index that needs
> > to be updated.
>
> In fact, an interesting optimization on top is just call
> vhost_add_used_idx(vq, n) instead of n vhost_add_used_idx(vq, 1). That's the
> reason I leave n in the API.
>
> Thanks
Right but you could increment some internal counter in the vq
structure then update the used index using some api
with a generic name, e.g. add_used_complete or something like this.
> >
> > > --
> > > 2.7.4
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next RFC 2/5] vhost: introduce helper to prefetch desc index
From: Michael S. Tsirkin @ 2017-09-27 22:57 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <17e9c3a9-7759-a674-bc00-414eabfed118@redhat.com>
On Wed, Sep 27, 2017 at 08:35:47AM +0800, Jason Wang wrote:
>
>
> On 2017年09月27日 03:19, Michael S. Tsirkin wrote:
> > On Fri, Sep 22, 2017 at 04:02:32PM +0800, Jason Wang wrote:
> > > 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)
> > why do you need to combine used update with prefetch?
>
> For better performance
Why is sticking a branch in there better than requesting the update
conditionally from the caller?
> and I believe we don't care about the overhead when
> we meet errors in tx.
That's a separate question, I do not really understand how
you can fetch a descriptor and update the used ring at the same
time. This allows the guest to overwrite the buffer.
I might be misunderstanding what is going on here though.
> >
> > > +{
> > > + 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();
> > Barrier before return is a very confusing API. I guess it's designed to
> > be used in a specific way to make it necessary - but what is it?
>
> Looks like a and we need do this after reading avail_idx.
>
> Thanks
>
> >
> >
> > > + 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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next RFC 0/5] batched tx processing in vhost_net
From: Michael S. Tsirkin @ 2017-09-27 22:28 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <afb7cad9-d760-b4d7-ecc5-518442e061b1@redhat.com>
On Wed, Sep 27, 2017 at 08:27:37AM +0800, Jason Wang wrote:
>
>
> On 2017年09月26日 21:45, Michael S. Tsirkin wrote:
> > On Fri, Sep 22, 2017 at 04:02:30PM +0800, Jason Wang wrote:
> > > 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.
> > Interesting, thanks for the patches. So IIUC most of the gain is really
> > overcoming some of the shortcomings of virtio 1.0 wrt cache utilization?
>
> Yes.
>
> Actually, looks like batching in 1.1 is not as easy as in 1.0.
>
> In 1.0, we could do something like:
>
> batch update used ring by user copy_to_user()
> smp_wmb()
> update used_idx
> In 1.1, we need more memory barriers, can't benefit from fast copy helpers?
>
> for () {
> update desc.addr
> smp_wmb()
> update desc.flag
> }
Yes but smp_wmb is a NOP on e.g. x86. We can switch to other types of
barriers as well. We do need to do the updates in order, so we might
need new APIs for that to avoid re-doing the translation all the time.
In 1.0 the last update is a cache miss always. You need batching to get
less misses. In 1.1 you don't have it so fundamentally there is less
need for batching. But batching does not always work. DPDK guys (which
batch things aggressively) already tried 1.1 and saw performance gains
so we do not need to argue theoretically.
> >
> > Which is fair enough (1.0 is already deployed) but I would like to avoid
> > making 1.1 support harder, and this patchset does this unfortunately,
>
> I think the new APIs do not expose more internal data structure of virtio
> than before? (vq->heads has already been used by vhost_net for years).
For sure we might need to change vring_used_elem.
> Consider the layout is re-designed completely, I don't see an easy method to
> reuse current 1.0 API for 1.1.
Current API just says you get buffers then you use them. It is not tied
to actual separate used ring.
> > see comments on individual patches. I'm sure it can be addressed though.
> >
> > > Test shows about ~22% improvement in tx pss.
> > Is this with or without tx napi in guest?
>
> MoonGen is used in guest for better numbers.
>
> Thanks
Not sure I understand. Did you set napi_tx to true or false?
> >
> > > 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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next RFC 5/5] vhost_net: basic tx virtqueue batched processing
From: Michael S. Tsirkin @ 2017-09-27 22:19 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <16ea7512-d770-21ef-edb6-3ada51f08592@redhat.com>
On Wed, Sep 27, 2017 at 10:04:18AM +0800, Jason Wang wrote:
>
>
> On 2017年09月27日 03:25, Michael S. Tsirkin wrote:
> > On Fri, Sep 22, 2017 at 04:02:35PM +0800, Jason Wang wrote:
> > > 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>
> > So where is the speedup coming from? I'd guess the ring is
> > hot in cache, it's faster to access it in one go, then
> > pass many packets to net stack. Is that right?
> >
> > Another possibility is better code cache locality.
>
> Yes, I think the speed up comes from:
>
> - less cache misses
> - less cache line bounce when virtqueue is about to be full (guest is faster
> than host which is the case of MoonGen)
> - less memory barriers
> - possible faster copy speed by using copy_to_user() on modern CPUs
>
> >
> > So how about this patchset is refactored:
> >
> > 1. use existing APIs just first get packets then
> > transmit them all then use them all
>
> Looks like current API can not get packets first, it only support get packet
> one by one (if you mean vhost_get_vq_desc()). And used ring updating may get
> more misses in this case.
Right. So if you do
for (...)
vhost_get_vq_desc
then later
for (...)
vhost_add_used
then you get most of benefits except maybe code cache misses
and copy_to_user.
> > 2. add new APIs and move the loop into vhost core
> > for more speedups
>
> I don't see any advantages, looks like just need some e.g callbacks in this
> case.
>
> Thanks
IUC callbacks pretty much destroy the code cache locality advantages,
IP is jumping around too much.
--
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH] drivers/s390/virtio: Remove the old KVM virtio transport
From: Halil Pasic @ 2017-09-27 14:35 UTC (permalink / raw)
To: Thomas Huth, Cornelia Huck
Cc: linux-s390, kvm, Heiko Carstens, virtualization,
Martin Schwidefsky
In-Reply-To: <1506512521-2244-1-git-send-email-thuth@redhat.com>
On 09/27/2017 01:42 PM, Thomas Huth wrote:
> There is no recent user space application available anymore which still
> supports this old virtio transport. Additionally, commit 3b2fbb3f06ef
> ("virtio/s390: deprecate old transport") introduced a deprecation message
> in the driver, and apparently nobody complained so far that it is still
> required. So let's simply remove it.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
Acked-by: Halil Pasic <pasic@linux.vnet.ibm.com>
(with or without the fixup))
^ permalink raw reply
* Re: [PATCH] drivers/s390/virtio: Remove the old KVM virtio transport
From: Heiko Carstens @ 2017-09-27 14:10 UTC (permalink / raw)
To: Thomas Huth
Cc: linux-s390, kvm, Cornelia Huck, virtualization,
Martin Schwidefsky
In-Reply-To: <66822fa4-f7fe-565a-bc51-9fe1493378f9@redhat.com>
On Wed, Sep 27, 2017 at 03:06:55PM +0200, Thomas Huth wrote:
> On 27.09.2017 14:55, Heiko Carstens wrote:
> > On Wed, Sep 27, 2017 at 01:42:01PM +0200, Thomas Huth wrote:
> >> diff --git a/drivers/s390/virtio/Makefile b/drivers/s390/virtio/Makefile
> >> index df40692..d9942e0 100644
> >> --- a/drivers/s390/virtio/Makefile
> >> +++ b/drivers/s390/virtio/Makefile
> >> @@ -7,7 +7,4 @@
> >> # as published by the Free Software Foundation.
> >>
> >> s390-virtio-objs := virtio_ccw.o
> >> -ifdef CONFIG_S390_GUEST_OLD_TRANSPORT
> >> -s390-virtio-objs += kvm_virtio.o
> >> -endif
> >> obj-$(CONFIG_S390_GUEST) += $(s390-virtio-objs)
> >
> > Would you mind to simplify the Makefile to just the single line
> >
> > obj-$(CONFIG_S390_GUEST) += virtio_ccw.o
> >
> > while you are touching it anyway?
>
> Sure ... I'll send a v2 ... or do you rather want to fix it when picking
> up the patch?
I will change it accordingly.
^ permalink raw reply
* Re: [PATCH] drivers/s390/virtio: Remove the old KVM virtio transport
From: Thomas Huth @ 2017-09-27 13:06 UTC (permalink / raw)
To: Heiko Carstens
Cc: linux-s390, kvm, Cornelia Huck, virtualization,
Martin Schwidefsky
In-Reply-To: <20170927125530.GE4032@osiris>
On 27.09.2017 14:55, Heiko Carstens wrote:
> On Wed, Sep 27, 2017 at 01:42:01PM +0200, Thomas Huth wrote:
>> diff --git a/drivers/s390/virtio/Makefile b/drivers/s390/virtio/Makefile
>> index df40692..d9942e0 100644
>> --- a/drivers/s390/virtio/Makefile
>> +++ b/drivers/s390/virtio/Makefile
>> @@ -7,7 +7,4 @@
>> # as published by the Free Software Foundation.
>>
>> s390-virtio-objs := virtio_ccw.o
>> -ifdef CONFIG_S390_GUEST_OLD_TRANSPORT
>> -s390-virtio-objs += kvm_virtio.o
>> -endif
>> obj-$(CONFIG_S390_GUEST) += $(s390-virtio-objs)
>
> Would you mind to simplify the Makefile to just the single line
>
> obj-$(CONFIG_S390_GUEST) += virtio_ccw.o
>
> while you are touching it anyway?
Sure ... I'll send a v2 ... or do you rather want to fix it when picking
up the patch?
Thomas
^ permalink raw reply
* Re: [PATCH] drivers/s390/virtio: Remove the old KVM virtio transport
From: Cornelia Huck @ 2017-09-27 13:03 UTC (permalink / raw)
To: Thomas Huth
Cc: linux-s390, kvm, Heiko Carstens, virtualization,
Martin Schwidefsky
In-Reply-To: <1506512521-2244-1-git-send-email-thuth@redhat.com>
On Wed, 27 Sep 2017 13:42:01 +0200
Thomas Huth <thuth@redhat.com> wrote:
> There is no recent user space application available anymore which still
> supports this old virtio transport. Additionally, commit 3b2fbb3f06ef
> ("virtio/s390: deprecate old transport") introduced a deprecation message
> in the driver, and apparently nobody complained so far that it is still
> required. So let's simply remove it.
It has done its job of uncovering problems in QEMU's tcg code, so I
guess we can remove it :)
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> arch/s390/Kconfig | 13 -
> drivers/s390/virtio/Makefile | 3 -
> drivers/s390/virtio/kvm_virtio.c | 515 ---------------------------------------
> 3 files changed, 531 deletions(-)
> delete mode 100644 drivers/s390/virtio/kvm_virtio.c
>
> diff --git a/drivers/s390/virtio/Makefile b/drivers/s390/virtio/Makefile
> index df40692..d9942e0 100644
> --- a/drivers/s390/virtio/Makefile
> +++ b/drivers/s390/virtio/Makefile
> @@ -7,7 +7,4 @@
> # as published by the Free Software Foundation.
>
> s390-virtio-objs := virtio_ccw.o
> -ifdef CONFIG_S390_GUEST_OLD_TRANSPORT
> -s390-virtio-objs += kvm_virtio.o
> -endif
> obj-$(CONFIG_S390_GUEST) += $(s390-virtio-objs)
We could simplify this? Does not really matter, though.
This seems to be all of it (other than the uapi stuff).
I can apply this and submit it through virtio, but as I don't have
anything else queued, I would be fine with it going through the s390
architecture tree. In that case,
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply
* Re: [PATCH] drivers/s390/virtio: Remove the old KVM virtio transport
From: Heiko Carstens @ 2017-09-27 12:55 UTC (permalink / raw)
To: Thomas Huth
Cc: linux-s390, kvm, Cornelia Huck, virtualization,
Martin Schwidefsky
In-Reply-To: <1506512521-2244-1-git-send-email-thuth@redhat.com>
On Wed, Sep 27, 2017 at 01:42:01PM +0200, Thomas Huth wrote:
> diff --git a/drivers/s390/virtio/Makefile b/drivers/s390/virtio/Makefile
> index df40692..d9942e0 100644
> --- a/drivers/s390/virtio/Makefile
> +++ b/drivers/s390/virtio/Makefile
> @@ -7,7 +7,4 @@
> # as published by the Free Software Foundation.
>
> s390-virtio-objs := virtio_ccw.o
> -ifdef CONFIG_S390_GUEST_OLD_TRANSPORT
> -s390-virtio-objs += kvm_virtio.o
> -endif
> obj-$(CONFIG_S390_GUEST) += $(s390-virtio-objs)
Would you mind to simplify the Makefile to just the single line
obj-$(CONFIG_S390_GUEST) += virtio_ccw.o
while you are touching it anyway?
^ permalink raw reply
* [PATCH] drivers/s390/virtio: Remove the old KVM virtio transport
From: Thomas Huth @ 2017-09-27 11:42 UTC (permalink / raw)
To: Halil Pasic, Cornelia Huck
Cc: linux-s390, kvm, Heiko Carstens, virtualization,
Martin Schwidefsky
There is no recent user space application available anymore which still
supports this old virtio transport. Additionally, commit 3b2fbb3f06ef
("virtio/s390: deprecate old transport") introduced a deprecation message
in the driver, and apparently nobody complained so far that it is still
required. So let's simply remove it.
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
arch/s390/Kconfig | 13 -
drivers/s390/virtio/Makefile | 3 -
drivers/s390/virtio/kvm_virtio.c | 515 ---------------------------------------
3 files changed, 531 deletions(-)
delete mode 100644 drivers/s390/virtio/kvm_virtio.c
diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index 48af970..a0a40dd 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -929,17 +929,4 @@ config S390_GUEST
Select this option if you want to run the kernel as a guest under
the KVM hypervisor.
-config S390_GUEST_OLD_TRANSPORT
- def_bool y
- prompt "Guest support for old s390 virtio transport (DEPRECATED)"
- depends on S390_GUEST
- help
- Enable this option to add support for the old s390-virtio
- transport (i.e. virtio devices NOT based on virtio-ccw). This
- type of virtio devices is only available on the experimental
- kuli userspace or with old (< 2.6) qemu. If you are running
- with a modern version of qemu (which supports virtio-ccw since
- 1.4 and uses it by default since version 2.4), you probably won't
- need this.
-
endmenu
diff --git a/drivers/s390/virtio/Makefile b/drivers/s390/virtio/Makefile
index df40692..d9942e0 100644
--- a/drivers/s390/virtio/Makefile
+++ b/drivers/s390/virtio/Makefile
@@ -7,7 +7,4 @@
# as published by the Free Software Foundation.
s390-virtio-objs := virtio_ccw.o
-ifdef CONFIG_S390_GUEST_OLD_TRANSPORT
-s390-virtio-objs += kvm_virtio.o
-endif
obj-$(CONFIG_S390_GUEST) += $(s390-virtio-objs)
diff --git a/drivers/s390/virtio/kvm_virtio.c b/drivers/s390/virtio/kvm_virtio.c
deleted file mode 100644
index a99d09a..0000000
--- a/drivers/s390/virtio/kvm_virtio.c
+++ /dev/null
@@ -1,515 +0,0 @@
-/*
- * virtio for kvm on s390
- *
- * Copyright IBM Corp. 2008
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License (version 2 only)
- * as published by the Free Software Foundation.
- *
- * Author(s): Christian Borntraeger <borntraeger@de.ibm.com>
- */
-
-#include <linux/kernel_stat.h>
-#include <linux/init.h>
-#include <linux/bootmem.h>
-#include <linux/err.h>
-#include <linux/virtio.h>
-#include <linux/virtio_config.h>
-#include <linux/slab.h>
-#include <linux/virtio_console.h>
-#include <linux/interrupt.h>
-#include <linux/virtio_ring.h>
-#include <linux/export.h>
-#include <linux/pfn.h>
-#include <asm/io.h>
-#include <asm/kvm_para.h>
-#include <asm/kvm_virtio.h>
-#include <asm/sclp.h>
-#include <asm/setup.h>
-#include <asm/irq.h>
-
-#define VIRTIO_SUBCODE_64 0x0D00
-
-/*
- * The pointer to our (page) of device descriptions.
- */
-static void *kvm_devices;
-static struct work_struct hotplug_work;
-
-struct kvm_device {
- struct virtio_device vdev;
- struct kvm_device_desc *desc;
-};
-
-#define to_kvmdev(vd) container_of(vd, struct kvm_device, vdev)
-
-/*
- * memory layout:
- * - kvm_device_descriptor
- * struct kvm_device_desc
- * - configuration
- * struct kvm_vqconfig
- * - feature bits
- * - config space
- */
-static struct kvm_vqconfig *kvm_vq_config(const struct kvm_device_desc *desc)
-{
- return (struct kvm_vqconfig *)(desc + 1);
-}
-
-static u8 *kvm_vq_features(const struct kvm_device_desc *desc)
-{
- return (u8 *)(kvm_vq_config(desc) + desc->num_vq);
-}
-
-static u8 *kvm_vq_configspace(const struct kvm_device_desc *desc)
-{
- return kvm_vq_features(desc) + desc->feature_len * 2;
-}
-
-/*
- * The total size of the config page used by this device (incl. desc)
- */
-static unsigned desc_size(const struct kvm_device_desc *desc)
-{
- return sizeof(*desc)
- + desc->num_vq * sizeof(struct kvm_vqconfig)
- + desc->feature_len * 2
- + desc->config_len;
-}
-
-/* This gets the device's feature bits. */
-static u64 kvm_get_features(struct virtio_device *vdev)
-{
- unsigned int i;
- u32 features = 0;
- struct kvm_device_desc *desc = to_kvmdev(vdev)->desc;
- u8 *in_features = kvm_vq_features(desc);
-
- for (i = 0; i < min(desc->feature_len * 8, 32); i++)
- if (in_features[i / 8] & (1 << (i % 8)))
- features |= (1 << i);
- return features;
-}
-
-static int kvm_finalize_features(struct virtio_device *vdev)
-{
- unsigned int i, bits;
- struct kvm_device_desc *desc = to_kvmdev(vdev)->desc;
- /* Second half of bitmap is features we accept. */
- u8 *out_features = kvm_vq_features(desc) + desc->feature_len;
-
- /* Give virtio_ring a chance to accept features. */
- vring_transport_features(vdev);
-
- /* Make sure we don't have any features > 32 bits! */
- BUG_ON((u32)vdev->features != vdev->features);
-
- memset(out_features, 0, desc->feature_len);
- bits = min_t(unsigned, desc->feature_len, sizeof(vdev->features)) * 8;
- for (i = 0; i < bits; i++) {
- if (__virtio_test_bit(vdev, i))
- out_features[i / 8] |= (1 << (i % 8));
- }
-
- return 0;
-}
-
-/*
- * Reading and writing elements in config space
- */
-static void kvm_get(struct virtio_device *vdev, unsigned int offset,
- void *buf, unsigned len)
-{
- struct kvm_device_desc *desc = to_kvmdev(vdev)->desc;
-
- BUG_ON(offset + len > desc->config_len);
- memcpy(buf, kvm_vq_configspace(desc) + offset, len);
-}
-
-static void kvm_set(struct virtio_device *vdev, unsigned int offset,
- const void *buf, unsigned len)
-{
- struct kvm_device_desc *desc = to_kvmdev(vdev)->desc;
-
- BUG_ON(offset + len > desc->config_len);
- memcpy(kvm_vq_configspace(desc) + offset, buf, len);
-}
-
-/*
- * The operations to get and set the status word just access
- * the status field of the device descriptor. set_status will also
- * make a hypercall to the host, to tell about status changes
- */
-static u8 kvm_get_status(struct virtio_device *vdev)
-{
- return to_kvmdev(vdev)->desc->status;
-}
-
-static void kvm_set_status(struct virtio_device *vdev, u8 status)
-{
- BUG_ON(!status);
- to_kvmdev(vdev)->desc->status = status;
- kvm_hypercall1(KVM_S390_VIRTIO_SET_STATUS,
- (unsigned long) to_kvmdev(vdev)->desc);
-}
-
-/*
- * To reset the device, we use the KVM_VIRTIO_RESET hypercall, using the
- * descriptor address. The Host will zero the status and all the
- * features.
- */
-static void kvm_reset(struct virtio_device *vdev)
-{
- kvm_hypercall1(KVM_S390_VIRTIO_RESET,
- (unsigned long) to_kvmdev(vdev)->desc);
-}
-
-/*
- * When the virtio_ring code wants to notify the Host, it calls us here and we
- * make a hypercall. We hand the address of the virtqueue so the Host
- * knows which virtqueue we're talking about.
- */
-static bool kvm_notify(struct virtqueue *vq)
-{
- long rc;
- struct kvm_vqconfig *config = vq->priv;
-
- rc = kvm_hypercall1(KVM_S390_VIRTIO_NOTIFY, config->address);
- if (rc < 0)
- return false;
- return true;
-}
-
-/*
- * This routine finds the first virtqueue described in the configuration of
- * this device and sets it up.
- */
-static struct virtqueue *kvm_find_vq(struct virtio_device *vdev,
- unsigned index,
- void (*callback)(struct virtqueue *vq),
- const char *name, bool ctx)
-{
- struct kvm_device *kdev = to_kvmdev(vdev);
- struct kvm_vqconfig *config;
- struct virtqueue *vq;
- int err;
-
- if (index >= kdev->desc->num_vq)
- return ERR_PTR(-ENOENT);
-
- if (!name)
- return NULL;
-
- config = kvm_vq_config(kdev->desc)+index;
-
- err = vmem_add_mapping(config->address,
- vring_size(config->num,
- KVM_S390_VIRTIO_RING_ALIGN));
- if (err)
- goto out;
-
- vq = vring_new_virtqueue(index, config->num, KVM_S390_VIRTIO_RING_ALIGN,
- vdev, true, ctx, (void *) config->address,
- kvm_notify, callback, name);
- if (!vq) {
- err = -ENOMEM;
- goto unmap;
- }
-
- /*
- * register a callback token
- * The host will sent this via the external interrupt parameter
- */
- config->token = (u64) vq;
-
- vq->priv = config;
- return vq;
-unmap:
- vmem_remove_mapping(config->address,
- vring_size(config->num,
- KVM_S390_VIRTIO_RING_ALIGN));
-out:
- return ERR_PTR(err);
-}
-
-static void kvm_del_vq(struct virtqueue *vq)
-{
- struct kvm_vqconfig *config = vq->priv;
-
- vring_del_virtqueue(vq);
- vmem_remove_mapping(config->address,
- vring_size(config->num,
- KVM_S390_VIRTIO_RING_ALIGN));
-}
-
-static void kvm_del_vqs(struct virtio_device *vdev)
-{
- struct virtqueue *vq, *n;
-
- list_for_each_entry_safe(vq, n, &vdev->vqs, list)
- kvm_del_vq(vq);
-}
-
-static int kvm_find_vqs(struct virtio_device *vdev, unsigned nvqs,
- struct virtqueue *vqs[],
- vq_callback_t *callbacks[],
- const char * const names[],
- const bool *ctx,
- struct irq_affinity *desc)
-{
- struct kvm_device *kdev = to_kvmdev(vdev);
- int i;
-
- /* We must have this many virtqueues. */
- if (nvqs > kdev->desc->num_vq)
- return -ENOENT;
-
- for (i = 0; i < nvqs; ++i) {
- vqs[i] = kvm_find_vq(vdev, i, callbacks[i], names[i],
- ctx ? ctx[i] : false);
- if (IS_ERR(vqs[i]))
- goto error;
- }
- return 0;
-
-error:
- kvm_del_vqs(vdev);
- return PTR_ERR(vqs[i]);
-}
-
-static const char *kvm_bus_name(struct virtio_device *vdev)
-{
- return "";
-}
-
-/*
- * The config ops structure as defined by virtio config
- */
-static const struct virtio_config_ops kvm_vq_configspace_ops = {
- .get_features = kvm_get_features,
- .finalize_features = kvm_finalize_features,
- .get = kvm_get,
- .set = kvm_set,
- .get_status = kvm_get_status,
- .set_status = kvm_set_status,
- .reset = kvm_reset,
- .find_vqs = kvm_find_vqs,
- .del_vqs = kvm_del_vqs,
- .bus_name = kvm_bus_name,
-};
-
-/*
- * The root device for the kvm virtio devices.
- * This makes them appear as /sys/devices/kvm_s390/0,1,2 not /sys/devices/0,1,2.
- */
-static struct device *kvm_root;
-
-/*
- * adds a new device and register it with virtio
- * appropriate drivers are loaded by the device model
- */
-static void add_kvm_device(struct kvm_device_desc *d, unsigned int offset)
-{
- struct kvm_device *kdev;
-
- kdev = kzalloc(sizeof(*kdev), GFP_KERNEL);
- if (!kdev) {
- printk(KERN_EMERG "Cannot allocate kvm dev %u type %u\n",
- offset, d->type);
- return;
- }
-
- kdev->vdev.dev.parent = kvm_root;
- kdev->vdev.id.device = d->type;
- kdev->vdev.config = &kvm_vq_configspace_ops;
- kdev->desc = d;
-
- if (register_virtio_device(&kdev->vdev) != 0) {
- printk(KERN_ERR "Failed to register kvm device %u type %u\n",
- offset, d->type);
- kfree(kdev);
- }
-}
-
-/*
- * scan_devices() simply iterates through the device page.
- * The type 0 is reserved to mean "end of devices".
- */
-static void scan_devices(void)
-{
- unsigned int i;
- struct kvm_device_desc *d;
-
- for (i = 0; i < PAGE_SIZE; i += desc_size(d)) {
- d = kvm_devices + i;
-
- if (d->type == 0)
- break;
-
- add_kvm_device(d, i);
- }
-}
-
-/*
- * match for a kvm device with a specific desc pointer
- */
-static int match_desc(struct device *dev, void *data)
-{
- struct virtio_device *vdev = dev_to_virtio(dev);
- struct kvm_device *kdev = to_kvmdev(vdev);
-
- return kdev->desc == data;
-}
-
-/*
- * hotplug_device tries to find changes in the device page.
- */
-static void hotplug_devices(struct work_struct *dummy)
-{
- unsigned int i;
- struct kvm_device_desc *d;
- struct device *dev;
-
- for (i = 0; i < PAGE_SIZE; i += desc_size(d)) {
- d = kvm_devices + i;
-
- /* end of list */
- if (d->type == 0)
- break;
-
- /* device already exists */
- dev = device_find_child(kvm_root, d, match_desc);
- if (dev) {
- /* XXX check for hotplug remove */
- put_device(dev);
- continue;
- }
-
- /* new device */
- printk(KERN_INFO "Adding new virtio device %p\n", d);
- add_kvm_device(d, i);
- }
-}
-
-/*
- * we emulate the request_irq behaviour on top of s390 extints
- */
-static void kvm_extint_handler(struct ext_code ext_code,
- unsigned int param32, unsigned long param64)
-{
- struct virtqueue *vq;
- u32 param;
-
- if ((ext_code.subcode & 0xff00) != VIRTIO_SUBCODE_64)
- return;
- inc_irq_stat(IRQEXT_VRT);
-
- /* The LSB might be overloaded, we have to mask it */
- vq = (struct virtqueue *)(param64 & ~1UL);
-
- /* We use ext_params to decide what this interrupt means */
- param = param32 & VIRTIO_PARAM_MASK;
-
- switch (param) {
- case VIRTIO_PARAM_CONFIG_CHANGED:
- virtio_config_changed(vq->vdev);
- break;
- case VIRTIO_PARAM_DEV_ADD:
- schedule_work(&hotplug_work);
- break;
- case VIRTIO_PARAM_VRING_INTERRUPT:
- default:
- vring_interrupt(0, vq);
- break;
- }
-}
-
-/*
- * For s390-virtio, we expect a page above main storage containing
- * the virtio configuration. Try to actually load from this area
- * in order to figure out if the host provides this page.
- */
-static int __init test_devices_support(unsigned long addr)
-{
- int ret = -EIO;
-
- asm volatile(
- "0: lura 0,%1\n"
- "1: xgr %0,%0\n"
- "2:\n"
- EX_TABLE(0b,2b)
- EX_TABLE(1b,2b)
- : "+d" (ret)
- : "a" (addr)
- : "0", "cc");
- return ret;
-}
-/*
- * Init function for virtio
- * devices are in a single page above top of "normal" + standby mem
- */
-static int __init kvm_devices_init(void)
-{
- int rc;
- unsigned long total_memory_size = sclp.rzm * sclp.rnmax;
-
- if (!MACHINE_IS_KVM)
- return -ENODEV;
-
- if (test_devices_support(total_memory_size) < 0)
- return -ENODEV;
-
- pr_warn("The s390-virtio transport is deprecated. Please switch to a modern host providing virtio-ccw.\n");
-
- rc = vmem_add_mapping(total_memory_size, PAGE_SIZE);
- if (rc)
- return rc;
-
- kvm_devices = (void *) total_memory_size;
-
- kvm_root = root_device_register("kvm_s390");
- if (IS_ERR(kvm_root)) {
- rc = PTR_ERR(kvm_root);
- printk(KERN_ERR "Could not register kvm_s390 root device");
- vmem_remove_mapping(total_memory_size, PAGE_SIZE);
- return rc;
- }
-
- INIT_WORK(&hotplug_work, hotplug_devices);
-
- irq_subclass_register(IRQ_SUBCLASS_SERVICE_SIGNAL);
- register_external_irq(EXT_IRQ_CP_SERVICE, kvm_extint_handler);
-
- scan_devices();
- return 0;
-}
-
-/* code for early console output with virtio_console */
-static int early_put_chars(u32 vtermno, const char *buf, int count)
-{
- char scratch[17];
- unsigned int len = count;
-
- if (len > sizeof(scratch) - 1)
- len = sizeof(scratch) - 1;
- scratch[len] = '\0';
- memcpy(scratch, buf, len);
- kvm_hypercall1(KVM_S390_VIRTIO_NOTIFY, __pa(scratch));
- return len;
-}
-
-static int __init s390_virtio_console_init(void)
-{
- if (sclp.has_vt220 || sclp.has_linemode)
- return -ENODEV;
- return virtio_cons_early_init(early_put_chars);
-}
-console_initcall(s390_virtio_console_init);
-
-
-/*
- * We do this after core stuff, but before the drivers.
- */
-postcore_initcall(kvm_devices_init);
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH v5 REPOST 1/6] hw_random: place mutex around read functions and buffers.
From: Pankaj Gupta @ 2017-09-27 6:35 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Herbert Xu, kvm, Rusty Russell, lkml, virtualization,
linux-crypto, Michael Buesch, Matt Mackall, amit shah, Amos Kong
In-Reply-To: <20170926165241.GB14833@dtor-ws>
>
> On Tue, Sep 26, 2017 at 02:36:57AM -0400, Pankaj Gupta wrote:
> >
> > >
> > > A bit late to a party, but:
> > >
> > > On Mon, Dec 8, 2014 at 12:50 AM, Amos Kong <akong@redhat.com> wrote:
> > > > From: Rusty Russell <rusty@rustcorp.com.au>
> > > >
> > > > There's currently a big lock around everything, and it means that we
> > > > can't query sysfs (eg /sys/devices/virtual/misc/hw_random/rng_current)
> > > > while the rng is reading. This is a real problem when the rng is slow,
> > > > or blocked (eg. virtio_rng with qemu's default /dev/random backend)
> > > >
> > > > This doesn't help (it leaves the current lock untouched), just adds a
> > > > lock to protect the read function and the static buffers, in
> > > > preparation
> > > > for transition.
> > > >
> > > > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> > > > ---
> > > ...
> > > >
> > > > @@ -160,13 +166,14 @@ static ssize_t rng_dev_read(struct file *filp,
> > > > char
> > > > __user *buf,
> > > > goto out_unlock;
> > > > }
> > > >
> > > > + mutex_lock(&reading_mutex);
> > >
> > > I think this breaks O_NONBLOCK: we have hwrng core thread that is
> > > constantly pumps underlying rng for data; the thread takes the mutex
> > > and calls rng_get_data() that blocks until RNG responds. This means
> > > that even user specified O_NONBLOCK here we'll be waiting until
> > > [hwrng] thread releases reading_mutex before we can continue.
> >
> > I think for 'virtio_rng' for 'O_NON_BLOCK' 'rng_get_data' returns
> > without waiting for data which can let mutex to be used by other
> > threads waiting if any?
> >
> > rng_dev_read
> > rng_get_data
> > virtio_read
>
> As I said in the paragraph above the code that potentially holds the
> mutex for long time is the thread in hwrng core: hwrng_fillfn(). As it
> calls rng_get_data() with "wait" argument == 1 it may block while
> holding reading_mutex, which, in turn, will block rng_dev_read(), even
> if it was called with O_NONBLOCK.
yes, 'hwrng_fillfn' does not consider O_NONBLOCK and can result in mutex wait
for other tasks. What if we pass zero for wait to 'hwrng_fill' to return early
if there is no data?
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -403,7 +403,7 @@ static int hwrng_fillfn(void *unused)
break;
mutex_lock(&reading_mutex);
rc = rng_get_data(rng, rng_fillbuf,
- rng_buffer_size(), 1);
+ rng_buffer_size(), 0);
mutex_unlock(&reading_mutex);
put_rng(rng);
if (rc <= 0) {
Thanks,
Pankaj
>
> Thanks.
>
> --
> Dmitry
>
^ permalink raw reply
* Re: [PATCH net-next RFC 0/5] batched tx processing in vhost_net
From: Jason Wang @ 2017-09-27 2:06 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20170926222528-mutt-send-email-mst@kernel.org>
On 2017年09月27日 03:26, Michael S. Tsirkin wrote:
> On Fri, Sep 22, 2017 at 04:02:30PM +0800, Jason Wang wrote:
>> 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()
> Please squash these new APIs into where they are used.
> This split-up just makes review harder for me as
> I can't figure out how the new APIs are used.
Ok.
Thanks
>
>
>> vhost_net: rename VHOST_RX_BATCH to VHOST_NET_BATCH
> This is ok as a separate patch.
>
>> 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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next RFC 5/5] vhost_net: basic tx virtqueue batched processing
From: Jason Wang @ 2017-09-27 2:04 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20170926222223-mutt-send-email-mst@kernel.org>
On 2017年09月27日 03:25, Michael S. Tsirkin wrote:
> On Fri, Sep 22, 2017 at 04:02:35PM +0800, Jason Wang wrote:
>> 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>
> So where is the speedup coming from? I'd guess the ring is
> hot in cache, it's faster to access it in one go, then
> pass many packets to net stack. Is that right?
>
> Another possibility is better code cache locality.
Yes, I think the speed up comes from:
- less cache misses
- less cache line bounce when virtqueue is about to be full (guest is
faster than host which is the case of MoonGen)
- less memory barriers
- possible faster copy speed by using copy_to_user() on modern CPUs
>
> So how about this patchset is refactored:
>
> 1. use existing APIs just first get packets then
> transmit them all then use them all
Looks like current API can not get packets first, it only support get
packet one by one (if you mean vhost_get_vq_desc()). And used ring
updating may get more misses in this case.
> 2. add new APIs and move the loop into vhost core
> for more speedups
I don't see any advantages, looks like just need some e.g callbacks in
this case.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v1 4/4] KVM/vmx: enable lbr for the guest
From: Wei Wang @ 2017-09-27 1:27 UTC (permalink / raw)
To: Andi Kleen
Cc: kvm, mst, rkrcmar, linux-kernel, virtualization, mingo, pbonzini
In-Reply-To: <20170926164114.GQ4311@tassilo.jf.intel.com>
On 09/27/2017 12:41 AM, Andi Kleen wrote:
>> 1) vCPU context switching and guest side task switching are not identical.
>> That is, when the vCPU is scheduled out, the guest task on the vCPU may not
> guest task lifetime has nothing to do with this. It's completely independent
> of what you do here on the VCPU level.
>
>> run out its time slice yet, so the task will continue to run when the vCPU
>> is
>> scheduled in by the host (lbr wasn't save by the guest task when the vCPU is
>> scheduled out in this case).
>>
>> It is possible to have the vCPU which runs the guest task (in use of lbr)
>> scheduled
>> out, followed by a new host task being scheduled in on the pCPU to run.
>> It is not guaranteed that the new host task does not use the LBR feature on
>> the
>> pCPU.
> Sure it may use the LBR, and the normal perf context switch
> will switch it and everything works fine.
>
> It's like any other per-task LBR user.
OK, I see the point, thanks.
Why couldn't we save the LBR_SELECT via task switching too?
Best,
Wei
^ permalink raw reply
* Re: [PATCH net-next RFC 3/5] vhost: introduce vhost_add_used_idx()
From: Jason Wang @ 2017-09-27 0:38 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20170926170047-mutt-send-email-mst@kernel.org>
On 2017年09月27日 03:13, Michael S. Tsirkin wrote:
> 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(+)
>>
>> 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);
> Please change the API to hide the fact that there's an index that needs
> to be updated.
In fact, an interesting optimization on top is just call
vhost_add_used_idx(vq, n) instead of n vhost_add_used_idx(vq, 1). That's
the reason I leave n in the API.
Thanks
>
>> --
>> 2.7.4
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next RFC 2/5] vhost: introduce helper to prefetch desc index
From: Jason Wang @ 2017-09-27 0:35 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20170926221435-mutt-send-email-mst@kernel.org>
On 2017年09月27日 03:19, Michael S. Tsirkin wrote:
> On Fri, Sep 22, 2017 at 04:02:32PM +0800, Jason Wang wrote:
>> 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)
> why do you need to combine used update with prefetch?
For better performance and I believe we don't care about the overhead
when we meet errors in tx.
>
>> +{
>> + 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();
> Barrier before return is a very confusing API. I guess it's designed to
> be used in a specific way to make it necessary - but what is it?
Looks like a and we need do this after reading avail_idx.
Thanks
>
>
>> + 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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ 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;
as well as URLs for NNTP newsgroup(s).