Prev: [PATCH 2/2] workqueues: export keventd_wq
Next: [PATCH 2/2] virtio-net: pass gfp to add_buf
From: Andrea Arcangeli on 10 May 2010 15:10
On Mon, May 10, 2010 at 12:41:07PM -0500, Christoph Lameter wrote:
> A simple way to disallow migration of pages is to increment the refcount
> of a page.

Ok for migrate but it won't prevent to crash in split_huge_page rmap
walk, nor the PG_lock. Why for a rmap bug have a migrate specific fix?
The fix that makes execve the only special place to handle in every
rmap walk, is at least more maintainable than a fix that makes one of
the rmap walk users special and won't fix the others, as there will be
more than just 1 user that requires this. My fix didn't make execve
special and it didn't require execve knowledge into the every rmap
walk like migrate (split_huge_page etc...) but as long as the kernel
doesn't crash I'm fine ;).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: KAMEZAWA Hiroyuki on 10 May 2010 20:20
On Mon, 10 May 2010 21:05:59 +0200
Andrea Arcangeli <aarcange(a)redhat.com> wrote:

> On Mon, May 10, 2010 at 12:41:07PM -0500, Christoph Lameter wrote:
> > A simple way to disallow migration of pages is to increment the refcount
> > of a page.
>
> Ok for migrate but it won't prevent to crash in split_huge_page rmap
> walk, nor the PG_lock. Why for a rmap bug have a migrate specific fix?
> The fix that makes execve the only special place to handle in every
> rmap walk, is at least more maintainable than a fix that makes one of
> the rmap walk users special and won't fix the others, as there will be
> more than just 1 user that requires this. My fix didn't make execve
> special and it didn't require execve knowledge into the every rmap
> walk like migrate (split_huge_page etc...) but as long as the kernel
> doesn't crash I'm fine ;).
>

At first, I like step-by-step approach even if it makes our cost double
because it's easy to understand and makes chasing change-log easy.

Ok, your split_huge_page() has some problems with current rmap+migration.
But I don't like a patch for never-happen-now bug in change-log.

I believe it can be fixed by the same approach for execs.
Renaming
#define VM_STACK_INCOMPLETE_SETUP
to be
#define VM_TEMPORARY_INCONSITENT_RMAP
in _your_ patch series and add some check in rmap_walk() seems enough.

Of course, I may misunderstand your problem. Could you show your patch
which meets the problem with rmap+migration ?

Thanks,
-Kame




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Christoph Lameter on 11 May 2010 10:50
On Mon, 10 May 2010, Mel Gorman wrote:

> > A simple way to disallow migration of pages is to increment the refcount
> > of a page.
> I guess it could be done by walking the page-tables in advance of the move
> and elevating the page count of any pages faulted and then finding those
> pages afterwards. The fail path would be a bit of a pain though if the page
> tables are partially moved though. It's unnecessarily complicated when the
> temporary stack can be easily avoided.

Faulting during exec? Dont we hold mmap_sem for write? A get_user_pages()
or so on the range will increment the refcount.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Christoph Lameter on 11 May 2010 12:00
On Tue, 11 May 2010, Mel Gorman wrote:

> Or just identify the temporary stack from the migration side instead of
> adding to the cost of exec?

Adding one off checks to a generic mechanism isnt really clean
programming. Using the provided means of disabling a generic mechanism is.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Andrea Arcangeli on 11 May 2010 12:40
On Tue, May 11, 2010 at 05:15:16PM +0100, Mel Gorman wrote:
> On Tue, May 11, 2010 at 10:56:00AM -0500, Christoph Lameter wrote:
> > On Tue, 11 May 2010, Mel Gorman wrote:
> >
> > > Or just identify the temporary stack from the migration side instead of
> > > adding to the cost of exec?
> >
> > Adding one off checks to a generic mechanism isnt really clean
> > programming. Using the provided means of disabling a generic mechanism is.
> >
>
> Andrea's solution is likely lighter than yours as it is one kmalloc and
> an insertion into the VM as opposed to a page table walk with reference
> counting. Better yet, it exists as a patch that has been tested and it
> fits in with the generic mechanism by guaranteeing that rmap_walk finds
> all the migration PTEs during the second walk.
>
> The problem remains the same - that class of solution increases the cost of
> a common operation (exec) to keep a much less operation (migration) happy.

page table walk adding reference counting is still a one off check,
the generic rmap_walk mechanism won't care about the reference
counting, still only migrate checks the page count... so it doesn't
move the needle in clean programming terms.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
First  |  Prev  | 
Pages: 1 2 3
Prev: [PATCH 2/2] workqueues: export keventd_wq
Next: [PATCH 2/2] virtio-net: pass gfp to add_buf