* [PATCH 1/1] staging: hv: Fix error checking in channel.c
From: Haiyang Zhang @ 2010-05-13 15:56 UTC (permalink / raw)
To: 'linux-kernel@vger.kernel.org',
'devel@driverdev.osuosl.org',
"'virtualization@lists.osdl.org'" <virtualiz>
[-- Attachment #1: Type: text/plain, Size: 1613 bytes --]
From: Haiyang Zhang <haiyangz@microsoft.com>
Subject: staging: hv: Fix error checking in channel.c
Fixed errors in return value checking code, which caused vmbus channel
not functioning.
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
---
drivers/staging/hv/channel.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/hv/channel.c b/drivers/staging/hv/channel.c
index 12c351e..f047c5a 100644
--- a/drivers/staging/hv/channel.c
+++ b/drivers/staging/hv/channel.c
@@ -204,13 +204,13 @@ int VmbusChannelOpen(struct vmbus_channel *NewChannel, u32 SendRingBufferSize,
RecvRingBufferSize) >> PAGE_SHIFT;
ret = RingBufferInit(&NewChannel->Outbound, out, SendRingBufferSize);
- if (!ret) {
+ if (ret != 0) {
err = ret;
goto errorout;
}
ret = RingBufferInit(&NewChannel->Inbound, in, RecvRingBufferSize);
- if (!ret) {
+ if (ret != 0) {
err = ret;
goto errorout;
}
@@ -228,7 +228,7 @@ int VmbusChannelOpen(struct vmbus_channel *NewChannel, u32 SendRingBufferSize,
RecvRingBufferSize,
&NewChannel->RingBufferGpadlHandle);
- if (!ret) {
+ if (ret != 0) {
err = ret;
goto errorout;
}
@@ -569,7 +569,7 @@ int VmbusChannelEstablishGpadl(struct vmbus_channel *Channel, void *Kbuffer,
ret = VmbusPostMessage(gpadlBody,
subMsgInfo->MessageSize -
sizeof(*subMsgInfo));
- if (!ret)
+ if (ret != 0)
goto Cleanup;
}
--
1.6.3.2
[-- Attachment #2: 0513-Fix-error-checking-in-channel.c.patch --]
[-- Type: application/octet-stream, Size: 1558 bytes --]
From: Haiyang Zhang <haiyangz@microsoft.com>
Subject: staging: hv: Fix error checking in channel.c
Fixed errors in return value checking code, which caused vmbus channel
not functioning.
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
---
drivers/staging/hv/channel.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/hv/channel.c b/drivers/staging/hv/channel.c
index 12c351e..f047c5a 100644
--- a/drivers/staging/hv/channel.c
+++ b/drivers/staging/hv/channel.c
@@ -204,13 +204,13 @@ int VmbusChannelOpen(struct vmbus_channel *NewChannel, u32 SendRingBufferSize,
RecvRingBufferSize) >> PAGE_SHIFT;
ret = RingBufferInit(&NewChannel->Outbound, out, SendRingBufferSize);
- if (!ret) {
+ if (ret != 0) {
err = ret;
goto errorout;
}
ret = RingBufferInit(&NewChannel->Inbound, in, RecvRingBufferSize);
- if (!ret) {
+ if (ret != 0) {
err = ret;
goto errorout;
}
@@ -228,7 +228,7 @@ int VmbusChannelOpen(struct vmbus_channel *NewChannel, u32 SendRingBufferSize,
RecvRingBufferSize,
&NewChannel->RingBufferGpadlHandle);
- if (!ret) {
+ if (ret != 0) {
err = ret;
goto errorout;
}
@@ -569,7 +569,7 @@ int VmbusChannelEstablishGpadl(struct vmbus_channel *Channel, void *Kbuffer,
ret = VmbusPostMessage(gpadlBody,
subMsgInfo->MessageSize -
sizeof(*subMsgInfo));
- if (!ret)
+ if (ret != 0)
goto Cleanup;
}
--
1.6.3.2
[-- Attachment #3: Type: text/plain, Size: 184 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
^ permalink raw reply related
* [GIT PULL] last minute vhost-net fix
From: Michael S. Tsirkin @ 2010-05-13 8:44 UTC (permalink / raw)
To: David Miller; +Cc: kvm, virtualization, netdev, linux-kernel
David, if it's not too late, please pull the following
last minute fix into 2.6.34.
Thanks!
The following changes since commit de02d72bb3cc5b3d4c873db4ca8291723dd48479:
Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless-2.6 (2010-05-10 22:53:41 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git net-2.6
Michael S. Tsirkin (1):
vhost: fix barrier pairing
drivers/vhost/vhost.c | 7 ++++++-
1 files changed, 6 insertions(+), 1 deletions(-)
--
MST
^ permalink raw reply
* [Reminder] KVM Forum 2010: Call for Papers
From: KVM Forum 2010 Program Committee @ 2010-05-12 17:44 UTC (permalink / raw)
To: kvm, qemu-devel, virtualization, libvir-list
Just a reminder...The submission deadline is in two days.
thanks,
-KVM Forum 2010 Program Commitee
--
=================================================================
CALL FOR PAPERS
KVM Forum 2010
=================================================================
DESCRIPTION
The KVM Forum is back! After a break last year we're proud to present
this year's gathering around KVM again. The idea is to have everyone
involved with KVM development come together to talk about the future
and current state of KVM, teaching everyone some pieces of the puzzle
they might be missing without.
So if you're a KVM developer, mark the dates in your calendar! If
possible, also submit a talk -- we're interested in a wide variety of
KVM topics, so don't hesitate to propose a talk on your work.
If you're not a KVM developer, please read on nevertheless (or jump to
END USER COLLABORATION).
DATES / LOCATION
Conference: August 9 - 10, 2010
Location: Renaissance Boston Waterfront in Boston, MA
Abstracts due: May 14th, 2010
Notification: May 28th, 2010
Yes, we're colocated with LinuxCon. Tickets for the KVM Forum also count
for LinuxCon.
http://events.linuxfoundation.org/component/registrationpro/?func=details&did=34
PROCESS
At first check if it's before May 14th. If you're past that date, you're
out of luck. Now try to think hard and come up with a great idea that
you could talk about. Once you have that set, we need you to write up
a short abstract (~150 words) on it. In your submission please note
how long your talk will take. Slots vary in length up to one hour.
Also include in your proposal the proposal type -- one of: technical talk,
breakout session, or end-user talk. Add that information to the abstract
and submit it at the following URL:
http://events.linuxfoundation.org/cfp/cfp-add
Now, wait until May 24th. You will receive a notification on whether
your talk was accepted or not.
SCOPE OF TALKS
We have a list of suggested presentation topics below. These suggestions
are just for guidance, please feel free to submit a proposal on any
of these or related topics. In general, the more it's about backend
infrastructure, the better.
KVM
- Scaling and performance
- Nested virtualization
- I/O improvements
- Driver domains
- Time keeping
- Memory management (page sharing, swapping, huge pages, etc)
- Fault tolerance
- VEPA, vswitch
Embedded KVM
- KVM on ARM, PPC, MIPS, ...?
- Real-time requirements host/guest
- Device pass-through w/o iommu
- Custom device/platform models
QEMU
- Device model improvements
- New devices
- Security model
- Scaling and performance
- Desktop virtualization
- Increasing robustness
- Management interfaces
- QMP protocol and implementation
- Live migration
Virtio
- Speeding up existing devices
- Vhost
- Alternatives
- Using virtio in non-kvm environments
- Virtio on non-Linux
Management infrastructure
- Libvirt
- Kvm autotest
- Easy networking
- Qemud
BREAKOUT SESSION
We will reserve some time each day to break out for working sessions.
These sessions will be less formal than a presentation and more focused
on developing a solution to some real development issue. If you are
interested in getting developers together to hack on some code, submit
your proposal and just make it clear it's a breakout session proposal.
END-USER COLLABORATION
One of the big challenges as developers is to know what, where and how
people actually use our software. To solve this issue at least a little,
there will be a few slots reserved for end users talking about their
deployments, problems and achievements.
So if you have a KVM based deployment running in production or are about
to roll out one, please also submit a talk (see PROCESS), and simply
mark it asn an end-user collaboration proposal. We would love to have
an open discussion of fields where KVM/Qemu can still improve and you
would have the unique chance to steer that process!
Keep in mind that most of the Forum will be focused on development though,
so we suggest you also come with a good portion of technical interest :-).
And of course, no product marketing please! The purpose is to engage
with KVM developers.
LIGHTNING TALKS
In addition to submitted talks we will also have some room for lightning
talks. So if you have something you think might be done until the KVM
Forum, but you're not sure you could fill 15 minutes with it. Or if you
don't know if you'll make it until there, just keep in mind that you
will still get the chance to talk about it. Lightning talk submissions
and scheduling will be handled on-site at KVM Forum.
Thank you for your interest in KVM. We're looking forward to your
submissions and seeing you at the KVM Forum 2010 in August! Now, start
thinking about that talk you want to give.
Thanks,
your KVM Forum 2010 Program Commitee
Alexander Graf, Novell
Anthony Liguori, IBM
Avi Kivity, Red Hat
Chris Wright, Red Hat
Dor Laor, Red Hat
Jan Kiszka, Siemens
Please contact us with any questions or comments.
KVM-Forum-2010-PC@redhat.com
^ permalink raw reply
* Re: [PATCH RFC] vhost: fix barrier pairing
From: Juan Quintela @ 2010-05-12 9:22 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rusty Russell, David S. Miller, Paul E. McKenney, kvm,
virtualization, netdev, linux-kernel
In-Reply-To: <20100511172633.GA9091@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> According to memory-barriers.txt, an smp memory barrier
> should always be paired with another smp memory barrier,
> and I quote "a lack of appropriate pairing is almost certainly an
> error".
>
> In case of vhost, failure to flush out used index
> update before looking at the interrupt disable flag
> could result in missed interrupts, resulting in
> networking hang under stress.
>
> This might happen when flags read bypasses used index write.
> So we see interrupts disabled and do not interrupt, at the
> same time guest writes flags value to enable interrupt,
> reads an old used index value, thinks that
> used ring is empty and waits for interrupt.
>
> Note: the barrier we pair with here is in
> drivers/virtio/virtio_ring.c, function
> vring_enable_cb.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> Dave, I think this is needed in 2.6.34, I'll send a pull
> request after doing some more testing.
>
> Rusty, Juan, could you take a look as well please?
> Thanks!
I would have prefered to put it:
void vhost_add_used_and_signal(struct vhost_dev *dev,
struct vhost_virtqueue *vq,
unsigned int head, int len)
{
vhost_add_used(vq, head, len);
>>>> smp_mb();
vhost_signal(dev, vq);
}
Because it looks strange to have a barrier as the 1st instruction of a
function. And this way it is clearer (at least to me) what we are
protecting.
But on the other hand, we would have to put a comment explainingthat all
users of vhost_signal() have to put that smp_mb() so .....
Perhaps just improving the commet stating that the corresponding barrier
is there?
> Note: the barrier we pair with here is in
> drivers/virtio/virtio_ring.c, function
> vring_enable_cb.
Good catch.
Later, Juan.
^ permalink raw reply
* Re: [Qemu-devel] [PATCH RFC] virtio: put last seen used index into ring itself
From: Michael S. Tsirkin @ 2010-05-11 19:52 UTC (permalink / raw)
To: Avi Kivity; +Cc: qemu-devel, linux-kernel, kvm, virtualization
In-Reply-To: <4BE9AF9A.8080005@redhat.com>
On Tue, May 11, 2010 at 10:27:22PM +0300, Avi Kivity wrote:
> On 05/07/2010 06:23 AM, Rusty Russell wrote:
>> On Thu, 6 May 2010 07:30:00 pm Avi Kivity wrote:
>>
>>> On 05/05/2010 11:58 PM, Michael S. Tsirkin wrote:
>>>
>>>> + /* We publish the last-seen used index at the end of the available ring.
>>>> + * It is at the end for backwards compatibility. */
>>>> + vr->last_used_idx =&(vr)->avail->ring[num];
>>>> + /* Verify that last used index does not spill over the used ring. */
>>>> + BUG_ON((void *)vr->last_used_idx +
>>>> + sizeof *vr->last_used_idx> (void *)vr->used);
>>>> }
>>>>
>>>>
>>> Shouldn't this be on its own cache line?
>>>
>> It's next to the available ring; because that's where the guest publishes
>> its data. That whole page is guest-write, host-read.
>>
>> Putting it on a cacheline by itself would be a slight pessimization; the host
>> cpu would have to get the last_used_idx cacheline and the avail descriptor
>> cacheline every time. This way, they are sometimes the same cacheline.
>>
>
> If one peer writes the tail of the available ring, while the other reads
> last_used_idx, it's a false bounce, no?
>
> Having things on the same cacheline is only worthwhile if they are
> accessed at the same time.
Yes, this is what I was trying to say.
avail flags and used index *are* accessed at the same time, so
there could be an advantage to sharing a cache line there.
All this should be kept in mind if we ever do
VIRTIO_RING_F_NEW_LAYOUT.
--
MST
^ permalink raw reply
* Re: [PATCH RFC] virtio: put last seen used index into ring itself
From: Michael S. Tsirkin @ 2010-05-11 19:48 UTC (permalink / raw)
To: Ryan Harper; +Cc: qemu-devel, linux-kernel, kvm, virtualization
In-Reply-To: <20100511184607.GU14379@us.ibm.com>
On Tue, May 11, 2010 at 01:46:08PM -0500, Ryan Harper wrote:
> * Michael S. Tsirkin <mst@redhat.com> [2010-05-05 16:37]:
> > Generally, the Host end of the virtio ring doesn't need to see where
> > Guest is up to in consuming the ring. However, to completely understand
> > what's going on from the outside, this information must be exposed.
> > For example, host can reduce the number of interrupts by detecting
> > that the guest is currently handling previous buffers.
> >
> > Fortunately, we have room to expand: the ring is always a whole number
> > of pages and there's hundreds of bytes of padding after the avail ring
> > and the used ring, whatever the number of descriptors (which must be a
> > power of 2).
> >
> > We add a feature bit so the guest can tell the host that it's writing
> > out the current value there, if it wants to use that.
> >
> > This is based on a patch by Rusty Russell, with the main difference
> > being that we dedicate a feature bit to guest to tell the host it is
> > writing the used index. This way we don't need to force host to publish
> > the last available index until we have a use for it.
> >
> > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >
> > Rusty,
> > this is a simplified form of a patch you posted in the past.
> > I have a vhost patch that, using this feature, shows external
> > to host bandwidth grow from 5 to 7 GB/s, by avoiding
> > an interrupt in the window after previous interrupt
> > was sent and before interrupts were disabled for the vq.
> > With vhost under some external to host loads I see
> > this window being hit about 30% sometimes.
> >
> > I'm finalizing the host bits and plan to send
> > the final version for inclusion when all's ready,
> > but I'd like to hear comments meanwhile.
> >
> > drivers/virtio/virtio_ring.c | 28 +++++++++++++++++-----------
> > include/linux/virtio_ring.h | 14 +++++++++++++-
> > 2 files changed, 30 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 1ca8890..7729aba 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -89,9 +89,6 @@ struct vring_virtqueue
> > /* Number we've added since last sync. */
> > unsigned int num_added;
> >
> > - /* Last used index we've seen. */
> > - u16 last_used_idx;
> > -
> > /* How to notify other side. FIXME: commonalize hcalls! */
> > void (*notify)(struct virtqueue *vq);
> >
> > @@ -285,12 +282,13 @@ static void detach_buf(struct vring_virtqueue *vq, unsigned int head)
> >
> > static inline bool more_used(const struct vring_virtqueue *vq)
> > {
> > - return vq->last_used_idx != vq->vring.used->idx;
> > + return *vq->vring.last_used_idx != vq->vring.used->idx;
> > }
> >
> > void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
> > {
> > struct vring_virtqueue *vq = to_vvq(_vq);
> > + struct vring_used_elem *u;
> > void *ret;
> > unsigned int i;
> >
> > @@ -307,12 +305,13 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
> > return NULL;
> > }
> >
> > - /* Only get used array entries after they have been exposed by host. */
> > - virtio_rmb();
> > -
> > - i = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].id;
> > - *len = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].len;
> > + /* Only get used array entries after they have been exposed by host.
> > + * Need mb(), not just rmb() because we write last_used_idx below. */
> > + virtio_mb();
> >
> > + u = &vq->vring.used->ring[*vq->vring.last_used_idx % vq->vring.num];
> > + i = u->id;
> > + *len = u->len;
> > if (unlikely(i >= vq->vring.num)) {
> > BAD_RING(vq, "id %u out of range\n", i);
> > return NULL;
> > @@ -325,7 +324,8 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
> > /* detach_buf clears data, so grab it now. */
> > ret = vq->data[i];
> > detach_buf(vq, i);
> > - vq->last_used_idx++;
> > + (*vq->vring.last_used_idx)++;
> > +
> > END_USE(vq);
> > return ret;
> > }
> > @@ -431,7 +431,7 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
> > vq->vq.name = name;
> > vq->notify = notify;
> > vq->broken = false;
> > - vq->last_used_idx = 0;
> > + *vq->vring.last_used_idx = 0;
> > vq->num_added = 0;
> > list_add_tail(&vq->vq.list, &vdev->vqs);
> > #ifdef DEBUG
> > @@ -440,6 +440,10 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
> >
> > vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC);
> >
> > + /* We publish used index whether Host offers it or not: if not, it's
> > + * junk space anyway. But calling this acknowledges the feature. */
> > + virtio_has_feature(vdev, VIRTIO_RING_F_PUBLISH_USED);
> > +
>
> You use VIRTIO_RING_F_PUBLISH_USED here, but
> VIRTIO_RING_F_PUBLISH_INDICES below...
>
>
> > /* No callback? Tell other side not to bother us. */
> > if (!callback)
> > vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
> > @@ -473,6 +477,8 @@ void vring_transport_features(struct virtio_device *vdev)
> > switch (i) {
> > case VIRTIO_RING_F_INDIRECT_DESC:
> > break;
> > + case VIRTIO_RING_F_PUBLISH_INDICES:
> > + break;
>
> Here ^^^
>
> > default:
> > /* We don't understand this bit. */
> > clear_bit(i, vdev->features);
> > diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
> > index e4d144b..9d01de9 100644
> > --- a/include/linux/virtio_ring.h
> > +++ b/include/linux/virtio_ring.h
> > @@ -29,6 +29,9 @@
> > /* We support indirect buffer descriptors */
> > #define VIRTIO_RING_F_INDIRECT_DESC 28
> >
> > +/* The Guest publishes last-seen used index at the end of the avail ring. */
> > +#define VIRTIO_RING_F_PUBLISH_USED 29
> > +
>
> And here; is it PUBLISHED_USED or PUBLISHED_INDICIES ?
>
>
> --
> Ryan Harper
> Software Engineer; Linux Technology Center
> IBM Corp., Austin, Tx
> ryanh@us.ibm.com
Thanks, this is PUBLISHED_USED all over.
This is fixed in the version I'm testing.
--
MST
^ permalink raw reply
* Re: [Qemu-devel] [PATCH RFC] virtio: put last seen used index into ring itself
From: Avi Kivity @ 2010-05-11 19:27 UTC (permalink / raw)
To: Rusty Russell
Cc: qemu-devel, virtualization, linux-kernel, kvm, Michael S. Tsirkin
In-Reply-To: <201005071253.53393.rusty@rustcorp.com.au>
On 05/07/2010 06:23 AM, Rusty Russell wrote:
> On Thu, 6 May 2010 07:30:00 pm Avi Kivity wrote:
>
>> On 05/05/2010 11:58 PM, Michael S. Tsirkin wrote:
>>
>>> + /* We publish the last-seen used index at the end of the available ring.
>>> + * It is at the end for backwards compatibility. */
>>> + vr->last_used_idx =&(vr)->avail->ring[num];
>>> + /* Verify that last used index does not spill over the used ring. */
>>> + BUG_ON((void *)vr->last_used_idx +
>>> + sizeof *vr->last_used_idx> (void *)vr->used);
>>> }
>>>
>>>
>> Shouldn't this be on its own cache line?
>>
> It's next to the available ring; because that's where the guest publishes
> its data. That whole page is guest-write, host-read.
>
> Putting it on a cacheline by itself would be a slight pessimization; the host
> cpu would have to get the last_used_idx cacheline and the avail descriptor
> cacheline every time. This way, they are sometimes the same cacheline.
>
If one peer writes the tail of the available ring, while the other reads
last_used_idx, it's a false bounce, no?
Having things on the same cacheline is only worthwhile if they are
accessed at the same time.
--
Do not meddle in the internals of kernels, for they are subtle and quick to panic.
^ permalink raw reply
* Re: [PATCH RFC] virtio: put last seen used index into ring itself
From: Ryan Harper @ 2010-05-11 18:46 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, linux-kernel, kvm, virtualization
In-Reply-To: <20100505205814.GA7090@redhat.com>
* Michael S. Tsirkin <mst@redhat.com> [2010-05-05 16:37]:
> Generally, the Host end of the virtio ring doesn't need to see where
> Guest is up to in consuming the ring. However, to completely understand
> what's going on from the outside, this information must be exposed.
> For example, host can reduce the number of interrupts by detecting
> that the guest is currently handling previous buffers.
>
> Fortunately, we have room to expand: the ring is always a whole number
> of pages and there's hundreds of bytes of padding after the avail ring
> and the used ring, whatever the number of descriptors (which must be a
> power of 2).
>
> We add a feature bit so the guest can tell the host that it's writing
> out the current value there, if it wants to use that.
>
> This is based on a patch by Rusty Russell, with the main difference
> being that we dedicate a feature bit to guest to tell the host it is
> writing the used index. This way we don't need to force host to publish
> the last available index until we have a use for it.
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> Rusty,
> this is a simplified form of a patch you posted in the past.
> I have a vhost patch that, using this feature, shows external
> to host bandwidth grow from 5 to 7 GB/s, by avoiding
> an interrupt in the window after previous interrupt
> was sent and before interrupts were disabled for the vq.
> With vhost under some external to host loads I see
> this window being hit about 30% sometimes.
>
> I'm finalizing the host bits and plan to send
> the final version for inclusion when all's ready,
> but I'd like to hear comments meanwhile.
>
> drivers/virtio/virtio_ring.c | 28 +++++++++++++++++-----------
> include/linux/virtio_ring.h | 14 +++++++++++++-
> 2 files changed, 30 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 1ca8890..7729aba 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -89,9 +89,6 @@ struct vring_virtqueue
> /* Number we've added since last sync. */
> unsigned int num_added;
>
> - /* Last used index we've seen. */
> - u16 last_used_idx;
> -
> /* How to notify other side. FIXME: commonalize hcalls! */
> void (*notify)(struct virtqueue *vq);
>
> @@ -285,12 +282,13 @@ static void detach_buf(struct vring_virtqueue *vq, unsigned int head)
>
> static inline bool more_used(const struct vring_virtqueue *vq)
> {
> - return vq->last_used_idx != vq->vring.used->idx;
> + return *vq->vring.last_used_idx != vq->vring.used->idx;
> }
>
> void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
> {
> struct vring_virtqueue *vq = to_vvq(_vq);
> + struct vring_used_elem *u;
> void *ret;
> unsigned int i;
>
> @@ -307,12 +305,13 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
> return NULL;
> }
>
> - /* Only get used array entries after they have been exposed by host. */
> - virtio_rmb();
> -
> - i = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].id;
> - *len = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].len;
> + /* Only get used array entries after they have been exposed by host.
> + * Need mb(), not just rmb() because we write last_used_idx below. */
> + virtio_mb();
>
> + u = &vq->vring.used->ring[*vq->vring.last_used_idx % vq->vring.num];
> + i = u->id;
> + *len = u->len;
> if (unlikely(i >= vq->vring.num)) {
> BAD_RING(vq, "id %u out of range\n", i);
> return NULL;
> @@ -325,7 +324,8 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
> /* detach_buf clears data, so grab it now. */
> ret = vq->data[i];
> detach_buf(vq, i);
> - vq->last_used_idx++;
> + (*vq->vring.last_used_idx)++;
> +
> END_USE(vq);
> return ret;
> }
> @@ -431,7 +431,7 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
> vq->vq.name = name;
> vq->notify = notify;
> vq->broken = false;
> - vq->last_used_idx = 0;
> + *vq->vring.last_used_idx = 0;
> vq->num_added = 0;
> list_add_tail(&vq->vq.list, &vdev->vqs);
> #ifdef DEBUG
> @@ -440,6 +440,10 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
>
> vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC);
>
> + /* We publish used index whether Host offers it or not: if not, it's
> + * junk space anyway. But calling this acknowledges the feature. */
> + virtio_has_feature(vdev, VIRTIO_RING_F_PUBLISH_USED);
> +
You use VIRTIO_RING_F_PUBLISH_USED here, but
VIRTIO_RING_F_PUBLISH_INDICES below...
> /* No callback? Tell other side not to bother us. */
> if (!callback)
> vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
> @@ -473,6 +477,8 @@ void vring_transport_features(struct virtio_device *vdev)
> switch (i) {
> case VIRTIO_RING_F_INDIRECT_DESC:
> break;
> + case VIRTIO_RING_F_PUBLISH_INDICES:
> + break;
Here ^^^
> default:
> /* We don't understand this bit. */
> clear_bit(i, vdev->features);
> diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
> index e4d144b..9d01de9 100644
> --- a/include/linux/virtio_ring.h
> +++ b/include/linux/virtio_ring.h
> @@ -29,6 +29,9 @@
> /* We support indirect buffer descriptors */
> #define VIRTIO_RING_F_INDIRECT_DESC 28
>
> +/* The Guest publishes last-seen used index at the end of the avail ring. */
> +#define VIRTIO_RING_F_PUBLISH_USED 29
> +
And here; is it PUBLISHED_USED or PUBLISHED_INDICIES ?
--
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
ryanh@us.ibm.com
^ permalink raw reply
* virtio: power management
From: Michael S. Tsirkin @ 2010-05-11 18:12 UTC (permalink / raw)
To: Rusty Russell; +Cc: netdev, linux-kernel, kvm, virtualization
Anyone looked at power management with virtio?
virtio-pci has callbacks to save/restore pci config
on suspend/resume, but it seems that more action,
such as restoring queue state, would be needed
for e.g. suspend to disk to work.
Rusty, any hints on the code in virtio-pci
that deals with suspend? Is it incomplete?
Thanks,
--
MST
^ permalink raw reply
* [PATCH RFC] vhost: fix barrier pairing
From: Michael S. Tsirkin @ 2010-05-11 17:26 UTC (permalink / raw)
To: Michael S. Tsirkin, Juan Quintela, Rusty Russell, David S. Miller,
Paul
According to memory-barriers.txt, an smp memory barrier
should always be paired with another smp memory barrier,
and I quote "a lack of appropriate pairing is almost certainly an
error".
In case of vhost, failure to flush out used index
update before looking at the interrupt disable flag
could result in missed interrupts, resulting in
networking hang under stress.
This might happen when flags read bypasses used index write.
So we see interrupts disabled and do not interrupt, at the
same time guest writes flags value to enable interrupt,
reads an old used index value, thinks that
used ring is empty and waits for interrupt.
Note: the barrier we pair with here is in
drivers/virtio/virtio_ring.c, function
vring_enable_cb.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
Dave, I think this is needed in 2.6.34, I'll send a pull
request after doing some more testing.
Rusty, Juan, could you take a look as well please?
Thanks!
drivers/vhost/vhost.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index e69d238..14fa2f5 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1035,7 +1035,10 @@ int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head, int len)
/* This actually signals the guest, using eventfd. */
void vhost_signal(struct vhost_dev *dev, struct vhost_virtqueue *vq)
{
- __u16 flags = 0;
+ __u16 flags;
+ /* Flush out used index updates. */
+ smp_mb();
+
if (get_user(flags, &vq->avail->flags)) {
vq_err(vq, "Failed to get flags");
return;
--
1.7.1.12.g42b7f
^ permalink raw reply related
* [PATCH 1/1] staging: hv: Optimize adj_guesttime function and add more detailed comments (Resubmit with some cleanups)
From: Haiyang Zhang @ 2010-05-11 15:11 UTC (permalink / raw)
To: 'linux-kernel@vger.kernel.org',
'devel@driverdev.osuosl.org',
"'virtualization@lists.osdl.org'" <virtualiz>
[-- Attachment #1: Type: text/plain, Size: 2275 bytes --]
From: Haiyang Zhang <haiyangz@microsoft.com>
Subject: Optimize adj_guesttime function and add more detailed comments
Credits go to Joe Perches [joe@perches.com] for suggesting the changes.
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/staging/hv/hv_utils.c | 33 +++++++++++++++++++++++----------
1 files changed, 23 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/hv/hv_utils.c b/drivers/staging/hv/hv_utils.c
index 8f1d3ba..db45d97 100644
--- a/drivers/staging/hv/hv_utils.c
+++ b/drivers/staging/hv/hv_utils.c
@@ -106,31 +106,44 @@ static void shutdown_onchannelcallback(void *context)
orderly_poweroff(false);
}
-
/*
- * Synchronize time with host after reboot, restore, etc.
+ * Set guest time to host UTC time.
*/
-static void adj_guesttime(u64 hosttime, u8 flags)
+static inline void do_adj_guesttime(u64 hosttime)
{
s64 host_tns;
struct timespec host_ts;
- static s32 scnt = 50;
host_tns = (hosttime - WLTIMEDELTA) * 100;
host_ts = ns_to_timespec(host_tns);
+ do_settimeofday(&host_ts);
+}
+
+/*
+ * Synchronize time with host after reboot, restore, etc.
+ *
+ * ICTIMESYNCFLAG_SYNC flag bit indicates reboot, restore events of the VM.
+ * After reboot the flag ICTIMESYNCFLAG_SYNC is included in the first time
+ * message after the timesync channel is opened. Since the hv_utils module is
+ * loaded after hv_vmbus, the first message is usually missed. The other
+ * thing is, systime is automatically set to emulated hardware clock which may
+ * not be UTC time or in the same time zone. So, to override these effects, we
+ * use the first 50 time samples for initial system time setting.
+ */
+static inline void adj_guesttime(u64 hosttime, u8 flags)
+{
+ static s32 scnt = 50;
+
if ((flags & ICTIMESYNCFLAG_SYNC) != 0) {
- do_settimeofday(&host_ts);
+ do_adj_guesttime(hosttime);
return;
}
- if ((flags & ICTIMESYNCFLAG_SAMPLE) != 0 &&
- scnt > 0) {
+ if ((flags & ICTIMESYNCFLAG_SAMPLE) != 0 && scnt > 0) {
scnt--;
- do_settimeofday(&host_ts);
+ do_adj_guesttime(hosttime);
}
-
- return;
}
/*
--
1.6.3.2
[-- Attachment #2: 0511-timesync-optimize.patch --]
[-- Type: application/octet-stream, Size: 2200 bytes --]
From: Haiyang Zhang <haiyangz@microsoft.com>
Subject: Optimize adj_guesttime function and add more detailed comments
Credits go to Joe Perches [joe@perches.com] for suggesting the changes.
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/staging/hv/hv_utils.c | 33 +++++++++++++++++++++++----------
1 files changed, 23 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/hv/hv_utils.c b/drivers/staging/hv/hv_utils.c
index 8f1d3ba..db45d97 100644
--- a/drivers/staging/hv/hv_utils.c
+++ b/drivers/staging/hv/hv_utils.c
@@ -106,31 +106,44 @@ static void shutdown_onchannelcallback(void *context)
orderly_poweroff(false);
}
-
/*
- * Synchronize time with host after reboot, restore, etc.
+ * Set guest time to host UTC time.
*/
-static void adj_guesttime(u64 hosttime, u8 flags)
+static inline void do_adj_guesttime(u64 hosttime)
{
s64 host_tns;
struct timespec host_ts;
- static s32 scnt = 50;
host_tns = (hosttime - WLTIMEDELTA) * 100;
host_ts = ns_to_timespec(host_tns);
+ do_settimeofday(&host_ts);
+}
+
+/*
+ * Synchronize time with host after reboot, restore, etc.
+ *
+ * ICTIMESYNCFLAG_SYNC flag bit indicates reboot, restore events of the VM.
+ * After reboot the flag ICTIMESYNCFLAG_SYNC is included in the first time
+ * message after the timesync channel is opened. Since the hv_utils module is
+ * loaded after hv_vmbus, the first message is usually missed. The other
+ * thing is, systime is automatically set to emulated hardware clock which may
+ * not be UTC time or in the same time zone. So, to override these effects, we
+ * use the first 50 time samples for initial system time setting.
+ */
+static inline void adj_guesttime(u64 hosttime, u8 flags)
+{
+ static s32 scnt = 50;
+
if ((flags & ICTIMESYNCFLAG_SYNC) != 0) {
- do_settimeofday(&host_ts);
+ do_adj_guesttime(hosttime);
return;
}
- if ((flags & ICTIMESYNCFLAG_SAMPLE) != 0 &&
- scnt > 0) {
+ if ((flags & ICTIMESYNCFLAG_SAMPLE) != 0 && scnt > 0) {
scnt--;
- do_settimeofday(&host_ts);
+ do_adj_guesttime(hosttime);
}
-
- return;
}
/*
--
1.6.3.2
[-- Attachment #3: Type: text/plain, Size: 184 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
^ permalink raw reply related
* RE: [PATCH 1/1] staging: hv: Optimize adj_guesttime function and add more detailed comments
From: Haiyang Zhang @ 2010-05-11 14:44 UTC (permalink / raw)
To: Joe Perches
Cc: 'linux-kernel@vger.kernel.org',
'devel@driverdev.osuosl.org',
'virtualization@lists.osdl.org', 'gregkh@suse.de'
In-Reply-To: <1273536158.20514.31.camel@Joe-Laptop.home>
> Just a couple of style nits.
>
> return at the end of a void function() is unnecessary.
Thank you for pointing this out.
I'll update the patch soon.
- Haiyang
^ permalink raw reply
* Re: Patch to auto-load MSFT PV NIC driver
From: Ky Srinivasan @ 2010-05-11 13:55 UTC (permalink / raw)
To: Greg KH, Hank Janssen
Cc: devel@driverdev.osuosl.org, Virtualization@lists.osdl.org,
Haiyang Zhang
In-Reply-To: <20100510172125.GA1482@kroah.com>
>>> On 5/10/2010 at 1:21 PM, in message <20100510172125.GA1482@kroah.com>, Greg KH
<greg@kroah.com> wrote:
> On Mon, May 10, 2010 at 05:05:02PM +0000, Hank Janssen wrote:
>>
>> >Sent: Saturday, May 08, 2010 7:27 AM - Hank Janssen
>> >>On Sat, May 08, 2010 at 01:52:01PM +0000, Hank Janssen wrote:>
>> >>
>> >> I am not sure if this is the right approach. hv_netvsc takes a dependency
> on hv_vmbus.
>> >> hv_vmbus does have the same DMI detection logic in it. But unless hv_vmbus
> has loaded
>> >> up competely, hv_netvsc will fail on loadup. And I do not think we can
> guarantee that
>> >> hv_vmbus has loaded yet.
>> >
>> >Yes you can, the dependancies in the module will take care of it. Try
>> >it, if you try to load the hv_netvsc module before hv_vmbus, modprobe
>> >will load hv_vmbus first.
>>
>> In my testing I have seen issues with timing. It takes a little while for
> VMBus to start
>> getting the initialization taken care of with Hyper-V. And I have seen netvsc
> error with
>> unresolved symbols because vmbus had not completed the initialization yet.
>
> Odd, that shouldn't happen, unless you are loading modules in parallel.
This is indeed strange. Given the module dependency, modprobe is supposed to load all the dependencies first before loading the dependent module. I am wondering if there is some other race in the modules that may be causing this problem.
K. Y
>
> If we switch to a proper discovery type bus, this should not be an
> issue.
>
> thanks,
>
> greg k-h
^ permalink raw reply
* Re: [PATCH 1/1] staging: hv: Optimize adj_guesttime function and add more detailed comments
From: Joe Perches @ 2010-05-11 0:02 UTC (permalink / raw)
To: Haiyang Zhang
Cc: 'linux-kernel@vger.kernel.org',
'devel@driverdev.osuosl.org',
'virtualization@lists.osdl.org', 'gregkh@suse.de'
In-Reply-To: <1FB5E1D5CA062146B38059374562DF72662AA29C@TK5EX14MBXC130.redmond.corp.microsoft.com>
On Mon, 2010-05-10 at 18:12 +0000, Haiyang Zhang wrote:
> diff --git a/drivers/staging/hv/hv_utils.c b/drivers/staging/hv/hv_utils.c
> index 8f1d3ba..c0f57a1 100644
> --- a/drivers/staging/hv/hv_utils.c
> +++ b/drivers/staging/hv/hv_utils.c
> @@ -106,28 +106,45 @@ static void shutdown_onchannelcallback(void *context)
> orderly_poweroff(false);
> }
>
> -
> /*
> - * Synchronize time with host after reboot, restore, etc.
> + * Set guest time to host UTC time.
> */
> -static void adj_guesttime(u64 hosttime, u8 flags)
> +static inline void do_adj_guesttime(u64 hosttime)
> {
> s64 host_tns;
> struct timespec host_ts;
> - static s32 scnt = 50;
>
> host_tns = (hosttime - WLTIMEDELTA) * 100;
> host_ts = ns_to_timespec(host_tns);
>
> + do_settimeofday(&host_ts);
> +
> + return;
> +}
Just a couple of style nits.
return at the end of a void function() is unnecessary.
> +
> +/*
> + * Synchronize time with host after reboot, restore, etc.
> + *
> + * ICTIMESYNCFLAG_SYNC flag bit indicates reboot, restore events of the VM.
> + * After reboot the flag ICTIMESYNCFLAG_SYNC is included in the first time
> + * message after the timesync channel is opened. Since the hv_utils module is
> + * loaded after hv_vmbus, the first message is usually missed. The other
> + * thing is, systime is automatically set to emulated hardware clock which may
> + * not be UTC time or in the same time zone. So, to override these effects, we
> + * use the first 50 time samples for initial system time setting.
> + */
> +static inline void adj_guesttime(u64 hosttime, u8 flags)
> +{
> + static s32 scnt = 50;
> +
> if ((flags & ICTIMESYNCFLAG_SYNC) != 0) {
> - do_settimeofday(&host_ts);
> + do_adj_guesttime(hosttime);
> return;
> }
>
> - if ((flags & ICTIMESYNCFLAG_SAMPLE) != 0 &&
> - scnt > 0) {
> + if ((flags & ICTIMESYNCFLAG_SAMPLE) != 0 && scnt > 0) {
> scnt--;
> - do_settimeofday(&host_ts);
> + do_adj_guesttime(hosttime);
> }
>
> return;
here too
^ permalink raw reply
* Re: RFC: Network Plugin Architecture (NPA) for vmxnet3
From: Pankaj Thakkar @ 2010-05-10 20:46 UTC (permalink / raw)
To: Avi Kivity
Cc: pv-drivers@vmware.com, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org
In-Reply-To: <4BE284CE.7010808@redhat.com>
On Thu, May 06, 2010 at 01:58:54AM -0700, Avi Kivity wrote:
> > We don't pass the whole VF to the guest. Only the BAR which is responsible for
> > TX/RX/intr is mapped into guest space.
>
> Does the SR/IOV spec guarantee that you will have such a separation?
No. This is a guideline which we provided to IHVs and would have to be enforced
through testing/certification.
> How can you unmap the VF without guest cooperation? If you're executing
> Plugin code, you can't yank anything out.
In our Kawela plugin we don't have any reads from the memory space at all.
Hence you can yank the VF anytime (the code loaded in the guest address space
will keep on executing). Even if there were reads we can map the memory
pages to a NULL page and return 0xffffffff so that the plugin can detect this
and return an error to the shell. Remember there are no control operations in
the plugin and the code is really small (about 1k lines compared to 5k lines in
the full VF driver).
>
> Are plugins executed with preemption/interrupts disabled?
Depends on the model. Today the plugin code for checking the TX/RX rings runs
in the deferred napi context.
> What ISAs do those plugins support?
x86 and x64.
Thanks,
-pankaj
^ permalink raw reply
* [PATCH 1/1] staging: hv: Optimize adj_guesttime function and add more detailed comments
From: Haiyang Zhang @ 2010-05-10 18:12 UTC (permalink / raw)
To: 'linux-kernel@vger.kernel.org',
'devel@driverdev.osuosl.org',
"'virtualization@lists.osdl.org'" <virtualiz>
[-- Attachment #1: Type: text/plain, Size: 2276 bytes --]
From: Haiyang Zhang <haiyangz@microsoft.com>
Subject: Optimize adj_guesttime function and add more detailed comments
Credits go to Joe Perches [joe@perches.com] for suggesting the changes.
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/staging/hv/hv_utils.c | 33 +++++++++++++++++++++++++--------
1 files changed, 25 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/hv/hv_utils.c b/drivers/staging/hv/hv_utils.c
index 8f1d3ba..c0f57a1 100644
--- a/drivers/staging/hv/hv_utils.c
+++ b/drivers/staging/hv/hv_utils.c
@@ -106,28 +106,45 @@ static void shutdown_onchannelcallback(void *context)
orderly_poweroff(false);
}
-
/*
- * Synchronize time with host after reboot, restore, etc.
+ * Set guest time to host UTC time.
*/
-static void adj_guesttime(u64 hosttime, u8 flags)
+static inline void do_adj_guesttime(u64 hosttime)
{
s64 host_tns;
struct timespec host_ts;
- static s32 scnt = 50;
host_tns = (hosttime - WLTIMEDELTA) * 100;
host_ts = ns_to_timespec(host_tns);
+ do_settimeofday(&host_ts);
+
+ return;
+}
+
+/*
+ * Synchronize time with host after reboot, restore, etc.
+ *
+ * ICTIMESYNCFLAG_SYNC flag bit indicates reboot, restore events of the VM.
+ * After reboot the flag ICTIMESYNCFLAG_SYNC is included in the first time
+ * message after the timesync channel is opened. Since the hv_utils module is
+ * loaded after hv_vmbus, the first message is usually missed. The other
+ * thing is, systime is automatically set to emulated hardware clock which may
+ * not be UTC time or in the same time zone. So, to override these effects, we
+ * use the first 50 time samples for initial system time setting.
+ */
+static inline void adj_guesttime(u64 hosttime, u8 flags)
+{
+ static s32 scnt = 50;
+
if ((flags & ICTIMESYNCFLAG_SYNC) != 0) {
- do_settimeofday(&host_ts);
+ do_adj_guesttime(hosttime);
return;
}
- if ((flags & ICTIMESYNCFLAG_SAMPLE) != 0 &&
- scnt > 0) {
+ if ((flags & ICTIMESYNCFLAG_SAMPLE) != 0 && scnt > 0) {
scnt--;
- do_settimeofday(&host_ts);
+ do_adj_guesttime(hosttime);
}
return;
--
1.6.3.2
[-- Attachment #2: 0507-timesync-optimize.patch --]
[-- Type: application/octet-stream, Size: 2202 bytes --]
From: Haiyang Zhang <haiyangz@microsoft.com>
Subject: Optimize adj_guesttime function and add more detailed comments
Credits go to Joe Perches [joe@perches.com] for suggesting the changes.
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/staging/hv/hv_utils.c | 33 +++++++++++++++++++++++++--------
1 files changed, 25 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/hv/hv_utils.c b/drivers/staging/hv/hv_utils.c
index 8f1d3ba..c0f57a1 100644
--- a/drivers/staging/hv/hv_utils.c
+++ b/drivers/staging/hv/hv_utils.c
@@ -106,28 +106,45 @@ static void shutdown_onchannelcallback(void *context)
orderly_poweroff(false);
}
-
/*
- * Synchronize time with host after reboot, restore, etc.
+ * Set guest time to host UTC time.
*/
-static void adj_guesttime(u64 hosttime, u8 flags)
+static inline void do_adj_guesttime(u64 hosttime)
{
s64 host_tns;
struct timespec host_ts;
- static s32 scnt = 50;
host_tns = (hosttime - WLTIMEDELTA) * 100;
host_ts = ns_to_timespec(host_tns);
+ do_settimeofday(&host_ts);
+
+ return;
+}
+
+/*
+ * Synchronize time with host after reboot, restore, etc.
+ *
+ * ICTIMESYNCFLAG_SYNC flag bit indicates reboot, restore events of the VM.
+ * After reboot the flag ICTIMESYNCFLAG_SYNC is included in the first time
+ * message after the timesync channel is opened. Since the hv_utils module is
+ * loaded after hv_vmbus, the first message is usually missed. The other
+ * thing is, systime is automatically set to emulated hardware clock which may
+ * not be UTC time or in the same time zone. So, to override these effects, we
+ * use the first 50 time samples for initial system time setting.
+ */
+static inline void adj_guesttime(u64 hosttime, u8 flags)
+{
+ static s32 scnt = 50;
+
if ((flags & ICTIMESYNCFLAG_SYNC) != 0) {
- do_settimeofday(&host_ts);
+ do_adj_guesttime(hosttime);
return;
}
- if ((flags & ICTIMESYNCFLAG_SAMPLE) != 0 &&
- scnt > 0) {
+ if ((flags & ICTIMESYNCFLAG_SAMPLE) != 0 && scnt > 0) {
scnt--;
- do_settimeofday(&host_ts);
+ do_adj_guesttime(hosttime);
}
return;
--
1.6.3.2
[-- Attachment #3: Type: text/plain, Size: 184 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
^ permalink raw reply related
* Re: [PATCHv7] add mergeable buffers support to vhost_net
From: David Stevens @ 2010-05-10 17:46 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, kvm-owner, netdev, netdev-owner, virtualization
In-Reply-To: <20100510172557.GD28798@redhat.com>
netdev-owner@vger.kernel.org wrote on 05/10/2010 10:25:57 AM:
> On Mon, May 10, 2010 at 10:09:03AM -0700, David Stevens wrote:
> > Since "datalen" carries the difference and will be negative by that
amount
> > from the original loop, what about just adding something like:
> >
> > }
> > if (headcount)
> > heads[headcount-1].len += datalen;
> > [and really, headcount >0 since datalen > 0, so just:
> >
> > heads[headcount-1].len += datalen;
> >
> > +-DLS
>
> This works too, just does more checks and comparisons.
> I am still surprised that you were unable to reproduce the problem.
>
I'm sure it happened, and probably had a performance
penalty on my systems too, but not as much as yours.
I didn't see any obvious performance difference running
with the patch, though; not sure why. I'll instrument to
see how often it's happening, I think.
But fixed now, good catch!
+-DLS
^ permalink raw reply
* Re: [PATCHv7] add mergeable buffers support to vhost_net
From: Michael S. Tsirkin @ 2010-05-10 17:31 UTC (permalink / raw)
To: David L Stevens; +Cc: netdev, kvm, virtualization
In-Reply-To: <1272488232.11307.4.camel@w-dls.beaverton.ibm.com>
On Wed, Apr 28, 2010 at 01:57:12PM -0700, David L Stevens wrote:
> @@ -218,18 +248,19 @@ static void handle_rx(struct vhost_net *
> use_mm(net->dev.mm);
> mutex_lock(&vq->mutex);
> vhost_disable_notify(vq);
> - hdr_size = vq->hdr_size;
> + vhost_hlen = vq->vhost_hlen;
>
> vq_log = unlikely(vhost_has_feature(&net->dev, VHOST_F_LOG_ALL)) ?
> vq->log : NULL;
>
> - for (;;) {
> - head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
> - ARRAY_SIZE(vq->iov),
> - &out, &in,
> - vq_log, &log);
> + while ((datalen = vhost_head_len(vq, sock->sk))) {
> + headcount = vhost_get_desc_n(vq, vq->heads,
> + datalen + vhost_hlen,
> + &in, vq_log, &log);
> + if (headcount < 0)
> + break;
> /* OK, now we need to know about added descriptors. */
> - if (head == vq->num) {
> + if (!headcount) {
> if (unlikely(vhost_enable_notify(vq))) {
> /* They have slipped one in as we were
> * doing that: check again. */
So I think this breaks handling for a failure mode where
we get an skb that is larger than the max packet guest
can get. The right thing to do in this case is to
drop the skb, we currently do this by passing
truncate flag to recvmsg.
In particular, with mergeable buffers off, if we get an skb
that does not fit in a single packet, this code will
spread it over multiple buffers.
You should be able to reproduce this fairly easily
by disabling both indirect buffers and mergeable buffers
on qemu command line. With current code TCP still
works by falling back on small packets. I think
with your code it will get stuck forever once
we get an skb that is too large for us to handle.
--
MST
^ permalink raw reply
* Re: [PATCHv7] add mergeable buffers support to vhost_net
From: Michael S. Tsirkin @ 2010-05-10 17:25 UTC (permalink / raw)
To: David Stevens; +Cc: kvm, kvm-owner, netdev, virtualization
In-Reply-To: <OF1F65A110.7C63CDB3-ON8825771F.005DD80E-8825771F.005E3509@us.ibm.com>
On Mon, May 10, 2010 at 10:09:03AM -0700, David Stevens wrote:
> Since "datalen" carries the difference and will be negative by that amount
> from the original loop, what about just adding something like:
>
> }
> if (headcount)
> heads[headcount-1].len += datalen;
> [and really, headcount >0 since datalen > 0, so just:
>
> heads[headcount-1].len += datalen;
>
> +-DLS
This works too, just does more checks and comparisons.
I am still surprised that you were unable to reproduce the problem.
>
> kvm-owner@vger.kernel.org wrote on 05/10/2010 09:43:03 AM:
>
> > On Wed, Apr 28, 2010 at 01:57:12PM -0700, David L Stevens wrote:
> > > @@ -218,18 +248,19 @@ static void handle_rx(struct vhost_net *
> > > use_mm(net->dev.mm);
> > > mutex_lock(&vq->mutex);
> > > vhost_disable_notify(vq);
> > > - hdr_size = vq->hdr_size;
> > > + vhost_hlen = vq->vhost_hlen;
> > >
> > > vq_log = unlikely(vhost_has_feature(&net->dev, VHOST_F_LOG_ALL)) ?
> > > vq->log : NULL;
> > >
> > > - for (;;) {
> > > - head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
> > > - ARRAY_SIZE(vq->iov),
> > > - &out, &in,
> > > - vq_log, &log);
> > > + while ((datalen = vhost_head_len(vq, sock->sk))) {
> > > + headcount = vhost_get_desc_n(vq, vq->heads,
> > > + datalen + vhost_hlen,
> > > + &in, vq_log, &log);
> > > + if (headcount < 0)
> > > + break;
> > > /* OK, now we need to know about added descriptors. */
> > > - if (head == vq->num) {
> > > + if (!headcount) {
> > > if (unlikely(vhost_enable_notify(vq))) {
> > > /* They have slipped one in as we were
> > > * doing that: check again. */
> > > @@ -241,46 +272,53 @@ static void handle_rx(struct vhost_net *
> > > break;
> > > }
> > > /* We don't need to be notified again. */
> > > - if (out) {
> > > - vq_err(vq, "Unexpected descriptor format for RX: "
> > > - "out %d, int %d\n",
> > > - out, in);
> > > - break;
> > > - }
> > > - /* Skip header. TODO: support TSO/mergeable rx buffers. */
> > > - s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, in);
> > > + if (vhost_hlen)
> > > + /* Skip header. TODO: support TSO. */
> > > + s = move_iovec_hdr(vq->iov, vq->hdr, vhost_hlen, in);
> > > + else
> > > + s = copy_iovec_hdr(vq->iov, vq->hdr, vq->sock_hlen, in);
> > > msg.msg_iovlen = in;
> > > len = iov_length(vq->iov, in);
> > > /* Sanity check */
> > > if (!len) {
> > > vq_err(vq, "Unexpected header len for RX: "
> > > "%zd expected %zd\n",
> > > - iov_length(vq->hdr, s), hdr_size);
> > > + iov_length(vq->hdr, s), vhost_hlen);
> > > break;
> > > }
> > > err = sock->ops->recvmsg(NULL, sock, &msg,
> > > len, MSG_DONTWAIT | MSG_TRUNC);
> > > /* TODO: Check specific error and bomb out unless EAGAIN? */
> > > if (err < 0) {
> > > - vhost_discard_vq_desc(vq);
> > > + vhost_discard_desc(vq, headcount);
> > > break;
> > > }
> > > - /* TODO: Should check and handle checksum. */
> > > - if (err > len) {
> > > - pr_err("Discarded truncated rx packet: "
> > > - " len %d > %zd\n", err, len);
> > > - vhost_discard_vq_desc(vq);
> > > + if (err != datalen) {
> > > + pr_err("Discarded rx packet: "
> > > + " len %d, expected %zd\n", err, datalen);
> > > + vhost_discard_desc(vq, headcount);
> > > continue;
> > > }
> > > len = err;
> > > - err = memcpy_toiovec(vq->hdr, (unsigned char *)&hdr, hdr_size);
> > > - if (err) {
> > > - vq_err(vq, "Unable to write vnet_hdr at addr %p: %d\n",
> > > - vq->iov->iov_base, err);
> > > + if (vhost_hlen &&
> > > + memcpy_toiovecend(vq->hdr, (unsigned char *)&hdr, 0,
> > > + vhost_hlen)) {
> > > + vq_err(vq, "Unable to write vnet_hdr at addr %p\n",
> > > + vq->iov->iov_base);
> > > break;
> > > }
> > > - len += hdr_size;
> > > - vhost_add_used_and_signal(&net->dev, vq, head, len);
> > > + /* TODO: Should check and handle checksum. */
> > > + if (vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF) &&
> > > + memcpy_toiovecend(vq->hdr, (unsigned char *)&headcount,
> > > + offsetof(typeof(hdr), num_buffers),
> > > + sizeof(hdr.num_buffers))) {
> > > + vq_err(vq, "Failed num_buffers write");
> > > + vhost_discard_desc(vq, headcount);
> > > + break;
> > > + }
> > > + len += vhost_hlen;
> > > + vhost_add_used_and_signal_n(&net->dev, vq, vq->heads,
> > > + headcount);
> > > if (unlikely(vq_log))
> > > vhost_log_write(vq, vq_log, log, len);
> > > total_len += len;
> >
> > OK I think I see the bug here: vhost_add_used_and_signal_n
> > does not get the actual length, it gets the iovec length from vhost.
> > Guest virtio uses this as packet length, with bad results.
> >
> > So I have applied the follows and it seems to have fixed the problem:
> >
> > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > index c16db02..9d7496d 100644
> > --- a/drivers/vhost/net.c
> > +++ b/drivers/vhost/net.c
> > @@ -219,7 +219,7 @@ static int peek_head_len(struct vhost_virtqueue *vq,
>
> > struct sock *sk)
> > /* This is a multi-buffer version of vhost_get_desc, that works if
> > * vq has read descriptors only.
> > * @vq - the relevant virtqueue
> > - * @datalen - data length we'll be reading
> > + * @datalen - data length we'll be reading. must be > 0
> > * @iovcount - returned count of io vectors we fill
> > * @log - vhost log
> > * @log_num - log offset
> > @@ -236,9 +236,10 @@ static int get_rx_bufs(struct vhost_virtqueue *vq,
> > int seg = 0;
> > int headcount = 0;
> > unsigned d;
> > + size_t len;
> > int r, nlogs = 0;
> >
> > - while (datalen > 0) {
> > + for (;;) {
> > if (unlikely(headcount >= VHOST_NET_MAX_SG)) {
> > r = -ENOBUFS;
> > goto err;
> > @@ -260,16 +261,20 @@ static int get_rx_bufs(struct vhost_virtqueue *vq,
> > nlogs += *log_num;
> > log += *log_num;
> > }
> > + len = iov_length(vq->iov + seg, in);
> > + seg += in;
> > heads[headcount].id = d;
> > - heads[headcount].len = iov_length(vq->iov + seg, in);
> > - datalen -= heads[headcount].len;
> > + if (datalen <= len)
> > + break;
> > + heads[headcount].len = len;
> > ++headcount;
> > - seg += in;
> > + datalen -= len;
> > }
> > + heads[headcount].len = datalen;
> > *iovcount = seg;
> > if (unlikely(log))
> > *log_num = nlogs;
> > - return headcount;
> > + return headcount + 1;
> > err:
> > vhost_discard_desc(vq, headcount);
> > return r;
> >
> > --
> > MST
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: Patch to auto-load MSFT PV NIC driver
From: Greg KH @ 2010-05-10 17:21 UTC (permalink / raw)
To: Hank Janssen
Cc: devel@driverdev.osuosl.org, Virtualization@lists.osdl.org,
Ky Srinivasan, Haiyang Zhang
In-Reply-To: <8AFC7968D54FB448A30D8F38F259C5621AFFC507@TK5EX14MBXC114.redmond.corp.microsoft.com>
On Mon, May 10, 2010 at 05:05:02PM +0000, Hank Janssen wrote:
>
> >Sent: Saturday, May 08, 2010 7:27 AM - Hank Janssen
> >>On Sat, May 08, 2010 at 01:52:01PM +0000, Hank Janssen wrote:>
> >>
> >> I am not sure if this is the right approach. hv_netvsc takes a dependency on hv_vmbus.
> >> hv_vmbus does have the same DMI detection logic in it. But unless hv_vmbus has loaded
> >> up competely, hv_netvsc will fail on loadup. And I do not think we can guarantee that
> >> hv_vmbus has loaded yet.
> >
> >Yes you can, the dependancies in the module will take care of it. Try
> >it, if you try to load the hv_netvsc module before hv_vmbus, modprobe
> >will load hv_vmbus first.
>
> In my testing I have seen issues with timing. It takes a little while for VMBus to start
> getting the initialization taken care of with Hyper-V. And I have seen netvsc error with
> unresolved symbols because vmbus had not completed the initialization yet.
Odd, that shouldn't happen, unless you are loading modules in parallel.
If we switch to a proper discovery type bus, this should not be an
issue.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCHv7] add mergeable buffers support to vhost_net
From: David Stevens @ 2010-05-10 17:09 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, kvm-owner, netdev, virtualization
In-Reply-To: <20100510164303.GA28798@redhat.com>
Since "datalen" carries the difference and will be negative by that amount
from the original loop, what about just adding something like:
}
if (headcount)
heads[headcount-1].len += datalen;
[and really, headcount >0 since datalen > 0, so just:
heads[headcount-1].len += datalen;
+-DLS
kvm-owner@vger.kernel.org wrote on 05/10/2010 09:43:03 AM:
> On Wed, Apr 28, 2010 at 01:57:12PM -0700, David L Stevens wrote:
> > @@ -218,18 +248,19 @@ static void handle_rx(struct vhost_net *
> > use_mm(net->dev.mm);
> > mutex_lock(&vq->mutex);
> > vhost_disable_notify(vq);
> > - hdr_size = vq->hdr_size;
> > + vhost_hlen = vq->vhost_hlen;
> >
> > vq_log = unlikely(vhost_has_feature(&net->dev, VHOST_F_LOG_ALL)) ?
> > vq->log : NULL;
> >
> > - for (;;) {
> > - head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
> > - ARRAY_SIZE(vq->iov),
> > - &out, &in,
> > - vq_log, &log);
> > + while ((datalen = vhost_head_len(vq, sock->sk))) {
> > + headcount = vhost_get_desc_n(vq, vq->heads,
> > + datalen + vhost_hlen,
> > + &in, vq_log, &log);
> > + if (headcount < 0)
> > + break;
> > /* OK, now we need to know about added descriptors. */
> > - if (head == vq->num) {
> > + if (!headcount) {
> > if (unlikely(vhost_enable_notify(vq))) {
> > /* They have slipped one in as we were
> > * doing that: check again. */
> > @@ -241,46 +272,53 @@ static void handle_rx(struct vhost_net *
> > break;
> > }
> > /* We don't need to be notified again. */
> > - if (out) {
> > - vq_err(vq, "Unexpected descriptor format for RX: "
> > - "out %d, int %d\n",
> > - out, in);
> > - break;
> > - }
> > - /* Skip header. TODO: support TSO/mergeable rx buffers. */
> > - s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, in);
> > + if (vhost_hlen)
> > + /* Skip header. TODO: support TSO. */
> > + s = move_iovec_hdr(vq->iov, vq->hdr, vhost_hlen, in);
> > + else
> > + s = copy_iovec_hdr(vq->iov, vq->hdr, vq->sock_hlen, in);
> > msg.msg_iovlen = in;
> > len = iov_length(vq->iov, in);
> > /* Sanity check */
> > if (!len) {
> > vq_err(vq, "Unexpected header len for RX: "
> > "%zd expected %zd\n",
> > - iov_length(vq->hdr, s), hdr_size);
> > + iov_length(vq->hdr, s), vhost_hlen);
> > break;
> > }
> > err = sock->ops->recvmsg(NULL, sock, &msg,
> > len, MSG_DONTWAIT | MSG_TRUNC);
> > /* TODO: Check specific error and bomb out unless EAGAIN? */
> > if (err < 0) {
> > - vhost_discard_vq_desc(vq);
> > + vhost_discard_desc(vq, headcount);
> > break;
> > }
> > - /* TODO: Should check and handle checksum. */
> > - if (err > len) {
> > - pr_err("Discarded truncated rx packet: "
> > - " len %d > %zd\n", err, len);
> > - vhost_discard_vq_desc(vq);
> > + if (err != datalen) {
> > + pr_err("Discarded rx packet: "
> > + " len %d, expected %zd\n", err, datalen);
> > + vhost_discard_desc(vq, headcount);
> > continue;
> > }
> > len = err;
> > - err = memcpy_toiovec(vq->hdr, (unsigned char *)&hdr, hdr_size);
> > - if (err) {
> > - vq_err(vq, "Unable to write vnet_hdr at addr %p: %d\n",
> > - vq->iov->iov_base, err);
> > + if (vhost_hlen &&
> > + memcpy_toiovecend(vq->hdr, (unsigned char *)&hdr, 0,
> > + vhost_hlen)) {
> > + vq_err(vq, "Unable to write vnet_hdr at addr %p\n",
> > + vq->iov->iov_base);
> > break;
> > }
> > - len += hdr_size;
> > - vhost_add_used_and_signal(&net->dev, vq, head, len);
> > + /* TODO: Should check and handle checksum. */
> > + if (vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF) &&
> > + memcpy_toiovecend(vq->hdr, (unsigned char *)&headcount,
> > + offsetof(typeof(hdr), num_buffers),
> > + sizeof(hdr.num_buffers))) {
> > + vq_err(vq, "Failed num_buffers write");
> > + vhost_discard_desc(vq, headcount);
> > + break;
> > + }
> > + len += vhost_hlen;
> > + vhost_add_used_and_signal_n(&net->dev, vq, vq->heads,
> > + headcount);
> > if (unlikely(vq_log))
> > vhost_log_write(vq, vq_log, log, len);
> > total_len += len;
>
> OK I think I see the bug here: vhost_add_used_and_signal_n
> does not get the actual length, it gets the iovec length from vhost.
> Guest virtio uses this as packet length, with bad results.
>
> So I have applied the follows and it seems to have fixed the problem:
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index c16db02..9d7496d 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -219,7 +219,7 @@ static int peek_head_len(struct vhost_virtqueue *vq,
> struct sock *sk)
> /* This is a multi-buffer version of vhost_get_desc, that works if
> * vq has read descriptors only.
> * @vq - the relevant virtqueue
> - * @datalen - data length we'll be reading
> + * @datalen - data length we'll be reading. must be > 0
> * @iovcount - returned count of io vectors we fill
> * @log - vhost log
> * @log_num - log offset
> @@ -236,9 +236,10 @@ static int get_rx_bufs(struct vhost_virtqueue *vq,
> int seg = 0;
> int headcount = 0;
> unsigned d;
> + size_t len;
> int r, nlogs = 0;
>
> - while (datalen > 0) {
> + for (;;) {
> if (unlikely(headcount >= VHOST_NET_MAX_SG)) {
> r = -ENOBUFS;
> goto err;
> @@ -260,16 +261,20 @@ static int get_rx_bufs(struct vhost_virtqueue *vq,
> nlogs += *log_num;
> log += *log_num;
> }
> + len = iov_length(vq->iov + seg, in);
> + seg += in;
> heads[headcount].id = d;
> - heads[headcount].len = iov_length(vq->iov + seg, in);
> - datalen -= heads[headcount].len;
> + if (datalen <= len)
> + break;
> + heads[headcount].len = len;
> ++headcount;
> - seg += in;
> + datalen -= len;
> }
> + heads[headcount].len = datalen;
> *iovcount = seg;
> if (unlikely(log))
> *log_num = nlogs;
> - return headcount;
> + return headcount + 1;
> err:
> vhost_discard_desc(vq, headcount);
> return r;
>
> --
> MST
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* RE: Patch to auto-load MSFT PV NIC driver
From: Hank Janssen @ 2010-05-10 17:05 UTC (permalink / raw)
To: Greg KH
Cc: devel@driverdev.osuosl.org, Virtualization@lists.osdl.org,
Ky Srinivasan, Haiyang Zhang
In-Reply-To: <20100508142716.GA32133@kroah.com>
>Sent: Saturday, May 08, 2010 7:27 AM - Hank Janssen
>>On Sat, May 08, 2010 at 01:52:01PM +0000, Hank Janssen wrote:>
>>
>> I am not sure if this is the right approach. hv_netvsc takes a dependency on hv_vmbus.
>> hv_vmbus does have the same DMI detection logic in it. But unless hv_vmbus has loaded
>> up competely, hv_netvsc will fail on loadup. And I do not think we can guarantee that
>> hv_vmbus has loaded yet.
>
>Yes you can, the dependancies in the module will take care of it. Try
>it, if you try to load the hv_netvsc module before hv_vmbus, modprobe
>will load hv_vmbus first.
In my testing I have seen issues with timing. It takes a little while for VMBus to start
getting the initialization taken care of with Hyper-V. And I have seen netvsc error with
unresolved symbols because vmbus had not completed the initialization yet.
Thanks,
Hank.
^ permalink raw reply
* Re: [PATCHv7] add mergeable buffers support to vhost_net
From: Michael S. Tsirkin @ 2010-05-10 16:43 UTC (permalink / raw)
To: David L Stevens; +Cc: netdev, kvm, virtualization
In-Reply-To: <1272488232.11307.4.camel@w-dls.beaverton.ibm.com>
On Wed, Apr 28, 2010 at 01:57:12PM -0700, David L Stevens wrote:
> @@ -218,18 +248,19 @@ static void handle_rx(struct vhost_net *
> use_mm(net->dev.mm);
> mutex_lock(&vq->mutex);
> vhost_disable_notify(vq);
> - hdr_size = vq->hdr_size;
> + vhost_hlen = vq->vhost_hlen;
>
> vq_log = unlikely(vhost_has_feature(&net->dev, VHOST_F_LOG_ALL)) ?
> vq->log : NULL;
>
> - for (;;) {
> - head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
> - ARRAY_SIZE(vq->iov),
> - &out, &in,
> - vq_log, &log);
> + while ((datalen = vhost_head_len(vq, sock->sk))) {
> + headcount = vhost_get_desc_n(vq, vq->heads,
> + datalen + vhost_hlen,
> + &in, vq_log, &log);
> + if (headcount < 0)
> + break;
> /* OK, now we need to know about added descriptors. */
> - if (head == vq->num) {
> + if (!headcount) {
> if (unlikely(vhost_enable_notify(vq))) {
> /* They have slipped one in as we were
> * doing that: check again. */
> @@ -241,46 +272,53 @@ static void handle_rx(struct vhost_net *
> break;
> }
> /* We don't need to be notified again. */
> - if (out) {
> - vq_err(vq, "Unexpected descriptor format for RX: "
> - "out %d, int %d\n",
> - out, in);
> - break;
> - }
> - /* Skip header. TODO: support TSO/mergeable rx buffers. */
> - s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, in);
> + if (vhost_hlen)
> + /* Skip header. TODO: support TSO. */
> + s = move_iovec_hdr(vq->iov, vq->hdr, vhost_hlen, in);
> + else
> + s = copy_iovec_hdr(vq->iov, vq->hdr, vq->sock_hlen, in);
> msg.msg_iovlen = in;
> len = iov_length(vq->iov, in);
> /* Sanity check */
> if (!len) {
> vq_err(vq, "Unexpected header len for RX: "
> "%zd expected %zd\n",
> - iov_length(vq->hdr, s), hdr_size);
> + iov_length(vq->hdr, s), vhost_hlen);
> break;
> }
> err = sock->ops->recvmsg(NULL, sock, &msg,
> len, MSG_DONTWAIT | MSG_TRUNC);
> /* TODO: Check specific error and bomb out unless EAGAIN? */
> if (err < 0) {
> - vhost_discard_vq_desc(vq);
> + vhost_discard_desc(vq, headcount);
> break;
> }
> - /* TODO: Should check and handle checksum. */
> - if (err > len) {
> - pr_err("Discarded truncated rx packet: "
> - " len %d > %zd\n", err, len);
> - vhost_discard_vq_desc(vq);
> + if (err != datalen) {
> + pr_err("Discarded rx packet: "
> + " len %d, expected %zd\n", err, datalen);
> + vhost_discard_desc(vq, headcount);
> continue;
> }
> len = err;
> - err = memcpy_toiovec(vq->hdr, (unsigned char *)&hdr, hdr_size);
> - if (err) {
> - vq_err(vq, "Unable to write vnet_hdr at addr %p: %d\n",
> - vq->iov->iov_base, err);
> + if (vhost_hlen &&
> + memcpy_toiovecend(vq->hdr, (unsigned char *)&hdr, 0,
> + vhost_hlen)) {
> + vq_err(vq, "Unable to write vnet_hdr at addr %p\n",
> + vq->iov->iov_base);
> break;
> }
> - len += hdr_size;
> - vhost_add_used_and_signal(&net->dev, vq, head, len);
> + /* TODO: Should check and handle checksum. */
> + if (vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF) &&
> + memcpy_toiovecend(vq->hdr, (unsigned char *)&headcount,
> + offsetof(typeof(hdr), num_buffers),
> + sizeof(hdr.num_buffers))) {
> + vq_err(vq, "Failed num_buffers write");
> + vhost_discard_desc(vq, headcount);
> + break;
> + }
> + len += vhost_hlen;
> + vhost_add_used_and_signal_n(&net->dev, vq, vq->heads,
> + headcount);
> if (unlikely(vq_log))
> vhost_log_write(vq, vq_log, log, len);
> total_len += len;
OK I think I see the bug here: vhost_add_used_and_signal_n
does not get the actual length, it gets the iovec length from vhost.
Guest virtio uses this as packet length, with bad results.
So I have applied the follows and it seems to have fixed the problem:
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index c16db02..9d7496d 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -219,7 +219,7 @@ static int peek_head_len(struct vhost_virtqueue *vq, struct sock *sk)
/* This is a multi-buffer version of vhost_get_desc, that works if
* vq has read descriptors only.
* @vq - the relevant virtqueue
- * @datalen - data length we'll be reading
+ * @datalen - data length we'll be reading. must be > 0
* @iovcount - returned count of io vectors we fill
* @log - vhost log
* @log_num - log offset
@@ -236,9 +236,10 @@ static int get_rx_bufs(struct vhost_virtqueue *vq,
int seg = 0;
int headcount = 0;
unsigned d;
+ size_t len;
int r, nlogs = 0;
- while (datalen > 0) {
+ for (;;) {
if (unlikely(headcount >= VHOST_NET_MAX_SG)) {
r = -ENOBUFS;
goto err;
@@ -260,16 +261,20 @@ static int get_rx_bufs(struct vhost_virtqueue *vq,
nlogs += *log_num;
log += *log_num;
}
+ len = iov_length(vq->iov + seg, in);
+ seg += in;
heads[headcount].id = d;
- heads[headcount].len = iov_length(vq->iov + seg, in);
- datalen -= heads[headcount].len;
+ if (datalen <= len)
+ break;
+ heads[headcount].len = len;
++headcount;
- seg += in;
+ datalen -= len;
}
+ heads[headcount].len = datalen;
*iovcount = seg;
if (unlikely(log))
*log_num = nlogs;
- return headcount;
+ return headcount + 1;
err:
vhost_discard_desc(vq, headcount);
return r;
--
MST
^ permalink raw reply related
* Re: Patch to auto-load MSFT PV NIC driver
From: Greg KH @ 2010-05-10 15:49 UTC (permalink / raw)
To: Jeremy Fitzhardinge
Cc: devel@driverdev.osuosl.org, Virtualization@lists.osdl.org,
Ky Srinivasan, Haiyang Zhang
In-Reply-To: <4BE5ED79.9080008@goop.org>
On Sat, May 08, 2010 at 04:02:17PM -0700, Jeremy Fitzhardinge wrote:
> On 05/08/2010 07:27 AM, Greg KH wrote:
> > How would you do that? The symbol resolution will not allow hv_netvsc
> > to be loaded without hv_vmbus being around.
> >
> > Now I do think that just adding the same pci/dmi ids to all of the
> > modules is a bit odd, and that something else needs to be done here, but
> > at the moment, I can't think of anything better.
> >
>
> Could you make hv_vmbus a real bus implementation, which knows how to
> enumerate and probe devices hanging off it?
Yes, that's the correct solution here. I originally thought this
couldn't be done, but in looking at the code some more, I think it can
happen for the network driver, and perhaps some others (but not the
block, as both devices work off of the same interface to the hypervisor,
so which one do we load?)
I'll look into that later if I get the chance.
thanks,
greg k-h
^ permalink raw reply
* Re: virtio: put last_used and last_avail index into ring itself.
From: Rusty Russell @ 2010-05-10 3:11 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Eric Dumazet, kvm, netdev, linux-kernel, virtualization, linux-mm,
s.hetze, hpa, Daniel Walker, mingo, akpm
In-Reply-To: <20100509085733.GD16775@redhat.com>
On Sun, 9 May 2010 06:27:33 pm Michael S. Tsirkin wrote:
> On Fri, May 07, 2010 at 12:35:39PM +0930, Rusty Russell wrote:
> > Then there's padding to page boundary. That puts us on a cacheline again
> > for the used ring; also 2 bytes per entry.
> >
>
> Hmm, is used ring really 2 bytes per entry?
Err, no, I am an idiot.
> /* u32 is used here for ids for padding reasons. */
> struct vring_used_elem {
> /* Index of start of used descriptor chain. */
> __u32 id;
> /* Total length of the descriptor chain which was used (written to) */
> __u32 len;
> };
>
> struct vring_used {
> __u16 flags;
> __u16 idx;
> struct vring_used_elem ring[];
> };
OK, now I get it. Sorry, I was focussed on the avail ring.
> I thought that used ring has 8 bytes per entry, and that struct
> vring_used is aligned at page boundary, this
> would mean that ring element is at offset 4 bytes from page boundary.
> Thus with cacheline size 128 bytes, each 4th element crosses
> a cacheline boundary. If we had a 4 byte padding after idx, each
> used element would always be completely within a single cacheline.
I think the numbers are: every 16th entry hits two cachelines. So currently
the first 15 entries are "free" (assuming we hit the idx cacheline anyway),
then 1 in 16 cost 2 cachelines. That makes the aligned version win when
N > 240.
But, we access the array linearly. So the extra cacheline cost is in fact
amortized. I doubt it could be measured, but maybe vring_get_buf() should
prefetch? While you're there, we could use an & rather than a mod on the
calculation, which may actually be measurable :)
Cheers,
Rusty.
^ 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