Not really merged? Re: [merged] x86-paravirt-pte-updates-in-kunmap_atomic-need-to-be-synchronous-regardless-of-lazy_mmu-mode.patch removed from -mm tree

Not really merged? Re: [merged] x86-paravirt-pte-updates-in-kunmap_atomic-need-to-be-synchronous-regardless-of-lazy_mmu-mode.patch removed from -mm tree

[Konrad Rzeszutek Wilk]

On Fri, Oct 14, 2011 at 12:51:48PM -0700, akpm@google.com wrote:
> 
> The patch titled
>      Subject: x86/paravirt: PTE updates in k(un)map_atomic need to be synchronous, regardless of lazy_mmu mode
> has been removed from the -mm tree.  Its filename was
>      x86-paravirt-pte-updates-in-kunmap_atomic-need-to-be-synchronous-regardless-of-lazy_mmu-mode.patch
> 
> This patch was dropped because it was merged into mainline or a subsystem tree

Hey Andrew,

I am actually not seeing this in mainline? Was it accidently dropped out of your tree?

If that is the case I can convience you to put it back in or can I drive it to Linus with
your Ack-ed by?

> 
> The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/
> 
> ------------------------------------------------------
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Subject: x86/paravirt: PTE updates in k(un)map_atomic need to be synchronous, regardless of lazy_mmu mode
> 
> Fix an outstanding issue that has been reported since 2.6.37.  Under a
> heavy loaded machine processing "fork()" calls could crash with:
> 
> BUG: unable to handle kernel paging request at f573fc8c
> IP: [<c01abc54>] swap_count_continued+0x104/0x180
> *pdpt = 000000002a3b9027 *pde = 0000000001bed067 *pte = 0000000000000000
> Oops: 0000 [#1] SMP
> Modules linked in:
> Pid: 1638, comm: apache2 Not tainted 3.0.4-linode37 #1
> EIP: 0061:[<c01abc54>] EFLAGS: 00210246 CPU: 3
> EIP is at swap_count_continued+0x104/0x180
> .. snip..
> Call Trace:
>  [<c01ac222>] ? __swap_duplicate+0xc2/0x160
>  [<c01040f7>] ? pte_mfn_to_pfn+0x87/0xe0
>  [<c01ac2e4>] ? swap_duplicate+0x14/0x40
>  [<c01a0a6b>] ? copy_pte_range+0x45b/0x500
>  [<c01a0ca5>] ? copy_page_range+0x195/0x200
>  [<c01328c6>] ? dup_mmap+0x1c6/0x2c0
>  [<c0132cf8>] ? dup_mm+0xa8/0x130
>  [<c013376a>] ? copy_process+0x98a/0xb30
>  [<c013395f>] ? do_fork+0x4f/0x280
>  [<c01573b3>] ? getnstimeofday+0x43/0x100
>  [<c010f770>] ? sys_clone+0x30/0x40
>  [<c06c048d>] ? ptregs_clone+0x15/0x48
>  [<c06bfb71>] ? syscall_call+0x7/0xb
> 
> The problem is that in copy_page_range() we turn lazy mode on, and then in
> swap_entry_free() we call swap_count_continued() which ends up in:
> 
>          map = kmap_atomic(page, KM_USER0) + offset;
> 
> and then later we touch *map.
> 
> Since we are running in batched mode (lazy) we don't actually set up the
> PTE mappings and the kmap_atomic is not done synchronously and ends up
> trying to dereference a page that has not been set.
> 
> Looking at kmap_atomic_prot_pfn(), it uses 'arch_flush_lazy_mmu_mode' and
> doing the same in kmap_atomic_prot() and __kunmap_atomic() makes the problem
> go away.
> 
> Interestingly, commit b8bcfe997e4615 ("x86/paravirt: remove lazy mode in
> interrupts") removed part of this to fix an interrupt issue - but it went
> to far and did not consider this scenario.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> Cc: <stable@kernel.org>
> Signed-off-by: Andrew Morton <akpm@google.com>
> ---
> 
>  arch/x86/mm/highmem_32.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff -puN arch/x86/mm/highmem_32.c~x86-paravirt-pte-updates-in-kunmap_atomic-need-to-be-synchronous-regardless-of-lazy_mmu-mode arch/x86/mm/highmem_32.c
> --- a/arch/x86/mm/highmem_32.c~x86-paravirt-pte-updates-in-kunmap_atomic-need-to-be-synchronous-regardless-of-lazy_mmu-mode
> +++ a/arch/x86/mm/highmem_32.c
> @@ -45,6 +45,7 @@ void *kmap_atomic_prot(struct page *page
>  	vaddr = __fix_to_virt(FIX_KMAP_BEGIN + idx);
>  	BUG_ON(!pte_none(*(kmap_pte-idx)));
>  	set_pte(kmap_pte-idx, mk_pte(page, prot));
> +	arch_flush_lazy_mmu_mode();
>  
>  	return (void *)vaddr;
>  }
> @@ -88,6 +89,7 @@ void __kunmap_atomic(void *kvaddr)
>  		 */
>  		kpte_clear_flush(kmap_pte-idx, vaddr);
>  		kmap_atomic_idx_pop();
> +		arch_flush_lazy_mmu_mode();
>  	}
>  #ifdef CONFIG_DEBUG_HIGHMEM
>  	else {
> _
> 
> Patches currently in -mm which might be from konrad.wilk@oracle.com are
> 
> origin.patch
> linux-next.patch
> stop_machine-make-stop_machine-safe-and-efficient-to-call-early.patch

Re: Not really merged? Re: [merged] x86-paravirt-pte-updates-in-kunmap_atomic-need-to-be-synchronous-regardless-of-lazy_mmu-mode.patch removed from -mm tree

[Andrew Morton]

On Tue, 25 Oct 2011 14:24:50 -0400
Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:

> On Fri, Oct 14, 2011 at 12:51:48PM -0700, akpm@google.com wrote:
> > 
> > The patch titled
> >      Subject: x86/paravirt: PTE updates in k(un)map_atomic need to be synchronous, regardless of lazy_mmu mode
> > has been removed from the -mm tree.  Its filename was
> >      x86-paravirt-pte-updates-in-kunmap_atomic-need-to-be-synchronous-regardless-of-lazy_mmu-mode.patch
> > 
> > This patch was dropped because it was merged into mainline or a subsystem tree
> 
> Hey Andrew,
> 
> I am actually not seeing this in mainline? Was it accidently dropped out of your tree?

hm, well spotted.  I'm not sure what happened here - possibly the patch
was merged into an x86 tree (and hence linux-next) but later got lost. 
Or possibly not, and I just screwed up.

Either way, it's a pretty important patch - we marked it for -stable
backporting.

> If that is the case I can convience you to put it back in or can I drive it to Linus with
> your Ack-ed by?

I resurrected my copy and shall send it along to the x86 guys soon.

Re: Not really merged? Re: [merged] x86-paravirt-pte-updates-in-kunmap_atomic-need-to-be-synchronous-regardless-of-lazy_mmu-mode.patch removed from -mm tree

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Tue, 25 Oct 2011 14:24:50 -0400
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> 
> > On Fri, Oct 14, 2011 at 12:51:48PM -0700, akpm@google.com wrote:
> > > 
> > > The patch titled
> > >      Subject: x86/paravirt: PTE updates in k(un)map_atomic need to be synchronous, regardless of lazy_mmu mode
> > > has been removed from the -mm tree.  Its filename was
> > >      x86-paravirt-pte-updates-in-kunmap_atomic-need-to-be-synchronous-regardless-of-lazy_mmu-mode.patch
> > > 
> > > This patch was dropped because it was merged into mainline or a subsystem tree
> > 
> > Hey Andrew,
> > 
> > I am actually not seeing this in mainline? Was it accidently dropped out of your tree?
> 
> hm, well spotted.  I'm not sure what happened here - possibly the 
> patch was merged into an x86 tree (and hence linux-next) but later 
> got lost. Or possibly not, and I just screwed up.

No, a patch with the -i 'paravirt.*lazy' pattern never touched -tip, 
even temporarily.

Could it be that someone else (say the Xen guys) picked it up, it 
went into linux-next, you thought it's applied - but then they 
dropped it?

Do we have a full log of all linux-next patches?

> Either way, it's a pretty important patch - we marked it for 
> -stable backporting.

Agreed.

But IMO it's at least as important to figure out what went wrong. I 
somehow doubt it that you spuriously dropped it - that someone else 
messes up has a far higher likelihood.

> > If that is the case I can convience you to put it back in or can 
> > I drive it to Linus with your Ack-ed by?
> 
> I resurrected my copy and shall send it along to the x86 guys soon.

Thanks,

	Ingo

Re: Not really merged? Re: [merged] x86-paravirt-pte-updates-in-kunmap_atomic-need-to-be-synchronous-regardless-of-lazy_mmu-mode.patch removed from -mm tree

[Andrew Morton]

On Fri, 28 Oct 2011 09:08:39 +0200 Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > On Tue, 25 Oct 2011 14:24:50 -0400
> > Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> > 
> > > On Fri, Oct 14, 2011 at 12:51:48PM -0700, akpm@google.com wrote:
> > > > 
> > > > The patch titled
> > > >      Subject: x86/paravirt: PTE updates in k(un)map_atomic need to be synchronous, regardless of lazy_mmu mode
> > > > has been removed from the -mm tree.  Its filename was
> > > >      x86-paravirt-pte-updates-in-kunmap_atomic-need-to-be-synchronous-regardless-of-lazy_mmu-mode.patch
> > > > 
> > > > This patch was dropped because it was merged into mainline or a subsystem tree
> > > 
> > > Hey Andrew,
> > > 
> > > I am actually not seeing this in mainline? Was it accidently dropped out of your tree?
> > 
> > hm, well spotted.  I'm not sure what happened here - possibly the 
> > patch was merged into an x86 tree (and hence linux-next) but later 
> > got lost. Or possibly not, and I just screwed up.
> 
> No, a patch with the -i 'paravirt.*lazy' pattern never touched -tip, 
> even temporarily.
> 
> Could it be that someone else (say the Xen guys) picked it up, it 
> went into linux-next, you thought it's applied - but then they 
> dropped it?
> 
> Do we have a full log of all linux-next patches?

Don't know.

The patch was present in the linux-next which I pulled on 14 Oct.  It
is no longer in linux-next.

Here's my copy:


From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Subject: x86/paravirt: PTE updates in k(un)map_atomic need to be synchronous, regardless of lazy_mmu mode

Fix an outstanding issue that has been reported since 2.6.37.  Under a
heavy loaded machine processing "fork()" calls could crash with:

BUG: unable to handle kernel paging request at f573fc8c
IP: [<c01abc54>] swap_count_continued+0x104/0x180
*pdpt = 000000002a3b9027 *pde = 0000000001bed067 *pte = 0000000000000000
Oops: 0000 [#1] SMP
Modules linked in:
Pid: 1638, comm: apache2 Not tainted 3.0.4-linode37 #1
EIP: 0061:[<c01abc54>] EFLAGS: 00210246 CPU: 3
EIP is at swap_count_continued+0x104/0x180
.. snip..
Call Trace:
 [<c01ac222>] ? __swap_duplicate+0xc2/0x160
 [<c01040f7>] ? pte_mfn_to_pfn+0x87/0xe0
 [<c01ac2e4>] ? swap_duplicate+0x14/0x40
 [<c01a0a6b>] ? copy_pte_range+0x45b/0x500
 [<c01a0ca5>] ? copy_page_range+0x195/0x200
 [<c01328c6>] ? dup_mmap+0x1c6/0x2c0
 [<c0132cf8>] ? dup_mm+0xa8/0x130
 [<c013376a>] ? copy_process+0x98a/0xb30
 [<c013395f>] ? do_fork+0x4f/0x280
 [<c01573b3>] ? getnstimeofday+0x43/0x100
 [<c010f770>] ? sys_clone+0x30/0x40
 [<c06c048d>] ? ptregs_clone+0x15/0x48
 [<c06bfb71>] ? syscall_call+0x7/0xb

The problem is that in copy_page_range() we turn lazy mode on, and then in
swap_entry_free() we call swap_count_continued() which ends up in:

         map = kmap_atomic(page, KM_USER0) + offset;

and then later we touch *map.

Since we are running in batched mode (lazy) we don't actually set up the
PTE mappings and the kmap_atomic is not done synchronously and ends up
trying to dereference a page that has not been set.

Looking at kmap_atomic_prot_pfn(), it uses 'arch_flush_lazy_mmu_mode' and
doing the same in kmap_atomic_prot() and __kunmap_atomic() makes the problem
go away.

Interestingly, commit b8bcfe997e4615 ("x86/paravirt: remove lazy mode in
interrupts") removed part of this to fix an interrupt issue - but it went
to far and did not consider this scenario.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
Cc: <stable@kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 arch/x86/mm/highmem_32.c |    2 ++
 1 file changed, 2 insertions(+)

diff -puN arch/x86/mm/highmem_32.c~x86-paravirt-pte-updates-in-kunmap_atomic-need-to-be-synchronous-regardless-of-lazy_mmu-mode arch/x86/mm/highmem_32.c
--- a/arch/x86/mm/highmem_32.c~x86-paravirt-pte-updates-in-kunmap_atomic-need-to-be-synchronous-regardless-of-lazy_mmu-mode
+++ a/arch/x86/mm/highmem_32.c
@@ -45,6 +45,7 @@ void *kmap_atomic_prot(struct page *page
 	vaddr = __fix_to_virt(FIX_KMAP_BEGIN + idx);
 	BUG_ON(!pte_none(*(kmap_pte-idx)));
 	set_pte(kmap_pte-idx, mk_pte(page, prot));
+	arch_flush_lazy_mmu_mode();
 
 	return (void *)vaddr;
 }
@@ -88,6 +89,7 @@ void __kunmap_atomic(void *kvaddr)
 		 */
 		kpte_clear_flush(kmap_pte-idx, vaddr);
 		kmap_atomic_idx_pop();
+		arch_flush_lazy_mmu_mode();
 	}
 #ifdef CONFIG_DEBUG_HIGHMEM
 	else {
_


> But IMO it's at least as important to figure out what went wrong. I 
> somehow doubt it that you spuriously dropped it - that someone else 
> messes up has a far higher likelihood.

My drop was legitimate. 

Here's the commit from the Oct 14 linux-next:


commit ab67482036cee590753dd42b7f66aada97e6dcde
Author:     Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
AuthorDate: Fri Sep 23 17:02:29 2011 -0400
Commit:     Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CommitDate: Mon Sep 26 09:12:37 2011 -0400

    x86/paravirt: Partially revert "remove lazy mode in interrupts"
    
    which has git commit b8bcfe997e46150fedcc3f5b26b846400122fdd9.
    
    The unintended consequence of removing the flushing of MMU
    updates when doing kmap_atomic (or kunmap_atomic) is that we can
    hit a dereference bug when processing a "fork()" under a heavy loaded
    machine. Specifically we can hit:
    
    BUG: unable to handle kernel paging request at f573fc8c
    IP: [<c01abc54>] swap_count_continued+0x104/0x180
    *pdpt = 000000002a3b9027 *pde = 0000000001bed067 *pte = 0000000000000000
    Oops: 0000 [#1] SMP
    Modules linked in:
    Pid: 1638, comm: apache2 Not tainted 3.0.4-linode37 #1
    EIP: 0061:[<c01abc54>] EFLAGS: 00210246 CPU: 3
    EIP is at swap_count_continued+0x104/0x180
    .. snip..
    Call Trace:
     [<c01ac222>] ? __swap_duplicate+0xc2/0x160
     [<c01040f7>] ? pte_mfn_to_pfn+0x87/0xe0
     [<c01ac2e4>] ? swap_duplicate+0x14/0x40
     [<c01a0a6b>] ? copy_pte_range+0x45b/0x500
     [<c01a0ca5>] ? copy_page_range+0x195/0x200
     [<c01328c6>] ? dup_mmap+0x1c6/0x2c0
     [<c0132cf8>] ? dup_mm+0xa8/0x130
     [<c013376a>] ? copy_process+0x98a/0xb30
     [<c013395f>] ? do_fork+0x4f/0x280
     [<c01573b3>] ? getnstimeofday+0x43/0x100
     [<c010f770>] ? sys_clone+0x30/0x40
     [<c06c048d>] ? ptregs_clone+0x15/0x48
     [<c06bfb71>] ? syscall_call+0x7/0xb
    
    The problem looks that in copy_page_range we turn lazy mode on, and then
    in swap_entry_free we call swap_count_continued which ends up in:
    
             map = kmap_atomic(page, KM_USER0) + offset;
    
    and then later touches *map.
    
    Since we are running in batched mode (lazy) we don't actually set up the
    PTE mappings and the kmap_atomic is not done synchronously and ends up
    trying to dereference a page that has not been set.
    
    Looking at kmap_atomic_prot_pfn, it uses 'arch_flush_lazy_mmu_mode' and
    sprinkling that in kmap_atomic_prot and __kunmap_atomic makes the problem
    go away.
    
    CC: Thomas Gleixner <tglx@linutronix.de>
    CC: Ingo Molnar <mingo@redhat.com>
    CC: "H. Peter Anvin" <hpa@zytor.com>
    CC: x86@kernel.org
    CC: Peter Zijlstra <a.p.zijlstra@chello.nl>
    CC: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
    CC: stable@kernel.org
    Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

diff --git a/arch/x86/mm/highmem_32.c b/arch/x86/mm/highmem_32.c
index b499626..f4f29b1 100644
--- a/arch/x86/mm/highmem_32.c
+++ b/arch/x86/mm/highmem_32.c
@@ -45,6 +45,7 @@ void *kmap_atomic_prot(struct page *page, pgprot_t prot)
 	vaddr = __fix_to_virt(FIX_KMAP_BEGIN + idx);
 	BUG_ON(!pte_none(*(kmap_pte-idx)));
 	set_pte(kmap_pte-idx, mk_pte(page, prot));
+	arch_flush_lazy_mmu_mode();
 
 	return (void *)vaddr;
 }
@@ -88,6 +89,7 @@ void __kunmap_atomic(void *kvaddr)
 		 */
 		kpte_clear_flush(kmap_pte-idx, vaddr);
 		kmap_atomic_idx_pop();
+		arch_flush_lazy_mmu_mode();
 	}
 #ifdef CONFIG_DEBUG_HIGHMEM
 	else {


I'm not sure what to make of that.  The signoffs imply that Konrad is
running his own git tree, but I don't think he is.  Or someone (Jeremy
or Rusty I think) merged it but didn't add a signoff.

Note that the patch was merged using its old name "x86/paravirt:
Partially revert "remove lazy mode in interrupts"".  The patch got
renamed to "x86/paravirt: PTE updates in k(un)map_atomic need to be
synchronous, regardless of lazy_mmu mode" and perhaps this prompted
someone to drop the old-named patch then lose the new-named one.

[Xen tree maintenance] Re: Not really merged? Re: [merged] x86-paravirt-pte-updates-in-kunmap_atomic-need-to-be-synchronous-regardless-of-lazy_mmu-mode.patch removed from -mm tree

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> > > > Hey Andrew,
> > > > 
> > > > I am actually not seeing this in mainline? Was it accidently 
> > > > dropped out of your tree?
> > > 
> > > hm, well spotted.  I'm not sure what happened here - possibly 
> > > the patch was merged into an x86 tree (and hence linux-next) 
> > > but later got lost. Or possibly not, and I just screwed up.
>
> > No, a patch with the -i 'paravirt.*lazy' pattern never touched 
> > -tip, even temporarily.
> >
> > Could it be that someone else (say the Xen guys) picked it up, it 
> > went into linux-next, you thought it's applied - but then they 
> > dropped it?
> > 
> > Do we have a full log of all linux-next patches?
> 
> Don't know.
> 
> The patch was present in the linux-next which I pulled on 14 Oct.  
> It is no longer in linux-next.
[...]

> My drop was legitimate. 
> 
> Here's the commit from the Oct 14 linux-next:
> 
> 
> commit ab67482036cee590753dd42b7f66aada97e6dcde
> Author:     Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> AuthorDate: Fri Sep 23 17:02:29 2011 -0400
> Commit:     Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CommitDate: Mon Sep 26 09:12:37 2011 -0400

ARGH!

Konrad, STOP THIS CRAP!

This is the Nth time you have interefered with the x86 tree's 
workflow - and now you have caused patch loss in -mm.

I'll have to insist on the Xen tree being merged via the x86 tree 
again, this clearly does not work, your maintenance practices are 
VERY incompetent and you are not learning.

The rule is very simple: don't EVER push anything arch/x86/ into 
linux-next that is outside the arch/x86/xen/ and 
arch/x86/include/asm/xen/ patterns, without very clear acks from the 
x86 maintainers and a binding promise to carry that patch upstream!

Thanks,

	Ingo

Re: [Xen tree maintenance] Re: Not really merged? Re: [merged] x86-paravirt-pte-updates-in-kunmap_atomic-need-to-be-synchronous-regardless-of-lazy_mmu-mode.patch removed from -mm tree

[Konrad Rzeszutek Wilk]

> > commit ab67482036cee590753dd42b7f66aada97e6dcde
> > Author:     Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > AuthorDate: Fri Sep 23 17:02:29 2011 -0400
> > Commit:     Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > CommitDate: Mon Sep 26 09:12:37 2011 -0400

Aha! And I am not sure why I put that in my #linux-next branch. That
is not good - thanks for identifying the cause of that patch being
dropped, and I am sorry for pushing it there without getting the
proper Acks.

> 
> ARGH!
> 
> Konrad, STOP THIS CRAP!
> 
> This is the Nth time you have interefered with the x86 tree's 
> workflow - and now you have caused patch loss in -mm.
> 
> I'll have to insist on the Xen tree being merged via the x86 tree 
> again, this clearly does not work, your maintenance practices are 
> VERY incompetent and you are not learning.
> 
> The rule is very simple: don't EVER push anything arch/x86/ into 
> linux-next that is outside the arch/x86/xen/ and 
> arch/x86/include/asm/xen/ patterns, without very clear acks from the 
> x86 maintainers and a binding promise to carry that patch upstream!

Of course.

If you have some time, can you tell me what other times this has
caused trouble? I remember Jeremy did a mistake some time ago
(so patch with a very very old time date) that screwed up something.

And .... oh, the compile time one that Randy found and it did not get
fixed for some very emberassingly long time. If I remember right that was
in Jeremy's tree (xen) - not mine (xen-two):
http://marc.info/?i=20110804193534.GB12729@elte.hu

We do have _two_ Xen trees (xen and xen-two) - which is not making things
easier to figure out who is at fault for what.

You would not have a good overview of maintainer practices so that both
me and Jeremy can refresh our memory of what to do and especially
what _not_ to do?

Re: Not really merged? Re: [merged] x86-paravirt-pte-updates-in-kunmap_atomic-need-to-be-synchronous-regardless-of-lazy_mmu-mode.patch removed from -mm tree

[Konrad Rzeszutek Wilk]

> > But IMO it's at least as important to figure out what went wrong. I 
> > somehow doubt it that you spuriously dropped it - that someone else 
> > messes up has a far higher likelihood.
> 
> My drop was legitimate. 
> 
> Here's the commit from the Oct 14 linux-next:
> 
> 
> commit ab67482036cee590753dd42b7f66aada97e6dcde
> Author:     Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> AuthorDate: Fri Sep 23 17:02:29 2011 -0400
> Commit:     Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CommitDate: Mon Sep 26 09:12:37 2011 -0400
.. snip..
> 
> I'm not sure what to make of that.  The signoffs imply that Konrad is
> running his own git tree, but I don't think he is.  Or someone (Jeremy
> or Rusty I think) merged it but didn't add a signoff.

Hey Andrew,

I am running my own tree (git://git.kernel.org/pub/scm/linux/kernel/git/konrad/xen.git)

> Note that the patch was merged using its old name "x86/paravirt:
> Partially revert "remove lazy mode in interrupts"".  The patch got
> renamed to "x86/paravirt: PTE updates in k(un)map_atomic need to be
> synchronous, regardless of lazy_mmu mode" and perhaps this prompted
> someone to drop the old-named patch then lose the new-named one.

I am at loss as why I had that patch in my tree. I *might* have merged
in my #linux-next to compile/build/run-time test and then pushed that
tree. But I can't recall exactly why I would have done that (the push).

The fault is with me and I am sorry for making you (and Ingo) spend the
whole night digging through linux-next git history to figure this out.

Re: Not really merged? Re: [merged] x86-paravirt-pte-updates-in-kunmap_atomic-need-to-be-synchronous-regardless-of-lazy_mmu-mode.patch removed from -mm tree

[Stefan Bader]

On 28.10.2011 09:39, Andrew Morton wrote:
> On Fri, 28 Oct 2011 09:08:39 +0200 Ingo Molnar <mingo@elte.hu> wrote:
> 
>>
>> * Andrew Morton <akpm@linux-foundation.org> wrote:
>>
>>> On Tue, 25 Oct 2011 14:24:50 -0400
>>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
>>>
>>>> On Fri, Oct 14, 2011 at 12:51:48PM -0700, akpm@google.com wrote:
>>>>>
>>>>> The patch titled
>>>>>      Subject: x86/paravirt: PTE updates in k(un)map_atomic need to be synchronous, regardless of lazy_mmu mode
>>>>> has been removed from the -mm tree.  Its filename was
>>>>>      x86-paravirt-pte-updates-in-kunmap_atomic-need-to-be-synchronous-regardless-of-lazy_mmu-mode.patch
>>>>>
>>>>> This patch was dropped because it was merged into mainline or a subsystem tree
>>>>
>>>> Hey Andrew,
>>>>
>>>> I am actually not seeing this in mainline? Was it accidently dropped out of your tree?
>>>
>>> hm, well spotted.  I'm not sure what happened here - possibly the 
>>> patch was merged into an x86 tree (and hence linux-next) but later 
>>> got lost. Or possibly not, and I just screwed up.
>>
>> No, a patch with the -i 'paravirt.*lazy' pattern never touched -tip, 
>> even temporarily.
>>
>> Could it be that someone else (say the Xen guys) picked it up, it 
>> went into linux-next, you thought it's applied - but then they 
>> dropped it?
>>
>> Do we have a full log of all linux-next patches?
> 
> Don't know.
> 
> The patch was present in the linux-next which I pulled on 14 Oct.  It
> is no longer in linux-next.
> 

So, as of today, this seems to be back on the master branch of linux-next (I
guess from Andrew putting it back, but I am never sure with linux-next). But I
am not sure how/when this would go into Linus tree. I assume without any
specific action maybe merge window for 3.3...
We got some positive feedback on it from users running into the problem. So it
seems like a valuable change. From the discusions so far I take that technically
the change did not trigger resistance. For that reason I wanted to ask whether
there is a chance that this looks important enough to be pushed before the next
merge window...

Thanks,
Stefan

> Here's my copy:
> 
> 
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Subject: x86/paravirt: PTE updates in k(un)map_atomic need to be synchronous, regardless of lazy_mmu mode
> 
> Fix an outstanding issue that has been reported since 2.6.37.  Under a
> heavy loaded machine processing "fork()" calls could crash with:
> 
> BUG: unable to handle kernel paging request at f573fc8c
> IP: [<c01abc54>] swap_count_continued+0x104/0x180
> *pdpt = 000000002a3b9027 *pde = 0000000001bed067 *pte = 0000000000000000
> Oops: 0000 [#1] SMP
> Modules linked in:
> Pid: 1638, comm: apache2 Not tainted 3.0.4-linode37 #1
> EIP: 0061:[<c01abc54>] EFLAGS: 00210246 CPU: 3
> EIP is at swap_count_continued+0x104/0x180
> .. snip..
> Call Trace:
>  [<c01ac222>] ? __swap_duplicate+0xc2/0x160
>  [<c01040f7>] ? pte_mfn_to_pfn+0x87/0xe0
>  [<c01ac2e4>] ? swap_duplicate+0x14/0x40
>  [<c01a0a6b>] ? copy_pte_range+0x45b/0x500
>  [<c01a0ca5>] ? copy_page_range+0x195/0x200
>  [<c01328c6>] ? dup_mmap+0x1c6/0x2c0
>  [<c0132cf8>] ? dup_mm+0xa8/0x130
>  [<c013376a>] ? copy_process+0x98a/0xb30
>  [<c013395f>] ? do_fork+0x4f/0x280
>  [<c01573b3>] ? getnstimeofday+0x43/0x100
>  [<c010f770>] ? sys_clone+0x30/0x40
>  [<c06c048d>] ? ptregs_clone+0x15/0x48
>  [<c06bfb71>] ? syscall_call+0x7/0xb
> 
> The problem is that in copy_page_range() we turn lazy mode on, and then in
> swap_entry_free() we call swap_count_continued() which ends up in:
> 
>          map = kmap_atomic(page, KM_USER0) + offset;
> 
> and then later we touch *map.
> 
> Since we are running in batched mode (lazy) we don't actually set up the
> PTE mappings and the kmap_atomic is not done synchronously and ends up
> trying to dereference a page that has not been set.
> 
> Looking at kmap_atomic_prot_pfn(), it uses 'arch_flush_lazy_mmu_mode' and
> doing the same in kmap_atomic_prot() and __kunmap_atomic() makes the problem
> go away.
> 
> Interestingly, commit b8bcfe997e4615 ("x86/paravirt: remove lazy mode in
> interrupts") removed part of this to fix an interrupt issue - but it went
> to far and did not consider this scenario.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> Cc: <stable@kernel.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  arch/x86/mm/highmem_32.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff -puN arch/x86/mm/highmem_32.c~x86-paravirt-pte-updates-in-kunmap_atomic-need-to-be-synchronous-regardless-of-lazy_mmu-mode arch/x86/mm/highmem_32.c
> --- a/arch/x86/mm/highmem_32.c~x86-paravirt-pte-updates-in-kunmap_atomic-need-to-be-synchronous-regardless-of-lazy_mmu-mode
> +++ a/arch/x86/mm/highmem_32.c
> @@ -45,6 +45,7 @@ void *kmap_atomic_prot(struct page *page
>  	vaddr = __fix_to_virt(FIX_KMAP_BEGIN + idx);
>  	BUG_ON(!pte_none(*(kmap_pte-idx)));
>  	set_pte(kmap_pte-idx, mk_pte(page, prot));
> +	arch_flush_lazy_mmu_mode();
>  
>  	return (void *)vaddr;
>  }
> @@ -88,6 +89,7 @@ void __kunmap_atomic(void *kvaddr)
>  		 */
>  		kpte_clear_flush(kmap_pte-idx, vaddr);
>  		kmap_atomic_idx_pop();
> +		arch_flush_lazy_mmu_mode();
>  	}
>  #ifdef CONFIG_DEBUG_HIGHMEM
>  	else {
> _
> 
> 
>> But IMO it's at least as important to figure out what went wrong. I 
>> somehow doubt it that you spuriously dropped it - that someone else 
>> messes up has a far higher likelihood.
> 
> My drop was legitimate. 
> 
> Here's the commit from the Oct 14 linux-next:
> 
> 
> commit ab67482036cee590753dd42b7f66aada97e6dcde
> Author:     Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> AuthorDate: Fri Sep 23 17:02:29 2011 -0400
> Commit:     Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CommitDate: Mon Sep 26 09:12:37 2011 -0400
> 
>     x86/paravirt: Partially revert "remove lazy mode in interrupts"
>     
>     which has git commit b8bcfe997e46150fedcc3f5b26b846400122fdd9.
>     
>     The unintended consequence of removing the flushing of MMU
>     updates when doing kmap_atomic (or kunmap_atomic) is that we can
>     hit a dereference bug when processing a "fork()" under a heavy loaded
>     machine. Specifically we can hit:
>     
>     BUG: unable to handle kernel paging request at f573fc8c
>     IP: [<c01abc54>] swap_count_continued+0x104/0x180
>     *pdpt = 000000002a3b9027 *pde = 0000000001bed067 *pte = 0000000000000000
>     Oops: 0000 [#1] SMP
>     Modules linked in:
>     Pid: 1638, comm: apache2 Not tainted 3.0.4-linode37 #1
>     EIP: 0061:[<c01abc54>] EFLAGS: 00210246 CPU: 3
>     EIP is at swap_count_continued+0x104/0x180
>     .. snip..
>     Call Trace:
>      [<c01ac222>] ? __swap_duplicate+0xc2/0x160
>      [<c01040f7>] ? pte_mfn_to_pfn+0x87/0xe0
>      [<c01ac2e4>] ? swap_duplicate+0x14/0x40
>      [<c01a0a6b>] ? copy_pte_range+0x45b/0x500
>      [<c01a0ca5>] ? copy_page_range+0x195/0x200
>      [<c01328c6>] ? dup_mmap+0x1c6/0x2c0
>      [<c0132cf8>] ? dup_mm+0xa8/0x130
>      [<c013376a>] ? copy_process+0x98a/0xb30
>      [<c013395f>] ? do_fork+0x4f/0x280
>      [<c01573b3>] ? getnstimeofday+0x43/0x100
>      [<c010f770>] ? sys_clone+0x30/0x40
>      [<c06c048d>] ? ptregs_clone+0x15/0x48
>      [<c06bfb71>] ? syscall_call+0x7/0xb
>     
>     The problem looks that in copy_page_range we turn lazy mode on, and then
>     in swap_entry_free we call swap_count_continued which ends up in:
>     
>              map = kmap_atomic(page, KM_USER0) + offset;
>     
>     and then later touches *map.
>     
>     Since we are running in batched mode (lazy) we don't actually set up the
>     PTE mappings and the kmap_atomic is not done synchronously and ends up
>     trying to dereference a page that has not been set.
>     
>     Looking at kmap_atomic_prot_pfn, it uses 'arch_flush_lazy_mmu_mode' and
>     sprinkling that in kmap_atomic_prot and __kunmap_atomic makes the problem
>     go away.
>     
>     CC: Thomas Gleixner <tglx@linutronix.de>
>     CC: Ingo Molnar <mingo@redhat.com>
>     CC: "H. Peter Anvin" <hpa@zytor.com>
>     CC: x86@kernel.org
>     CC: Peter Zijlstra <a.p.zijlstra@chello.nl>
>     CC: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
>     CC: stable@kernel.org
>     Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> diff --git a/arch/x86/mm/highmem_32.c b/arch/x86/mm/highmem_32.c
> index b499626..f4f29b1 100644
> --- a/arch/x86/mm/highmem_32.c
> +++ b/arch/x86/mm/highmem_32.c
> @@ -45,6 +45,7 @@ void *kmap_atomic_prot(struct page *page, pgprot_t prot)
>  	vaddr = __fix_to_virt(FIX_KMAP_BEGIN + idx);
>  	BUG_ON(!pte_none(*(kmap_pte-idx)));
>  	set_pte(kmap_pte-idx, mk_pte(page, prot));
> +	arch_flush_lazy_mmu_mode();
>  
>  	return (void *)vaddr;
>  }
> @@ -88,6 +89,7 @@ void __kunmap_atomic(void *kvaddr)
>  		 */
>  		kpte_clear_flush(kmap_pte-idx, vaddr);
>  		kmap_atomic_idx_pop();
> +		arch_flush_lazy_mmu_mode();
>  	}
>  #ifdef CONFIG_DEBUG_HIGHMEM
>  	else {
> 
> 
> I'm not sure what to make of that.  The signoffs imply that Konrad is
> running his own git tree, but I don't think he is.  Or someone (Jeremy
> or Rusty I think) merged it but didn't add a signoff.
> 
> Note that the patch was merged using its old name "x86/paravirt:
> Partially revert "remove lazy mode in interrupts"".  The patch got
> renamed to "x86/paravirt: PTE updates in k(un)map_atomic need to be
> synchronous, regardless of lazy_mmu mode" and perhaps this prompted
> someone to drop the old-named patch then lose the new-named one.
> 
> 
> --
> 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/

Re: Not really merged? Re: [merged] x86-paravirt-pte-updates-in-kunmap_atomic-need-to-be-synchronous-regardless-of-lazy_mmu-mode.patch removed from -mm tree

[Andrew Morton]

On Wed, 30 Nov 2011 12:02:49 +0100 Stefan Bader <stefan.bader@canonical.com> wrote:

> So, as of today, this seems to be back on the master branch of linux-next (I
> guess from Andrew putting it back, but I am never sure with linux-next). But I
> am not sure how/when this would go into Linus tree. I assume without any
> specific action maybe merge window for 3.3...
> We got some positive feedback on it from users running into the problem. So it
> seems like a valuable change. From the discusions so far I take that technically
> the change did not trigger resistance. For that reason I wanted to ask whether
> there is a chance that this looks important enough to be pushed before the next
> merge window...

I sent this patch to the x86 maintainers two weeks ago.  It was
ignored, as were the other 11 patches I sent.  Later I will resend them
all.  If they are again ignored I will later send them yet again, and
so on.

Re: Not really merged? Re: [merged] x86-paravirt-pte-updates-in-kunmap_atomic-need-to-be-synchronous-regardless-of-lazy_mmu-mode.patch removed from -mm tree

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Wed, 30 Nov 2011 12:02:49 +0100 Stefan Bader <stefan.bader@canonical.com> wrote:
> 
> > So, as of today, this seems to be back on the master branch of linux-next (I
> > guess from Andrew putting it back, but I am never sure with linux-next). But I
> > am not sure how/when this would go into Linus tree. I assume without any
> > specific action maybe merge window for 3.3...
> > We got some positive feedback on it from users running into the problem. So it
> > seems like a valuable change. From the discusions so far I take that technically
> > the change did not trigger resistance. For that reason I wanted to ask whether
> > there is a chance that this looks important enough to be pushed before the next
> > merge window...
> 
> I sent this patch to the x86 maintainers two weeks ago.  It 
> was ignored, as were the other 11 patches I sent.  Later I 
> will resend them all.  If they are again ignored I will later 
> send them yet again, and so on.

they are still sitting in my mbox - working down the backlog 
now.

Thanks,

	Ingo