* Re: [Pv-drivers] [PATCH 08/12] VMCI: resource object implementation.
From: Greg KH @ 2012-10-30 15:51 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: pv-drivers, linux-kernel, George Zhang, virtualization
In-Reply-To: <20121030052016.GF32055@dtor-ws.eng.vmware.com>
On Mon, Oct 29, 2012 at 10:20:16PM -0700, Dmitry Torokhov wrote:
> On Mon, Oct 29, 2012 at 07:29:05PM -0700, Greg KH wrote:
> > On Mon, Oct 29, 2012 at 06:04:58PM -0700, George Zhang wrote:
> > > +static struct vmci_resource *vmci_resource_lookup(struct vmci_handle handle)
> > > +{
> > > + struct vmci_resource *r, *resource = NULL;
> > > + struct hlist_node *node;
> > > + unsigned int idx = vmci_resource_hash(handle);
> > > +
> > > + BUG_ON(VMCI_HANDLE_EQUAL(handle, VMCI_INVALID_HANDLE));
> >
> > You just crashed a machine, with no chance for recovery. Not a good
> > idea. Never a good idea. Customers just lost data, and now they are
> > mad. Make sure you at least print out your email address so they know
> > who to blame :)
> >
> > Seriously, never BUG() in a driver, warn, sure, but this just looks like
> > a debugging assert(). Please remove all of these, they are sprinkled
> > all over the driver code here, I'm only responding to one of them here.
> >
> > Even better yet, properly handle the error and keep on going, that's
> > what the rest of the kernel does. Or should :)
>
> For public APIs it certainly makes sense to check and handle erroneous input;
It's not "public", it's an in-kernel api. See the static up there? :)
> internally it often makes sense to simply enforce invariants, because if
> we managed to get into that state that we consider impossible we can't really
> trust anything.
Then error out, don't crash the box. Again, this really looks like an
ASSERT() you are trying to catch, which you know how well we like those
in kernel code...
> FWIW:
> [dtor@dtor-ws kernel]$ grep -r BUG_ON . | wc -l
> 11269
I'm not saying that those are acceptable either, I just don't want to
add any more to the kernel.
greg k-h
^ permalink raw reply
* Re: [PATCH net-next 8/8] vhost-net: reduce vq polling on tx zerocopy
From: Michael S. Tsirkin @ 2012-10-30 15:54 UTC (permalink / raw)
To: Vlad Yasevich
Cc: Alexander Duyck, Ian Campbell, kvm, netdev, linux-kernel,
virtualization, Eric Dumazet, Andrew Morton, David S. Miller
In-Reply-To: <508FF6A1.5060201@redhat.com>
On Tue, Oct 30, 2012 at 11:47:45AM -0400, Vlad Yasevich wrote:
> On 10/29/2012 11:49 AM, Michael S. Tsirkin wrote:
> >It seems that to avoid deadlocks it is enough to poll vq before
> > we are going to use the last buffer. This should be faster than
> >c70aa540c7a9f67add11ad3161096fb95233aa2e.
> >
> >Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >---
> > drivers/vhost/net.c | 12 ++++++++++--
> > 1 file changed, 10 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> >index 8e9de79..3967f82 100644
> >--- a/drivers/vhost/net.c
> >+++ b/drivers/vhost/net.c
> >@@ -197,8 +197,16 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, int status)
> > {
> > struct vhost_ubuf_ref *ubufs = ubuf->ctx;
> > struct vhost_virtqueue *vq = ubufs->vq;
> >-
> >- vhost_poll_queue(&vq->poll);
> >+ int cnt = atomic_read(&ubufs->kref.refcount);
> >+
> >+ /*
> >+ * Trigger polling thread if guest stopped submitting new buffers:
> >+ * in this case, the refcount after decrement will eventually reach 1
> >+ * so here it is 2.
> >+ * We also trigger polling periodically after each 16 packets.
> >+ */
> >+ if (cnt <= 2 || !(cnt % 16))
>
> Why 16? Does it make sense to make it configurable?
>
> -vlad
Probably not but I'll add a comment explaining why.
> >+ vhost_poll_queue(&vq->poll);
> > /* set len to mark this desc buffers done DMA */
> > vq->heads[ubuf->desc].len = status ?
> > VHOST_DMA_FAILED_LEN : VHOST_DMA_DONE_LEN;
> >
^ permalink raw reply
* Re: [PATCH net-next 2/8] skb: api to report errors for zero copy skbs
From: Michael S. Tsirkin @ 2012-10-30 15:54 UTC (permalink / raw)
To: Vlad Yasevich
Cc: Alexander Duyck, Ian Campbell, kvm, netdev, linux-kernel,
virtualization, Eric Dumazet, Andrew Morton, David S. Miller
In-Reply-To: <508FF5D0.6080107@redhat.com>
On Tue, Oct 30, 2012 at 11:44:16AM -0400, Vlad Yasevich wrote:
> On 10/29/2012 11:49 AM, Michael S. Tsirkin wrote:
> >Orphaning frags for zero copy skbs needs to allocate data in atomic
> >context so is has a chance to fail. If it does we currently discard
> >the skb which is safe, but we don't report anything to the caller,
> >so it can not recover by e.g. disabling zero copy.
> >
> >Add an API to free skb reporting such errors: this is used
> >by tun in case orphaning frags fails.
> >
> >Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >---
> > include/linux/skbuff.h | 1 +
> > net/core/skbuff.c | 19 +++++++++++++++++++
> > 2 files changed, 20 insertions(+)
> >
> >diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> >index 8bac11b..0644432 100644
> >--- a/include/linux/skbuff.h
> >+++ b/include/linux/skbuff.h
> >@@ -568,6 +568,7 @@ static inline struct rtable *skb_rtable(const struct sk_buff *skb)
> > }
> >
> > extern void kfree_skb(struct sk_buff *skb);
> >+extern void skb_tx_error(struct sk_buff *skb, int err);
> > extern void consume_skb(struct sk_buff *skb);
> > extern void __kfree_skb(struct sk_buff *skb);
> > extern struct kmem_cache *skbuff_head_cache;
> >diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> >index eb31f6e..ad99c64 100644
> >--- a/net/core/skbuff.c
> >+++ b/net/core/skbuff.c
> >@@ -635,6 +635,25 @@ void kfree_skb(struct sk_buff *skb)
> > EXPORT_SYMBOL(kfree_skb);
> >
> > /**
> >+ * kfree_skb_on_error - report an sk_buff xmit error
> >+ * @skb: buffer that triggered an error
> >+ *
> >+ * Report xmit error if a device callback is tracking this skb.
> >+ */
>
> Nit: Comment doesn't match new function.
>
> -vlad
Good catch, thanks.
> >+void skb_tx_error(struct sk_buff *skb, int err)
> >+{
> >+ if (skb_shinfo(skb)->tx_flags & SKBTX_DEV_ZEROCOPY) {
> >+ struct ubuf_info *uarg;
> >+
> >+ uarg = skb_shinfo(skb)->destructor_arg;
> >+ if (uarg->callback)
> >+ uarg->callback(uarg, err);
> >+ skb_shinfo(skb)->tx_flags &= ~SKBTX_DEV_ZEROCOPY;
> >+ }
> >+}
> >+EXPORT_SYMBOL(skb_tx_error);
> >+
> >+/**
> > * consume_skb - free an skbuff
> > * @skb: buffer to free
> > *
> >
^ permalink raw reply
* Re: [Pv-drivers] [PATCH 01/12] VMCI: context implementation.
From: Dmitry Torokhov @ 2012-10-30 15:56 UTC (permalink / raw)
To: Greg KH; +Cc: pv-drivers, linux-kernel, George Zhang, virtualization
In-Reply-To: <20121030154652.GB14167@kroah.com>
On Tuesday, October 30, 2012 08:46:52 AM Greg KH wrote:
> On Mon, Oct 29, 2012 at 09:01:40PM -0700, Dmitry Torokhov wrote:
> > Hi Greg,
> >
> > On Mon, Oct 29, 2012 at 07:10:58PM -0700, Greg KH wrote:
> > > On Mon, Oct 29, 2012 at 06:03:42PM -0700, George Zhang wrote:
> > > > +/*
> > > > + * Releases the VMCI context. If this is the last reference to
> > > > + * the context it will be deallocated. A context is created with
> > > > + * a reference count of one, and on destroy, it is removed from
> > > > + * the context list before its reference count is
> > > > + * decremented. Thus, if we reach zero, we are sure that nobody
> > > > + * else are about to increment it (they need the entry in the
> > > > + * context list for that). This function musn't be called with a
> > > > + * lock held.
> > > > + */
> > > > +void vmci_ctx_release(struct vmci_ctx *context)
> > > > +{
> > > > + ASSERT(context);
> > > > + kref_put(&context->kref, ctx_free_ctx);
> > > > +}
> > > > +
> > >
> > > Hm, are you _sure_ you should be calling this without a lock held?
> > > That's usually kref-101, you MUST hold a lock when calling put,
> > > otherwise you can race a kref_get() call, and all hell can break loose.
> > >
> > > Because of this, some saner people (like Al Viro), have suggested that I
> > > force the kref_put() and kref_get() calls pass in a spinlock just to
> > > enforce this.
> > >
> > > So, tell me what I'm missing here, and why you put the comment here
> > > saying that it really is supposed to be called without a lock held? How
> > > is that safe?
> >
> > Contexts are created/registered in vmci_ctx_init_ctx() and unregistered in
> > vmci_ctx_release_ctx() and these operations are protected by
> > ctx_list.lock spinlock. Context lookup (vmci_ctx_get) also uses spinlock
> > to traverse list of registered contexts and then grabs reference to the
> > [valid] context. The use of kref_put() without additional locking in
> > vmci_ctx_release() is fine as there is no chance of another thread
> > bumping count from 0 to 1.
>
> As I didn't see all callers of this holding that spinlock, it was
> confusing. You should put this type of description somewhere so that
> other reviewers don't have the same questions.
Fair enough, we'll add better comments to this code.
Thanks,
Dmitry
^ permalink raw reply
* Re: [Pv-drivers] [PATCH 05/12] VMCI: event handling implementation.
From: Dmitry Torokhov @ 2012-10-30 15:59 UTC (permalink / raw)
To: Greg KH; +Cc: pv-drivers, linux-kernel, George Zhang, virtualization
In-Reply-To: <20121030155041.GF14167@kroah.com>
On Tuesday, October 30, 2012 08:50:41 AM Greg KH wrote:
> On Mon, Oct 29, 2012 at 10:01:52PM -0700, Dmitry Torokhov wrote:
> > On Mon, Oct 29, 2012 at 07:26:05PM -0700, Greg KH wrote:
> > > On Mon, Oct 29, 2012 at 06:04:27PM -0700, George Zhang wrote:
> > > > +static void event_signal_destroy(struct kref *kref)
> > > > +{
> > > > + struct vmci_subscription *entry =
> > > > + container_of(kref, struct vmci_subscription, kref);
> > > > +
> > > > + complete(&entry->done);
> > > > +}
> > >
> > > Didn't you just leak memory here? What frees the structure up?
> >
> > event_unregister_subscription() waits for that completion and frees the
> > structure. We want event_unregister_subscription() to wait until all
> > fired callbacks completed before unregister is complete.
>
> So all calls to this can just sit and spin waiting for others to clean
> up? Odd, but ok.
Not all as there is logically only one owner of the subscription so
naturally it waits until all notification callbacks are done.
Frankly we have a change that gets rid of delayed ecvent callbacks
and so the refcounting is no longer needed at all.
Thanks,
Dmitry
^ permalink raw reply
* Re: [Pv-drivers] [PATCH 08/12] VMCI: resource object implementation.
From: Dmitry Torokhov @ 2012-10-30 16:11 UTC (permalink / raw)
To: Greg KH; +Cc: pv-drivers, linux-kernel, George Zhang, virtualization
In-Reply-To: <20121030155159.GG14167@kroah.com>
On Tuesday, October 30, 2012 08:51:59 AM Greg KH wrote:
> On Mon, Oct 29, 2012 at 10:20:16PM -0700, Dmitry Torokhov wrote:
> > On Mon, Oct 29, 2012 at 07:29:05PM -0700, Greg KH wrote:
> > > On Mon, Oct 29, 2012 at 06:04:58PM -0700, George Zhang wrote:
> > > > +static struct vmci_resource *vmci_resource_lookup(struct vmci_handle
> > > > handle) +{
> > > > + struct vmci_resource *r, *resource = NULL;
> > > > + struct hlist_node *node;
> > > > + unsigned int idx = vmci_resource_hash(handle);
> > > > +
> > > > + BUG_ON(VMCI_HANDLE_EQUAL(handle, VMCI_INVALID_HANDLE));
> > >
> > > You just crashed a machine, with no chance for recovery. Not a good
> > > idea. Never a good idea. Customers just lost data, and now they are
> > > mad. Make sure you at least print out your email address so they know
> > > who to blame :)
> > >
> > > Seriously, never BUG() in a driver, warn, sure, but this just looks like
> > > a debugging assert(). Please remove all of these, they are sprinkled
> > > all over the driver code here, I'm only responding to one of them here.
> > >
> > > Even better yet, properly handle the error and keep on going, that's
> > > what the rest of the kernel does. Or should :)
> >
> > For public APIs it certainly makes sense to check and handle erroneous
> > input;
> It's not "public", it's an in-kernel api. See the static up there? :)
Yes, exactly, that is not public but internal and that is why it might
be acceptable to enforce invariant. Cross-subsystem (i.e public from
the in-kernel POV) APIs should of course check and refuse bad input.
>
> > internally it often makes sense to simply enforce invariants, because if
> > we managed to get into that state that we consider impossible we can't
> > really trust anything.
>
> Then error out, don't crash the box. Again, this really looks like an
> ASSERT() you are trying to catch, which you know how well we like those
> in kernel code...
At certain point it simply does not make sense to add error handling
paths to handle situation that should be impossible to happen.
>
> > FWIW:
> > [dtor@dtor-ws kernel]$ grep -r BUG_ON . | wc -l
> > 11269
>
> I'm not saying that those are acceptable either, I just don't want to
> add any more to the kernel.
I do not think eradicating BUG_ONs from kernel in general is a good idea.
At some point you should just die with as much information as possible.
That said we'll go and see what could be converted from BUG_ON to WARN_ON
and what could be handled without taking the box down...
Thanks,
Dmitry
^ permalink raw reply
* Re: [Pv-drivers] [PATCH 00/12] VMCI for Linux upstreaming
From: Dmitry Torokhov @ 2012-10-30 16:18 UTC (permalink / raw)
To: Greg KH; +Cc: pv-drivers, linux-kernel, George Zhang, virtualization
In-Reply-To: <20121030154801.GC14167@kroah.com>
On Tue, Oct 30, 2012 at 08:48:01AM -0700, Greg KH wrote:
> On Mon, Oct 29, 2012 at 09:07:44PM -0700, Dmitry Torokhov wrote:
> > Hi Greg,
> >
> > On Mon, Oct 29, 2012 at 07:19:38PM -0700, Greg KH wrote:
> > > On Mon, Oct 29, 2012 at 06:03:28PM -0700, George Zhang wrote:
> > > > drivers/misc/Kconfig | 1
> > > > drivers/misc/Makefile | 2
> > > > drivers/misc/vmw_vmci/Kconfig | 16
> > > > drivers/misc/vmw_vmci/Makefile | 43
> > >
> > > Meta comment here, why drivers/misc/? The other hypervisor
> > > infrastructures all have their own directory under drivers/ Should we
> > > be moving everything to drivers/hyperv/ somehow?
> >
> > drivers/hyperv is not the best name for obvious reasons...
>
> Sorry, yes :)
:)
>
> > I think that even if we had a special directory for vmci having network
> > drivers in Dave's realm and pvscsi in James's is best option, so the new
> > directory would contain vmci and the balloon driver (vsock will go into
> > net/). Given that balloon is already in drivers/misc it looked like
> > obvious place for VMCI as well.
>
> I agree that the individual drivers should go in the subsystem area,
> it's this "hypervisor bus core" type code that I'm questioning. Right
> now every hypervisor is putting that logic in a different place in the
> kernel, having some consistency here would be nice.
Hmm, I wonder if miscellaneous and core hypervisor drivers should end
up in drivers/platform:
drivers/platform/hyperv
drivers/platform/olpc
drivers/platform/vmware
drivers/platform/xen
drivers/platform/x86
But really we'd like to get VMCI into mainline first and move to a new
place later if such a better place is found.
Thanks,
Dmitry
^ permalink raw reply
* Re: [PATCH] Add a page cache-backed balloon device driver.
From: Mike Waychison @ 2012-10-30 16:25 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Frank Swiderski, Andrea Arcangeli, Rik van Riel, kvm,
linux-kernel@vger.kernel.org, virtualization
In-Reply-To: <20121030152925.GA7023@redhat.com>
On Tue, Oct 30, 2012 at 8:29 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Sep 11, 2012 at 12:10:18AM +0300, Michael S. Tsirkin wrote:
>> > On the plus side, having an exit taken here on each page turns out to
>> > be relatively cheap, as the vmexit from the page fault should be
>> > faster to process as it is fully handled within the host kernel.
>> >
>> > Perhaps some combination of both methods is required? I'm not sure :\
>>
>> Perhaps some benchmarking is in order :)
>> Can you try telling host, potentially MADV_WILL_NEED
>> in that case like qemu does, then run your proprietary test
>> and see if things work well enough?
>
> Ping. Had a chance to try that?
Not yet. We've been focused on other things recently and probably
won't have a chance to return to this for a couple months at best.
Sorry :(
^ permalink raw reply
* Re: [Pv-drivers] [PATCH 00/12] VMCI for Linux upstreaming
From: Greg KH @ 2012-10-30 16:27 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: pv-drivers, linux-kernel, George Zhang, virtualization
In-Reply-To: <20121030161807.GA9709@dtor-ws.eng.vmware.com>
On Tue, Oct 30, 2012 at 09:18:07AM -0700, Dmitry Torokhov wrote:
> > > I think that even if we had a special directory for vmci having network
> > > drivers in Dave's realm and pvscsi in James's is best option, so the new
> > > directory would contain vmci and the balloon driver (vsock will go into
> > > net/). Given that balloon is already in drivers/misc it looked like
> > > obvious place for VMCI as well.
> >
> > I agree that the individual drivers should go in the subsystem area,
> > it's this "hypervisor bus core" type code that I'm questioning. Right
> > now every hypervisor is putting that logic in a different place in the
> > kernel, having some consistency here would be nice.
>
> Hmm, I wonder if miscellaneous and core hypervisor drivers should end
> up in drivers/platform:
>
> drivers/platform/hyperv
> drivers/platform/olpc
> drivers/platform/vmware
> drivers/platform/xen
> drivers/platform/x86
That makes sense to me, nice.
> But really we'd like to get VMCI into mainline first and move to a new
> place later if such a better place is found.
Heh, no one wants to fight for something to help everyone out, they just
want their own code accepted :)
greg k-h
^ permalink raw reply
* Re: [rfc net-next v6 0/3] Multiqueue virtio-net
From: Rick Jones @ 2012-10-30 19:05 UTC (permalink / raw)
To: Jason Wang
Cc: krkumar2, kvm, mst, netdev, linux-kernel, virtualization, davem
In-Reply-To: <1351591403-23065-1-git-send-email-jasowang@redhat.com>
On 10/30/2012 03:03 AM, Jason Wang wrote:
> Hi all:
>
> This series is an update version of multiqueue virtio-net driver based on
> Krishna Kumar's work to let virtio-net use multiple rx/tx queues to do the
> packets reception and transmission. Please review and comments.
>
> Changes from v5:
> - Align the implementation with the RFC spec update v4
> - Switch the mode between single mode and multiqueue mode without reset
> - Remove the 256 limitation of queues
> - Use helpers to do the mapping between virtqueues and tx/rx queues
> - Use commbined channels instead of separated rx/tx queus when do the queue
> number configuartion
> - Other coding style comments from Michael
>
> Reference:
> - A protype implementation of qemu-kvm support could by found in
> git://github.com/jasowang/qemu-kvm-mq.git
> - V5 could be found at http://lwn.net/Articles/505388/
> - V4 could be found at https://lkml.org/lkml/2012/6/25/120
> - V2 could be found at http://lwn.net/Articles/467283/
> - Michael virtio-spec: http://www.spinics.net/lists/netdev/msg209986.html
>
> Perf Numbers:
>
> - Pktgen test shows the receiving capability of the multiqueue virtio-net were
> dramatically improved.
> - Netperf result shows latency were greately improved according to the test
> result.
I suppose it is technically correct to say that latency was improved,
but usually for aggregate request/response tests I tend to talk about
the aggregate transactions per second.
Do you have a hypothesis as to why the improvement dropped going to 20
concurrent sessions from 10?
rick jones
> Netperf Local VM to VM test:
> - VM1 and its vcpu/vhost thread in numa node 0
> - VM2 and its vcpu/vhost thread in numa node 1
> - a script is used to lauch the netperf with demo mode and do the postprocessing
> to measure the aggreagte result with the help of timestamp
> - average of 3 runs
>
> TCP_RR:
> size/session/+lat%/+normalize%
> 1/ 1/ 0%/ 0%
> 1/ 10/ +52%/ +6%
> 1/ 20/ +27%/ +5%
> 64/ 1/ 0%/ 0%
> 64/ 10/ +45%/ +4%
> 64/ 20/ +28%/ +7%
> 256/ 1/ -1%/ 0%
> 256/ 10/ +38%/ +2%
> 256/ 20/ +27%/ +6%
> TCP_CRR:
> size/session/+lat%/+normalize%
> 1/ 1/ -7%/ -12%
> 1/ 10/ +34%/ +3%
> 1/ 20/ +3%/ -8%
> 64/ 1/ -7%/ -3%
> 64/ 10/ +32%/ +1%
> 64/ 20/ +4%/ -7%
> 256/ 1/ -6%/ -18%
> 256/ 10/ +33%/ 0%
> 256/ 20/ +4%/ -8%
> STREAM:
> size/session/+thu%/+normalize%
> 1/ 1/ -3%/ 0%
> 1/ 2/ -1%/ 0%
> 1/ 4/ -2%/ 0%
> 64/ 1/ 0%/ +1%
> 64/ 2/ -6%/ -6%
> 64/ 4/ -8%/ -14%
> 256/ 1/ 0%/ 0%
> 256/ 2/ -48%/ -52%
> 256/ 4/ -50%/ -55%
> 512/ 1/ +4%/ +5%
> 512/ 2/ -29%/ -33%
> 512/ 4/ -37%/ -49%
> 1024/ 1/ +6%/ +7%
> 1024/ 2/ -46%/ -51%
> 1024/ 4/ -15%/ -17%
> 4096/ 1/ +1%/ +1%
> 4096/ 2/ +16%/ -2%
> 4096/ 4/ +31%/ -10%
> 16384/ 1/ 0%/ 0%
> 16384/ 2/ +16%/ +9%
> 16384/ 4/ +17%/ -9%
>
> Netperf test between external host and guest over 10gb(ixgbe):
> - VM thread and vhost threads were pinned int the node 0
> - a script is used to lauch the netperf with demo mode and do the postprocessing
> to measure the aggreagte result with the help of timestamp
> - average of 3 runs
>
> TCP_RR:
> size/session/+lat%/+normalize%
> 1/ 1/ 0%/ +6%
> 1/ 10/ +41%/ +2%
> 1/ 20/ +10%/ -3%
> 64/ 1/ 0%/ -10%
> 64/ 10/ +39%/ +1%
> 64/ 20/ +22%/ +2%
> 256/ 1/ 0%/ +2%
> 256/ 10/ +26%/ -17%
> 256/ 20/ +24%/ +10%
> TCP_CRR:
> size/session/+lat%/+normalize%
> 1/ 1/ -3%/ -3%
> 1/ 10/ +34%/ -3%
> 1/ 20/ 0%/ -15%
> 64/ 1/ -3%/ -3%
> 64/ 10/ +34%/ -3%
> 64/ 20/ -1%/ -16%
> 256/ 1/ -1%/ -3%
> 256/ 10/ +38%/ -2%
> 256/ 20/ -2%/ -17%
> TCP_STREAM:(guest receiving)
> size/session/+thu%/+normalize%
> 1/ 1/ +1%/ +14%
> 1/ 2/ 0%/ +4%
> 1/ 4/ -2%/ -24%
> 64/ 1/ -6%/ +1%
> 64/ 2/ +1%/ +1%
> 64/ 4/ -1%/ -11%
> 256/ 1/ +3%/ +4%
> 256/ 2/ 0%/ -1%
> 256/ 4/ 0%/ -15%
> 512/ 1/ +4%/ 0%
> 512/ 2/ -10%/ -12%
> 512/ 4/ 0%/ -11%
> 1024/ 1/ -5%/ 0%
> 1024/ 2/ -11%/ -16%
> 1024/ 4/ +3%/ -11%
> 4096/ 1/ +27%/ +6%
> 4096/ 2/ 0%/ -12%
> 4096/ 4/ 0%/ -20%
> 16384/ 1/ 0%/ -2%
> 16384/ 2/ 0%/ -9%
> 16384/ 4/ +10%/ -2%
> TCP_MAERTS:(guest sending)
> 1/ 1/ -1%/ 0%
> 1/ 2/ 0%/ 0%
> 1/ 4/ -5%/ 0%
> 64/ 1/ 0%/ 0%
> 64/ 2/ -7%/ -8%
> 64/ 4/ -7%/ -8%
> 256/ 1/ 0%/ 0%
> 256/ 2/ -28%/ -28%
> 256/ 4/ -28%/ -29%
> 512/ 1/ 0%/ 0%
> 512/ 2/ -15%/ -13%
> 512/ 4/ -53%/ -59%
> 1024/ 1/ +4%/ +13%
> 1024/ 2/ -7%/ -18%
> 1024/ 4/ +1%/ -18%
> 4096/ 1/ +2%/ 0%
> 4096/ 2/ +3%/ -19%
> 4096/ 4/ -1%/ -19%
> 16384/ 1/ -3%/ -1%
> 16384/ 2/ 0%/ -12%
> 16384/ 4/ 0%/ -10%
>
> Jason Wang (2):
> virtio_net: multiqueue support
> virtio-net: change the number of queues through ethtool
>
> Krishna Kumar (1):
> virtio_net: Introduce VIRTIO_NET_F_MULTIQUEUE
>
> drivers/net/virtio_net.c | 790 ++++++++++++++++++++++++++++-----------
> include/uapi/linux/virtio_net.h | 19 +
> 2 files changed, 594 insertions(+), 215 deletions(-)
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" 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: [Pv-drivers] [PATCH 00/12] VMCI for Linux upstreaming
From: Dmitry Torokhov @ 2012-10-30 19:43 UTC (permalink / raw)
To: Greg KH; +Cc: pv-drivers, linux-kernel, George Zhang, virtualization
In-Reply-To: <20121030162723.GA16135@kroah.com>
On Tue, Oct 30, 2012 at 09:27:23AM -0700, Greg KH wrote:
> On Tue, Oct 30, 2012 at 09:18:07AM -0700, Dmitry Torokhov wrote:
> > > > I think that even if we had a special directory for vmci having network
> > > > drivers in Dave's realm and pvscsi in James's is best option, so the new
> > > > directory would contain vmci and the balloon driver (vsock will go into
> > > > net/). Given that balloon is already in drivers/misc it looked like
> > > > obvious place for VMCI as well.
> > >
> > > I agree that the individual drivers should go in the subsystem area,
> > > it's this "hypervisor bus core" type code that I'm questioning. Right
> > > now every hypervisor is putting that logic in a different place in the
> > > kernel, having some consistency here would be nice.
> >
> > Hmm, I wonder if miscellaneous and core hypervisor drivers should end
> > up in drivers/platform:
> >
> > drivers/platform/hyperv
> > drivers/platform/olpc
> > drivers/platform/vmware
> > drivers/platform/xen
> > drivers/platform/x86
>
> That makes sense to me, nice.
>
> > But really we'd like to get VMCI into mainline first and move to a new
> > place later if such a better place is found.
>
> Heh, no one wants to fight for something to help everyone out, they just
> want their own code accepted :)
No, this is more about finding a person who would be maintaining it and
thus would review our code. For now we got you tagged and do not want to
let go of you :P
Or should we maintain our own stuff and have Linus pull it directly,
like Xen guys appear to be doing? Really, moving it is not an issue for
us.
Thanks,
Dmitry
^ permalink raw reply
* Re: [Pv-drivers] [PATCH 00/12] VMCI for Linux upstreaming
From: Greg KH @ 2012-10-30 19:59 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: pv-drivers, linux-kernel, George Zhang, virtualization
In-Reply-To: <20121030194354.GA32137@dtor-ws.eng.vmware.com>
On Tue, Oct 30, 2012 at 12:43:54PM -0700, Dmitry Torokhov wrote:
> On Tue, Oct 30, 2012 at 09:27:23AM -0700, Greg KH wrote:
> > On Tue, Oct 30, 2012 at 09:18:07AM -0700, Dmitry Torokhov wrote:
> > > > > I think that even if we had a special directory for vmci having network
> > > > > drivers in Dave's realm and pvscsi in James's is best option, so the new
> > > > > directory would contain vmci and the balloon driver (vsock will go into
> > > > > net/). Given that balloon is already in drivers/misc it looked like
> > > > > obvious place for VMCI as well.
> > > >
> > > > I agree that the individual drivers should go in the subsystem area,
> > > > it's this "hypervisor bus core" type code that I'm questioning. Right
> > > > now every hypervisor is putting that logic in a different place in the
> > > > kernel, having some consistency here would be nice.
> > >
> > > Hmm, I wonder if miscellaneous and core hypervisor drivers should end
> > > up in drivers/platform:
> > >
> > > drivers/platform/hyperv
> > > drivers/platform/olpc
> > > drivers/platform/vmware
> > > drivers/platform/xen
> > > drivers/platform/x86
> >
> > That makes sense to me, nice.
> >
> > > But really we'd like to get VMCI into mainline first and move to a new
> > > place later if such a better place is found.
> >
> > Heh, no one wants to fight for something to help everyone out, they just
> > want their own code accepted :)
>
> No, this is more about finding a person who would be maintaining it and
> thus would review our code. For now we got you tagged and do not want to
> let go of you :P
>
> Or should we maintain our own stuff and have Linus pull it directly,
> like Xen guys appear to be doing? Really, moving it is not an issue for
> us.
No, I don't mind doing some of the review, and getting it to Linus to
start with, but over time, having you all be the owners and push stuff
directly to him is fine with me.
So I agree, for now, let's leave it as-is.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH] virtio-ccw: Clarifications.
From: Rusty Russell @ 2012-10-31 1:11 UTC (permalink / raw)
To: Cornelia Huck; +Cc: virtualization
In-Reply-To: <1351515758-22295-2-git-send-email-cornelia.huck@de.ibm.com>
Cornelia Huck <cornelia.huck@de.ibm.com> writes:
> Clarify how indicator location is communicated and that the
> feature bits are little-endian.
>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Thanks, applied!
Cheers,
Rusty.
^ permalink raw reply
* Re: Linux-next changes for module and virtio trees.
From: Rusty Russell @ 2012-10-31 3:28 UTC (permalink / raw)
To: Stephen Rothwell; +Cc: linux-next, linux-kernel, kvm-devel, virtualization
In-Reply-To: <20121003011239.3445171ec17e422a289d2997@canb.auug.org.au>
Stephen Rothwell <sfr@canb.auug.org.au> writes:
> Hi Rusty,
>
> On Tue, 02 Oct 2012 15:56:56 +0930 Rusty Russell <rusty@rustcorp.com.au> wrote:
>>
>> Please remove my quilt tree
>> http://ozlabs.org/~rusty/kernel/rr-latest/ from linux-next, and use my
>> git trees from now on:
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux.git
>> Branches:
>> modules-next
>> virtio-next
>
> Done.
And please add my 'fixes' branch from the same tree.
Thanks,
Rusty.
^ permalink raw reply
* [PATCHv2 net-next 0/8] enable/disable zero copy tx dynamically
From: Michael S. Tsirkin @ 2012-10-31 10:31 UTC (permalink / raw)
Cc: Vlad Yasevich, Alexander Duyck, Ian Campbell, kvm, netdev,
linux-kernel, virtualization, Eric Dumazet, Andrew Morton,
David S. Miller
tun supports zero copy transmit since 0690899b4d4501b3505be069b9a687e68ccbe15b,
however you can only enable this mode if you know your workload does not
trigger heavy guest to host/host to guest traffic - otherwise you
get a (minor) performance regression.
This patchset addresses this problem by notifying the owner
device when callback is invoked because of a data copy.
This makes it possible to detect whether zero copy is appropriate
dynamically: we start in zero copy mode, when we detect
data copied we disable zero copy for a while.
With this patch applied, I get the same performance for
guest to host and guest to guest both with and without zero copy tx.
Changes from v1:
Comment fixups in patches 2 and 8 suggested by Vlad Yasevich,
no changes to other patches
Michael S. Tsirkin (8):
skb: report completion status for zero copy skbs
skb: api to report errors for zero copy skbs
tun: report orphan frags errors to zero copy callback
vhost-net: cleanup macros for DMA status tracking
vhost: track zero copy failures using DMA length
vhost: move -net specific code out
vhost-net: select tx zero copy dynamically
vhost-net: reduce vq polling on tx zerocopy
drivers/net/tun.c | 1 +
drivers/vhost/net.c | 111 +++++++++++++++++++++++++++++++++++++++++++---
drivers/vhost/tcm_vhost.c | 1 +
drivers/vhost/vhost.c | 52 +++-------------------
drivers/vhost/vhost.h | 11 ++---
include/linux/skbuff.h | 5 ++-
net/core/skbuff.c | 24 +++++++++-
7 files changed, 144 insertions(+), 61 deletions(-)
--
MST
^ permalink raw reply
* [PATCHv2 net-next 1/8] skb: report completion status for zero copy skbs
From: Michael S. Tsirkin @ 2012-10-31 10:31 UTC (permalink / raw)
Cc: Vlad Yasevich, Alexander Duyck, Ian Campbell, kvm, netdev,
linux-kernel, virtualization, Eric Dumazet, Andrew Morton,
David S. Miller
In-Reply-To: <cover.1351679008.git.mst@redhat.com>
Even if skb is marked for zero copy, net core might still decide
to copy it later which is somewhat slower than a copy in user context:
besides copying the data we need to pin/unpin the pages.
Add a parameter reporting such cases through zero copy callback:
if this happens a lot, device can take this into account
and switch to copying in user context.
This patch updates all users but ignores the passed value for now:
it will be used by follow-up patches.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/vhost/vhost.c | 2 +-
drivers/vhost/vhost.h | 2 +-
include/linux/skbuff.h | 4 +++-
net/core/skbuff.c | 4 ++--
4 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 99ac2cb..92308b6 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1600,7 +1600,7 @@ void vhost_ubuf_put_and_wait(struct vhost_ubuf_ref *ubufs)
kfree(ubufs);
}
-void vhost_zerocopy_callback(struct ubuf_info *ubuf)
+void vhost_zerocopy_callback(struct ubuf_info *ubuf, int zerocopy_status)
{
struct vhost_ubuf_ref *ubufs = ubuf->ctx;
struct vhost_virtqueue *vq = ubufs->vq;
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 1125af3..eb7263c3 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -191,7 +191,7 @@ bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *);
int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
unsigned int log_num, u64 len);
-void vhost_zerocopy_callback(struct ubuf_info *);
+void vhost_zerocopy_callback(struct ubuf_info *, int);
int vhost_zerocopy_signal_used(struct vhost_virtqueue *vq);
#define vq_err(vq, fmt, ...) do { \
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 6a2c34e..8bac11b 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -235,11 +235,13 @@ enum {
/*
* The callback notifies userspace to release buffers when skb DMA is done in
* lower device, the skb last reference should be 0 when calling this.
+ * The zerocopy_status argument is 0 if zero copy transmit occurred,
+ * 1 on successful data copy; < 0 on out of memory error.
* The ctx field is used to track device context.
* The desc field is used to track userspace buffer index.
*/
struct ubuf_info {
- void (*callback)(struct ubuf_info *);
+ void (*callback)(struct ubuf_info *, int zerocopy_status);
void *ctx;
unsigned long desc;
};
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 6e04b1f..eb31f6e 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -519,7 +519,7 @@ static void skb_release_data(struct sk_buff *skb)
uarg = skb_shinfo(skb)->destructor_arg;
if (uarg->callback)
- uarg->callback(uarg);
+ uarg->callback(uarg, 0);
}
if (skb_has_frag_list(skb))
@@ -797,7 +797,7 @@ int skb_copy_ubufs(struct sk_buff *skb, gfp_t gfp_mask)
for (i = 0; i < num_frags; i++)
skb_frag_unref(skb, i);
- uarg->callback(uarg);
+ uarg->callback(uarg, 1);
/* skb frags point to kernel buffers */
for (i = num_frags - 1; i >= 0; i--) {
--
MST
^ permalink raw reply related
* [PATCHv2 net-next 2/8] skb: api to report errors for zero copy skbs
From: Michael S. Tsirkin @ 2012-10-31 10:31 UTC (permalink / raw)
Cc: Vlad Yasevich, Alexander Duyck, Ian Campbell, kvm, netdev,
linux-kernel, virtualization, Eric Dumazet, Andrew Morton,
David S. Miller
In-Reply-To: <cover.1351679008.git.mst@redhat.com>
Orphaning frags for zero copy skbs needs to allocate data in atomic
context so is has a chance to fail. If it does we currently discard
the skb which is safe, but we don't report anything to the caller,
so it can not recover by e.g. disabling zero copy.
Add an API to free skb reporting such errors: this is used
by tun in case orphaning frags fails.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/linux/skbuff.h | 1 +
net/core/skbuff.c | 20 ++++++++++++++++++++
2 files changed, 21 insertions(+)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 8bac11b..0644432 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -568,6 +568,7 @@ static inline struct rtable *skb_rtable(const struct sk_buff *skb)
}
extern void kfree_skb(struct sk_buff *skb);
+extern void skb_tx_error(struct sk_buff *skb, int err);
extern void consume_skb(struct sk_buff *skb);
extern void __kfree_skb(struct sk_buff *skb);
extern struct kmem_cache *skbuff_head_cache;
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index eb31f6e..2e7a1fd 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -635,6 +635,26 @@ void kfree_skb(struct sk_buff *skb)
EXPORT_SYMBOL(kfree_skb);
/**
+ * skb_tx_error - report an sk_buff xmit error
+ * @skb: buffer that triggered an error
+ *
+ * Report xmit error if a device callback is tracking this skb.
+ * skb must be freed afterwards.
+ */
+void skb_tx_error(struct sk_buff *skb, int err)
+{
+ if (skb_shinfo(skb)->tx_flags & SKBTX_DEV_ZEROCOPY) {
+ struct ubuf_info *uarg;
+
+ uarg = skb_shinfo(skb)->destructor_arg;
+ if (uarg->callback)
+ uarg->callback(uarg, err);
+ skb_shinfo(skb)->tx_flags &= ~SKBTX_DEV_ZEROCOPY;
+ }
+}
+EXPORT_SYMBOL(skb_tx_error);
+
+/**
* consume_skb - free an skbuff
* @skb: buffer to free
*
--
MST
^ permalink raw reply related
* [PATCHv2 net-next 3/8] tun: report orphan frags errors to zero copy callback
From: Michael S. Tsirkin @ 2012-10-31 10:31 UTC (permalink / raw)
Cc: Vlad Yasevich, Alexander Duyck, Ian Campbell, kvm, netdev,
linux-kernel, virtualization, Eric Dumazet, Andrew Morton,
David S. Miller
In-Reply-To: <cover.1351679008.git.mst@redhat.com>
When tun transmits a zero copy skb, it orphans the frags
which might need to allocate extra memory, in atomic context.
If that fails, notify ubufs callback before freeing the skb
as a hint that device should disable zerocopy mode.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/net/tun.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 3157519..613f826 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -433,6 +433,7 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
drop:
dev->stats.tx_dropped++;
+ skb_tx_error(skb, -ENOMEM);
kfree_skb(skb);
return NETDEV_TX_OK;
}
--
MST
^ permalink raw reply related
* [PATCHv2 net-next 8/8] vhost-net: reduce vq polling on tx zerocopy
From: Michael S. Tsirkin @ 2012-10-31 10:31 UTC (permalink / raw)
Cc: Vlad Yasevich, Alexander Duyck, Ian Campbell, kvm, netdev,
linux-kernel, virtualization, Eric Dumazet, Andrew Morton,
David S. Miller
In-Reply-To: <cover.1351679008.git.mst@redhat.com>
It seems that to avoid deadlocks it is enough to poll vq before
we are going to use the last buffer. This is faster than
c70aa540c7a9f67add11ad3161096fb95233aa2e.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/vhost/net.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 8e9de79..88beedb 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -197,8 +197,18 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, int status)
{
struct vhost_ubuf_ref *ubufs = ubuf->ctx;
struct vhost_virtqueue *vq = ubufs->vq;
-
- vhost_poll_queue(&vq->poll);
+ int cnt = atomic_read(&ubufs->kref.refcount);
+
+ /*
+ * Trigger polling thread if guest stopped submitting new buffers:
+ * in this case, the refcount after decrement will eventually reach 1
+ * so here it is 2.
+ * We also trigger polling periodically after each 16 packets
+ * (the value 16 here is more or less arbitrary, it's tuned to trigger
+ * less than 10% of times).
+ */
+ if (cnt <= 2 || !(cnt % 16))
+ vhost_poll_queue(&vq->poll);
/* set len to mark this desc buffers done DMA */
vq->heads[ubuf->desc].len = status ?
VHOST_DMA_FAILED_LEN : VHOST_DMA_DONE_LEN;
--
MST
^ permalink raw reply related
* [PATCHv2 net-next 4/8] vhost-net: cleanup macros for DMA status tracking
From: Michael S. Tsirkin @ 2012-10-31 10:31 UTC (permalink / raw)
Cc: Vlad Yasevich, Alexander Duyck, Ian Campbell, kvm, netdev,
linux-kernel, virtualization, Eric Dumazet, Andrew Morton,
David S. Miller
In-Reply-To: <cover.1351679008.git.mst@redhat.com>
Better document macros for DMA tracking. Add an
explicit one for DMA in progress instead of
relying on user supplying len != 1.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/vhost/net.c | 3 ++-
drivers/vhost/vhost.c | 2 +-
drivers/vhost/vhost.h | 12 +++++++++---
3 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 072cbba..f80ae5f 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -237,7 +237,8 @@ static void handle_tx(struct vhost_net *net)
} else {
struct ubuf_info *ubuf = &vq->ubuf_info[head];
- vq->heads[vq->upend_idx].len = len;
+ vq->heads[vq->upend_idx].len =
+ VHOST_DMA_IN_PROGRESS;
ubuf->callback = vhost_zerocopy_callback;
ubuf->ctx = vq->ubufs;
ubuf->desc = vq->upend_idx;
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 92308b6..906fd9f 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1606,7 +1606,7 @@ void vhost_zerocopy_callback(struct ubuf_info *ubuf, int zerocopy_status)
struct vhost_virtqueue *vq = ubufs->vq;
vhost_poll_queue(&vq->poll);
- /* set len = 1 to mark this desc buffers done DMA */
+ /* set len to mark this desc buffers done DMA */
vq->heads[ubuf->desc].len = VHOST_DMA_DONE_LEN;
kref_put(&ubufs->kref, vhost_zerocopy_done_signal);
}
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index eb7263c3..ad72a1f 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -13,9 +13,15 @@
#include <linux/virtio_ring.h>
#include <linux/atomic.h>
-/* This is for zerocopy, used buffer len is set to 1 when lower device DMA
- * done */
-#define VHOST_DMA_DONE_LEN 1
+/*
+ * For transmit, used buffer len is unused; we override it to track buffer
+ * status internally; used for zerocopy tx only.
+ */
+/* Lower device DMA done */
+#define VHOST_DMA_DONE_LEN 2
+/* Lower device DMA in progress */
+#define VHOST_DMA_IN_PROGRESS 1
+/* Buffer unused */
#define VHOST_DMA_CLEAR_LEN 0
struct vhost_device;
--
MST
^ permalink raw reply related
* [PATCHv2 net-next 5/8] vhost: track zero copy failures using DMA length
From: Michael S. Tsirkin @ 2012-10-31 10:31 UTC (permalink / raw)
Cc: Vlad Yasevich, Alexander Duyck, Ian Campbell, kvm, netdev,
linux-kernel, virtualization, Eric Dumazet, Andrew Morton,
David S. Miller
In-Reply-To: <cover.1351679008.git.mst@redhat.com>
This will be used to disable zerocopy when error rate
is high.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/vhost/vhost.c | 7 ++++---
drivers/vhost/vhost.h | 4 ++++
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 906fd9f..5affce3 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -425,7 +425,7 @@ int vhost_zerocopy_signal_used(struct vhost_virtqueue *vq)
int j = 0;
for (i = vq->done_idx; i != vq->upend_idx; i = (i + 1) % UIO_MAXIOV) {
- if ((vq->heads[i].len == VHOST_DMA_DONE_LEN)) {
+ if (VHOST_DMA_IS_DONE(vq->heads[i].len)) {
vq->heads[i].len = VHOST_DMA_CLEAR_LEN;
vhost_add_used_and_signal(vq->dev, vq,
vq->heads[i].id, 0);
@@ -1600,13 +1600,14 @@ void vhost_ubuf_put_and_wait(struct vhost_ubuf_ref *ubufs)
kfree(ubufs);
}
-void vhost_zerocopy_callback(struct ubuf_info *ubuf, int zerocopy_status)
+void vhost_zerocopy_callback(struct ubuf_info *ubuf, int status)
{
struct vhost_ubuf_ref *ubufs = ubuf->ctx;
struct vhost_virtqueue *vq = ubufs->vq;
vhost_poll_queue(&vq->poll);
/* set len to mark this desc buffers done DMA */
- vq->heads[ubuf->desc].len = VHOST_DMA_DONE_LEN;
+ vq->heads[ubuf->desc].len = status ?
+ VHOST_DMA_FAILED_LEN : VHOST_DMA_DONE_LEN;
kref_put(&ubufs->kref, vhost_zerocopy_done_signal);
}
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index ad72a1f..6fdf31d 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -17,6 +17,8 @@
* For transmit, used buffer len is unused; we override it to track buffer
* status internally; used for zerocopy tx only.
*/
+/* Lower device DMA failed */
+#define VHOST_DMA_FAILED_LEN 3
/* Lower device DMA done */
#define VHOST_DMA_DONE_LEN 2
/* Lower device DMA in progress */
@@ -24,6 +26,8 @@
/* Buffer unused */
#define VHOST_DMA_CLEAR_LEN 0
+#define VHOST_DMA_IS_DONE(len) ((len) >= VHOST_DMA_DONE_LEN)
+
struct vhost_device;
struct vhost_work;
--
MST
^ permalink raw reply related
* [PATCHv2 net-next 6/8] vhost: move -net specific code out
From: Michael S. Tsirkin @ 2012-10-31 10:31 UTC (permalink / raw)
Cc: Vlad Yasevich, Alexander Duyck, Ian Campbell, kvm, netdev,
linux-kernel, virtualization, Eric Dumazet, Andrew Morton,
David S. Miller
In-Reply-To: <cover.1351679008.git.mst@redhat.com>
Zerocopy handling code is vhost-net specific.
Move it from vhost.c/vhost.h out to net.c
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/vhost/net.c | 45 ++++++++++++++++++++++++++++++++++++++++
drivers/vhost/tcm_vhost.c | 1 +
drivers/vhost/vhost.c | 53 +++++++----------------------------------------
drivers/vhost/vhost.h | 21 +++----------------
4 files changed, 56 insertions(+), 64 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index f80ae5f..532fc88 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -126,6 +126,42 @@ static void tx_poll_start(struct vhost_net *net, struct socket *sock)
net->tx_poll_state = VHOST_NET_POLL_STARTED;
}
+/* In case of DMA done not in order in lower device driver for some reason.
+ * upend_idx is used to track end of used idx, done_idx is used to track head
+ * of used idx. Once lower device DMA done contiguously, we will signal KVM
+ * guest used idx.
+ */
+int vhost_zerocopy_signal_used(struct vhost_virtqueue *vq)
+{
+ int i;
+ int j = 0;
+
+ for (i = vq->done_idx; i != vq->upend_idx; i = (i + 1) % UIO_MAXIOV) {
+ if (VHOST_DMA_IS_DONE(vq->heads[i].len)) {
+ vq->heads[i].len = VHOST_DMA_CLEAR_LEN;
+ vhost_add_used_and_signal(vq->dev, vq,
+ vq->heads[i].id, 0);
+ ++j;
+ } else
+ break;
+ }
+ if (j)
+ vq->done_idx = i;
+ return j;
+}
+
+static void vhost_zerocopy_callback(struct ubuf_info *ubuf, int status)
+{
+ struct vhost_ubuf_ref *ubufs = ubuf->ctx;
+ struct vhost_virtqueue *vq = ubufs->vq;
+
+ vhost_poll_queue(&vq->poll);
+ /* set len to mark this desc buffers done DMA */
+ vq->heads[ubuf->desc].len = status ?
+ VHOST_DMA_FAILED_LEN : VHOST_DMA_DONE_LEN;
+ vhost_ubuf_put(ubufs);
+}
+
/* Expects to be always run from workqueue - which acts as
* read-size critical section for our kind of RCU. */
static void handle_tx(struct vhost_net *net)
@@ -594,9 +630,18 @@ static int vhost_net_release(struct inode *inode, struct file *f)
struct vhost_net *n = f->private_data;
struct socket *tx_sock;
struct socket *rx_sock;
+ int i;
vhost_net_stop(n, &tx_sock, &rx_sock);
vhost_net_flush(n);
+ vhost_dev_stop(&n->dev);
+ for (i = 0; i < n->dev.nvqs; ++i) {
+ /* Wait for all lower device DMAs done. */
+ if (n->dev.vqs[i].ubufs)
+ vhost_ubuf_put_and_wait(n->dev.vqs[i].ubufs);
+
+ vhost_zerocopy_signal_used(n, &n->dev.vqs[i]);
+ }
vhost_dev_cleanup(&n->dev, false);
if (tx_sock)
fput(tx_sock->file);
diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index aa31692..23c138f 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -895,6 +895,7 @@ static int vhost_scsi_release(struct inode *inode, struct file *f)
vhost_scsi_clear_endpoint(s, &backend);
}
+ vhost_dev_stop(&s->dev);
vhost_dev_cleanup(&s->dev, false);
kfree(s);
return 0;
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 5affce3..ef8f598 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -26,10 +26,6 @@
#include <linux/kthread.h>
#include <linux/cgroup.h>
-#include <linux/net.h>
-#include <linux/if_packet.h>
-#include <linux/if_arp.h>
-
#include "vhost.h"
enum {
@@ -414,28 +410,16 @@ long vhost_dev_reset_owner(struct vhost_dev *dev)
return 0;
}
-/* In case of DMA done not in order in lower device driver for some reason.
- * upend_idx is used to track end of used idx, done_idx is used to track head
- * of used idx. Once lower device DMA done contiguously, we will signal KVM
- * guest used idx.
- */
-int vhost_zerocopy_signal_used(struct vhost_virtqueue *vq)
+void vhost_dev_stop(struct vhost_dev *dev)
{
int i;
- int j = 0;
-
- for (i = vq->done_idx; i != vq->upend_idx; i = (i + 1) % UIO_MAXIOV) {
- if (VHOST_DMA_IS_DONE(vq->heads[i].len)) {
- vq->heads[i].len = VHOST_DMA_CLEAR_LEN;
- vhost_add_used_and_signal(vq->dev, vq,
- vq->heads[i].id, 0);
- ++j;
- } else
- break;
+
+ for (i = 0; i < dev->nvqs; ++i) {
+ if (dev->vqs[i].kick && dev->vqs[i].handle_kick) {
+ vhost_poll_stop(&dev->vqs[i].poll);
+ vhost_poll_flush(&dev->vqs[i].poll);
+ }
}
- if (j)
- vq->done_idx = i;
- return j;
}
/* Caller should have device mutex if and only if locked is set */
@@ -444,17 +428,6 @@ void vhost_dev_cleanup(struct vhost_dev *dev, bool locked)
int i;
for (i = 0; i < dev->nvqs; ++i) {
- if (dev->vqs[i].kick && dev->vqs[i].handle_kick) {
- vhost_poll_stop(&dev->vqs[i].poll);
- vhost_poll_flush(&dev->vqs[i].poll);
- }
- /* Wait for all lower device DMAs done. */
- if (dev->vqs[i].ubufs)
- vhost_ubuf_put_and_wait(dev->vqs[i].ubufs);
-
- /* Signal guest as appropriate. */
- vhost_zerocopy_signal_used(&dev->vqs[i]);
-
if (dev->vqs[i].error_ctx)
eventfd_ctx_put(dev->vqs[i].error_ctx);
if (dev->vqs[i].error)
@@ -1599,15 +1572,3 @@ void vhost_ubuf_put_and_wait(struct vhost_ubuf_ref *ubufs)
wait_event(ubufs->wait, !atomic_read(&ubufs->kref.refcount));
kfree(ubufs);
}
-
-void vhost_zerocopy_callback(struct ubuf_info *ubuf, int status)
-{
- struct vhost_ubuf_ref *ubufs = ubuf->ctx;
- struct vhost_virtqueue *vq = ubufs->vq;
-
- vhost_poll_queue(&vq->poll);
- /* set len to mark this desc buffers done DMA */
- vq->heads[ubuf->desc].len = status ?
- VHOST_DMA_FAILED_LEN : VHOST_DMA_DONE_LEN;
- kref_put(&ubufs->kref, vhost_zerocopy_done_signal);
-}
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 6fdf31d..5e19e3d 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -7,27 +7,11 @@
#include <linux/mutex.h>
#include <linux/poll.h>
#include <linux/file.h>
-#include <linux/skbuff.h>
#include <linux/uio.h>
#include <linux/virtio_config.h>
#include <linux/virtio_ring.h>
#include <linux/atomic.h>
-/*
- * For transmit, used buffer len is unused; we override it to track buffer
- * status internally; used for zerocopy tx only.
- */
-/* Lower device DMA failed */
-#define VHOST_DMA_FAILED_LEN 3
-/* Lower device DMA done */
-#define VHOST_DMA_DONE_LEN 2
-/* Lower device DMA in progress */
-#define VHOST_DMA_IN_PROGRESS 1
-/* Buffer unused */
-#define VHOST_DMA_CLEAR_LEN 0
-
-#define VHOST_DMA_IS_DONE(len) ((len) >= VHOST_DMA_DONE_LEN)
-
struct vhost_device;
struct vhost_work;
@@ -80,6 +64,8 @@ struct vhost_ubuf_ref *vhost_ubuf_alloc(struct vhost_virtqueue *, bool zcopy);
void vhost_ubuf_put(struct vhost_ubuf_ref *);
void vhost_ubuf_put_and_wait(struct vhost_ubuf_ref *);
+struct ubuf_info;
+
/* The virtqueue structure describes a queue attached to a device. */
struct vhost_virtqueue {
struct vhost_dev *dev;
@@ -177,6 +163,7 @@ long vhost_dev_init(struct vhost_dev *, struct vhost_virtqueue *vqs, int nvqs);
long vhost_dev_check_owner(struct vhost_dev *);
long vhost_dev_reset_owner(struct vhost_dev *);
void vhost_dev_cleanup(struct vhost_dev *, bool locked);
+void vhost_dev_stop(struct vhost_dev *);
long vhost_dev_ioctl(struct vhost_dev *, unsigned int ioctl, unsigned long arg);
int vhost_vq_access_ok(struct vhost_virtqueue *vq);
int vhost_log_access_ok(struct vhost_dev *);
@@ -201,8 +188,6 @@ bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *);
int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
unsigned int log_num, u64 len);
-void vhost_zerocopy_callback(struct ubuf_info *, int);
-int vhost_zerocopy_signal_used(struct vhost_virtqueue *vq);
#define vq_err(vq, fmt, ...) do { \
pr_debug(pr_fmt(fmt), ##__VA_ARGS__); \
--
MST
^ permalink raw reply related
* [PATCHv2 net-next 7/8] vhost-net: select tx zero copy dynamically
From: Michael S. Tsirkin @ 2012-10-31 10:31 UTC (permalink / raw)
Cc: Vlad Yasevich, Alexander Duyck, Ian Campbell, kvm, netdev,
linux-kernel, virtualization, Eric Dumazet, Andrew Morton,
David S. Miller
In-Reply-To: <cover.1351679008.git.mst@redhat.com>
Even when vhost-net is in zero-copy transmit mode,
net core might still decide to copy the skb later
which is somewhat slower than a copy in user
context: data copy overhead is added to the cost of
page pin/unpin. The result is that enabling tx zero copy
option leads to higher CPU utilization for guest to guest
and guest to host traffic.
To fix this, suppress zero copy tx after a given number of
packets triggered late data copy. Re-enable periodically
to detect workload changes.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/vhost/net.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 50 insertions(+), 5 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 532fc88..8e9de79 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -42,6 +42,21 @@ MODULE_PARM_DESC(experimental_zcopytx, "Enable Experimental Zero Copy TX");
#define VHOST_MAX_PEND 128
#define VHOST_GOODCOPY_LEN 256
+/*
+ * For transmit, used buffer len is unused; we override it to track buffer
+ * status internally; used for zerocopy tx only.
+ */
+/* Lower device DMA failed */
+#define VHOST_DMA_FAILED_LEN 3
+/* Lower device DMA done */
+#define VHOST_DMA_DONE_LEN 2
+/* Lower device DMA in progress */
+#define VHOST_DMA_IN_PROGRESS 1
+/* Buffer unused */
+#define VHOST_DMA_CLEAR_LEN 0
+
+#define VHOST_DMA_IS_DONE(len) ((len) >= VHOST_DMA_DONE_LEN)
+
enum {
VHOST_NET_VQ_RX = 0,
VHOST_NET_VQ_TX = 1,
@@ -62,8 +77,33 @@ struct vhost_net {
* We only do this when socket buffer fills up.
* Protected by tx vq lock. */
enum vhost_net_poll_state tx_poll_state;
+ /* Number of TX recently submitted.
+ * Protected by tx vq lock. */
+ unsigned tx_packets;
+ /* Number of times zerocopy TX recently failed.
+ * Protected by tx vq lock. */
+ unsigned tx_zcopy_err;
};
+static void vhost_net_tx_packet(struct vhost_net *net)
+{
+ ++net->tx_packets;
+ if (net->tx_packets < 1024)
+ return;
+ net->tx_packets = 0;
+ net->tx_zcopy_err = 0;
+}
+
+static void vhost_net_tx_err(struct vhost_net *net)
+{
+ ++net->tx_zcopy_err;
+}
+
+static bool vhost_net_tx_select_zcopy(struct vhost_net *net)
+{
+ return net->tx_packets / 64 >= net->tx_zcopy_err;
+}
+
static bool vhost_sock_zcopy(struct socket *sock)
{
return unlikely(experimental_zcopytx) &&
@@ -131,12 +171,15 @@ static void tx_poll_start(struct vhost_net *net, struct socket *sock)
* of used idx. Once lower device DMA done contiguously, we will signal KVM
* guest used idx.
*/
-int vhost_zerocopy_signal_used(struct vhost_virtqueue *vq)
+static int vhost_zerocopy_signal_used(struct vhost_net *net,
+ struct vhost_virtqueue *vq)
{
int i;
int j = 0;
for (i = vq->done_idx; i != vq->upend_idx; i = (i + 1) % UIO_MAXIOV) {
+ if (vq->heads[i].len == VHOST_DMA_FAILED_LEN)
+ vhost_net_tx_err(net);
if (VHOST_DMA_IS_DONE(vq->heads[i].len)) {
vq->heads[i].len = VHOST_DMA_CLEAR_LEN;
vhost_add_used_and_signal(vq->dev, vq,
@@ -208,7 +251,7 @@ static void handle_tx(struct vhost_net *net)
for (;;) {
/* Release DMAs done buffers first */
if (zcopy)
- vhost_zerocopy_signal_used(vq);
+ vhost_zerocopy_signal_used(net, vq);
head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
ARRAY_SIZE(vq->iov),
@@ -263,7 +306,8 @@ static void handle_tx(struct vhost_net *net)
/* use msg_control to pass vhost zerocopy ubuf info to skb */
if (zcopy) {
vq->heads[vq->upend_idx].id = head;
- if (len < VHOST_GOODCOPY_LEN) {
+ if (!vhost_net_tx_select_zcopy(net) ||
+ len < VHOST_GOODCOPY_LEN) {
/* copy don't need to wait for DMA done */
vq->heads[vq->upend_idx].len =
VHOST_DMA_DONE_LEN;
@@ -305,8 +349,9 @@ static void handle_tx(struct vhost_net *net)
if (!zcopy)
vhost_add_used_and_signal(&net->dev, vq, head, 0);
else
- vhost_zerocopy_signal_used(vq);
+ vhost_zerocopy_signal_used(net, vq);
total_len += len;
+ vhost_net_tx_packet(net);
if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
vhost_poll_queue(&vq->poll);
break;
@@ -774,7 +819,7 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
if (oldubufs) {
vhost_ubuf_put_and_wait(oldubufs);
mutex_lock(&vq->mutex);
- vhost_zerocopy_signal_used(vq);
+ vhost_zerocopy_signal_used(n, vq);
mutex_unlock(&vq->mutex);
}
--
MST
^ permalink raw reply related
* Re: [rfc net-next v6 0/3] Multiqueue virtio-net
From: Jason Wang @ 2012-10-31 10:33 UTC (permalink / raw)
To: Rick Jones
Cc: krkumar2, kvm, mst, netdev, linux-kernel, virtualization, davem
In-Reply-To: <509024F4.8080408@hp.com>
On 10/31/2012 03:05 AM, Rick Jones wrote:
> On 10/30/2012 03:03 AM, Jason Wang wrote:
>> Hi all:
>>
>> This series is an update version of multiqueue virtio-net driver
>> based on
>> Krishna Kumar's work to let virtio-net use multiple rx/tx queues to
>> do the
>> packets reception and transmission. Please review and comments.
>>
>> Changes from v5:
>> - Align the implementation with the RFC spec update v4
>> - Switch the mode between single mode and multiqueue mode without reset
>> - Remove the 256 limitation of queues
>> - Use helpers to do the mapping between virtqueues and tx/rx queues
>> - Use commbined channels instead of separated rx/tx queus when do the
>> queue
>> number configuartion
>> - Other coding style comments from Michael
>>
>> Reference:
>> - A protype implementation of qemu-kvm support could by found in
>> git://github.com/jasowang/qemu-kvm-mq.git
>> - V5 could be found at http://lwn.net/Articles/505388/
>> - V4 could be found at https://lkml.org/lkml/2012/6/25/120
>> - V2 could be found at http://lwn.net/Articles/467283/
>> - Michael virtio-spec:
>> http://www.spinics.net/lists/netdev/msg209986.html
>>
>> Perf Numbers:
>>
>> - Pktgen test shows the receiving capability of the multiqueue
>> virtio-net were
>> dramatically improved.
>> - Netperf result shows latency were greately improved according to
>> the test
>> result.
>
> I suppose it is technically correct to say that latency was improved,
> but usually for aggregate request/response tests I tend to talk about
> the aggregate transactions per second.
Sure.
>
> Do you have a hypothesis as to why the improvement dropped going to 20
> concurrent sessions from 10?
>
> rick jones
I'm investigating this issuse currently, but with no much ideas. The
aggregate transactions per second scales pretty well even with 20
cocurrent sessions when doing test between a local host and a local vm.
Looks like some bottleneck were reached when doing testing over 10gb or
vms as even if I increase the number of sessions, the result would not
increase.
^ permalink raw reply
* Re: Linux-next changes for module and virtio trees.
From: Stephen Rothwell @ 2012-10-31 11:53 UTC (permalink / raw)
To: Rusty Russell; +Cc: linux-next, linux-kernel, kvm-devel, virtualization
In-Reply-To: <87pq3z5nhs.fsf@rustcorp.com.au>
[-- Attachment #1.1: Type: text/plain, Size: 2164 bytes --]
Hi Rusty,
On Wed, 31 Oct 2012 13:58:15 +1030 Rusty Russell <rusty@rustcorp.com.au> wrote:
>
> Stephen Rothwell <sfr@canb.auug.org.au> writes:
> >
> > On Tue, 02 Oct 2012 15:56:56 +0930 Rusty Russell <rusty@rustcorp.com.au> wrote:
> >>
> >> Please remove my quilt tree
> >> http://ozlabs.org/~rusty/kernel/rr-latest/ from linux-next, and use my
> >> git trees from now on:
> >>
> >> git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux.git
> >> Branches:
> >> modules-next
> >> virtio-next
> >
> > Done.
>
> And please add my 'fixes' branch from the same tree.
Done.
Thanks for adding your subsystem tree as a participant of linux-next. As
you may know, this is not a judgment of your code. The purpose of
linux-next is for integration testing and to lower the impact of
conflicts between subsystems in the next merge window.
You will need to ensure that the patches/commits in your tree/series have
been:
* submitted under GPL v2 (or later) and include the Contributor's
Signed-off-by,
* posted to the relevant mailing list,
* reviewed by you (or another maintainer of your subsystem tree),
* successfully unit tested, and
* destined for the current or next Linux merge window.
Basically, this should be just what you would send to Linus (or ask him
to fetch). It is allowed to be rebased if you deem it necessary.
--
Cheers,
Stephen Rothwell
sfr@canb.auug.org.au
Legal Stuff:
By participating in linux-next, your subsystem tree contributions are
public and will be included in the linux-next trees. You may be sent
e-mail messages indicating errors or other issues when the
patches/commits from your subsystem tree are merged and tested in
linux-next. These messages may also be cross-posted to the linux-next
mailing list, the linux-kernel mailing list, etc. The linux-next tree
project and IBM (my employer) make no warranties regarding the linux-next
project, the testing procedures, the results, the e-mails, etc. If you
don't agree to these ground rules, let me know and I'll remove your tree
from participation in linux-next.
[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
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