* Re: Flow Control and Port Mirroring Revisited
From: Simon Horman @ 2011-01-19 9:11 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rick Jones, Jesse Gross, Eric Dumazet, Rusty Russell,
virtualization, dev, virtualization, netdev, kvm
In-Reply-To: <20110118201333.GD18760@redhat.com>
On Tue, Jan 18, 2011 at 10:13:33PM +0200, Michael S. Tsirkin wrote:
> On Tue, Jan 18, 2011 at 11:41:22AM -0800, Rick Jones wrote:
> > >So it won't be all that simple to implement well, and before we try,
> > >I'd like to know whether there are applications that are helped
> > >by it. For example, we could try to measure latency at various
> > >pps and see whether the backpressure helps. netperf has -b, -w
> > >flags which might help these measurements.
> >
> > Those options are enabled when one adds --enable-burst to the
> > pre-compilation ./configure of netperf (one doesn't have to
> > recompile netserver). However, if one is also looking at latency
> > statistics via the -j option in the top-of-trunk, or simply at the
> > histogram with --enable-histogram on the ./configure and a verbosity
> > level of 2 (global -v 2) then one wants the very top of trunk
> > netperf from:
> >
> > http://www.netperf.org/svn/netperf2/trunk
> >
> > to get the recently added support for accurate (netperf level) RTT
> > measuremnts on burst-mode request/response tests.
> >
> > happy benchmarking,
> >
> > rick jones
Thanks Rick, that is really helpful.
> > PS - the enhanced latency statistics from -j are only available in
> > the "omni" version of the TCP_RR test. To get that add a
> > --enable-omni to the ./configure - and in this case both netperf and
> > netserver have to be recompiled.
>
>
> Is this TCP only? I would love to get latency data from UDP as well.
At a glance, -- -T UDP is what you are after.
^ permalink raw reply
* Re: [PATCH] vhost: rcu annotation fixup
From: Michael S. Tsirkin @ 2011-01-19 5:18 UTC (permalink / raw)
To: Mel Gorman; +Cc: Jason Wang, kvm, virtualization, netdev, linux-kernel
In-Reply-To: <20110119004040.GE18984@csn.ul.ie>
On Wed, Jan 19, 2011 at 12:40:40AM +0000, Mel Gorman wrote:
> On Tue, Jan 18, 2011 at 01:08:45PM +0200, Michael S. Tsirkin wrote:
> > When built with rcu checks enabled, vhost triggers
> > bogus warnings as vhost features are read without
> > dev->mutex sometimes.
> > Fixing it properly is not trivial as vhost.h does not
> > know which lockdep classes it will be used under.
> > Disable the warning by stubbing out the check for now.
> >
>
> What is the harm in leaving the bogus warnings until the difficult fix
> happens? RCU checks enabled does not seem like something that is enabled
> in production.
I would like to run with rcu checks enabled sometimes to debug kvm,
which has an elaborate rcu strategy. Bogus warnings in the log
make it easy to overlook the real ones. Further, the rcu macros
used are a form of documentation. If we have
- rcu_dereference_index_check(dev->acked_features,
- lockdep_is_held(&dev->mutex));
this means 'taken in rcu read side critical section or under mutex',
+ acked_features = rcu_dereference_index_check(dev->acked_features, 1);
means 'not checked'.
> If this patch is applied, there is always the risk that
> it'll be simply forgotten about.
Well, that's why I put in a TODO.
If there's a demand for that, I can add a Kconfig option to
trigger a warning at each unchecked rcu call in vhost-net
but I doubt it'll get a lof of use :)
> --
> Mel Gorman
> Part-time Phd Student Linux Technology Center
> University of Limerick IBM Dublin Software Lab
^ permalink raw reply
* Re: [PATCH] vhost: rcu annotation fixup
From: Mel Gorman @ 2011-01-19 0:40 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Jason Wang, kvm, virtualization, netdev, linux-kernel
In-Reply-To: <20110118110845.GA11555@redhat.com>
On Tue, Jan 18, 2011 at 01:08:45PM +0200, Michael S. Tsirkin wrote:
> When built with rcu checks enabled, vhost triggers
> bogus warnings as vhost features are read without
> dev->mutex sometimes.
> Fixing it properly is not trivial as vhost.h does not
> know which lockdep classes it will be used under.
> Disable the warning by stubbing out the check for now.
>
What is the harm in leaving the bogus warnings until the difficult fix
happens? RCU checks enabled does not seem like something that is enabled
in production. If this patch is applied, there is always the risk that
it'll be simply forgotten about.
--
Mel Gorman
Part-time Phd Student Linux Technology Center
University of Limerick IBM Dublin Software Lab
^ permalink raw reply
* Re: Flow Control and Port Mirroring Revisited
From: Rick Jones @ 2011-01-18 21:28 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Simon Horman, Jesse Gross, Eric Dumazet, Rusty Russell,
virtualization, dev, virtualization, netdev, kvm
In-Reply-To: <20110118201333.GD18760@redhat.com>
Michael S. Tsirkin wrote:
> On Tue, Jan 18, 2011 at 11:41:22AM -0800, Rick Jones wrote:
>
>>PS - the enhanced latency statistics from -j are only available in
>>the "omni" version of the TCP_RR test. To get that add a
>>--enable-omni to the ./configure - and in this case both netperf and
>>netserver have to be recompiled.
>
> Is this TCP only? I would love to get latency data from UDP as well.
I believe it will work with UDP request response as well. The omni test code
strives to be protocol agnostic. (I'm sure there are bugs of course, there
always are.)
There is though the added complication of there being no specific matching of
requests to responses. The code as written takes advantage of TCP's in-order
semantics and recovery from packet loss. In a "plain" UDP_RR test, with one at
a time transactions, if either the request or response are lost, data flow
effectively stops there until the timer expires. So, one has "reasonable" RTT
numbers from before that point. In a burst UDP RR test, the code doesn't know
which request/response was lost and so the matching being done to get RTTs will
be off by each lost datagram. And if something were re-ordered the timstamps
would be off even without a datagram loss event.
To "fix" that would require netperf do something it has not yet done in 18-odd
years :) That is actually echo something back from the netserver on the RR test
- either an id, or a timestamp. That means "dirtying" the buffers which means
still more cache misses, from places other than the actual stack. Not beyond the
realm of the possible, but it would be a bit of departure for "normal" operation
(*) and could enforce a minimum request/response size beyond the present single
byte (ok, perhaps only two or four bytes :). But that, perhaps, is a discussion
best left to netperf-talk at netperf.org.
happy benchmarking,
rick jones
(*) netperf does have the concept of reading from and/or dirtying buffers,
put-in back in the days of COW/page-remapping in HP-UX 9.0, but that was mainly
to force COW and/or show the effect of the required data cache purges/flushes.
As such it was made conditional on DIRTY being defined.
^ permalink raw reply
* Re: [PATCH] vhost: rcu annotation fixup
From: Paul E. McKenney @ 2011-01-18 20:28 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Jason Wang, kvm, virtualization, netdev, linux-kernel
In-Reply-To: <20110118201031.GC18760@redhat.com>
On Tue, Jan 18, 2011 at 10:10:31PM +0200, Michael S. Tsirkin wrote:
> On Tue, Jan 18, 2011 at 11:02:33AM -0800, Paul E. McKenney wrote:
> > On Tue, Jan 18, 2011 at 07:55:00PM +0200, Michael S. Tsirkin wrote:
> > > On Tue, Jan 18, 2011 at 09:48:34AM -0800, Paul E. McKenney wrote:
> > > > On Tue, Jan 18, 2011 at 01:08:45PM +0200, Michael S. Tsirkin wrote:
> > > > > When built with rcu checks enabled, vhost triggers
> > > > > bogus warnings as vhost features are read without
> > > > > dev->mutex sometimes.
> > > > > Fixing it properly is not trivial as vhost.h does not
> > > > > know which lockdep classes it will be used under.
> > > > > Disable the warning by stubbing out the check for now.
> > > > >
> > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > ---
> > > > > drivers/vhost/vhost.h | 4 +---
> > > > > 1 files changed, 1 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> > > > > index 2af44b7..2d03a31 100644
> > > > > --- a/drivers/vhost/vhost.h
> > > > > +++ b/drivers/vhost/vhost.h
> > > > > @@ -173,9 +173,7 @@ static inline int vhost_has_feature(struct vhost_dev *dev, int bit)
> > > > > {
> > > > > unsigned acked_features;
> > > > >
> > > > > - acked_features =
> > > > > - rcu_dereference_index_check(dev->acked_features,
> > > > > - lockdep_is_held(&dev->mutex));
> > > > > + acked_features = rcu_dereference_index_check(dev->acked_features, 1);
> > > >
> > > > Ouch!!!
> > > >
> > > > Could you please at least add a comment?
> > >
> > > Yes, OK.
> > >
> > > > Alternatively, pass in the lock that is held and check for that? Given
> > > > that this is a static inline, the compiler should be able to optimize
> > > > the argument away when !PROVE_RCU, correct?
> > > >
> > > > Thanx, Paul
> > >
> > > Hopefully, yes. We don't always have a lock: the idea was
> > > to create a lockdep for these cases. But we can't pass
> > > the pointer to that ...
> >
> > I suppose you could pass a pointer to the lockdep map structure.
> > Not sure if this makes sense, but it would handle the situation.
>
> Will it compile with lockdep disabled too? What will the pointer be?
One (crude) approach would be to make the pointer void* if lockdep
is disabled.
> > Alternatively, create a helper function that checks the possibilities
> > and screams if none of them are in effect.
> >
> > Thanx, Paul
>
> The problem here is the callee needs to know about all callers.
As does the guy reading the code. ;-)
Thanx, Paul
> > > > > return acked_features & (1 << bit);
> > > > > }
> > > > >
> > > > > --
> > > > > 1.7.3.2.91.g446ac
> > > > > --
> > > > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > > > > the body of a message to majordomo@vger.kernel.org
> > > > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > > > Please read the FAQ at http://www.tux.org/lkml/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: Flow Control and Port Mirroring Revisited
From: Michael S. Tsirkin @ 2011-01-18 20:13 UTC (permalink / raw)
To: Rick Jones
Cc: Simon Horman, Jesse Gross, Eric Dumazet, Rusty Russell,
virtualization, dev, virtualization, netdev, kvm
In-Reply-To: <4D35ECE2.4040901@hp.com>
On Tue, Jan 18, 2011 at 11:41:22AM -0800, Rick Jones wrote:
> >So it won't be all that simple to implement well, and before we try,
> >I'd like to know whether there are applications that are helped
> >by it. For example, we could try to measure latency at various
> >pps and see whether the backpressure helps. netperf has -b, -w
> >flags which might help these measurements.
>
> Those options are enabled when one adds --enable-burst to the
> pre-compilation ./configure of netperf (one doesn't have to
> recompile netserver). However, if one is also looking at latency
> statistics via the -j option in the top-of-trunk, or simply at the
> histogram with --enable-histogram on the ./configure and a verbosity
> level of 2 (global -v 2) then one wants the very top of trunk
> netperf from:
>
> http://www.netperf.org/svn/netperf2/trunk
>
> to get the recently added support for accurate (netperf level) RTT
> measuremnts on burst-mode request/response tests.
>
> happy benchmarking,
>
> rick jones
>
> PS - the enhanced latency statistics from -j are only available in
> the "omni" version of the TCP_RR test. To get that add a
> --enable-omni to the ./configure - and in this case both netperf and
> netserver have to be recompiled.
Is this TCP only? I would love to get latency data from UDP as well.
> For very basic output one can
> peruse the output of:
>
> src/netperf -t omni -- -O /?
>
> and then pick those outputs of interest and put them into an output
> selection file which one then passes to either (test-specific) -o,
> -O or -k to get CVS, "Human" or keyval output respectively. E.G.
>
> raj@tardy:~/netperf2_trunk$ cat foo
> THROUGHPUT,THROUGHPUT_UNITS
> RT_LATENCY,MIN_LATENCY,MEAN_LATENCY,MAX_LATENCY
> P50_LATENCY,P90_LATENCY,P99_LATENCY,STDDEV_LATENCY
>
> when foo is passed to -o one will get those all on one line of CSV.
> To -O one gets three lines of more netperf-classic-like "human"
> readable output, and when one passes that to -k one gets a string of
> keyval output a la:
>
> raj@tardy:~/netperf2_trunk$ src/netperf -t omni -j -v 2 -- -r 1 -d rr -k foo
> OMNI TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to localhost
> (127.0.0.1) port 0 AF_INET : histogram
> THROUGHPUT=29454.12
> THROUGHPUT_UNITS=Trans/s
> RT_LATENCY=33.951
> MIN_LATENCY=19
> MEAN_LATENCY=32.00
> MAX_LATENCY=126
> P50_LATENCY=32
> P90_LATENCY=38
> P99_LATENCY=41
> STDDEV_LATENCY=5.46
>
> Histogram of request/response times
> UNIT_USEC : 0: 0: 0: 0: 0: 0: 0: 0: 0: 0
> TEN_USEC : 0: 3553: 45244: 237790: 7859: 86: 10: 3: 0: 0
> HUNDRED_USEC : 0: 2: 0: 0: 0: 0: 0: 0: 0: 0
> UNIT_MSEC : 0: 0: 0: 0: 0: 0: 0: 0: 0: 0
> TEN_MSEC : 0: 0: 0: 0: 0: 0: 0: 0: 0: 0
> HUNDRED_MSEC : 0: 0: 0: 0: 0: 0: 0: 0: 0: 0
> UNIT_SEC : 0: 0: 0: 0: 0: 0: 0: 0: 0: 0
> TEN_SEC : 0: 0: 0: 0: 0: 0: 0: 0: 0: 0
> >100_SECS: 0
> HIST_TOTAL: 294547
^ permalink raw reply
* Re: [PATCH] vhost: rcu annotation fixup
From: Michael S. Tsirkin @ 2011-01-18 20:10 UTC (permalink / raw)
To: Paul E. McKenney; +Cc: Jason Wang, kvm, virtualization, netdev, linux-kernel
In-Reply-To: <20110118190232.GM2193@linux.vnet.ibm.com>
On Tue, Jan 18, 2011 at 11:02:33AM -0800, Paul E. McKenney wrote:
> On Tue, Jan 18, 2011 at 07:55:00PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Jan 18, 2011 at 09:48:34AM -0800, Paul E. McKenney wrote:
> > > On Tue, Jan 18, 2011 at 01:08:45PM +0200, Michael S. Tsirkin wrote:
> > > > When built with rcu checks enabled, vhost triggers
> > > > bogus warnings as vhost features are read without
> > > > dev->mutex sometimes.
> > > > Fixing it properly is not trivial as vhost.h does not
> > > > know which lockdep classes it will be used under.
> > > > Disable the warning by stubbing out the check for now.
> > > >
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > > drivers/vhost/vhost.h | 4 +---
> > > > 1 files changed, 1 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> > > > index 2af44b7..2d03a31 100644
> > > > --- a/drivers/vhost/vhost.h
> > > > +++ b/drivers/vhost/vhost.h
> > > > @@ -173,9 +173,7 @@ static inline int vhost_has_feature(struct vhost_dev *dev, int bit)
> > > > {
> > > > unsigned acked_features;
> > > >
> > > > - acked_features =
> > > > - rcu_dereference_index_check(dev->acked_features,
> > > > - lockdep_is_held(&dev->mutex));
> > > > + acked_features = rcu_dereference_index_check(dev->acked_features, 1);
> > >
> > > Ouch!!!
> > >
> > > Could you please at least add a comment?
> >
> > Yes, OK.
> >
> > > Alternatively, pass in the lock that is held and check for that? Given
> > > that this is a static inline, the compiler should be able to optimize
> > > the argument away when !PROVE_RCU, correct?
> > >
> > > Thanx, Paul
> >
> > Hopefully, yes. We don't always have a lock: the idea was
> > to create a lockdep for these cases. But we can't pass
> > the pointer to that ...
>
> I suppose you could pass a pointer to the lockdep map structure.
> Not sure if this makes sense, but it would handle the situation.
Will it compile with lockdep disabled too? What will the pointer be?
> Alternatively, create a helper function that checks the possibilities
> and screams if none of them are in effect.
>
> Thanx, Paul
The problem here is the callee needs to know about all callers.
> > > > return acked_features & (1 << bit);
> > > > }
> > > >
> > > > --
> > > > 1.7.3.2.91.g446ac
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > > Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: Flow Control and Port Mirroring Revisited
From: Rick Jones @ 2011-01-18 19:41 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Simon Horman, Jesse Gross, Eric Dumazet, Rusty Russell,
virtualization, dev, virtualization, netdev, kvm
In-Reply-To: <20110117102655.GH23479@redhat.com>
> So it won't be all that simple to implement well, and before we try,
> I'd like to know whether there are applications that are helped
> by it. For example, we could try to measure latency at various
> pps and see whether the backpressure helps. netperf has -b, -w
> flags which might help these measurements.
Those options are enabled when one adds --enable-burst to the pre-compilation
./configure of netperf (one doesn't have to recompile netserver). However, if
one is also looking at latency statistics via the -j option in the top-of-trunk,
or simply at the histogram with --enable-histogram on the ./configure and a
verbosity level of 2 (global -v 2) then one wants the very top of trunk netperf
from:
http://www.netperf.org/svn/netperf2/trunk
to get the recently added support for accurate (netperf level) RTT measuremnts
on burst-mode request/response tests.
happy benchmarking,
rick jones
PS - the enhanced latency statistics from -j are only available in the "omni"
version of the TCP_RR test. To get that add a --enable-omni to the ./configure
- and in this case both netperf and netserver have to be recompiled. For very
basic output one can peruse the output of:
src/netperf -t omni -- -O /?
and then pick those outputs of interest and put them into an output selection
file which one then passes to either (test-specific) -o, -O or -k to get CVS,
"Human" or keyval output respectively. E.G.
raj@tardy:~/netperf2_trunk$ cat foo
THROUGHPUT,THROUGHPUT_UNITS
RT_LATENCY,MIN_LATENCY,MEAN_LATENCY,MAX_LATENCY
P50_LATENCY,P90_LATENCY,P99_LATENCY,STDDEV_LATENCY
when foo is passed to -o one will get those all on one line of CSV. To -O one
gets three lines of more netperf-classic-like "human" readable output, and when
one passes that to -k one gets a string of keyval output a la:
raj@tardy:~/netperf2_trunk$ src/netperf -t omni -j -v 2 -- -r 1 -d rr -k foo
OMNI TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to localhost (127.0.0.1) port 0
AF_INET : histogram
THROUGHPUT=29454.12
THROUGHPUT_UNITS=Trans/s
RT_LATENCY=33.951
MIN_LATENCY=19
MEAN_LATENCY=32.00
MAX_LATENCY=126
P50_LATENCY=32
P90_LATENCY=38
P99_LATENCY=41
STDDEV_LATENCY=5.46
Histogram of request/response times
UNIT_USEC : 0: 0: 0: 0: 0: 0: 0: 0: 0: 0
TEN_USEC : 0: 3553: 45244: 237790: 7859: 86: 10: 3: 0: 0
HUNDRED_USEC : 0: 2: 0: 0: 0: 0: 0: 0: 0: 0
UNIT_MSEC : 0: 0: 0: 0: 0: 0: 0: 0: 0: 0
TEN_MSEC : 0: 0: 0: 0: 0: 0: 0: 0: 0: 0
HUNDRED_MSEC : 0: 0: 0: 0: 0: 0: 0: 0: 0: 0
UNIT_SEC : 0: 0: 0: 0: 0: 0: 0: 0: 0: 0
TEN_SEC : 0: 0: 0: 0: 0: 0: 0: 0: 0: 0
>100_SECS: 0
HIST_TOTAL: 294547
^ permalink raw reply
* Re: [PATCH] vhost: rcu annotation fixup
From: Paul E. McKenney @ 2011-01-18 19:02 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Jason Wang, kvm, virtualization, netdev, linux-kernel
In-Reply-To: <20110118175500.GA6935@redhat.com>
On Tue, Jan 18, 2011 at 07:55:00PM +0200, Michael S. Tsirkin wrote:
> On Tue, Jan 18, 2011 at 09:48:34AM -0800, Paul E. McKenney wrote:
> > On Tue, Jan 18, 2011 at 01:08:45PM +0200, Michael S. Tsirkin wrote:
> > > When built with rcu checks enabled, vhost triggers
> > > bogus warnings as vhost features are read without
> > > dev->mutex sometimes.
> > > Fixing it properly is not trivial as vhost.h does not
> > > know which lockdep classes it will be used under.
> > > Disable the warning by stubbing out the check for now.
> > >
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > > drivers/vhost/vhost.h | 4 +---
> > > 1 files changed, 1 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> > > index 2af44b7..2d03a31 100644
> > > --- a/drivers/vhost/vhost.h
> > > +++ b/drivers/vhost/vhost.h
> > > @@ -173,9 +173,7 @@ static inline int vhost_has_feature(struct vhost_dev *dev, int bit)
> > > {
> > > unsigned acked_features;
> > >
> > > - acked_features =
> > > - rcu_dereference_index_check(dev->acked_features,
> > > - lockdep_is_held(&dev->mutex));
> > > + acked_features = rcu_dereference_index_check(dev->acked_features, 1);
> >
> > Ouch!!!
> >
> > Could you please at least add a comment?
>
> Yes, OK.
>
> > Alternatively, pass in the lock that is held and check for that? Given
> > that this is a static inline, the compiler should be able to optimize
> > the argument away when !PROVE_RCU, correct?
> >
> > Thanx, Paul
>
> Hopefully, yes. We don't always have a lock: the idea was
> to create a lockdep for these cases. But we can't pass
> the pointer to that ...
I suppose you could pass a pointer to the lockdep map structure.
Not sure if this makes sense, but it would handle the situation.
Alternatively, create a helper function that checks the possibilities
and screams if none of them are in effect.
Thanx, Paul
> > > return acked_features & (1 << bit);
> > > }
> > >
> > > --
> > > 1.7.3.2.91.g446ac
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* [PATCHv2] vhost: rcu annotation fixup
From: Michael S. Tsirkin @ 2011-01-18 18:08 UTC (permalink / raw)
To: Paul E. McKenney, jasowang
Cc: Michael S. Tsirkin, kvm, virtualization, netdev, linux-kernel
When built with rcu checks enabled, vhost triggers
bogus warnings as vhost features are read without
dev->mutex sometimes, and private pointer is read
with our kind of rcu where work serves as a
read side critical section.
Fixing it properly is not trivial.
Disable the warnings by stubbing out the checks for now.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
Changes from v1: add TODO, fix more warnings.
drivers/vhost/net.c | 9 +++++----
drivers/vhost/vhost.h | 6 +++---
2 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 9b3ca10..f616cef 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -128,8 +128,7 @@ static void handle_tx(struct vhost_net *net)
size_t hdr_size;
struct socket *sock;
- /* TODO: check that we are running from vhost_worker?
- * Not sure it's worth it, it's straight-forward enough. */
+ /* TODO: check that we are running from vhost_worker? */
sock = rcu_dereference_check(vq->private_data, 1);
if (!sock)
return;
@@ -306,7 +305,8 @@ static void handle_rx_big(struct vhost_net *net)
size_t len, total_len = 0;
int err;
size_t hdr_size;
- struct socket *sock = rcu_dereference(vq->private_data);
+ /* TODO: check that we are running from vhost_worker? */
+ struct socket *sock = rcu_dereference_check(vq->private_data, 1);
if (!sock || skb_queue_empty(&sock->sk->sk_receive_queue))
return;
@@ -415,7 +415,8 @@ static void handle_rx_mergeable(struct vhost_net *net)
int err, headcount;
size_t vhost_hlen, sock_hlen;
size_t vhost_len, sock_len;
- struct socket *sock = rcu_dereference(vq->private_data);
+ /* TODO: check that we are running from vhost_worker? */
+ struct socket *sock = rcu_dereference_check(vq->private_data, 1);
if (!sock || skb_queue_empty(&sock->sk->sk_receive_queue))
return;
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 2af44b7..b3363ae 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -173,9 +173,9 @@ static inline int vhost_has_feature(struct vhost_dev *dev, int bit)
{
unsigned acked_features;
- acked_features =
- rcu_dereference_index_check(dev->acked_features,
- lockdep_is_held(&dev->mutex));
+ /* TODO: check that we are running from vhost_worker or dev mutex is
+ * held? */
+ acked_features = rcu_dereference_index_check(dev->acked_features, 1);
return acked_features & (1 << bit);
}
--
1.7.3.2.91.g446ac
^ permalink raw reply related
* Re: [PATCH] vhost: rcu annotation fixup
From: Michael S. Tsirkin @ 2011-01-18 17:55 UTC (permalink / raw)
To: Paul E. McKenney; +Cc: Jason Wang, kvm, virtualization, netdev, linux-kernel
In-Reply-To: <20110118174834.GF2193@linux.vnet.ibm.com>
On Tue, Jan 18, 2011 at 09:48:34AM -0800, Paul E. McKenney wrote:
> On Tue, Jan 18, 2011 at 01:08:45PM +0200, Michael S. Tsirkin wrote:
> > When built with rcu checks enabled, vhost triggers
> > bogus warnings as vhost features are read without
> > dev->mutex sometimes.
> > Fixing it properly is not trivial as vhost.h does not
> > know which lockdep classes it will be used under.
> > Disable the warning by stubbing out the check for now.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > drivers/vhost/vhost.h | 4 +---
> > 1 files changed, 1 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> > index 2af44b7..2d03a31 100644
> > --- a/drivers/vhost/vhost.h
> > +++ b/drivers/vhost/vhost.h
> > @@ -173,9 +173,7 @@ static inline int vhost_has_feature(struct vhost_dev *dev, int bit)
> > {
> > unsigned acked_features;
> >
> > - acked_features =
> > - rcu_dereference_index_check(dev->acked_features,
> > - lockdep_is_held(&dev->mutex));
> > + acked_features = rcu_dereference_index_check(dev->acked_features, 1);
>
> Ouch!!!
>
> Could you please at least add a comment?
Yes, OK.
> Alternatively, pass in the lock that is held and check for that? Given
> that this is a static inline, the compiler should be able to optimize
> the argument away when !PROVE_RCU, correct?
>
> Thanx, Paul
Hopefully, yes. We don't always have a lock: the idea was
to create a lockdep for these cases. But we can't pass
the pointer to that ...
> > return acked_features & (1 << bit);
> > }
> >
> > --
> > 1.7.3.2.91.g446ac
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: [PATCH] vhost: rcu annotation fixup
From: Paul E. McKenney @ 2011-01-18 17:48 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Jason Wang, kvm, virtualization, netdev, linux-kernel
In-Reply-To: <20110118110845.GA11555@redhat.com>
On Tue, Jan 18, 2011 at 01:08:45PM +0200, Michael S. Tsirkin wrote:
> When built with rcu checks enabled, vhost triggers
> bogus warnings as vhost features are read without
> dev->mutex sometimes.
> Fixing it properly is not trivial as vhost.h does not
> know which lockdep classes it will be used under.
> Disable the warning by stubbing out the check for now.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/vhost/vhost.h | 4 +---
> 1 files changed, 1 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index 2af44b7..2d03a31 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -173,9 +173,7 @@ static inline int vhost_has_feature(struct vhost_dev *dev, int bit)
> {
> unsigned acked_features;
>
> - acked_features =
> - rcu_dereference_index_check(dev->acked_features,
> - lockdep_is_held(&dev->mutex));
> + acked_features = rcu_dereference_index_check(dev->acked_features, 1);
Ouch!!!
Could you please at least add a comment?
Alternatively, pass in the lock that is held and check for that? Given
that this is a static inline, the compiler should be able to optimize
the argument away when !PROVE_RCU, correct?
Thanx, Paul
> return acked_features & (1 << bit);
> }
>
> --
> 1.7.3.2.91.g446ac
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* [PATCH] vhost: rcu annotation fixup
From: Michael S. Tsirkin @ 2011-01-18 11:08 UTC (permalink / raw)
To: Jason Wang, kvm, virtualization, netdev, linux-kernel
When built with rcu checks enabled, vhost triggers
bogus warnings as vhost features are read without
dev->mutex sometimes.
Fixing it properly is not trivial as vhost.h does not
know which lockdep classes it will be used under.
Disable the warning by stubbing out the check for now.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/vhost/vhost.h | 4 +---
1 files changed, 1 insertions(+), 3 deletions(-)
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 2af44b7..2d03a31 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -173,9 +173,7 @@ static inline int vhost_has_feature(struct vhost_dev *dev, int bit)
{
unsigned acked_features;
- acked_features =
- rcu_dereference_index_check(dev->acked_features,
- lockdep_is_held(&dev->mutex));
+ acked_features = rcu_dereference_index_check(dev->acked_features, 1);
return acked_features & (1 << bit);
}
--
1.7.3.2.91.g446ac
^ permalink raw reply related
* Re: [PATCH 1/3] vhost-net: check the support of mergeable buffer outside the receive loop
From: Jason Wang @ 2011-01-18 9:15 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Jason Wang, virtualization, netdev, kvm, linux-kernel
In-Reply-To: <20110118043650.GA2233@redhat.com>
Michael S. Tsirkin writes:
> On Tue, Jan 18, 2011 at 12:26:17PM +0800, Jason Wang wrote:
> > Michael S. Tsirkin writes:
> > > On Mon, Jan 17, 2011 at 04:10:59PM +0800, Jason Wang wrote:
> > > > No need to check the support of mergeable buffer inside the recevie
> > > > loop as the whole handle_rx()_xx is in the read critical region. So
> > > > this patch move it ahead of the receiving loop.
> > > >
> > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > >
> > > Well feature check is mostly just features & bit
> > > so why would it be slower? Because of the rcu
> > > adding memory barriers? Do you observe a
> > > measureable speedup with this patch?
> > >
> >
> > I do not measure the performance for just this patch, maybe not obvious. And it
> > can also help the code unification.
> >
> > > Apropos, I noticed that the check in vhost_has_feature
> > > is wrong: it uses the same kind of RCU as the
> > > private pointer. So we'll have to fix that properly
> > > by adding more lockdep classes, but for now
> > > we'll need to make
> > > the check 1 || lockdep_is_held(&dev->mutex);
> > > and add a TODO.
> > >
> >
> > Not sure, lockdep_is_head(&dev->mutex) maybe not accurate but sufficient, as it
> > was always held in the read critical region.
>
> Not really, when we call vhost_has_feature from the vq handling thread
> it's not.
>
Seems not, see vhost_net_ioctl().
> > > > ---
> > > > drivers/vhost/net.c | 5 +++--
> > > > 1 files changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > > > index 14fc189..95e49de 100644
> > > > --- a/drivers/vhost/net.c
> > > > +++ b/drivers/vhost/net.c
> > > > @@ -411,7 +411,7 @@ static void handle_rx_mergeable(struct vhost_net *net)
> > > > };
> > > >
> > > > size_t total_len = 0;
> > > > - int err, headcount;
> > > > + int err, headcount, mergeable;
> > > > size_t vhost_hlen, sock_hlen;
> > > > size_t vhost_len, sock_len;
> > > > struct socket *sock = rcu_dereference(vq->private_data);
> > > > @@ -425,6 +425,7 @@ static void handle_rx_mergeable(struct vhost_net *net)
> > > >
> > > > vq_log = unlikely(vhost_has_feature(&net->dev, VHOST_F_LOG_ALL)) ?
> > > > vq->log : NULL;
> > > > + mergeable = vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF);
> > > >
> > > > while ((sock_len = peek_head_len(sock->sk))) {
> > > > sock_len += sock_hlen;
> > > > @@ -474,7 +475,7 @@ static void handle_rx_mergeable(struct vhost_net *net)
> > > > break;
> > > > }
> > > > /* TODO: Should check and handle checksum. */
> > > > - if (vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF) &&
> > > > + if (likely(mergeable) &&
> > > > memcpy_toiovecend(vq->hdr, (unsigned char *)&headcount,
> > > > offsetof(typeof(hdr), num_buffers),
> > > > sizeof hdr.num_buffers)) {
^ permalink raw reply
* Re: [PATCH 2/3] vhost-net: Unify the code of mergeable and big buffer handling
From: Jason Wang @ 2011-01-18 7:41 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Jason Wang, virtualization, netdev, kvm, linux-kernel
In-Reply-To: <20110118043725.GB2233@redhat.com>
Michael S. Tsirkin writes:
> On Tue, Jan 18, 2011 at 11:05:33AM +0800, Jason Wang wrote:
> > Michael S. Tsirkin writes:
> > > On Mon, Jan 17, 2011 at 04:11:08PM +0800, Jason Wang wrote:
> > > > Codes duplication were found between the handling of mergeable and big
> > > > buffers, so this patch tries to unify them. This could be easily done
> > > > by adding a quota to the get_rx_bufs() which is used to limit the
> > > > number of buffers it returns (for mergeable buffer, the quota is
> > > > simply UIO_MAXIOV, for big buffers, the quota is just 1), and then the
> > > > previous handle_rx_mergeable() could be resued also for big buffers.
> > > >
> > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > >
> > > We actually started this way. However the code turned out
> > > to have measureable overhead when handle_rx_mergeable
> > > is called with quota 1.
> > > So before applying this I'd like to see some data
> > > to show this is not the case anymore.
> > >
> >
> > I've run a round of test (Host->Guest) for these three patches on my desktop:
>
> Yes but what if you only apply patch 3?
>
Result here, slightly better than without it but worse than applying all the three
patches except for big buffer size at 2048 but the differnece could be neglected.
mergeable buffers:
TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 10.66.91.161 (10.66.91.161) port 0 AF_INET
Recv Send Send Utilization Service Demand
Socket Socket Message Elapsed Send Recv Send Recv
Size Size Size Time Throughput local remote local remote
bytes bytes bytes secs. 10^6bits/s % S % S us/KB us/KB
87380 16384 64 60.00 584.91 69.41 26.10 38.882 7.310
87380 16384 256 60.00 1194.05 72.81 34.82 19.980 4.778
87380 16384 512 60.00 1503.93 74.23 36.86 16.174 4.016
87380 16384 1024 60.00 2004.57 73.53 37.59 12.019 3.073
87380 16384 2048 60.00 3445.96 73.76 38.88 7.014 1.849
big buffers:
TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 10.66.91.129 (10.66.91.129) port 0 AF_INET
Recv Send Send Utilization Service Demand
Socket Socket Message Elapsed Send Recv Send Recv
Size Size Size Time Throughput local remote local remote
bytes bytes bytes secs. 10^6bits/s % S % S us/KB us/KB
87380 16384 64 60.00 646.95 72.25 27.05 36.595 6.850
87380 16384 256 60.00 1258.61 74.88 33.01 19.495 4.297
87380 16384 512 60.00 1655.95 74.14 33.96 14.671 3.360
87380 16384 1024 60.00 3220.32 74.24 33.31 7.555 1.695
87380 16384 2048 60.00 4516.40 73.88 42.10 5.360 1.527
> > Without these patches
> >
> > mergeable buffers:
> > TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 10.66.91.42 (10.66.91.42) port 0 AF_INET
> > Recv Send Send Utilization Service Demand
> > Socket Socket Message Elapsed Send Recv Send Recv
> > Size Size Size Time Throughput local remote local remote
> > bytes bytes bytes secs. 10^6bits/s % S % S us/KB us/KB
> >
> > 87380 16384 64 60.00 575.87 69.20 26.36 39.375 7.499
> > 87380 16384 256 60.01 1123.57 73.16 34.73 21.335 5.064
> > 87380 16384 512 60.00 1351.12 75.26 35.80 18.251 4.341
> > 87380 16384 1024 60.00 1955.31 74.73 37.67 12.523 3.156
> > 87380 16384 2048 60.01 3411.92 74.82 39.49 7.186 1.896
> >
> > bug buffers:
> > Netperf test results
> > TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 10.66.91.109 (10.66.91.109) port 0 AF_INET
> > Recv Send Send Utilization Service Demand
> > Socket Socket Message Elapsed Send Recv Send Recv
> > Size Size Size Time Throughput local remote local remote
> > bytes bytes bytes secs. 10^6bits/s % S % S us/KB us/KB
> >
> > 87380 16384 64 60.00 567.10 72.06 26.13 41.638 7.550
> > 87380 16384 256 60.00 1143.69 74.66 32.58 21.392 4.667
> > 87380 16384 512 60.00 1460.92 73.94 33.40 16.585 3.746
> > 87380 16384 1024 60.00 3454.85 77.49 33.89 7.349 1.607
> > 87380 16384 2048 60.00 3781.11 76.51 38.38 6.631 1.663
> >
> > With these patches:
> >
> > mergeable buffers:
> > TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 10.66.91.236 (10.66.91.236) port 0 AF_INET
> > Recv Send Send Utilization Service Demand
> > Socket Socket Message Elapsed Send Recv Send Recv
> > Size Size Size Time Throughput local remote local remote
> > bytes bytes bytes secs. 10^6bits/s % S % S us/KB us/KB
> >
> > 87380 16384 64 60.00 657.53 71.27 26.42 35.517 6.583
> > 87380 16384 256 60.00 1217.73 74.34 34.67 20.004 4.665
> > 87380 16384 512 60.00 1575.25 75.27 37.12 15.658 3.861
> > 87380 16384 1024 60.00 2416.07 74.77 37.20 10.140 2.522
> > 87380 16384 2048 60.00 3702.29 77.31 36.29 6.842 1.606
> >
> > big buffers:
> > Netperf test results
> > TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 10.66.91.202 (10.66.91.202) port 0 AF_INET
> > Recv Send Send Utilization Service Demand
> > Socket Socket Message Elapsed Send Recv Send Recv
> > Size Size Size Time Throughput local remote local remote
> > bytes bytes bytes secs. 10^6bits/s % S % S us/KB us/KB
> >
> > 87380 16384 64 60.00 647.67 71.86 27.26 36.356 6.895
> > 87380 16384 256 60.00 1265.82 76.19 36.54 19.724 4.729
> > 87380 16384 512 60.00 1796.64 76.06 39.48 13.872 3.601
> > 87380 16384 1024 60.00 4008.37 77.05 36.47 6.299 1.491
> > 87380 16384 2048 60.00 4468.56 75.18 41.79 5.513 1.532
> >
> > Looks like the unification does not hurt the performance, and with those patches
> > we can get some improvement. BTW, the regression of mergeable buffer still
> > exist.
> >
> > > > ---
> > > > drivers/vhost/net.c | 128 +++------------------------------------------------
> > > > 1 files changed, 7 insertions(+), 121 deletions(-)
> > > >
> > > > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > > > index 95e49de..c32a2e4 100644
> > > > --- a/drivers/vhost/net.c
> > > > +++ b/drivers/vhost/net.c
> > > > @@ -227,6 +227,7 @@ static int peek_head_len(struct sock *sk)
> > > > * @iovcount - returned count of io vectors we fill
> > > > * @log - vhost log
> > > > * @log_num - log offset
> > > > + * @quota - headcount quota, 1 for big buffer
> > > > * returns number of buffer heads allocated, negative on error
> > > > */
> > > > static int get_rx_bufs(struct vhost_virtqueue *vq,
> > > > @@ -234,7 +235,8 @@ static int get_rx_bufs(struct vhost_virtqueue *vq,
> > > > int datalen,
> > > > unsigned *iovcount,
> > > > struct vhost_log *log,
> > > > - unsigned *log_num)
> > > > + unsigned *log_num,
> > > > + unsigned int quota)
> > > > {
> > > > unsigned int out, in;
> > > > int seg = 0;
> > > > @@ -242,7 +244,7 @@ static int get_rx_bufs(struct vhost_virtqueue *vq,
> > > > unsigned d;
> > > > int r, nlogs = 0;
> > > >
> > > > - while (datalen > 0) {
> > > > + while (datalen > 0 && headcount < quota) {
> > > > if (unlikely(seg >= UIO_MAXIOV)) {
> > > > r = -ENOBUFS;
> > > > goto err;
> > > > @@ -282,116 +284,7 @@ err:
> > > >
> > > > /* Expects to be always run from workqueue - which acts as
> > > > * read-size critical section for our kind of RCU. */
> > > > -static void handle_rx_big(struct vhost_net *net)
> > > > -{
> > > > - struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_RX];
> > > > - unsigned out, in, log, s;
> > > > - int head;
> > > > - struct vhost_log *vq_log;
> > > > - struct msghdr msg = {
> > > > - .msg_name = NULL,
> > > > - .msg_namelen = 0,
> > > > - .msg_control = NULL, /* FIXME: get and handle RX aux data. */
> > > > - .msg_controllen = 0,
> > > > - .msg_iov = vq->iov,
> > > > - .msg_flags = MSG_DONTWAIT,
> > > > - };
> > > > -
> > > > - struct virtio_net_hdr hdr = {
> > > > - .flags = 0,
> > > > - .gso_type = VIRTIO_NET_HDR_GSO_NONE
> > > > - };
> > > > -
> > > > - size_t len, total_len = 0;
> > > > - int err;
> > > > - size_t hdr_size;
> > > > - struct socket *sock = rcu_dereference(vq->private_data);
> > > > - if (!sock || skb_queue_empty(&sock->sk->sk_receive_queue))
> > > > - return;
> > > > -
> > > > - mutex_lock(&vq->mutex);
> > > > - vhost_disable_notify(vq);
> > > > - hdr_size = 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);
> > > > - /* On error, stop handling until the next kick. */
> > > > - if (unlikely(head < 0))
> > > > - break;
> > > > - /* OK, now we need to know about added descriptors. */
> > > > - if (head == vq->num) {
> > > > - if (unlikely(vhost_enable_notify(vq))) {
> > > > - /* They have slipped one in as we were
> > > > - * doing that: check again. */
> > > > - vhost_disable_notify(vq);
> > > > - continue;
> > > > - }
> > > > - /* Nothing new? Wait for eventfd to tell us
> > > > - * they refilled. */
> > > > - 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);
> > > > - 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);
> > > > - 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, 1);
> > > > - break;
> > > > - }
> > > > - /* TODO: Should check and handle checksum. */
> > > > - if (err > len) {
> > > > - pr_debug("Discarded truncated rx packet: "
> > > > - " len %d > %zd\n", err, len);
> > > > - vhost_discard_vq_desc(vq, 1);
> > > > - 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);
> > > > - break;
> > > > - }
> > > > - len += hdr_size;
> > > > - vhost_add_used_and_signal(&net->dev, vq, head, len);
> > > > - if (unlikely(vq_log))
> > > > - vhost_log_write(vq, vq_log, log, len);
> > > > - total_len += len;
> > > > - if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
> > > > - vhost_poll_queue(&vq->poll);
> > > > - break;
> > > > - }
> > > > - }
> > > > -
> > > > - mutex_unlock(&vq->mutex);
> > > > -}
> > > > -
> > > > -/* Expects to be always run from workqueue - which acts as
> > > > - * read-size critical section for our kind of RCU. */
> > > > -static void handle_rx_mergeable(struct vhost_net *net)
> > > > +static void handle_rx(struct vhost_net *net)
> > > > {
> > > > struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_RX];
> > > > unsigned uninitialized_var(in), log;
> > > > @@ -431,7 +324,8 @@ static void handle_rx_mergeable(struct vhost_net *net)
> > > > sock_len += sock_hlen;
> > > > vhost_len = sock_len + vhost_hlen;
> > > > headcount = get_rx_bufs(vq, vq->heads, vhost_len,
> > > > - &in, vq_log, &log);
> > > > + &in, vq_log, &log,
> > > > + likely(mergeable) ? UIO_MAXIOV : 1);
> > > > /* On error, stop handling until the next kick. */
> > > > if (unlikely(headcount < 0))
> > > > break;
> > > > @@ -497,14 +391,6 @@ static void handle_rx_mergeable(struct vhost_net *net)
> > > > mutex_unlock(&vq->mutex);
> > > > }
> > > >
> > > > -static void handle_rx(struct vhost_net *net)
> > > > -{
> > > > - if (vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF))
> > > > - handle_rx_mergeable(net);
> > > > - else
> > > > - handle_rx_big(net);
> > > > -}
> > > > -
> > > > static void handle_tx_kick(struct vhost_work *work)
> > > > {
> > > > struct vhost_virtqueue *vq = container_of(work, struct vhost_virtqueue,
^ permalink raw reply
* Re: [PATCH 2/3] vhost-net: Unify the code of mergeable and big buffer handling
From: Michael S. Tsirkin @ 2011-01-18 4:37 UTC (permalink / raw)
To: Jason Wang; +Cc: virtualization, netdev, kvm, linux-kernel
In-Reply-To: <19765.893.776528.869640@gargle.gargle.HOWL>
On Tue, Jan 18, 2011 at 11:05:33AM +0800, Jason Wang wrote:
> Michael S. Tsirkin writes:
> > On Mon, Jan 17, 2011 at 04:11:08PM +0800, Jason Wang wrote:
> > > Codes duplication were found between the handling of mergeable and big
> > > buffers, so this patch tries to unify them. This could be easily done
> > > by adding a quota to the get_rx_bufs() which is used to limit the
> > > number of buffers it returns (for mergeable buffer, the quota is
> > > simply UIO_MAXIOV, for big buffers, the quota is just 1), and then the
> > > previous handle_rx_mergeable() could be resued also for big buffers.
> > >
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> >
> > We actually started this way. However the code turned out
> > to have measureable overhead when handle_rx_mergeable
> > is called with quota 1.
> > So before applying this I'd like to see some data
> > to show this is not the case anymore.
> >
>
> I've run a round of test (Host->Guest) for these three patches on my desktop:
Yes but what if you only apply patch 3?
> Without these patches
>
> mergeable buffers:
> TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 10.66.91.42 (10.66.91.42) port 0 AF_INET
> Recv Send Send Utilization Service Demand
> Socket Socket Message Elapsed Send Recv Send Recv
> Size Size Size Time Throughput local remote local remote
> bytes bytes bytes secs. 10^6bits/s % S % S us/KB us/KB
>
> 87380 16384 64 60.00 575.87 69.20 26.36 39.375 7.499
> 87380 16384 256 60.01 1123.57 73.16 34.73 21.335 5.064
> 87380 16384 512 60.00 1351.12 75.26 35.80 18.251 4.341
> 87380 16384 1024 60.00 1955.31 74.73 37.67 12.523 3.156
> 87380 16384 2048 60.01 3411.92 74.82 39.49 7.186 1.896
>
> bug buffers:
> Netperf test results
> TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 10.66.91.109 (10.66.91.109) port 0 AF_INET
> Recv Send Send Utilization Service Demand
> Socket Socket Message Elapsed Send Recv Send Recv
> Size Size Size Time Throughput local remote local remote
> bytes bytes bytes secs. 10^6bits/s % S % S us/KB us/KB
>
> 87380 16384 64 60.00 567.10 72.06 26.13 41.638 7.550
> 87380 16384 256 60.00 1143.69 74.66 32.58 21.392 4.667
> 87380 16384 512 60.00 1460.92 73.94 33.40 16.585 3.746
> 87380 16384 1024 60.00 3454.85 77.49 33.89 7.349 1.607
> 87380 16384 2048 60.00 3781.11 76.51 38.38 6.631 1.663
>
> With these patches:
>
> mergeable buffers:
> TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 10.66.91.236 (10.66.91.236) port 0 AF_INET
> Recv Send Send Utilization Service Demand
> Socket Socket Message Elapsed Send Recv Send Recv
> Size Size Size Time Throughput local remote local remote
> bytes bytes bytes secs. 10^6bits/s % S % S us/KB us/KB
>
> 87380 16384 64 60.00 657.53 71.27 26.42 35.517 6.583
> 87380 16384 256 60.00 1217.73 74.34 34.67 20.004 4.665
> 87380 16384 512 60.00 1575.25 75.27 37.12 15.658 3.861
> 87380 16384 1024 60.00 2416.07 74.77 37.20 10.140 2.522
> 87380 16384 2048 60.00 3702.29 77.31 36.29 6.842 1.606
>
> big buffers:
> Netperf test results
> TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 10.66.91.202 (10.66.91.202) port 0 AF_INET
> Recv Send Send Utilization Service Demand
> Socket Socket Message Elapsed Send Recv Send Recv
> Size Size Size Time Throughput local remote local remote
> bytes bytes bytes secs. 10^6bits/s % S % S us/KB us/KB
>
> 87380 16384 64 60.00 647.67 71.86 27.26 36.356 6.895
> 87380 16384 256 60.00 1265.82 76.19 36.54 19.724 4.729
> 87380 16384 512 60.00 1796.64 76.06 39.48 13.872 3.601
> 87380 16384 1024 60.00 4008.37 77.05 36.47 6.299 1.491
> 87380 16384 2048 60.00 4468.56 75.18 41.79 5.513 1.532
>
> Looks like the unification does not hurt the performance, and with those patches
> we can get some improvement. BTW, the regression of mergeable buffer still
> exist.
>
> > > ---
> > > drivers/vhost/net.c | 128 +++------------------------------------------------
> > > 1 files changed, 7 insertions(+), 121 deletions(-)
> > >
> > > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > > index 95e49de..c32a2e4 100644
> > > --- a/drivers/vhost/net.c
> > > +++ b/drivers/vhost/net.c
> > > @@ -227,6 +227,7 @@ static int peek_head_len(struct sock *sk)
> > > * @iovcount - returned count of io vectors we fill
> > > * @log - vhost log
> > > * @log_num - log offset
> > > + * @quota - headcount quota, 1 for big buffer
> > > * returns number of buffer heads allocated, negative on error
> > > */
> > > static int get_rx_bufs(struct vhost_virtqueue *vq,
> > > @@ -234,7 +235,8 @@ static int get_rx_bufs(struct vhost_virtqueue *vq,
> > > int datalen,
> > > unsigned *iovcount,
> > > struct vhost_log *log,
> > > - unsigned *log_num)
> > > + unsigned *log_num,
> > > + unsigned int quota)
> > > {
> > > unsigned int out, in;
> > > int seg = 0;
> > > @@ -242,7 +244,7 @@ static int get_rx_bufs(struct vhost_virtqueue *vq,
> > > unsigned d;
> > > int r, nlogs = 0;
> > >
> > > - while (datalen > 0) {
> > > + while (datalen > 0 && headcount < quota) {
> > > if (unlikely(seg >= UIO_MAXIOV)) {
> > > r = -ENOBUFS;
> > > goto err;
> > > @@ -282,116 +284,7 @@ err:
> > >
> > > /* Expects to be always run from workqueue - which acts as
> > > * read-size critical section for our kind of RCU. */
> > > -static void handle_rx_big(struct vhost_net *net)
> > > -{
> > > - struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_RX];
> > > - unsigned out, in, log, s;
> > > - int head;
> > > - struct vhost_log *vq_log;
> > > - struct msghdr msg = {
> > > - .msg_name = NULL,
> > > - .msg_namelen = 0,
> > > - .msg_control = NULL, /* FIXME: get and handle RX aux data. */
> > > - .msg_controllen = 0,
> > > - .msg_iov = vq->iov,
> > > - .msg_flags = MSG_DONTWAIT,
> > > - };
> > > -
> > > - struct virtio_net_hdr hdr = {
> > > - .flags = 0,
> > > - .gso_type = VIRTIO_NET_HDR_GSO_NONE
> > > - };
> > > -
> > > - size_t len, total_len = 0;
> > > - int err;
> > > - size_t hdr_size;
> > > - struct socket *sock = rcu_dereference(vq->private_data);
> > > - if (!sock || skb_queue_empty(&sock->sk->sk_receive_queue))
> > > - return;
> > > -
> > > - mutex_lock(&vq->mutex);
> > > - vhost_disable_notify(vq);
> > > - hdr_size = 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);
> > > - /* On error, stop handling until the next kick. */
> > > - if (unlikely(head < 0))
> > > - break;
> > > - /* OK, now we need to know about added descriptors. */
> > > - if (head == vq->num) {
> > > - if (unlikely(vhost_enable_notify(vq))) {
> > > - /* They have slipped one in as we were
> > > - * doing that: check again. */
> > > - vhost_disable_notify(vq);
> > > - continue;
> > > - }
> > > - /* Nothing new? Wait for eventfd to tell us
> > > - * they refilled. */
> > > - 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);
> > > - 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);
> > > - 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, 1);
> > > - break;
> > > - }
> > > - /* TODO: Should check and handle checksum. */
> > > - if (err > len) {
> > > - pr_debug("Discarded truncated rx packet: "
> > > - " len %d > %zd\n", err, len);
> > > - vhost_discard_vq_desc(vq, 1);
> > > - 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);
> > > - break;
> > > - }
> > > - len += hdr_size;
> > > - vhost_add_used_and_signal(&net->dev, vq, head, len);
> > > - if (unlikely(vq_log))
> > > - vhost_log_write(vq, vq_log, log, len);
> > > - total_len += len;
> > > - if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
> > > - vhost_poll_queue(&vq->poll);
> > > - break;
> > > - }
> > > - }
> > > -
> > > - mutex_unlock(&vq->mutex);
> > > -}
> > > -
> > > -/* Expects to be always run from workqueue - which acts as
> > > - * read-size critical section for our kind of RCU. */
> > > -static void handle_rx_mergeable(struct vhost_net *net)
> > > +static void handle_rx(struct vhost_net *net)
> > > {
> > > struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_RX];
> > > unsigned uninitialized_var(in), log;
> > > @@ -431,7 +324,8 @@ static void handle_rx_mergeable(struct vhost_net *net)
> > > sock_len += sock_hlen;
> > > vhost_len = sock_len + vhost_hlen;
> > > headcount = get_rx_bufs(vq, vq->heads, vhost_len,
> > > - &in, vq_log, &log);
> > > + &in, vq_log, &log,
> > > + likely(mergeable) ? UIO_MAXIOV : 1);
> > > /* On error, stop handling until the next kick. */
> > > if (unlikely(headcount < 0))
> > > break;
> > > @@ -497,14 +391,6 @@ static void handle_rx_mergeable(struct vhost_net *net)
> > > mutex_unlock(&vq->mutex);
> > > }
> > >
> > > -static void handle_rx(struct vhost_net *net)
> > > -{
> > > - if (vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF))
> > > - handle_rx_mergeable(net);
> > > - else
> > > - handle_rx_big(net);
> > > -}
> > > -
> > > static void handle_tx_kick(struct vhost_work *work)
> > > {
> > > struct vhost_virtqueue *vq = container_of(work, struct vhost_virtqueue,
^ permalink raw reply
* Re: [PATCH 1/3] vhost-net: check the support of mergeable buffer outside the receive loop
From: Michael S. Tsirkin @ 2011-01-18 4:36 UTC (permalink / raw)
To: Jason Wang; +Cc: virtualization, netdev, kvm, linux-kernel
In-Reply-To: <19765.5737.352179.50100@gargle.gargle.HOWL>
On Tue, Jan 18, 2011 at 12:26:17PM +0800, Jason Wang wrote:
> Michael S. Tsirkin writes:
> > On Mon, Jan 17, 2011 at 04:10:59PM +0800, Jason Wang wrote:
> > > No need to check the support of mergeable buffer inside the recevie
> > > loop as the whole handle_rx()_xx is in the read critical region. So
> > > this patch move it ahead of the receiving loop.
> > >
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> >
> > Well feature check is mostly just features & bit
> > so why would it be slower? Because of the rcu
> > adding memory barriers? Do you observe a
> > measureable speedup with this patch?
> >
>
> I do not measure the performance for just this patch, maybe not obvious. And it
> can also help the code unification.
>
> > Apropos, I noticed that the check in vhost_has_feature
> > is wrong: it uses the same kind of RCU as the
> > private pointer. So we'll have to fix that properly
> > by adding more lockdep classes, but for now
> > we'll need to make
> > the check 1 || lockdep_is_held(&dev->mutex);
> > and add a TODO.
> >
>
> Not sure, lockdep_is_head(&dev->mutex) maybe not accurate but sufficient, as it
> was always held in the read critical region.
Not really, when we call vhost_has_feature from the vq handling thread
it's not.
> > > ---
> > > drivers/vhost/net.c | 5 +++--
> > > 1 files changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > > index 14fc189..95e49de 100644
> > > --- a/drivers/vhost/net.c
> > > +++ b/drivers/vhost/net.c
> > > @@ -411,7 +411,7 @@ static void handle_rx_mergeable(struct vhost_net *net)
> > > };
> > >
> > > size_t total_len = 0;
> > > - int err, headcount;
> > > + int err, headcount, mergeable;
> > > size_t vhost_hlen, sock_hlen;
> > > size_t vhost_len, sock_len;
> > > struct socket *sock = rcu_dereference(vq->private_data);
> > > @@ -425,6 +425,7 @@ static void handle_rx_mergeable(struct vhost_net *net)
> > >
> > > vq_log = unlikely(vhost_has_feature(&net->dev, VHOST_F_LOG_ALL)) ?
> > > vq->log : NULL;
> > > + mergeable = vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF);
> > >
> > > while ((sock_len = peek_head_len(sock->sk))) {
> > > sock_len += sock_hlen;
> > > @@ -474,7 +475,7 @@ static void handle_rx_mergeable(struct vhost_net *net)
> > > break;
> > > }
> > > /* TODO: Should check and handle checksum. */
> > > - if (vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF) &&
> > > + if (likely(mergeable) &&
> > > memcpy_toiovecend(vq->hdr, (unsigned char *)&headcount,
> > > offsetof(typeof(hdr), num_buffers),
> > > sizeof hdr.num_buffers)) {
^ permalink raw reply
* Re: [PATCH 1/3] vhost-net: check the support of mergeable buffer outside the receive loop
From: Jason Wang @ 2011-01-18 4:26 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Jason Wang, virtualization, netdev, kvm, linux-kernel
In-Reply-To: <20110117084644.GB23479@redhat.com>
Michael S. Tsirkin writes:
> On Mon, Jan 17, 2011 at 04:10:59PM +0800, Jason Wang wrote:
> > No need to check the support of mergeable buffer inside the recevie
> > loop as the whole handle_rx()_xx is in the read critical region. So
> > this patch move it ahead of the receiving loop.
> >
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
>
> Well feature check is mostly just features & bit
> so why would it be slower? Because of the rcu
> adding memory barriers? Do you observe a
> measureable speedup with this patch?
>
I do not measure the performance for just this patch, maybe not obvious. And it
can also help the code unification.
> Apropos, I noticed that the check in vhost_has_feature
> is wrong: it uses the same kind of RCU as the
> private pointer. So we'll have to fix that properly
> by adding more lockdep classes, but for now
> we'll need to make
> the check 1 || lockdep_is_held(&dev->mutex);
> and add a TODO.
>
Not sure, lockdep_is_head(&dev->mutex) maybe not accurate but sufficient, as it
was always held in the read critical region.
> > ---
> > drivers/vhost/net.c | 5 +++--
> > 1 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > index 14fc189..95e49de 100644
> > --- a/drivers/vhost/net.c
> > +++ b/drivers/vhost/net.c
> > @@ -411,7 +411,7 @@ static void handle_rx_mergeable(struct vhost_net *net)
> > };
> >
> > size_t total_len = 0;
> > - int err, headcount;
> > + int err, headcount, mergeable;
> > size_t vhost_hlen, sock_hlen;
> > size_t vhost_len, sock_len;
> > struct socket *sock = rcu_dereference(vq->private_data);
> > @@ -425,6 +425,7 @@ static void handle_rx_mergeable(struct vhost_net *net)
> >
> > vq_log = unlikely(vhost_has_feature(&net->dev, VHOST_F_LOG_ALL)) ?
> > vq->log : NULL;
> > + mergeable = vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF);
> >
> > while ((sock_len = peek_head_len(sock->sk))) {
> > sock_len += sock_hlen;
> > @@ -474,7 +475,7 @@ static void handle_rx_mergeable(struct vhost_net *net)
> > break;
> > }
> > /* TODO: Should check and handle checksum. */
> > - if (vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF) &&
> > + if (likely(mergeable) &&
> > memcpy_toiovecend(vq->hdr, (unsigned char *)&headcount,
> > offsetof(typeof(hdr), num_buffers),
> > sizeof hdr.num_buffers)) {
^ permalink raw reply
* Re: [PATCH 2/3] vhost-net: Unify the code of mergeable and big buffer handling
From: Jason Wang @ 2011-01-18 3:05 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Jason Wang, virtualization, netdev, kvm, linux-kernel
In-Reply-To: <20110117083644.GA23479@redhat.com>
Michael S. Tsirkin writes:
> On Mon, Jan 17, 2011 at 04:11:08PM +0800, Jason Wang wrote:
> > Codes duplication were found between the handling of mergeable and big
> > buffers, so this patch tries to unify them. This could be easily done
> > by adding a quota to the get_rx_bufs() which is used to limit the
> > number of buffers it returns (for mergeable buffer, the quota is
> > simply UIO_MAXIOV, for big buffers, the quota is just 1), and then the
> > previous handle_rx_mergeable() could be resued also for big buffers.
> >
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
>
> We actually started this way. However the code turned out
> to have measureable overhead when handle_rx_mergeable
> is called with quota 1.
> So before applying this I'd like to see some data
> to show this is not the case anymore.
>
I've run a round of test (Host->Guest) for these three patches on my desktop:
Without these patches
mergeable buffers:
TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 10.66.91.42 (10.66.91.42) port 0 AF_INET
Recv Send Send Utilization Service Demand
Socket Socket Message Elapsed Send Recv Send Recv
Size Size Size Time Throughput local remote local remote
bytes bytes bytes secs. 10^6bits/s % S % S us/KB us/KB
87380 16384 64 60.00 575.87 69.20 26.36 39.375 7.499
87380 16384 256 60.01 1123.57 73.16 34.73 21.335 5.064
87380 16384 512 60.00 1351.12 75.26 35.80 18.251 4.341
87380 16384 1024 60.00 1955.31 74.73 37.67 12.523 3.156
87380 16384 2048 60.01 3411.92 74.82 39.49 7.186 1.896
bug buffers:
Netperf test results
TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 10.66.91.109 (10.66.91.109) port 0 AF_INET
Recv Send Send Utilization Service Demand
Socket Socket Message Elapsed Send Recv Send Recv
Size Size Size Time Throughput local remote local remote
bytes bytes bytes secs. 10^6bits/s % S % S us/KB us/KB
87380 16384 64 60.00 567.10 72.06 26.13 41.638 7.550
87380 16384 256 60.00 1143.69 74.66 32.58 21.392 4.667
87380 16384 512 60.00 1460.92 73.94 33.40 16.585 3.746
87380 16384 1024 60.00 3454.85 77.49 33.89 7.349 1.607
87380 16384 2048 60.00 3781.11 76.51 38.38 6.631 1.663
With these patches:
mergeable buffers:
TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 10.66.91.236 (10.66.91.236) port 0 AF_INET
Recv Send Send Utilization Service Demand
Socket Socket Message Elapsed Send Recv Send Recv
Size Size Size Time Throughput local remote local remote
bytes bytes bytes secs. 10^6bits/s % S % S us/KB us/KB
87380 16384 64 60.00 657.53 71.27 26.42 35.517 6.583
87380 16384 256 60.00 1217.73 74.34 34.67 20.004 4.665
87380 16384 512 60.00 1575.25 75.27 37.12 15.658 3.861
87380 16384 1024 60.00 2416.07 74.77 37.20 10.140 2.522
87380 16384 2048 60.00 3702.29 77.31 36.29 6.842 1.606
big buffers:
Netperf test results
TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 10.66.91.202 (10.66.91.202) port 0 AF_INET
Recv Send Send Utilization Service Demand
Socket Socket Message Elapsed Send Recv Send Recv
Size Size Size Time Throughput local remote local remote
bytes bytes bytes secs. 10^6bits/s % S % S us/KB us/KB
87380 16384 64 60.00 647.67 71.86 27.26 36.356 6.895
87380 16384 256 60.00 1265.82 76.19 36.54 19.724 4.729
87380 16384 512 60.00 1796.64 76.06 39.48 13.872 3.601
87380 16384 1024 60.00 4008.37 77.05 36.47 6.299 1.491
87380 16384 2048 60.00 4468.56 75.18 41.79 5.513 1.532
Looks like the unification does not hurt the performance, and with those patches
we can get some improvement. BTW, the regression of mergeable buffer still
exist.
> > ---
> > drivers/vhost/net.c | 128 +++------------------------------------------------
> > 1 files changed, 7 insertions(+), 121 deletions(-)
> >
> > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > index 95e49de..c32a2e4 100644
> > --- a/drivers/vhost/net.c
> > +++ b/drivers/vhost/net.c
> > @@ -227,6 +227,7 @@ static int peek_head_len(struct sock *sk)
> > * @iovcount - returned count of io vectors we fill
> > * @log - vhost log
> > * @log_num - log offset
> > + * @quota - headcount quota, 1 for big buffer
> > * returns number of buffer heads allocated, negative on error
> > */
> > static int get_rx_bufs(struct vhost_virtqueue *vq,
> > @@ -234,7 +235,8 @@ static int get_rx_bufs(struct vhost_virtqueue *vq,
> > int datalen,
> > unsigned *iovcount,
> > struct vhost_log *log,
> > - unsigned *log_num)
> > + unsigned *log_num,
> > + unsigned int quota)
> > {
> > unsigned int out, in;
> > int seg = 0;
> > @@ -242,7 +244,7 @@ static int get_rx_bufs(struct vhost_virtqueue *vq,
> > unsigned d;
> > int r, nlogs = 0;
> >
> > - while (datalen > 0) {
> > + while (datalen > 0 && headcount < quota) {
> > if (unlikely(seg >= UIO_MAXIOV)) {
> > r = -ENOBUFS;
> > goto err;
> > @@ -282,116 +284,7 @@ err:
> >
> > /* Expects to be always run from workqueue - which acts as
> > * read-size critical section for our kind of RCU. */
> > -static void handle_rx_big(struct vhost_net *net)
> > -{
> > - struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_RX];
> > - unsigned out, in, log, s;
> > - int head;
> > - struct vhost_log *vq_log;
> > - struct msghdr msg = {
> > - .msg_name = NULL,
> > - .msg_namelen = 0,
> > - .msg_control = NULL, /* FIXME: get and handle RX aux data. */
> > - .msg_controllen = 0,
> > - .msg_iov = vq->iov,
> > - .msg_flags = MSG_DONTWAIT,
> > - };
> > -
> > - struct virtio_net_hdr hdr = {
> > - .flags = 0,
> > - .gso_type = VIRTIO_NET_HDR_GSO_NONE
> > - };
> > -
> > - size_t len, total_len = 0;
> > - int err;
> > - size_t hdr_size;
> > - struct socket *sock = rcu_dereference(vq->private_data);
> > - if (!sock || skb_queue_empty(&sock->sk->sk_receive_queue))
> > - return;
> > -
> > - mutex_lock(&vq->mutex);
> > - vhost_disable_notify(vq);
> > - hdr_size = 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);
> > - /* On error, stop handling until the next kick. */
> > - if (unlikely(head < 0))
> > - break;
> > - /* OK, now we need to know about added descriptors. */
> > - if (head == vq->num) {
> > - if (unlikely(vhost_enable_notify(vq))) {
> > - /* They have slipped one in as we were
> > - * doing that: check again. */
> > - vhost_disable_notify(vq);
> > - continue;
> > - }
> > - /* Nothing new? Wait for eventfd to tell us
> > - * they refilled. */
> > - 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);
> > - 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);
> > - 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, 1);
> > - break;
> > - }
> > - /* TODO: Should check and handle checksum. */
> > - if (err > len) {
> > - pr_debug("Discarded truncated rx packet: "
> > - " len %d > %zd\n", err, len);
> > - vhost_discard_vq_desc(vq, 1);
> > - 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);
> > - break;
> > - }
> > - len += hdr_size;
> > - vhost_add_used_and_signal(&net->dev, vq, head, len);
> > - if (unlikely(vq_log))
> > - vhost_log_write(vq, vq_log, log, len);
> > - total_len += len;
> > - if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
> > - vhost_poll_queue(&vq->poll);
> > - break;
> > - }
> > - }
> > -
> > - mutex_unlock(&vq->mutex);
> > -}
> > -
> > -/* Expects to be always run from workqueue - which acts as
> > - * read-size critical section for our kind of RCU. */
> > -static void handle_rx_mergeable(struct vhost_net *net)
> > +static void handle_rx(struct vhost_net *net)
> > {
> > struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_RX];
> > unsigned uninitialized_var(in), log;
> > @@ -431,7 +324,8 @@ static void handle_rx_mergeable(struct vhost_net *net)
> > sock_len += sock_hlen;
> > vhost_len = sock_len + vhost_hlen;
> > headcount = get_rx_bufs(vq, vq->heads, vhost_len,
> > - &in, vq_log, &log);
> > + &in, vq_log, &log,
> > + likely(mergeable) ? UIO_MAXIOV : 1);
> > /* On error, stop handling until the next kick. */
> > if (unlikely(headcount < 0))
> > break;
> > @@ -497,14 +391,6 @@ static void handle_rx_mergeable(struct vhost_net *net)
> > mutex_unlock(&vq->mutex);
> > }
> >
> > -static void handle_rx(struct vhost_net *net)
> > -{
> > - if (vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF))
> > - handle_rx_mergeable(net);
> > - else
> > - handle_rx_big(net);
> > -}
> > -
> > static void handle_tx_kick(struct vhost_work *work)
> > {
> > struct vhost_virtqueue *vq = container_of(work, struct vhost_virtqueue,
^ permalink raw reply
* Re: [PATCH] sched: provide scheduler_ipi() callback in response to smp_send_reschedule()
From: Peter Zijlstra @ 2011-01-17 20:43 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: linux-m32r-ja-rQhvJZKUsGBRYuoOT4C5/9i2O/JbrIOy,
linux-mips-6z/3iImG2C8G8FEW9MqTrA,
linux-m32r-rQhvJZKUsGBRYuoOT4C5/9i2O/JbrIOy,
linux-ia64-u79uwXL29TY76Z2rM5mHXA,
linux-sh-u79uwXL29TY76Z2rM5mHXA, Heiko Carstens, Howells,
Paul Mackerras, H. Peter Anvin, sparclinux-u79uwXL29TY76Z2rM5mHXA,
Linux-Arch, linux-s390-u79uwXL29TY76Z2rM5mHXA, Jesper Nilsson,
Mikael-ZG0+EudsQA8dtHy/vicBwGD2FQJk+8+b, Russell King, Takata,
x86-DgEjT+Ai2ygdnm+yROfE0A, James E.J. Bottomley,
virtualization-qjLDD68F18O7TbgM5vRIOg, Ingo Molnar,
xen-devel-GuqFBffKawuULHF6PoxzQEEOCMrvLtNR,
Chris-ZG0+EudsQA8dtHy/vicBwGD2FQJk+8+b, Matt Turner,
uclinux-dist-devel-ZG0+EudsQA8dtHy/vicBwGD2FQJk+8+b, Fen
In-Reply-To: <1295296310.2148.29.camel@pasglop>
On Tue, 2011-01-18 at 07:31 +1100, Benjamin Herrenschmidt wrote:
>
> Beware of false positive, I've used "fake" reschedule IPIs in the past
> for other things (like kicking a CPU out of sleep state for unrelated
> reasons). Nothing that I know that is upstream today but some of that
> might come back. I'd like to avoid having to add an atomic to know if
> it's a real reschedule, will the scheduler be smart enough to not bother
> with false positives ?
Yes it can deal with that, some will be for reschedules, some will be
for ttwu tail ends and x86 too uses this ipi for a few random other
things like kicking kvm out of guest context..
^ permalink raw reply
* Re: [PATCH] sched: provide scheduler_ipi() callback in response to smp_send_reschedule()
From: Benjamin Herrenschmidt @ 2011-01-17 20:31 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-m32r-ja, linux-mips, linux-ia64, linux-sh, H. Peter Anvin,
Heiko Carstens, David Howells, Paul Mackerras, Helge Deller,
sparclinux, Linux-Arch, linux-s390, Jesper Nilsson,
Jeremy Fitzhardinge, Russell King, Hirokazu Takata, x86,
James E.J. Bottomley, virtualization, Ingo Molnar, Matt Turner,
Fenghua Yu, Mike Frysinger <vapi>
In-Reply-To: <1295262433.30950.53.camel@laptop>
On Mon, 2011-01-17 at 12:07 +0100, Peter Zijlstra wrote:
> For future rework of try_to_wake_up() we'd like to push part of that
> onto the CPU the task is actually going to run on, in order to do so we
> need a generic callback from the existing scheduler IPI.
>
> This patch introduces such a generic callback: scheduler_ipi() and
> implements it as a NOP.
>
> I visited existing smp_send_reschedule() implementations and tried to
> add a call to scheduler_ipi() in their handler part, but esp. for MIPS
> I'm not quite sure I actually got all of them.
>
> Also, while reading through all this, I noticed the blackfin SMP code
> looks to be broken, it simply discards any IPI when low on memory.
Beware of false positive, I've used "fake" reschedule IPIs in the past
for other things (like kicking a CPU out of sleep state for unrelated
reasons). Nothing that I know that is upstream today but some of that
might come back. I'd like to avoid having to add an atomic to know if
it's a real reschedule, will the scheduler be smart enough to not bother
with false positives ?
Cheers,
Ben.
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> ---
> arch/alpha/kernel/smp.c | 1 +
> arch/arm/kernel/smp.c | 1 +
> arch/blackfin/mach-common/smp.c | 3 ++-
> arch/cris/arch-v32/kernel/smp.c | 13 ++++++++-----
> arch/ia64/kernel/irq_ia64.c | 2 ++
> arch/ia64/xen/irq_xen.c | 10 +++++++++-
> arch/m32r/kernel/smp.c | 2 +-
> arch/mips/kernel/smtc.c | 1 +
> arch/mips/sibyte/bcm1480/smp.c | 7 +++----
> arch/mips/sibyte/sb1250/smp.c | 7 +++----
> arch/mn10300/kernel/smp.c | 2 +-
> arch/parisc/kernel/smp.c | 1 +
> arch/powerpc/kernel/smp.c | 1 +
> arch/s390/kernel/smp.c | 6 +++---
> arch/sh/kernel/smp.c | 2 ++
> arch/sparc/kernel/smp_32.c | 2 +-
> arch/sparc/kernel/smp_64.c | 1 +
> arch/tile/kernel/smp.c | 1 +
> arch/um/kernel/smp.c | 2 +-
> arch/x86/kernel/smp.c | 1 +
> arch/x86/xen/smp.c | 1 +
> include/linux/sched.h | 1 +
> 22 files changed, 46 insertions(+), 22 deletions(-)
>
> diff --git a/arch/alpha/kernel/smp.c b/arch/alpha/kernel/smp.c
> index 42aa078..c4a570b 100644
> --- a/arch/alpha/kernel/smp.c
> +++ b/arch/alpha/kernel/smp.c
> @@ -587,6 +587,7 @@ handle_ipi(struct pt_regs *regs)
> case IPI_RESCHEDULE:
> /* Reschedule callback. Everything to be done
> is done by the interrupt return path. */
> + scheduler_ipi();
> break;
>
> case IPI_CALL_FUNC:
> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> index 9066473..ffde790 100644
> --- a/arch/arm/kernel/smp.c
> +++ b/arch/arm/kernel/smp.c
> @@ -579,6 +579,7 @@ asmlinkage void __exception do_IPI(struct pt_regs *regs)
> * nothing more to do - eveything is
> * done on the interrupt return path
> */
> + scheduler_ipi();
> break;
>
> case IPI_CALL_FUNC:
> diff --git a/arch/blackfin/mach-common/smp.c b/arch/blackfin/mach-common/smp.c
> index a17107a..e210f8a 100644
> --- a/arch/blackfin/mach-common/smp.c
> +++ b/arch/blackfin/mach-common/smp.c
> @@ -156,6 +156,7 @@ static irqreturn_t ipi_handler(int irq, void *dev_instance)
> case BFIN_IPI_RESCHEDULE:
> /* That's the easiest one; leave it to
> * return_from_int. */
> + scheduler_ipi();
> kfree(msg);
> break;
> case BFIN_IPI_CALL_FUNC:
> @@ -301,7 +302,7 @@ void smp_send_reschedule(int cpu)
>
> msg = kzalloc(sizeof(*msg), GFP_ATOMIC);
> if (!msg)
> - return;
> + return; /* XXX unreliable needs fixing ! */
> INIT_LIST_HEAD(&msg->list);
> msg->type = BFIN_IPI_RESCHEDULE;
>
> diff --git a/arch/cris/arch-v32/kernel/smp.c b/arch/cris/arch-v32/kernel/smp.c
> index 84fed3b..86e3c76 100644
> --- a/arch/cris/arch-v32/kernel/smp.c
> +++ b/arch/cris/arch-v32/kernel/smp.c
> @@ -340,15 +340,18 @@ irqreturn_t crisv32_ipi_interrupt(int irq, void *dev_id)
>
> ipi = REG_RD(intr_vect, irq_regs[smp_processor_id()], rw_ipi);
>
> + if (ipi.vector & IPI_SCHEDULE) {
> + scheduler_ipi();
> + }
> if (ipi.vector & IPI_CALL) {
> - func(info);
> + func(info);
> }
> if (ipi.vector & IPI_FLUSH_TLB) {
> - if (flush_mm == FLUSH_ALL)
> - __flush_tlb_all();
> - else if (flush_vma == FLUSH_ALL)
> + if (flush_mm == FLUSH_ALL)
> + __flush_tlb_all();
> + else if (flush_vma == FLUSH_ALL)
> __flush_tlb_mm(flush_mm);
> - else
> + else
> __flush_tlb_page(flush_vma, flush_addr);
> }
>
> diff --git a/arch/ia64/kernel/irq_ia64.c b/arch/ia64/kernel/irq_ia64.c
> index 9a26015..a580230 100644
> --- a/arch/ia64/kernel/irq_ia64.c
> +++ b/arch/ia64/kernel/irq_ia64.c
> @@ -31,6 +31,7 @@
> #include <linux/irq.h>
> #include <linux/ratelimit.h>
> #include <linux/acpi.h>
> +#include <linux/sched.h>
>
> #include <asm/delay.h>
> #include <asm/intrinsics.h>
> @@ -496,6 +497,7 @@ ia64_handle_irq (ia64_vector vector, struct pt_regs *regs)
> smp_local_flush_tlb();
> kstat_incr_irqs_this_cpu(irq, desc);
> } else if (unlikely(IS_RESCHEDULE(vector))) {
> + scheduler_ipi();
> kstat_incr_irqs_this_cpu(irq, desc);
> } else {
> ia64_setreg(_IA64_REG_CR_TPR, vector);
> diff --git a/arch/ia64/xen/irq_xen.c b/arch/ia64/xen/irq_xen.c
> index a3fb7cf..2f235a3 100644
> --- a/arch/ia64/xen/irq_xen.c
> +++ b/arch/ia64/xen/irq_xen.c
> @@ -92,6 +92,8 @@ static unsigned short saved_irq_cnt;
> static int xen_slab_ready;
>
> #ifdef CONFIG_SMP
> +#include <linux/sched.h>
> +
> /* Dummy stub. Though we may check XEN_RESCHEDULE_VECTOR before __do_IRQ,
> * it ends up to issue several memory accesses upon percpu data and
> * thus adds unnecessary traffic to other paths.
> @@ -99,7 +101,13 @@ static int xen_slab_ready;
> static irqreturn_t
> xen_dummy_handler(int irq, void *dev_id)
> {
> + return IRQ_HANDLED;
> +}
>
> +static irqreturn_t
> +xen_resched_handler(int irq, void *dev_id)
> +{
> + scheduler_ipi();
> return IRQ_HANDLED;
> }
>
> @@ -110,7 +118,7 @@ static struct irqaction xen_ipi_irqaction = {
> };
>
> static struct irqaction xen_resched_irqaction = {
> - .handler = xen_dummy_handler,
> + .handler = xen_resched_handler,
> .flags = IRQF_DISABLED,
> .name = "resched"
> };
> diff --git a/arch/m32r/kernel/smp.c b/arch/m32r/kernel/smp.c
> index 31cef20..f0ecc3f 100644
> --- a/arch/m32r/kernel/smp.c
> +++ b/arch/m32r/kernel/smp.c
> @@ -138,7 +138,7 @@ void smp_send_reschedule(int cpu_id)
> *==========================================================================*/
> void smp_reschedule_interrupt(void)
> {
> - /* nothing to do */
> + scheduler_ipi();
> }
>
> /*==========================================================================*
> diff --git a/arch/mips/kernel/smtc.c b/arch/mips/kernel/smtc.c
> index 39c0825..0443e91 100644
> --- a/arch/mips/kernel/smtc.c
> +++ b/arch/mips/kernel/smtc.c
> @@ -931,6 +931,7 @@ static void post_direct_ipi(int cpu, struct smtc_ipi *pipi)
> static void ipi_resched_interrupt(void)
> {
> /* Return from interrupt should be enough to cause scheduler check */
> + scheduler_ipi();
> }
>
> static void ipi_call_interrupt(void)
> diff --git a/arch/mips/sibyte/bcm1480/smp.c b/arch/mips/sibyte/bcm1480/smp.c
> index 47b347c..513a301 100644
> --- a/arch/mips/sibyte/bcm1480/smp.c
> +++ b/arch/mips/sibyte/bcm1480/smp.c
> @@ -20,6 +20,7 @@
> #include <linux/delay.h>
> #include <linux/smp.h>
> #include <linux/kernel_stat.h>
> +#include <linux/sched.h>
>
> #include <asm/mmu_context.h>
> #include <asm/io.h>
> @@ -189,10 +190,8 @@ void bcm1480_mailbox_interrupt(void)
> /* Clear the mailbox to clear the interrupt */
> __raw_writeq(((u64)action)<<48, mailbox_0_clear_regs[cpu]);
>
> - /*
> - * Nothing to do for SMP_RESCHEDULE_YOURSELF; returning from the
> - * interrupt will do the reschedule for us
> - */
> + if (actione & SMP_RESCHEDULE_YOURSELF)
> + scheduler_ipi();
>
> if (action & SMP_CALL_FUNCTION)
> smp_call_function_interrupt();
> diff --git a/arch/mips/sibyte/sb1250/smp.c b/arch/mips/sibyte/sb1250/smp.c
> index c00a5cb..38e7f6b 100644
> --- a/arch/mips/sibyte/sb1250/smp.c
> +++ b/arch/mips/sibyte/sb1250/smp.c
> @@ -21,6 +21,7 @@
> #include <linux/interrupt.h>
> #include <linux/smp.h>
> #include <linux/kernel_stat.h>
> +#include <linux/sched.h>
>
> #include <asm/mmu_context.h>
> #include <asm/io.h>
> @@ -177,10 +178,8 @@ void sb1250_mailbox_interrupt(void)
> /* Clear the mailbox to clear the interrupt */
> ____raw_writeq(((u64)action) << 48, mailbox_clear_regs[cpu]);
>
> - /*
> - * Nothing to do for SMP_RESCHEDULE_YOURSELF; returning from the
> - * interrupt will do the reschedule for us
> - */
> + if (action & SMP_RESCHEDULE_YOURSELF)
> + scheduler_ipi();
>
> if (action & SMP_CALL_FUNCTION)
> smp_call_function_interrupt();
> diff --git a/arch/mn10300/kernel/smp.c b/arch/mn10300/kernel/smp.c
> index 0dcd1c6..17f16ca 100644
> --- a/arch/mn10300/kernel/smp.c
> +++ b/arch/mn10300/kernel/smp.c
> @@ -471,7 +471,7 @@ void smp_send_stop(void)
> */
> static irqreturn_t smp_reschedule_interrupt(int irq, void *dev_id)
> {
> - /* do nothing */
> + scheduler_ipi();
> return IRQ_HANDLED;
> }
>
> diff --git a/arch/parisc/kernel/smp.c b/arch/parisc/kernel/smp.c
> index 69d63d3..f8f7970 100644
> --- a/arch/parisc/kernel/smp.c
> +++ b/arch/parisc/kernel/smp.c
> @@ -155,6 +155,7 @@ ipi_interrupt(int irq, void *dev_id)
>
> case IPI_RESCHEDULE:
> smp_debug(100, KERN_DEBUG "CPU%d IPI_RESCHEDULE\n", this_cpu);
> + scheduler_ipi();
> /*
> * Reschedule callback. Everything to be
> * done is done by the interrupt return path.
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index 68034bb..7ee0fc3 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -128,6 +128,7 @@ static irqreturn_t call_function_action(int irq, void *data)
> static irqreturn_t reschedule_action(int irq, void *data)
> {
> /* we just need the return path side effect of checking need_resched */
> + scheduler_ipi();
> return IRQ_HANDLED;
> }
>
> diff --git a/arch/s390/kernel/smp.c b/arch/s390/kernel/smp.c
> index 94cf510..61789e8 100644
> --- a/arch/s390/kernel/smp.c
> +++ b/arch/s390/kernel/smp.c
> @@ -163,12 +163,12 @@ static void do_ext_call_interrupt(unsigned int ext_int_code,
>
> /*
> * handle bit signal external calls
> - *
> - * For the ec_schedule signal we have to do nothing. All the work
> - * is done automatically when we return from the interrupt.
> */
> bits = xchg(&S390_lowcore.ext_call_fast, 0);
>
> + if (test_bit(ec_schedule, &bits))
> + scheduler_ipi();
> +
> if (test_bit(ec_call_function, &bits))
> generic_smp_call_function_interrupt();
>
> diff --git a/arch/sh/kernel/smp.c b/arch/sh/kernel/smp.c
> index 509b36b..6207561 100644
> --- a/arch/sh/kernel/smp.c
> +++ b/arch/sh/kernel/smp.c
> @@ -20,6 +20,7 @@
> #include <linux/module.h>
> #include <linux/cpu.h>
> #include <linux/interrupt.h>
> +#include <linux/sched.h>
> #include <asm/atomic.h>
> #include <asm/processor.h>
> #include <asm/system.h>
> @@ -323,6 +324,7 @@ void smp_message_recv(unsigned int msg)
> generic_smp_call_function_interrupt();
> break;
> case SMP_MSG_RESCHEDULE:
> + scheduler_ipi();
> break;
> case SMP_MSG_FUNCTION_SINGLE:
> generic_smp_call_function_single_interrupt();
> diff --git a/arch/sparc/kernel/smp_32.c b/arch/sparc/kernel/smp_32.c
> index 91c10fb..042d8c9 100644
> --- a/arch/sparc/kernel/smp_32.c
> +++ b/arch/sparc/kernel/smp_32.c
> @@ -125,7 +125,7 @@ struct linux_prom_registers smp_penguin_ctable __cpuinitdata = { 0 };
>
> void smp_send_reschedule(int cpu)
> {
> - /* See sparc64 */
> + scheduler_ipi();
> }
>
> void smp_send_stop(void)
> diff --git a/arch/sparc/kernel/smp_64.c b/arch/sparc/kernel/smp_64.c
> index b6a2b8f..68e26e2 100644
> --- a/arch/sparc/kernel/smp_64.c
> +++ b/arch/sparc/kernel/smp_64.c
> @@ -1369,6 +1369,7 @@ void smp_send_reschedule(int cpu)
> void __irq_entry smp_receive_signal_client(int irq, struct pt_regs *regs)
> {
> clear_softint(1 << irq);
> + scheduler_ipi();
> }
>
> /* This is a nop because we capture all other cpus
> diff --git a/arch/tile/kernel/smp.c b/arch/tile/kernel/smp.c
> index 9575b37..91a1ddf 100644
> --- a/arch/tile/kernel/smp.c
> +++ b/arch/tile/kernel/smp.c
> @@ -190,6 +190,7 @@ static irqreturn_t handle_reschedule_ipi(int irq, void *token)
> * profiler count in the meantime.
> */
> __get_cpu_var(irq_stat).irq_resched_count++;
> + scheduler_ipi();
>
> return IRQ_HANDLED;
> }
> diff --git a/arch/um/kernel/smp.c b/arch/um/kernel/smp.c
> index 106bf27..eefb107 100644
> --- a/arch/um/kernel/smp.c
> +++ b/arch/um/kernel/smp.c
> @@ -173,7 +173,7 @@ void IPI_handler(int cpu)
> break;
>
> case 'R':
> - set_tsk_need_resched(current);
> + scheduler_ipi();
> break;
>
> case 'S':
> diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
> index 513deac..e38c2d8 100644
> --- a/arch/x86/kernel/smp.c
> +++ b/arch/x86/kernel/smp.c
> @@ -202,6 +202,7 @@ void smp_reschedule_interrupt(struct pt_regs *regs)
> {
> ack_APIC_irq();
> inc_irq_stat(irq_resched_count);
> + scheduler_ipi();
> /*
> * KVM uses this interrupt to force a cpu out of guest mode
> */
> diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
> index 72a4c79..6196fb1 100644
> --- a/arch/x86/xen/smp.c
> +++ b/arch/x86/xen/smp.c
> @@ -53,6 +53,7 @@ static irqreturn_t xen_call_function_single_interrupt(int irq, void *dev_id);
> static irqreturn_t xen_reschedule_interrupt(int irq, void *dev_id)
> {
> inc_irq_stat(irq_resched_count);
> + scheduler_ipi();
>
> return IRQ_HANDLED;
> }
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 341acbb..aa458dc 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -2183,6 +2183,7 @@ extern void set_task_comm(struct task_struct *tsk, char *from);
> extern char *get_task_comm(char *to, struct task_struct *tsk);
>
> #ifdef CONFIG_SMP
> +static inline void scheduler_ipi(void) { }
> extern unsigned long wait_task_inactive(struct task_struct *, long match_state);
> #else
> static inline unsigned long wait_task_inactive(struct task_struct *p,
^ permalink raw reply
* Re: [PATCH] sched: provide scheduler_ipi() callback in response to smp_send_reschedule()
From: Peter Zijlstra @ 2011-01-17 20:16 UTC (permalink / raw)
To: Mike Frysinger
Cc: linux-m32r-ja-rQhvJZKUsGBRYuoOT4C5/9i2O/JbrIOy,
linux-mips-6z/3iImG2C8G8FEW9MqTrA,
linux-ia64-u79uwXL29TY76Z2rM5mHXA,
linux-sh-u79uwXL29TY76Z2rM5mHXA, Benjamin Herrenschmidt,
Heiko Carstens, James E.J. Bottomley, David Howells,
Paul Mackerras, H. Peter Anvin, sparclinux-u79uwXL29TY76Z2rM5mHXA,
Ralf-ZG0+EudsQA8dtHy/vicBwGD2FQJk+8+b, Linux-Arch,
linux-s390-u79uwXL29TY76Z2rM5mHXA, Jesper Nilsson,
Jeremy Fitzhardinge, Russell King, Hirokazu Takata,
x86-DgEjT+Ai2ygdnm+yROfE0A,
Koichi-ZG0+EudsQA8dtHy/vicBwGD2FQJk+8+b,
virtualization-qjLDD68F18O7TbgM5vRIOg, Ingo Molnar,
Matt Turner <mattst88>
In-Reply-To: <AANLkTik3hE=_34Lbs944MzKpkNzqY+kCxpxmncUM2HB7-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Mon, 2011-01-17 at 14:49 -0500, Mike Frysinger wrote:
> On Mon, Jan 17, 2011 at 06:07, Peter Zijlstra wrote:
> > Also, while reading through all this, I noticed the blackfin SMP code
> > looks to be broken, it simply discards any IPI when low on memory.
>
> not really. see changelog of commit 73a400646b8e26615f3ef1a0a4bc0cd0d5bd284c.
Ah, indeed, it appears my tree was simply out of date, very good!
^ permalink raw reply
* Re: [PATCH] sched: provide scheduler_ipi() callback in response to smp_send_reschedule()
From: Mike Frysinger @ 2011-01-17 19:49 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-m32r-ja-rQhvJZKUsGBRYuoOT4C5/9i2O/JbrIOy,
linux-mips-6z/3iImG2C8G8FEW9MqTrA,
linux-ia64-u79uwXL29TY76Z2rM5mHXA,
linux-sh-u79uwXL29TY76Z2rM5mHXA, Benjamin Herrenschmidt,
H. Peter Anvin, Heiko Carstens, David Howells, Paul Mackerras,
Helge Deller, sparclinux-u79uwXL29TY76Z2rM5mHXA, Linux-Arch,
linux-s390-u79uwXL29TY76Z2rM5mHXA, Jesper Nilsson,
Jeremy Fitzhardinge, Russell King, Hirokazu Takata,
x86-DgEjT+Ai2ygdnm+yROfE0A, James E.J. Bottomley,
virtualization-qjLDD68F18O7TbgM5vRIOg, Ingo Molnar, Matt Turner,
Fengh
In-Reply-To: <1295262433.30950.53.camel@laptop>
On Mon, Jan 17, 2011 at 06:07, Peter Zijlstra wrote:
> Also, while reading through all this, I noticed the blackfin SMP code
> looks to be broken, it simply discards any IPI when low on memory.
not really. see changelog of commit 73a400646b8e26615f3ef1a0a4bc0cd0d5bd284c.
-mike
^ permalink raw reply
* Re: [PATCH] sched: provide scheduler_ipi() callback in response to smp_send_reschedule()
From: David Daney @ 2011-01-17 19:18 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-m32r-ja-rQhvJZKUsGBRYuoOT4C5/9i2O/JbrIOy,
linux-mips-6z/3iImG2C8G8FEW9MqTrA,
linux-ia64-u79uwXL29TY76Z2rM5mHXA,
linux-sh-u79uwXL29TY76Z2rM5mHXA, Benjamin Herrenschmidt,
Heiko Carstens, David Howells, Paul Mackerras, H. Peter Anvin,
sparclinux-u79uwXL29TY76Z2rM5mHXA, Linux-Arch,
linux-s390-u79uwXL29TY76Z2rM5mHXA, Jesper Nilsson,
Jeremy Fitzhardinge, Russell King, Hirokazu Takata,
x86-DgEjT+Ai2ygdnm+yROfE0A, James E.J. Bottomley,
virtualization-qjLDD68F18O7TbgM5vRIOg, Ingo Molnar, Matt Turner,
Fenghua Yu
In-Reply-To: <1295262433.30950.53.camel@laptop>
On 01/17/2011 03:07 AM, Peter Zijlstra wrote:
> For future rework of try_to_wake_up() we'd like to push part of that
> onto the CPU the task is actually going to run on, in order to do so we
> need a generic callback from the existing scheduler IPI.
>
> This patch introduces such a generic callback: scheduler_ipi() and
> implements it as a NOP.
>
> I visited existing smp_send_reschedule() implementations and tried to
> add a call to scheduler_ipi() in their handler part, but esp. for MIPS
> I'm not quite sure I actually got all of them.
>
> Also, while reading through all this, I noticed the blackfin SMP code
> looks to be broken, it simply discards any IPI when low on memory.
>
> Signed-off-by: Peter Zijlstra<a.p.zijlstra-/NLkJaSkS4VmR6Xm/wNWPw@public.gmane.org>
> ---
> arch/alpha/kernel/smp.c | 1 +
> arch/arm/kernel/smp.c | 1 +
> arch/blackfin/mach-common/smp.c | 3 ++-
> arch/cris/arch-v32/kernel/smp.c | 13 ++++++++-----
> arch/ia64/kernel/irq_ia64.c | 2 ++
> arch/ia64/xen/irq_xen.c | 10 +++++++++-
> arch/m32r/kernel/smp.c | 2 +-
> arch/mips/kernel/smtc.c | 1 +
> arch/mips/sibyte/bcm1480/smp.c | 7 +++----
> arch/mips/sibyte/sb1250/smp.c | 7 +++----
[...]
Peter,
You will also have to patch the mailbox_interrupt() function in
arch/mips/cavium-octeon/smp.c
David Daney.
^ permalink raw reply
* Re: [PATCH 13/14] x86/ticketlock: add slowpath logic
From: Srivatsa Vaddagiri @ 2011-01-17 15:22 UTC (permalink / raw)
To: Jeremy Fitzhardinge
Cc: Xen-devel, Mathieu Desnoyers, Nick Piggin, Peter Zijlstra,
Linux Kernel Mailing List, Jan Beulich, Eric Dumazet,
Jeremy Fitzhardinge, suzuki, Avi Kivity, H. Peter Anvin,
Américo Wang, Linux Virtualization
In-Reply-To: <97ed99ae9160bdb6477284b333bd6708fb7a19cb.1289940821.git.jeremy.fitzhardinge@citrix.com>
On Tue, Nov 16, 2010 at 01:08:44PM -0800, Jeremy Fitzhardinge wrote:
> From: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
>
> Maintain a flag in both LSBs of the ticket lock which indicates whether
> anyone is in the lock slowpath and may need kicking when the current
> holder unlocks. The flags are set when the first locker enters
> the slowpath, and cleared when unlocking to an empty queue.
>
> In the specific implementation of lock_spinning(), make sure to set
> the slowpath flags on the lock just before blocking. We must do
> this before the last-chance pickup test to prevent a deadlock
> with the unlocker:
>
> Unlocker Locker
> test for lock pickup
> -> fail
> test slowpath + unlock
> -> false
> set slowpath flags
> block
>
> Whereas this works in any ordering:
>
> Unlocker Locker
> set slowpath flags
> test for lock pickup
> -> fail
> block
> test slowpath + unlock
> -> true, kick
I think this is still racy ..
Unlocker Locker
test slowpath
-> false
set slowpath flag
test for lock pickup
-> fail
block
unlock
unlock needs to happen first before testing slowpath? I have made that change
for my KVM guest and it seems to be working well with that change .. Will
cleanup and post my patches shortly
- vatsa
^ 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