* Re: [PATCH 1/2 v1] blkdrv: Add queue limits parameters for sg block drive
From: Paolo Bonzini @ 2012-08-23 10:52 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Stefan Hajnoczi, zwanp, linuxram, qemu-devel, virtualization,
Cong Meng
In-Reply-To: <CAJSP0QXG3yzxnwAJd3s0M0eSUM39XRdYEmGtj0h83_de4mxaAA@mail.gmail.com>
Il 23/08/2012 12:08, Stefan Hajnoczi ha scritto:
>> I'm still trying to understand the extent of the problem.
>>
>> The problem occurs for _USB_ CD-ROMs according to Ben. Passthrough of
>> USB storage devices should be done via USB passthrough, not virtio-scsi.
>> If we do USB passthrough via the SCSI layer we miss on all the quirks
>> that the OS may do based on the USB product/vendor pairs. There's no
>> end to these, and some of the quirks may cause the device to lock up or
>> corruption.
>>
>> I'd rather see a reproducer using SAS/ATA/ATAPI disks before punting.
>
> This issue affects passthrough: either an entire sg device or at least
> a SG_IO ioctl (e.g. a non-READ/WRITE SCSI command).
>
> To reproduce it, check host queue limits and guest virtio-scsi queue
> limits. Then pick a command that can exceed the limits and try it
> from inside the guest :).
Yes, so much is clear. But does it happen _in practice_? Do initiators
actually issue commands that are that big?
Paolo
^ permalink raw reply
* Re: [PATCH 1/2 v1] blkdrv: Add queue limits parameters for sg block drive
From: Stefan Hajnoczi @ 2012-08-23 12:08 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Stefan Hajnoczi, zwanp, linuxram, qemu-devel, virtualization,
Cong Meng
In-Reply-To: <50360B80.1080609@redhat.com>
On Thu, Aug 23, 2012 at 11:52 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 23/08/2012 12:08, Stefan Hajnoczi ha scritto:
>>> I'm still trying to understand the extent of the problem.
>>>
>>> The problem occurs for _USB_ CD-ROMs according to Ben. Passthrough of
>>> USB storage devices should be done via USB passthrough, not virtio-scsi.
>>> If we do USB passthrough via the SCSI layer we miss on all the quirks
>>> that the OS may do based on the USB product/vendor pairs. There's no
>>> end to these, and some of the quirks may cause the device to lock up or
>>> corruption.
>>>
>>> I'd rather see a reproducer using SAS/ATA/ATAPI disks before punting.
>>
>> This issue affects passthrough: either an entire sg device or at least
>> a SG_IO ioctl (e.g. a non-READ/WRITE SCSI command).
>>
>> To reproduce it, check host queue limits and guest virtio-scsi queue
>> limits. Then pick a command that can exceed the limits and try it
>> from inside the guest :).
>
> Yes, so much is clear. But does it happen _in practice_? Do initiators
> actually issue commands that are that big?
Here I think we need to err on the side of caution. A user passes
through a tape drive or exotic SCSI device. They run a vendor utility
inside the guest that issues a command that exceeds the host block
queue limits.
Passing through limits is intended to make SCSI device passthrough
work, in all cases.
Is the number of real cases where it happens small? Probably. I
still think we should make passthrough bulletproof.
Stefan
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Rafael Aquini @ 2012-08-23 12:13 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Peter Zijlstra, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton,
Paul E. McKenney
In-Reply-To: <20120823100107.GA17409@redhat.com>
On Thu, Aug 23, 2012 at 01:01:07PM +0300, Michael S. Tsirkin wrote:
> > So, when remove_common() calls leak_balloon() looping on
> > vb->num_pages, that won't become a tight loop.
> > The scheme was apparently working before this series, and it will remain working
> > after it.
>
> It seems that before we would always leak all requested memory
> in one go. I can't tell why we have a while loop there at all.
> Rusty, could you clarify please?
>
It seems that your claim isn't right. leak_balloon() cannot do it all at once,
as for each round it only releases 256 pages, at most; and the 'one go' would
require a couple of loop rounds at remove_common().
So, nothing has changed here.
> > Just as before, same thing here. If you leaked less than required, balloon()
> > will keep calling leak_balloon() until the balloon target is reached. This
> > scheme was working before, and it will keep working after this patch.
> >
>
> IIUC we never hit this path before.
>
So, how does balloon() works then?
> > > How about we signal config_change
> > > event when pages are back to pages_list?
> >
> > I really don't know what to tell you here, but, to me, it seems like an
> > overcomplication that isn't directly entangled with this patch purposes.
> > Besides, you cannot expect compation / migration happening and racing against
> > leak_balloon() all the time to make them signal events to the later, so we might
> > just be creating a wait-forever condition for leak_balloon(), IMHO.
>
> So use wait_event or similar, check for existance of isolated pages.
>
The thing here is expecting compaction as being an external event to signal
actions to the balloon driver won't work as you desire. Also, as far as the
balloon driver is concerned, it's only a matter of time to accomplish a total,
or partial, balloon leak, even when we have some pages isolated from balloon's
page list.
IMHO, you're attempting to complicate a simple thing that is already working
well. As said before, there are no guarantees you'll have isolated pages
by the time you're leaking the balloon, so you might leave it waiting forever
on something that will not happen. And if there are isolated pages while balloon
is leaking, they'll have their chance to get back to the list before the device
finishes its leaking job.
^ permalink raw reply
* Re: Re: [PATCH 2/5] trace-cmd: Use tracing directory to count CPUs
From: Masami Hiramatsu @ 2012-08-23 12:30 UTC (permalink / raw)
To: Steven Rostedt
Cc: Herbert Xu, Arnd Bergmann, qemu-devel, Frederic Weisbecker,
linux-kernel, Borislav Petkov, virtualization, Franch Ch. Eigler,
Ingo Molnar, Mathieu Desnoyers, Anthony Liguori,
Greg Kroah-Hartman, Amit Shah, yrl.pp-manager.tt
In-Reply-To: <1345712886.5069.41.camel@gandalf.local.home>
(2012/08/23 18:08), Steven Rostedt wrote:
> On Thu, 2012-08-23 at 12:00 +0900, Masami Hiramatsu wrote:
>> (2012/08/23 11:01), Masami Hiramatsu wrote:
>>> (2012/08/22 22:41), Steven Rostedt wrote:
>>>> On Wed, 2012-08-22 at 17:43 +0900, Yoshihiro YUNOMAE wrote:
>>>>> From: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>>>>>
>>>>> Count debugfs/tracing/per_cpu/cpu* to determine the
>>>>> number of CPUs.
>>>>
>>>> I'm curious, do you find that sysconf doesn't return the # of CPUs the
>>>> system has?
>>>
>>> No, sysconf returns the number of hosts CPUs, not guests.
>>>
>>>> I've had boxes where the per_cpu/cpu* had more cpus than the
>>>> box actually holds. But this was a bug in the kernel, not the tool. This
>>>> change log needs to have rational instead of just explaining what the
>>>> patch does.
>>>
>>> Ah, I see. Hmm, then this should be enabled by a command line
>>> option or an environment variable.
>>
>> Oops, I misunderstood. I'll add more comment for why this
>> should be tried instead of sysconf.
>
> And now that I understand why you are doing this, why not only do this
> if the TRACE_AGENT or DEBUG_TRACING_DIR is defined. That is, if we are
> doing it against a bare metal system, then sysconf should suffice, but
> if we are tracing against a guest, then it should use the tracing
> directory to determine the buffers.
>
> We could add options to override this, but I would think the default
> should just Do The Right Thing(tm).
Yeah, so I'd like to push this is the default method, and fix
the kernel bug (but I'm not sure that is a bug).
Thank you,
--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Michael S. Tsirkin @ 2012-08-23 12:34 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Peter Zijlstra, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton,
Paul E. McKenney
In-Reply-To: <20120823121338.GA3062@t510.redhat.com>
On Thu, Aug 23, 2012 at 09:13:39AM -0300, Rafael Aquini wrote:
> On Thu, Aug 23, 2012 at 01:01:07PM +0300, Michael S. Tsirkin wrote:
> > > So, when remove_common() calls leak_balloon() looping on
> > > vb->num_pages, that won't become a tight loop.
> > > The scheme was apparently working before this series, and it will remain working
> > > after it.
> >
> > It seems that before we would always leak all requested memory
> > in one go. I can't tell why we have a while loop there at all.
> > Rusty, could you clarify please?
> >
>
> It seems that your claim isn't right. leak_balloon() cannot do it all at once,
> as for each round it only releases 256 pages, at most; and the 'one go' would
> require a couple of loop rounds at remove_common().
You are right in this respect.
> So, nothing has changed here.
Yes, your patch does change things:
leak_balloon now might return without freeing any pages.
In that case we will not be making any progress, and just
spin, pinning CPU.
>
> > > Just as before, same thing here. If you leaked less than required, balloon()
> > > will keep calling leak_balloon() until the balloon target is reached. This
> > > scheme was working before, and it will keep working after this patch.
> > >
> >
> > IIUC we never hit this path before.
> >
> So, how does balloon() works then?
>
It gets a request to leak a given number of pages
and executes it, then tells host that it is done.
It never needs to spin busy-waiting on a CPU for this.
> > > > How about we signal config_change
> > > > event when pages are back to pages_list?
> > >
> > > I really don't know what to tell you here, but, to me, it seems like an
> > > overcomplication that isn't directly entangled with this patch purposes.
> > > Besides, you cannot expect compation / migration happening and racing against
> > > leak_balloon() all the time to make them signal events to the later, so we might
> > > just be creating a wait-forever condition for leak_balloon(), IMHO.
> >
> > So use wait_event or similar, check for existance of isolated pages.
> >
>
> The thing here is expecting compaction as being an external event to signal
> actions to the balloon driver won't work as you desire. Also, as far as the
> balloon driver is concerned, it's only a matter of time to accomplish a total,
> or partial, balloon leak, even when we have some pages isolated from balloon's
> page list.
>
> IMHO, you're attempting to complicate a simple thing that is already working
> well. As said before, there are no guarantees you'll have isolated pages
> by the time you're leaking the balloon, so you might leave it waiting forever
> on something that will not happen. And if there are isolated pages while balloon
> is leaking, they'll have their chance to get back to the list before the device
> finishes its leaking job.
Well busy wait pinning CPU is ugly. Instead we should block thread and
wake it up when done. I don't mind how we fix it specifically.
--
MST
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Rafael Aquini @ 2012-08-23 13:06 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Peter Zijlstra, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton,
Paul E. McKenney
In-Reply-To: <20120823123432.GA25659@redhat.com>
On Thu, Aug 23, 2012 at 03:34:32PM +0300, Michael S. Tsirkin wrote:
> > So, nothing has changed here.
>
> Yes, your patch does change things:
> leak_balloon now might return without freeing any pages.
> In that case we will not be making any progress, and just
> spin, pinning CPU.
That's a transitory condition, that migh happen if leak_balloon() takes place
when compaction, or migration are under their way and it might only affects the
module unload case. Also it won't pin CPU because it keeps releasing the locks
it grabs, as it loops. So, we are locubrating about rarities, IMHO.
>
> >
> > > > Just as before, same thing here. If you leaked less than required, balloon()
> > > > will keep calling leak_balloon() until the balloon target is reached. This
> > > > scheme was working before, and it will keep working after this patch.
> > > >
> > >
> > > IIUC we never hit this path before.
> > >
> > So, how does balloon() works then?
> >
>
> It gets a request to leak a given number of pages
> and executes it, then tells host that it is done.
> It never needs to spin busy-waiting on a CPU for this.
>
So, what this patch changes for the ordinary leak_balloon() case?
> Well busy wait pinning CPU is ugly. Instead we should block thread and
> wake it up when done. I don't mind how we fix it specifically.
>
I already told you that we do not do that by any mean introduced by this patch.
You're just being stubborn here. If those bits are broken, they were already
broken before I did come up with this proposal.
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Michael S. Tsirkin @ 2012-08-23 13:53 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Peter Zijlstra, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton,
Paul E. McKenney
In-Reply-To: <20120823130606.GB3746@t510.redhat.com>
On Thu, Aug 23, 2012 at 10:06:07AM -0300, Rafael Aquini wrote:
> On Thu, Aug 23, 2012 at 03:34:32PM +0300, Michael S. Tsirkin wrote:
> > > So, nothing has changed here.
> >
> > Yes, your patch does change things:
> > leak_balloon now might return without freeing any pages.
> > In that case we will not be making any progress, and just
> > spin, pinning CPU.
>
> That's a transitory condition, that migh happen if leak_balloon() takes place
> when compaction, or migration are under their way and it might only affects the
> module unload case.
Regular operation seems even more broken: host might ask
you to leak memory but because it is under compaction
you might leak nothing. No?
> Also it won't pin CPU because it keeps releasing the locks
> it grabs, as it loops.
What has releazing locks have to do with it?
> So, we are locubrating about rarities, IMHO.
> >
> > >
> > > > > Just as before, same thing here. If you leaked less than required, balloon()
> > > > > will keep calling leak_balloon() until the balloon target is reached. This
> > > > > scheme was working before, and it will keep working after this patch.
> > > > >
> > > >
> > > > IIUC we never hit this path before.
> > > >
> > > So, how does balloon() works then?
> > >
> >
> > It gets a request to leak a given number of pages
> > and executes it, then tells host that it is done.
> > It never needs to spin busy-waiting on a CPU for this.
> >
>
> So, what this patch changes for the ordinary leak_balloon() case?
>
That not all pages used by balloon are on &vb->pages list.
> > Well busy wait pinning CPU is ugly. Instead we should block thread and
> > wake it up when done. I don't mind how we fix it specifically.
> >
>
> I already told you that we do not do that by any mean introduced by this patch.
> You're just being stubborn here. If those bits are broken, they were already
> broken before I did come up with this proposal.
Sorry you don't address the points I am making. Maybe there are no
bugs. But it looks like there are. And assuming I am just seeing things
this just means patch needs more comments, in commit log and in
code to explain the design so that it stops looking like that.
Basically it was very simple: we assumed page->lru was never
touched for an allocated page, so it's safe to use it for
internal book-keeping by the driver.
Now, this is not the case anymore, you add some logic in mm/ that might
or might not touch page->lru depending on things like reference count.
And you are asking why things break even though you change very little
in balloon itself? Because the interface between balloon and mm is now
big, fragile and largely undocumented.
Another strangeness I just noticed: if we ever do an extra get_page in
balloon, compaction logic in mm will break, yes? But one expects to be
able to do get_page after alloc_page without ill effects
as long as one does put_page before free.
Just a thought: maybe it is cleaner to move all balloon page tracking
into mm/? Implement alloc_balloon/free_balloon with methods to fill and
leak pages, and callbacks to invoke when done. This should be good for
other hypervisors too. If you like this idea, I can even try to help out
by refactoring current code in this way, so that you can build on it.
But this is just a thought, not a must.
--
MST
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Rafael Aquini @ 2012-08-23 15:21 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Peter Zijlstra, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton,
Paul E. McKenney
In-Reply-To: <20120823135328.GB25709@redhat.com>
On Thu, Aug 23, 2012 at 04:53:29PM +0300, Michael S. Tsirkin wrote:
> On Thu, Aug 23, 2012 at 10:06:07AM -0300, Rafael Aquini wrote:
> > On Thu, Aug 23, 2012 at 03:34:32PM +0300, Michael S. Tsirkin wrote:
> > > > So, nothing has changed here.
> > >
> > > Yes, your patch does change things:
> > > leak_balloon now might return without freeing any pages.
> > > In that case we will not be making any progress, and just
> > > spin, pinning CPU.
> >
> > That's a transitory condition, that migh happen if leak_balloon() takes place
> > when compaction, or migration are under their way and it might only affects the
> > module unload case.
>
> Regular operation seems even more broken: host might ask
> you to leak memory but because it is under compaction
> you might leak nothing. No?
>
And that is exactely what it wants to do. If there is (temporarily) nothing to leak,
then not leaking is the only sane thing to do. Having balloon pages being migrated
does not break the leak at all, despite it can last a little longer.
> >
> > I already told you that we do not do that by any mean introduced by this patch.
> > You're just being stubborn here. If those bits are broken, they were already
> > broken before I did come up with this proposal.
>
> Sorry you don't address the points I am making. Maybe there are no
> bugs. But it looks like there are. And assuming I am just seeing things
> this just means patch needs more comments, in commit log and in
> code to explain the design so that it stops looking like that.
>
Yes, I belive you're biased here.
> Basically it was very simple: we assumed page->lru was never
> touched for an allocated page, so it's safe to use it for
> internal book-keeping by the driver.
>
> Now, this is not the case anymore, you add some logic in mm/ that might
> or might not touch page->lru depending on things like reference count.
> And you are asking why things break even though you change very little
> in balloon itself? Because the interface between balloon and mm is now
> big, fragile and largely undocumented.
>
The driver don't use page->lru as its bookeeping at all, it uses
vb->num_pages instead.
> Another strangeness I just noticed: if we ever do an extra get_page in
> balloon, compaction logic in mm will break, yes? But one expects to be
> able to do get_page after alloc_page without ill effects
> as long as one does put_page before free.
>
You can do it (bump up the balloon page refcount), and it will only prevent
balloon pages from being isolated and migrated, thus reducing the effectiveness of
defragmenting memory when balloon pages are present, just like it happens today.
It really doesn't seems the case of virtio_balloon driver, or any other driver,
which allocates pages directly from buddy to keep raising the page refcount,
though.
> Just a thought: maybe it is cleaner to move all balloon page tracking
> into mm/? Implement alloc_balloon/free_balloon with methods to fill and
> leak pages, and callbacks to invoke when done. This should be good for
> other hypervisors too. If you like this idea, I can even try to help out
> by refactoring current code in this way, so that you can build on it.
> But this is just a thought, not a must.
>
That seems to be a good thought to be on a future enhancements wish-list, for sure.
We can start thinking of it, and I surely would be more than happy on be doing
it along with you. But I don't think not having it right away is a dealbreaker
for this proposal, as is.
I'm not against your thoughts, and I'm really glad that you're providing such
good dicussion over this subject, but, now I'll wait for Rusty thoughts on
this one question.
Cheers!
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Michael S. Tsirkin @ 2012-08-23 15:54 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Peter Zijlstra, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton,
Paul E. McKenney
In-Reply-To: <20120823152128.GA8975@t510.redhat.com>
On Thu, Aug 23, 2012 at 12:21:29PM -0300, Rafael Aquini wrote:
> On Thu, Aug 23, 2012 at 04:53:29PM +0300, Michael S. Tsirkin wrote:
> > On Thu, Aug 23, 2012 at 10:06:07AM -0300, Rafael Aquini wrote:
> > > On Thu, Aug 23, 2012 at 03:34:32PM +0300, Michael S. Tsirkin wrote:
> > > > > So, nothing has changed here.
> > > >
> > > > Yes, your patch does change things:
> > > > leak_balloon now might return without freeing any pages.
> > > > In that case we will not be making any progress, and just
> > > > spin, pinning CPU.
> > >
> > > That's a transitory condition, that migh happen if leak_balloon() takes place
> > > when compaction, or migration are under their way and it might only affects the
> > > module unload case.
> >
> > Regular operation seems even more broken: host might ask
> > you to leak memory but because it is under compaction
> > you might leak nothing. No?
> >
>
> And that is exactely what it wants to do. If there is (temporarily) nothing to leak,
> then not leaking is the only sane thing to do.
It's an internal issue between balloon and mm. User does not care.
> Having balloon pages being migrated
> does not break the leak at all, despite it can last a little longer.
>
Not "longer" - apparently forever unless user resend the leak command.
It's wrong - it should
1. not tell host if nothing was done
2. after migration finished leak and tell host
> > >
> > > I already told you that we do not do that by any mean introduced by this patch.
> > > You're just being stubborn here. If those bits are broken, they were already
> > > broken before I did come up with this proposal.
> >
> > Sorry you don't address the points I am making. Maybe there are no
> > bugs. But it looks like there are. And assuming I am just seeing things
> > this just means patch needs more comments, in commit log and in
> > code to explain the design so that it stops looking like that.
> >
>
> Yes, I belive you're biased here.
>
>
> > Basically it was very simple: we assumed page->lru was never
> > touched for an allocated page, so it's safe to use it for
> > internal book-keeping by the driver.
> >
> > Now, this is not the case anymore, you add some logic in mm/ that might
> > or might not touch page->lru depending on things like reference count.
> > And you are asking why things break even though you change very little
> > in balloon itself? Because the interface between balloon and mm is now
> > big, fragile and largely undocumented.
> >
>
> The driver don't use page->lru as its bookeeping at all, it uses
> vb->num_pages instead.
$ grep lru drivers/virtio/virtio_balloon.c
list_add(&page->lru, &vb->pages);
page = list_first_entry(&vb->pages, struct page, lru);
list_del(&page->lru);
>
> > Another strangeness I just noticed: if we ever do an extra get_page in
> > balloon, compaction logic in mm will break, yes? But one expects to be
> > able to do get_page after alloc_page without ill effects
> > as long as one does put_page before free.
> >
>
> You can do it (bump up the balloon page refcount), and it will only prevent
> balloon pages from being isolated and migrated, thus reducing the effectiveness of
> defragmenting memory when balloon pages are present, just like it happens today.
>
> It really doesn't seems the case of virtio_balloon driver, or any other driver,
> which allocates pages directly from buddy to keep raising the page refcount,
> though.
>
E.g. network devices routinely play with pages they get from buddy,
this is used for sharing memory between skbs.
> > Just a thought: maybe it is cleaner to move all balloon page tracking
> > into mm/? Implement alloc_balloon/free_balloon with methods to fill and
> > leak pages, and callbacks to invoke when done. This should be good for
> > other hypervisors too. If you like this idea, I can even try to help out
> > by refactoring current code in this way, so that you can build on it.
> > But this is just a thought, not a must.
> >
>
> That seems to be a good thought to be on a future enhancements wish-list, for sure.
> We can start thinking of it, and I surely would be more than happy on be doing
> it along with you. But I don't think not having it right away is a dealbreaker
> for this proposal, as is.
I grant busywait on module unloading isn't a huge deal breaker.
Poking in mm internals is not a dealbreaker?
Not leaking as much as
you are asked to isn't?
> I'm not against your thoughts, and I'm really glad that you're providing such
> good dicussion over this subject, but, now I'll wait for Rusty thoughts on
> this one question.
>
> Cheers!
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Rik van Riel @ 2012-08-23 16:03 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rafael Aquini, Konrad Rzeszutek Wilk, Peter Zijlstra,
linux-kernel, virtualization, linux-mm, Andi Kleen, Minchan Kim,
Andrew Morton, Paul E. McKenney
In-Reply-To: <20120823155401.GA28876@redhat.com>
On 08/23/2012 11:54 AM, Michael S. Tsirkin wrote:
> On Thu, Aug 23, 2012 at 12:21:29PM -0300, Rafael Aquini wrote:
>> On Thu, Aug 23, 2012 at 04:53:29PM +0300, Michael S. Tsirkin wrote:
>>> On Thu, Aug 23, 2012 at 10:06:07AM -0300, Rafael Aquini wrote:
>>>> On Thu, Aug 23, 2012 at 03:34:32PM +0300, Michael S. Tsirkin wrote:
>>>>>> So, nothing has changed here.
>>>>>
>>>>> Yes, your patch does change things:
>>>>> leak_balloon now might return without freeing any pages.
>>>>> In that case we will not be making any progress, and just
>>>>> spin, pinning CPU.
>>>>
>>>> That's a transitory condition, that migh happen if leak_balloon() takes place
>>>> when compaction, or migration are under their way and it might only affects the
>>>> module unload case.
>>>
>>> Regular operation seems even more broken: host might ask
>>> you to leak memory but because it is under compaction
>>> you might leak nothing. No?
>>>
>>
>> And that is exactely what it wants to do. If there is (temporarily) nothing to leak,
>> then not leaking is the only sane thing to do.
>
> It's an internal issue between balloon and mm. User does not care.
>
>> Having balloon pages being migrated
>> does not break the leak at all, despite it can last a little longer.
>>
>
> Not "longer" - apparently forever unless user resend the leak command.
> It's wrong - it should
> 1. not tell host if nothing was done
> 2. after migration finished leak and tell host
Agreed. If the balloon is told to leak N pages, and could
not do so because those pages were locked, the balloon driver
needs to retry (maybe waiting on a page lock?) and not signal
completion until after the job has been completed.
Having the balloon driver wait on the page lock should be
fine, because compaction does not hold the page lock for
long.
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Rafael Aquini @ 2012-08-23 16:06 UTC (permalink / raw)
To: Rik van Riel
Cc: Konrad Rzeszutek Wilk, Michael S. Tsirkin, Peter Zijlstra,
linux-kernel, virtualization, linux-mm, Andi Kleen, Minchan Kim,
Andrew Morton, Paul E. McKenney
In-Reply-To: <50365443.1070104@redhat.com>
On Thu, Aug 23, 2012 at 12:03:15PM -0400, Rik van Riel wrote:
> >
> >Not "longer" - apparently forever unless user resend the leak command.
> >It's wrong - it should
> >1. not tell host if nothing was done
> >2. after migration finished leak and tell host
>
> Agreed. If the balloon is told to leak N pages, and could
> not do so because those pages were locked, the balloon driver
> needs to retry (maybe waiting on a page lock?) and not signal
> completion until after the job has been completed.
>
> Having the balloon driver wait on the page lock should be
> fine, because compaction does not hold the page lock for
> long.
And that is precisely what leak_balloon is doing. When it stumbles across a
locked page it gets rid of that leak round to give a shot for compaction to
finish its task.
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Michael S. Tsirkin @ 2012-08-23 16:10 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Peter Zijlstra, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton,
Paul E. McKenney
In-Reply-To: <20120823160647.GA10777@t510.redhat.com>
On Thu, Aug 23, 2012 at 01:06:48PM -0300, Rafael Aquini wrote:
> On Thu, Aug 23, 2012 at 12:03:15PM -0400, Rik van Riel wrote:
> > >
> > >Not "longer" - apparently forever unless user resend the leak command.
> > >It's wrong - it should
> > >1. not tell host if nothing was done
> > >2. after migration finished leak and tell host
> >
> > Agreed. If the balloon is told to leak N pages, and could
> > not do so because those pages were locked, the balloon driver
> > needs to retry (maybe waiting on a page lock?) and not signal
> > completion until after the job has been completed.
> >
> > Having the balloon driver wait on the page lock should be
> > fine, because compaction does not hold the page lock for
> > long.
>
> And that is precisely what leak_balloon is doing. When it stumbles across a
> locked page it gets rid of that leak round to give a shot for compaction to
> finish its task.
>
Yes but I do not see where it will retry.
If it does, please add a comment pointing out where it happens.
--
MST
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Michael S. Tsirkin @ 2012-08-23 16:25 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Peter Zijlstra, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton,
Paul E. McKenney
In-Reply-To: <20120823135328.GB25709@redhat.com>
On Thu, Aug 23, 2012 at 04:53:28PM +0300, Michael S. Tsirkin wrote:
> Basically it was very simple: we assumed page->lru was never
> touched for an allocated page, so it's safe to use it for
> internal book-keeping by the driver.
>
> Now, this is not the case anymore, you add some logic in mm/ that might
> or might not touch page->lru depending on things like reference count.
Another thought: would the issue go away if balloon used
page->private to link pages instead of LRU?
mm core could keep a reference on page to avoid it
being used while mm handles it (maybe it does already?).
If we do this, will not the only change to balloon be to tell mm that it
can use compaction for these pages when it allocates the page: using
some GPF flag or a new API?
--
MST
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Rafael Aquini @ 2012-08-23 17:28 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Peter Zijlstra, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton,
Paul E. McKenney
In-Reply-To: <20120823162504.GA1522@redhat.com>
On Thu, Aug 23, 2012 at 07:25:05PM +0300, Michael S. Tsirkin wrote:
> On Thu, Aug 23, 2012 at 04:53:28PM +0300, Michael S. Tsirkin wrote:
> > Basically it was very simple: we assumed page->lru was never
> > touched for an allocated page, so it's safe to use it for
> > internal book-keeping by the driver.
> >
> > Now, this is not the case anymore, you add some logic in mm/ that might
> > or might not touch page->lru depending on things like reference count.
>
> Another thought: would the issue go away if balloon used
> page->private to link pages instead of LRU?
> mm core could keep a reference on page to avoid it
> being used while mm handles it (maybe it does already?).
>
I don't think so. That would be a lot more trikier and complex, IMHO.
> If we do this, will not the only change to balloon be to tell mm that it
> can use compaction for these pages when it allocates the page: using
> some GPF flag or a new API?
>
What about keep a conter at virtio_balloon structure on how much pages are
isolated from balloon's list and check it at leak time?
if the counter gets > 0 than we can safely put leak_balloon() to wait until
balloon page list gets completely refilled. I guess that is simple to get
accomplished and potentially addresses all your concerns on this issue.
Cheers!
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Rik van Riel @ 2012-08-23 17:59 UTC (permalink / raw)
To: Rafael Aquini
Cc: Konrad Rzeszutek Wilk, Michael S. Tsirkin, Peter Zijlstra,
linux-kernel, virtualization, linux-mm, Andi Kleen, Minchan Kim,
Andrew Morton, Paul E. McKenney
In-Reply-To: <20120823172844.GC10777@t510.redhat.com>
On 08/23/2012 01:28 PM, Rafael Aquini wrote:
> What about keep a conter at virtio_balloon structure on how much pages are
> isolated from balloon's list and check it at leak time?
> if the counter gets > 0 than we can safely put leak_balloon() to wait until
> balloon page list gets completely refilled.
We only have to wait if we failed to leak enough
pages, and then only for as many additional pages
as we require.
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Michael S. Tsirkin @ 2012-08-23 23:36 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Peter Zijlstra, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton,
Paul E. McKenney
In-Reply-To: <20120823172844.GC10777@t510.redhat.com>
On Thu, Aug 23, 2012 at 02:28:45PM -0300, Rafael Aquini wrote:
> On Thu, Aug 23, 2012 at 07:25:05PM +0300, Michael S. Tsirkin wrote:
> > On Thu, Aug 23, 2012 at 04:53:28PM +0300, Michael S. Tsirkin wrote:
> > > Basically it was very simple: we assumed page->lru was never
> > > touched for an allocated page, so it's safe to use it for
> > > internal book-keeping by the driver.
> > >
> > > Now, this is not the case anymore, you add some logic in mm/ that might
> > > or might not touch page->lru depending on things like reference count.
> >
> > Another thought: would the issue go away if balloon used
> > page->private to link pages instead of LRU?
> > mm core could keep a reference on page to avoid it
> > being used while mm handles it (maybe it does already?).
> >
> I don't think so. That would be a lot more trikier and complex, IMHO.
What's tricky? Linking pages through a void * orivate pointer?
I can code it up in a couple of minutes.
It's middle of the night so too tired to test but still:
> > If we do this, will not the only change to balloon be to tell mm that it
> > can use compaction for these pages when it allocates the page: using
> > some GPF flag or a new API?
> >
>
> What about keep a conter at virtio_balloon structure on how much pages are
> isolated from balloon's list and check it at leak time?
> if the counter gets > 0 than we can safely put leak_balloon() to wait until
> balloon page list gets completely refilled. I guess that is simple to get
> accomplished and potentially addresses all your concerns on this issue.
>
> Cheers!
I would wake it each time after adding a page, then it
can stop waiting when it leaks enough.
But again, it's cleaner to just keep tracking all
pages, let mm hang on to them by keeping a reference.
--->
virtio-balloon: replace page->lru list with page->private.
The point is to free up page->lru for use by compaction.
Warning: completely untested, will provide tested version
if we agree on this direction.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 0908e60..b38f57ce 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -56,7 +56,7 @@ struct virtio_balloon
* Each page on this list adds VIRTIO_BALLOON_PAGES_PER_PAGE
* to num_pages above.
*/
- struct list_head pages;
+ void *pages;
/* The array of pfns we tell the Host about. */
unsigned int num_pfns;
@@ -141,7 +141,9 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
set_page_pfns(vb->pfns + vb->num_pfns, page);
vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
totalram_pages--;
- list_add(&page->lru, &vb->pages);
+ /* Add to list of pages */
+ page->private = vb->pages;
+ vb->pages = page->private;
}
/* Didn't get any? Oh well. */
@@ -171,8 +173,9 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num)
for (vb->num_pfns = 0; vb->num_pfns < num;
vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
- page = list_first_entry(&vb->pages, struct page, lru);
- list_del(&page->lru);
+ /* Delete from list of pages */
+ page = vb->pages;
+ vb->pages = page->private;
set_page_pfns(vb->pfns + vb->num_pfns, page);
vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
}
@@ -350,7 +353,7 @@ static int virtballoon_probe(struct virtio_device *vdev)
goto out;
}
- INIT_LIST_HEAD(&vb->pages);
+ vb->pages = NULL;
vb->num_pages = 0;
init_waitqueue_head(&vb->config_change);
init_waitqueue_head(&vb->acked);
--
MST
^ permalink raw reply related
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Rafael Aquini @ 2012-08-24 0:26 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Peter Zijlstra, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton,
Paul E. McKenney
In-Reply-To: <20120823233616.GB2775@redhat.com>
On Fri, Aug 24, 2012 at 02:36:16AM +0300, Michael S. Tsirkin wrote:
> On Thu, Aug 23, 2012 at 02:28:45PM -0300, Rafael Aquini wrote:
> > On Thu, Aug 23, 2012 at 07:25:05PM +0300, Michael S. Tsirkin wrote:
> > > On Thu, Aug 23, 2012 at 04:53:28PM +0300, Michael S. Tsirkin wrote:
> > > > Basically it was very simple: we assumed page->lru was never
> > > > touched for an allocated page, so it's safe to use it for
> > > > internal book-keeping by the driver.
> > > >
> > > > Now, this is not the case anymore, you add some logic in mm/ that might
> > > > or might not touch page->lru depending on things like reference count.
> > >
> > > Another thought: would the issue go away if balloon used
> > > page->private to link pages instead of LRU?
> > > mm core could keep a reference on page to avoid it
> > > being used while mm handles it (maybe it does already?).
> > >
> > I don't think so. That would be a lot more trikier and complex, IMHO.
>
> What's tricky? Linking pages through a void * orivate pointer?
> I can code it up in a couple of minutes.
> It's middle of the night so too tired to test but still:
>
> > > If we do this, will not the only change to balloon be to tell mm that it
> > > can use compaction for these pages when it allocates the page: using
> > > some GPF flag or a new API?
> > >
> >
> > What about keep a conter at virtio_balloon structure on how much pages are
> > isolated from balloon's list and check it at leak time?
> > if the counter gets > 0 than we can safely put leak_balloon() to wait until
> > balloon page list gets completely refilled. I guess that is simple to get
> > accomplished and potentially addresses all your concerns on this issue.
> >
> > Cheers!
>
> I would wake it each time after adding a page, then it
> can stop waiting when it leaks enough.
> But again, it's cleaner to just keep tracking all
> pages, let mm hang on to them by keeping a reference.
>
> --->
>
> virtio-balloon: replace page->lru list with page->private.
>
> The point is to free up page->lru for use by compaction.
> Warning: completely untested, will provide tested version
> if we agree on this direction.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
This way balloon driver will potentially release pages that were already
migrated and doesn't belong to it anymore, since the page under migration never
gets isolated from balloon's page list. It's a lot more dangerous than it was
before.
I'm working on having leak_balloon on the right way, as you correctly has
pointed. I was blind and biased. So, thank you for pointing me the way.
> ---
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 0908e60..b38f57ce 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -56,7 +56,7 @@ struct virtio_balloon
> * Each page on this list adds VIRTIO_BALLOON_PAGES_PER_PAGE
> * to num_pages above.
> */
> - struct list_head pages;
> + void *pages;
>
> /* The array of pfns we tell the Host about. */
> unsigned int num_pfns;
> @@ -141,7 +141,9 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
> set_page_pfns(vb->pfns + vb->num_pfns, page);
> vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
> totalram_pages--;
> - list_add(&page->lru, &vb->pages);
> + /* Add to list of pages */
> + page->private = vb->pages;
> + vb->pages = page->private;
> }
>
> /* Didn't get any? Oh well. */
> @@ -171,8 +173,9 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num)
>
> for (vb->num_pfns = 0; vb->num_pfns < num;
> vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
> - page = list_first_entry(&vb->pages, struct page, lru);
> - list_del(&page->lru);
> + /* Delete from list of pages */
> + page = vb->pages;
> + vb->pages = page->private;
> set_page_pfns(vb->pfns + vb->num_pfns, page);
> vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
> }
> @@ -350,7 +353,7 @@ static int virtballoon_probe(struct virtio_device *vdev)
> goto out;
> }
>
> - INIT_LIST_HEAD(&vb->pages);
> + vb->pages = NULL;
> vb->num_pages = 0;
> init_waitqueue_head(&vb->config_change);
> init_waitqueue_head(&vb->acked);
> --
> MST
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Rafael Aquini @ 2012-08-24 0:33 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Peter Zijlstra, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton,
Paul E. McKenney
In-Reply-To: <20120823233616.GB2775@redhat.com>
On Fri, Aug 24, 2012 at 02:36:16AM +0300, Michael S. Tsirkin wrote:
> I would wake it each time after adding a page, then it
> can stop waiting when it leaks enough.
> But again, it's cleaner to just keep tracking all
> pages, let mm hang on to them by keeping a reference.
>
Here is a rough idea on how it's getting:
Basically, I'm have introducing an atomic counter to track isolated pages, I
also have changed vb->num_pages into an atomic conter. All inc/dec operations
take place under pages_lock spinlock, and we only perform work under page lock.
It's still missing the wait-part (I'll write it during the weekend) and your
concerns (and mine) will be addressed, IMHO.
---8<---
+/*
+ *
+ */
+static inline void __wait_on_isolated_pages(struct virtio_balloon *vb,
+ size_t num)
+{
+ /* There are no isolated pages for this balloon device */
+ if (!atomic_read(&vb->num_isolated_pages))
+ return;
+
+ /* the leak target is smaller than # of pages on vb->pages list */
+ if (num < (atomic_read(&vb->num_pages) -
+ atomic_read(&vb->num_isolated_pages)))
+ return;
+ else {
+ spin_unlock(&vb->pages_lock);
+ /* wait stuff goes here */
+ spin_lock(&vb->pages_lock);
+ }
+}
+
static void leak_balloon(struct virtio_balloon *vb, size_t num)
{
- struct page *page;
+ /* The array of pfns we tell the Host about. */
+ unsigned int num_pfns;
+ u32 pfns[VIRTIO_BALLOON_ARRAY_PFNS_MAX];
/* We can only do one array worth at a time. */
- num = min(num, ARRAY_SIZE(vb->pfns));
+ num = min(num, ARRAY_SIZE(pfns));
- for (vb->num_pfns = 0; vb->num_pfns < num;
- vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
- page = list_first_entry(&vb->pages, struct page, lru);
- list_del(&page->lru);
- set_page_pfns(vb->pfns + vb->num_pfns, page);
- vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
+ for (num_pfns = 0; num_pfns < num;
+ num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
+ struct page *page = NULL;
+ spin_lock(&vb->pages_lock);
+ __wait_on_isolated_pages(vb, num);
+
+ if (!list_empty(&vb->pages))
+ page = list_first_entry(&vb->pages, struct page, lru);
+ /*
+ * Grab the page lock to avoid racing against threads isolating
+ * pages from, or migrating pages back to vb->pages list.
+ * (both tasks are done under page lock protection)
+ *
+ * Failing to grab the page lock here means this page is being
+ * isolated already, or its migration has not finished yet.
+ */
+ if (page && trylock_page(page)) {
+ clear_balloon_mapping(page);
+ list_del(&page->lru);
+ set_page_pfns(pfns + num_pfns, page);
+ atomic_sub(VIRTIO_BALLOON_PAGES_PER_PAGE,
+ &vb->num_pages);
+ unlock_page(page);
+ }
+ spin_unlock(&vb->pages_lock);
}
/*
@@ -182,8 +251,10 @@ static void leak_balloon(struct virtio_balloon *vb, size_t
num)
* virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
* is true, we *have* to do it in this order
*/
+ mutex_lock(&vb->balloon_lock);
tell_host(vb, vb->deflate_vq);
- release_pages_by_pfn(vb->pfns, vb->num_pfns);
+ mutex_unlock(&vb->balloon_lock);
+ release_pages_by_pfn(pfns, num_pfns);
}
---8<---
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Rafael Aquini @ 2012-08-24 0:38 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Peter Zijlstra, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton,
Paul E. McKenney
In-Reply-To: <20120824003353.GG10777@t510.redhat.com>
On Thu, Aug 23, 2012 at 09:33:53PM -0300, Rafael Aquini wrote:
> On Fri, Aug 24, 2012 at 02:36:16AM +0300, Michael S. Tsirkin wrote:
> > I would wake it each time after adding a page, then it
> > can stop waiting when it leaks enough.
> > But again, it's cleaner to just keep tracking all
> > pages, let mm hang on to them by keeping a reference.
> >
Btw, it's also late here, and there still some work to be done around those
bits, but I guess that has potential to get this issue nailed.
> Here is a rough idea on how it's getting:
>
> Basically, I'm have introducing an atomic counter to track isolated pages, I
> also have changed vb->num_pages into an atomic conter. All inc/dec operations
> take place under pages_lock spinlock, and we only perform work under page lock.
>
> It's still missing the wait-part (I'll write it during the weekend) and your
> concerns (and mine) will be addressed, IMHO.
>
> ---8<---
> +/*
> + *
> + */
> +static inline void __wait_on_isolated_pages(struct virtio_balloon *vb,
> + size_t num)
> +{
> + /* There are no isolated pages for this balloon device */
> + if (!atomic_read(&vb->num_isolated_pages))
> + return;
> +
> + /* the leak target is smaller than # of pages on vb->pages list */
> + if (num < (atomic_read(&vb->num_pages) -
> + atomic_read(&vb->num_isolated_pages)))
> + return;
> + else {
> + spin_unlock(&vb->pages_lock);
> + /* wait stuff goes here */
> + spin_lock(&vb->pages_lock);
> + }
> +}
> +
> static void leak_balloon(struct virtio_balloon *vb, size_t num)
> {
> - struct page *page;
> + /* The array of pfns we tell the Host about. */
> + unsigned int num_pfns;
> + u32 pfns[VIRTIO_BALLOON_ARRAY_PFNS_MAX];
>
> /* We can only do one array worth at a time. */
> - num = min(num, ARRAY_SIZE(vb->pfns));
> + num = min(num, ARRAY_SIZE(pfns));
>
> - for (vb->num_pfns = 0; vb->num_pfns < num;
> - vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
> - page = list_first_entry(&vb->pages, struct page, lru);
> - list_del(&page->lru);
> - set_page_pfns(vb->pfns + vb->num_pfns, page);
> - vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
> + for (num_pfns = 0; num_pfns < num;
> + num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
> + struct page *page = NULL;
> + spin_lock(&vb->pages_lock);
> + __wait_on_isolated_pages(vb, num);
> +
> + if (!list_empty(&vb->pages))
> + page = list_first_entry(&vb->pages, struct page, lru);
> + /*
> + * Grab the page lock to avoid racing against threads isolating
> + * pages from, or migrating pages back to vb->pages list.
> + * (both tasks are done under page lock protection)
> + *
> + * Failing to grab the page lock here means this page is being
> + * isolated already, or its migration has not finished yet.
> + */
> + if (page && trylock_page(page)) {
> + clear_balloon_mapping(page);
> + list_del(&page->lru);
> + set_page_pfns(pfns + num_pfns, page);
> + atomic_sub(VIRTIO_BALLOON_PAGES_PER_PAGE,
> + &vb->num_pages);
> + unlock_page(page);
> + }
> + spin_unlock(&vb->pages_lock);
> }
>
> /*
> @@ -182,8 +251,10 @@ static void leak_balloon(struct virtio_balloon *vb, size_t
> num)
> * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
> * is true, we *have* to do it in this order
> */
> + mutex_lock(&vb->balloon_lock);
> tell_host(vb, vb->deflate_vq);
> - release_pages_by_pfn(vb->pfns, vb->num_pfns);
> + mutex_unlock(&vb->balloon_lock);
> + release_pages_by_pfn(pfns, num_pfns);
> }
> ---8<---
^ permalink raw reply
* Re: [PATCH 1/2 v1] blkdrv: Add queue limits parameters for sg block drive
From: Nicholas A. Bellinger @ 2012-08-24 0:45 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Stefan Hajnoczi, zwanp, linuxram, qemu-devel, virtualization,
Cong Meng, Paolo Bonzini, Christoph Hellwig
In-Reply-To: <CAJSP0QXG3yzxnwAJd3s0M0eSUM39XRdYEmGtj0h83_de4mxaAA@mail.gmail.com>
On Thu, 2012-08-23 at 11:08 +0100, Stefan Hajnoczi wrote:
> On Thu, Aug 23, 2012 at 11:03 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > Il 23/08/2012 11:31, Cong Meng ha scritto:
> >>> For disks, this should be fixed simply by using scsi-block instead of
> >>> scsi-generic.
> >>>
> >>> CD-ROMs are indeed more complicated because burning CDs cannot be done
> >>> with syscalls. :/
> >>
> >> So, as the problem exist to CD-ROM, I will continue to get these patches
> >> move on.
> >
> > I'm still trying to understand the extent of the problem.
> >
> > The problem occurs for _USB_ CD-ROMs according to Ben. Passthrough of
> > USB storage devices should be done via USB passthrough, not virtio-scsi.
> > If we do USB passthrough via the SCSI layer we miss on all the quirks
> > that the OS may do based on the USB product/vendor pairs. There's no
> > end to these, and some of the quirks may cause the device to lock up or
> > corruption.
> >
> > I'd rather see a reproducer using SAS/ATA/ATAPI disks before punting.
>
> This issue affects passthrough: either an entire sg device or at least
> a SG_IO ioctl (e.g. a non-READ/WRITE SCSI command).
>
> To reproduce it, check host queue limits and guest virtio-scsi queue
> limits. Then pick a command that can exceed the limits and try it
> from inside the guest :).
>
Just following along on this thread, and wanted to add a few of my
experiences with this scenario from the kernel target perspective..
So up until very recently, TCM would accept an I/O request for an DATA
I/O type CDB with a max_sectors larger than the reported max_sectors for
it's TCM backend (regardless of backend type), and silently generate N
backend 'tasks' to complete the single initiator generated command.
Also FYI for Paolo, for control type CDBs I've never actually seen an
allocation length exceed max_sectors, so in practice AFAIK this only
happens for DATA I/O type CDBs.
This was historically required by the pSCSI backend driver (using a
number of old SCSI passthrough interfaces) in order to support this very
type of case described above, but over the years the logic ended up
creeping into various other non-passthrough backend drivers like IBLOCK
+FILEIO. So for v3.6-rc1 code, hch ended up removing the 'task' logic
thus allowing backends (and the layers below) to the I/O sectors >
max_sectors handling work, allowing modern pSCSI using struct request to
do the same. (hch assured me this works now for pSCSI)
Anyways, I think having the guest limit virtio-scsi DATA I/O to
max_sectors based upon the host accessible block limits is reasonable
approach to consider. Reducing this value even further based upon the
lowest max_sectors available amongst possible migration hosts would be a
good idea here to avoid having to reject any I/O's exceeding a new
host's device block queue limits.
--nab
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Rafael Aquini @ 2012-08-24 0:49 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Peter Zijlstra, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton,
Paul E. McKenney
In-Reply-To: <20120824003848.GH10777@t510.redhat.com>
On Thu, Aug 23, 2012 at 09:38:48PM -0300, Rafael Aquini wrote:
> On Thu, Aug 23, 2012 at 09:33:53PM -0300, Rafael Aquini wrote:
> > On Fri, Aug 24, 2012 at 02:36:16AM +0300, Michael S. Tsirkin wrote:
> > > I would wake it each time after adding a page, then it
> > > can stop waiting when it leaks enough.
> > > But again, it's cleaner to just keep tracking all
> > > pages, let mm hang on to them by keeping a reference.
> > >
>
> Btw, it's also late here, and there still some work to be done around those
> bits, but I guess that has potential to get this issue nailed.
>
> > Here is a rough idea on how it's getting:
> >
> > Basically, I'm have introducing an atomic counter to track isolated pages, I
> > also have changed vb->num_pages into an atomic conter. All inc/dec operations
> > take place under pages_lock spinlock, and we only perform work under page lock.
> >
> > It's still missing the wait-part (I'll write it during the weekend) and your
> > concerns (and mine) will be addressed, IMHO.
> >
> > ---8<---
> > +/*
> > + *
> > + */
> > +static inline void __wait_on_isolated_pages(struct virtio_balloon *vb,
> > + size_t num)
> > +{
> > + /* There are no isolated pages for this balloon device */
> > + if (!atomic_read(&vb->num_isolated_pages))
> > + return;
> > +
> > + /* the leak target is smaller than # of pages on vb->pages list */
> > + if (num < (atomic_read(&vb->num_pages) -
> > + atomic_read(&vb->num_isolated_pages)))
> > + return;
> > + else {
> > + spin_unlock(&vb->pages_lock);
> > + /* wait stuff goes here */
> > + spin_lock(&vb->pages_lock);
> > + }
> > +}
> > +
> > static void leak_balloon(struct virtio_balloon *vb, size_t num)
> > {
> > - struct page *page;
> > + /* The array of pfns we tell the Host about. */
> > + unsigned int num_pfns;
> > + u32 pfns[VIRTIO_BALLOON_ARRAY_PFNS_MAX];
> >
> > /* We can only do one array worth at a time. */
> > - num = min(num, ARRAY_SIZE(vb->pfns));
> > + num = min(num, ARRAY_SIZE(pfns));
> >
> > - for (vb->num_pfns = 0; vb->num_pfns < num;
> > - vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
> > - page = list_first_entry(&vb->pages, struct page, lru);
> > - list_del(&page->lru);
> > - set_page_pfns(vb->pfns + vb->num_pfns, page);
> > - vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
> > + for (num_pfns = 0; num_pfns < num;
> > + num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
Doh, here I need a while(num_pfns < num;)
> > + struct page *page = NULL;
> > + spin_lock(&vb->pages_lock);
> > + __wait_on_isolated_pages(vb, num);
> > +
> > + if (!list_empty(&vb->pages))
> > + page = list_first_entry(&vb->pages, struct page, lru);
> > + /*
> > + * Grab the page lock to avoid racing against threads isolating
> > + * pages from, or migrating pages back to vb->pages list.
> > + * (both tasks are done under page lock protection)
> > + *
> > + * Failing to grab the page lock here means this page is being
> > + * isolated already, or its migration has not finished yet.
> > + */
> > + if (page && trylock_page(page)) {
> > + clear_balloon_mapping(page);
> > + list_del(&page->lru);
> > + set_page_pfns(pfns + num_pfns, page);
> > + atomic_sub(VIRTIO_BALLOON_PAGES_PER_PAGE,
> > + &vb->num_pages);
and here: num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE;
> > + unlock_page(page);
> > + }
> > + spin_unlock(&vb->pages_lock);
> > }
> >
> > /*
> > @@ -182,8 +251,10 @@ static void leak_balloon(struct virtio_balloon *vb, size_t
> > num)
> > * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
> > * is true, we *have* to do it in this order
> > */
> > + mutex_lock(&vb->balloon_lock);
> > tell_host(vb, vb->deflate_vq);
> > - release_pages_by_pfn(vb->pfns, vb->num_pfns);
> > + mutex_unlock(&vb->balloon_lock);
> > + release_pages_by_pfn(pfns, num_pfns);
> > }
> > ---8<---
Well, I'll assume that's because I'm working for 14h already and running out of
caffeine.
^ permalink raw reply
* CFP: DataCloud 2012 - The Third International Workshop on Data Intensive Computing in the Clouds -- Co-located with SC 12
From: Kyle Chard @ 2012-08-24 2:42 UTC (permalink / raw)
To: virtualization
[-- Attachment #1.1: Type: text/plain, Size: 4930 bytes --]
Call for Papers: The Third International Workshop on Data Intensive
Computing in the Clouds (DataCloud 2012)
November 11, Salt Lake City, UT, USA
(http://www.cse.buffalo.edu/faculty/tkosar/datacloud2012)
Co-Located with Super Computing 2012, November 10-16, Salt Lake City, UT,
USA (http://sc12.supercomputing.org/)
----------------------------------------------------------------------------
-----------------------------------------
IMPORTANT DATES
Abstract Submission: August 27, 2012
Paper Submission: September 10, 2012
Notification of Acceptance: October 8, 2012
Final Paper Due: October 29, 2012
Workshop: November 11, 2012
----------------------------------------------------------------------------
-----------------------------------------
OVERVIEW
Applications and experiments in all areas of science are becoming
increasingly complex and more demanding in terms of their computational and
data requirements. Some applications generate data volumes reaching hundreds
of terabytes and even petabytes. As scientific applications become more data
intensive, the management of data resources and dataflow between the storage
and compute resources is becoming the main bottleneck. Analyzing,
visualizing, and disseminating these large data sets has become a major
challenge and data intensive computing is now considered as the ''fourth
paradigm'' in scientific discovery after theoretical, experimental, and
computational science.
DataCloud 2012 will provide the scientific community a dedicated forum for
discussing new research, development, and deployment efforts in running
data-intensive computing workloads on Cloud Computing infrastructures. The
DataCloud 2012 workshop will focus on the use of cloud-based technologies to
meet the new data intensive scientific challenges that are not well served
by the current supercomputers, grids or compute-intensive clouds. We believe
the workshop will be an excellent place to help the community define the
current state, determine future goals, and present architectures and
services for future clouds supporting data intensive computing.
----------------------------------------------------------------------------
-----------------------------------------
WORKSHOP SCOPE
Topics of interest include, but are not limited to:
* Data-intensive cloud computing applications, characteristics,
challenges
* Case studies of data intensive computing in the clouds
* Performance evaluation of data clouds, data grids, and data centers
* Energy-efficient data cloud design and management
* Data placement, scheduling, and interoperability in the clouds
* Accountability, QoS, and SLAs
* Data privacy and protection in a public cloud environment
* Distributed file systems for clouds
* Data streaming and parallelization
* New programming models for data-intensive cloud computing
* Scalability issues in clouds
* Social computing and massively social gaming
* 3D Internet and implications
* Future research challenges in data-intensive cloud computing
----------------------------------------------------------------------------
-----------------------------------------
ORGANIZERS
* Tevfik Kosar, University at Buffalo
* Ioan Raicu, Illinois Institute of Technology & Argonne National
Laboratory, USA
* Roger Barga, Microsoft Research
STEERING COMMITTEE
* Ian Foster, University of Chicago & Argonne National Laboratory, USA
* Geoffrey Fox, Indiana University
* James Hamilton, Amazon Web Services
* Manish Parashar, Rutgers University & National Science Foundation
* Dan Reed, Microsoft Research
* Rich Wolski, University of California, Santa Barbara
PROGRAMME COMMITTEE
. Samer Al-Kiswany, University of British Columbia, Canada
. Abhishek Chandra, University of Minnesota
. Rong N. ChangI, BM Research
. Kyle Chard, University of Chicago & Argonne National Laboratory
. Terence Critchlow, Pacific Northwest National Laboratory
. Murat Demirbas, University at Buffalo
. Jaliya Ekanayake, Microsoft Research
. Geoffrey Fox, Indiana University
. Dennis Gannon, Microsoft Research
. Rob Gillen, Oak Ridge National Laboratory
. Maria Indrawan, Monash University, Australia
. Hui Jin, Oracle
. Dan Katz, National Science Foundation
. Steven Ko, University at Buffalo
. Reagan Moore, University of North Carolina
. Judy Qui, Indiana University
. Lavanya Ramakrishnan, Lawrence Berkeley National Laboratory
. Kui Ren, University at Buffalo
. Yogesh Simmhan, University of Southern California, USA
. Borjo Sotomayor, University of Chicago
. Wei Tang, Argonne National Laboratory
. Bernard Traversat, Oracle
. Zhifeng Yun, Louisiana State University
. Ziming Zheng, Illinois Institute of Technology
[-- Attachment #1.2: Type: text/html, Size: 27589 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
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Rik van Riel @ 2012-08-24 3:12 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rafael Aquini, Konrad Rzeszutek Wilk, Peter Zijlstra,
linux-kernel, virtualization, linux-mm, Andi Kleen, Minchan Kim,
Andrew Morton, Paul E. McKenney
In-Reply-To: <20120823233616.GB2775@redhat.com>
On 08/23/2012 07:36 PM, Michael S. Tsirkin wrote:
> --->
>
> virtio-balloon: replace page->lru list with page->private.
>
> The point is to free up page->lru for use by compaction.
> Warning: completely untested, will provide tested version
> if we agree on this direction.
A singly linked list is not going to work for page migration,
which needs to get pages that might be in the middle of the
balloon list.
--
All rights reversed
^ permalink raw reply
* Re: [PATCH 1/2 v1] blkdrv: Add queue limits parameters for sg block drive
From: Paolo Bonzini @ 2012-08-24 7:56 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: Stefan Hajnoczi, zwanp, linuxram, qemu-devel, virtualization,
Cong Meng, Christoph Hellwig
In-Reply-To: <1345769101.10190.124.camel@haakon2.linux-iscsi.org>
Il 24/08/2012 02:45, Nicholas A. Bellinger ha scritto:
> So up until very recently, TCM would accept an I/O request for an DATA
> I/O type CDB with a max_sectors larger than the reported max_sectors for
> it's TCM backend (regardless of backend type), and silently generate N
> backend 'tasks' to complete the single initiator generated command.
This is what QEMU does if you use scsi-block, except for MMC devices
(because of the insanity of the commands used for burning).
> Also FYI for Paolo, for control type CDBs I've never actually seen an
> allocation length exceed max_sectors, so in practice AFAIK this only
> happens for DATA I/O type CDBs.
Yes, that was my impression as well.
> This was historically required by the pSCSI backend driver (using a
> number of old SCSI passthrough interfaces) in order to support this very
> type of case described above, but over the years the logic ended up
> creeping into various other non-passthrough backend drivers like IBLOCK
> +FILEIO. So for v3.6-rc1 code, hch ended up removing the 'task' logic
> thus allowing backends (and the layers below) to the I/O sectors >
> max_sectors handling work, allowing modern pSCSI using struct request to
> do the same. (hch assured me this works now for pSCSI)
So now LIO and QEMU work the same. (Did he test tapes too?)
> Anyways, I think having the guest limit virtio-scsi DATA I/O to
> max_sectors based upon the host accessible block limits is reasonable
> approach to consider. Reducing this value even further based upon the
> lowest max_sectors available amongst possible migration hosts would be a
> good idea here to avoid having to reject any I/O's exceeding a new
> host's device block queue limits.
Yeah, it's reasonable _assuming it is needed at all_. For disks, it is
not needed. For CD-ROMs it is, but right now we have only one report
and it is using USB so we don't know if the problem is in the drive or
rather in the USB bridge (whose quality usually leaves much to be desired).
So in the only observed case, the fix would really be a workaround; the
right thing to do with USB devices is to use USB passthrough.
Paolo
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Michael S. Tsirkin @ 2012-08-24 8:03 UTC (permalink / raw)
To: Rik van Riel
Cc: Rafael Aquini, Konrad Rzeszutek Wilk, Peter Zijlstra,
linux-kernel, virtualization, linux-mm, Andi Kleen, Minchan Kim,
Andrew Morton, Paul E. McKenney
In-Reply-To: <5036F111.4040607@redhat.com>
On Thu, Aug 23, 2012 at 11:12:17PM -0400, Rik van Riel wrote:
> On 08/23/2012 07:36 PM, Michael S. Tsirkin wrote:
>
> >--->
> >
> >virtio-balloon: replace page->lru list with page->private.
> >
> >The point is to free up page->lru for use by compaction.
> >Warning: completely untested, will provide tested version
> >if we agree on this direction.
>
> A singly linked list is not going to work for page migration,
> which needs to get pages that might be in the middle of the
> balloon list.
For virtballoon_migratepage? Hmm I think you are right. I'll
need to think it over but if we can think of no other way
to avoid ther need to handle isolation in virtio,
we'll just have to use the original plan and add
balloon core to mm.
> --
> All rights reversed
^ 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