Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1509293 > unrolled thread

CONFIG_VMAP_STACK, on-stack struct, and wake_up_bit

Back to article view | Back to linux.kernel

Contents

  CONFIG_VMAP_STACK, on-stack struct, and wake_up_bit Andreas Gruenbacher <agruenba@redhat.com> - 2016-10-26 15:00 +0200
    Re: CONFIG_VMAP_STACK, on-stack struct, and wake_up_bit Andy Lutomirski <luto@amacapital.net> - 2016-10-26 18:00 +0200
      Re: CONFIG_VMAP_STACK, on-stack struct, and wake_up_bit Linus Torvalds <torvalds@linux-foundation.org> - 2016-10-26 18:40 +0200
        Re: CONFIG_VMAP_STACK, on-stack struct, and wake_up_bit Linus Torvalds <torvalds@linux-foundation.org> - 2016-10-26 19:20 +0200
          Re: CONFIG_VMAP_STACK, on-stack struct, and wake_up_bit Linus Torvalds <torvalds@linux-foundation.org> - 2016-10-26 20:00 +0200
            Re: CONFIG_VMAP_STACK, on-stack struct, and wake_up_bit Bob Peterson <rpeterso@redhat.com> - 2016-10-26 20:10 +0200
              Re: CONFIG_VMAP_STACK, on-stack struct, and wake_up_bit Linus Torvalds <torvalds@linux-foundation.org> - 2016-10-26 20:20 +0200
                Re: CONFIG_VMAP_STACK, on-stack struct, and wake_up_bit Bob Peterson <rpeterso@redhat.com> - 2016-10-26 21:20 +0200
                Re: CONFIG_VMAP_STACK, on-stack struct, and wake_up_bit Bob Peterson <rpeterso@redhat.com> - 2016-10-26 23:10 +0200
                  Re: CONFIG_VMAP_STACK, on-stack struct, and wake_up_bit Linus Torvalds <torvalds@linux-foundation.org> - 2016-10-26 23:40 +0200
                    Re: CONFIG_VMAP_STACK, on-stack struct, and wake_up_bit Borislav Petkov <bp@suse.de> - 2016-10-27 00:50 +0200
                  Re: CONFIG_VMAP_STACK, on-stack struct, and wake_up_bit Borislav Petkov <bp@alien8.de> - 2016-10-27 01:20 +0200
                    Re: CONFIG_VMAP_STACK, on-stack struct, and wake_up_bit Borislav Petkov <bp@alien8.de> - 2016-10-27 16:00 +0200
                      Re: CONFIG_VMAP_STACK, on-stack struct, and wake_up_bit Bob Peterson <rpeterso@redhat.com> - 2016-10-27 21:00 +0200
                        Re: CONFIG_VMAP_STACK, on-stack struct, and wake_up_bit Borislav Petkov <bp@alien8.de> - 2016-10-27 21:30 +0200
                          Re: CONFIG_VMAP_STACK, on-stack struct, and wake_up_bit Bob Peterson <rpeterso@redhat.com> - 2016-10-27 23:10 +0200
                            Re: CONFIG_VMAP_STACK, on-stack struct, and wake_up_bit Borislav Petkov <bp@alien8.de> - 2016-10-27 23:20 +0200
                      [tip:x86/urgent] x86/microcode/AMD: Fix more fallout from  CONFIG_RANDOMIZE_MEMORY=y tip-bot for Borislav Petkov <tipbot@zytor.com> - 2016-10-28 10:50 +0200
          Re: CONFIG_VMAP_STACK, on-stack struct, and wake_up_bit Mel Gorman <mgorman@techsingularity.net> - 2016-10-26 22:40 +0200
            Re: CONFIG_VMAP_STACK, on-stack struct, and wake_up_bit Linus Torvalds <torvalds@linux-foundation.org> - 2016-10-26 23:30 +0200
              Re: CONFIG_VMAP_STACK, on-stack struct, and wake_up_bit Mel Gorman <mgorman@techsingularity.net> - 2016-10-27 00:10 +0200
                Re: CONFIG_VMAP_STACK, on-stack struct, and wake_up_bit Linus Torvalds <torvalds@linux-foundation.org> - 2016-10-27 00:20 +0200
                  Re: CONFIG_VMAP_STACK, on-stack struct, and wake_up_bit Mel Gorman <mgorman@techsingularity.net> - 2016-10-27 01:10 +0200
                    Re: CONFIG_VMAP_STACK, on-stack struct, and wake_up_bit Mel Gorman <mgorman@techsingularity.net> - 2016-10-27 11:20 +0200
                      Re: CONFIG_VMAP_STACK, on-stack struct, and wake_up_bit Mel Gorman <mgorman@techsingularity.net> - 2016-10-27 16:50 +0200
                      Re: CONFIG_VMAP_STACK, on-stack struct, and wake_up_bit Peter Zijlstra <peterz@infradead.org> - 2016-10-27 17:20 +0200
                    Re: CONFIG_VMAP_STACK, on-stack struct, and wake_up_bit Nicholas Piggin <npiggin@gmail.com> - 2016-10-27 16:10 +0200
                    Re: CONFIG_VMAP_STACK, on-stack struct, and wake_up_bit Peter Zijlstra <peterz@infradead.org> - 2016-10-27 16:50 +0200

Page 1 of 2  [1] 2  Next page →

#1509293 — CONFIG_VMAP_STACK, on-stack struct, and wake_up_bit

Hi,

CONFIG_VMAP_STACK has broken gfs2 and I'm trying to figure out what's
going on. What I'm seeing is the following: on a fresh gfs2 filesystem
created with:

  mkfs.gfs2 -p lock_nolock $DEVICE

I get the following BUG with 4.9-rc2, CONFIG_VMAP_STACK and
CONFIG_DEBUG_VIRTUAL turned on:

  kernel BUG at arch/x86/mm/physaddr.c:26!

Stack of kernel thread:

  __phys_addr(x)
  bit_waitqueue(word, bit)
  wake_up_bit(word = &gh->gh_iflags, bit = HIF_WAIT)
  gfs2_holder_wake(gh)

The gh here is on the stack of another kernel thread:

  static int fill_super(struct super_block *sb, struct gfs2_args
*args, int silent)
  {
    struct gfs2_holder mount_gh;
  }

Which is waiting on the bit with:

  wait_on_bit(&gh->gh_iflags, HIF_WAIT, TASK_UNINTERRUPTIBLE)

Is accessing a struct on another kernel thread's stack no longer working?

Thanks,
Andreas

[toc] | [next] | [standalone]

#1509556

On Wed, Oct 26, 2016 at 5:51 AM, Andreas Gruenbacher
<agruenba@redhat.com> wrote:
> Hi,
>
> CONFIG_VMAP_STACK has broken gfs2 and I'm trying to figure out what's
> going on. What I'm seeing is the following: on a fresh gfs2 filesystem
> created with:
>
>   mkfs.gfs2 -p lock_nolock $DEVICE
>
> I get the following BUG with 4.9-rc2, CONFIG_VMAP_STACK and
> CONFIG_DEBUG_VIRTUAL turned on:
>
>   kernel BUG at arch/x86/mm/physaddr.c:26!
>
> Stack of kernel thread:
>
>   __phys_addr(x)
>   bit_waitqueue(word, bit)
>   wake_up_bit(word = &gh->gh_iflags, bit = HIF_WAIT)
>   gfs2_holder_wake(gh)

It's this:

const struct zone *zone = page_zone(virt_to_page(word));

If the stack is vmalloced, then you can't find the page's zone like
that.  We could look it up the slow way (ick!), but maybe another
solution would be to do:

wait_queue_head_t *wait_table;
if (virt_addr_valid(word))
  wait_table = page_zone(virt_to_page(word))->wait_table;
else
  wait_table = funny_wait_table;

where funny_wait_table is an extra wait table just for funny addresses.

This will scale poorly on very large NUMA systems where many zones are
simultaneously using on-stack wait_bit bits, but I suspect this is a
very rare use case.

>
> Is accessing a struct on another kernel thread's stack no longer working?

That part should be fine.

[toc] | [prev] | [next] | [standalone]

#1509584

On Wed, Oct 26, 2016 at 8:51 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>>
>> I get the following BUG with 4.9-rc2, CONFIG_VMAP_STACK and
>> CONFIG_DEBUG_VIRTUAL turned on:
>>
>>   kernel BUG at arch/x86/mm/physaddr.c:26!
>
> const struct zone *zone = page_zone(virt_to_page(word));
>
> If the stack is vmalloced, then you can't find the page's zone like
> that.  We could look it up the slow way (ick!), but maybe another
> solution would be to do:

Christ. It's that damn bit-wait craziness again with the idiotic zone lookup.

I complained about it a couple of weeks ago for entirely unrelated
reasons: it absolutely sucks donkey ass through a straw from a cache
standpoint too. It makes the page_waitqueue() thing very expensive, to
the point where it shows up as taking up 3% of CPU time on a real
load.,

PeterZ had a patch that fixed most of the performance trouble because
the page_waitqueue is actually never realistically contested, and by
making the bit-waiting use *two* bits you can avoid the slow-path cost
entirely.

But here we have a totally different issue, namely that we want to
wait on a virtual address.

Quite frankly, I think the solution is to just rip out all the insane
zone crap. The most important use (by far) for the bit-waitqueue is
for the page locking, and with the "use a second bit to show
contention", there is absolutely no reason to try to do some crazy
per-zone thing. It's a slow-path that never matters, and rather than
make things scale well, the only thing it does is to pretty much
guarantee at least one extra cache miss.

Adding MelG and the mm list to the cc (PeterZ was already there) here
just for the heads up.

                   Linus

[toc] | [prev] | [next] | [standalone]

#1509613

[Multipart message — attachments visible in raw view] — view raw

On Wed, Oct 26, 2016 at 9:32 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Quite frankly, I think the solution is to just rip out all the insane
> zone crap.

IOW, something like the attached.

Advantage:

 - just look at the number of garbage lines removed!  21
insertions(+), 182 deletions(-)

 - it will actually speed up even the current case for all common
situations: no idiotic extra indirections that will take extra cache
misses

 - because the bit_wait_table array is now denser (256 entries is
about 6kB of data on 64-bit with no spinlock debugging, so ~100
cachelines), maybe it gets fewer cache misses too

 - we know how to handle the page_waitqueue contention issue, and it
has nothing to do with the stupid NUMA zones

The only case you actually get real page wait activity is IO, and I
suspect that hashing it out over ~100 cachelines will be more than
sufficient to avoid excessive contention, plus it's a cache-miss vs an
IO, so nobody sane cares.

The only reason it did that insane per-zone thing in the first place
that right now we access those wait-queues even when we damn well
shouldn't, and we have the solution for that.

Guys, holler if you hate this, but I think it's realistically the only
sane solution to the "wait queue on stack" issue.

Oh, and the patch is obviously entirely untested. I wouldn't want to
ruin my reputation by *testing* the patches I send out. What would be
the fun in that?

             Linus

[toc] | [prev] | [next] | [standalone]

#1509662

On Wed, Oct 26, 2016 at 10:15 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Oh, and the patch is obviously entirely untested. I wouldn't want to
> ruin my reputation by *testing* the patches I send out. What would be
> the fun in that?

So I tested it. It compiles, and it actually also solves the
performance problem I was complaining about a couple of weeks ago with
"unlock_page()" having an insane 3% CPU overhead when doing lots of
small script ("make -j16 test" in the git tree for those that weren't
involved in the original thread three weeks ago).

So quite frankly, I'll just commit it. It should fix the new problem
with gfs2 and CONFIG_VMAP_STACK, and I see no excuse for the crazy
zone stuff considering how harmful it is to everybody else.

I expect that when the NUMA people complain about page locking (if
they ever even notice), PeterZ will stand up like the hero he is, and
say "look here, I can solve this for you".

                    Linus

[toc] | [prev] | [next] | [standalone]

#1509672

----- Original Message -----
| On Wed, Oct 26, 2016 at 10:15 AM, Linus Torvalds
| <torvalds@linux-foundation.org> wrote:
| >
| > Oh, and the patch is obviously entirely untested. I wouldn't want to
| > ruin my reputation by *testing* the patches I send out. What would be
| > the fun in that?
| 
| So I tested it. It compiles, and it actually also solves the
| performance problem I was complaining about a couple of weeks ago with
| "unlock_page()" having an insane 3% CPU overhead when doing lots of
| small script ("make -j16 test" in the git tree for those that weren't
| involved in the original thread three weeks ago).
| 
| So quite frankly, I'll just commit it. It should fix the new problem
| with gfs2 and CONFIG_VMAP_STACK, and I see no excuse for the crazy
| zone stuff considering how harmful it is to everybody else.
| 
| I expect that when the NUMA people complain about page locking (if
| they ever even notice), PeterZ will stand up like the hero he is, and
| say "look here, I can solve this for you".
| 
|                     Linus
| 
I can test it for you, if you give me about an hour.

Bob Peterson

[toc] | [prev] | [next] | [standalone]

#1509678

On Wed, Oct 26, 2016 at 11:04 AM, Bob Peterson <rpeterso@redhat.com> wrote:
>
> I can test it for you, if you give me about an hour.

I can definitely wait an hour, it would be lovely to see more testing.
Especially if you have a NUMA machine and an interesting workload.

And if you actually have that NUMA machine and a load that shows the
page_waietutu effects, it would also be lovely if you can then
_additionally_ test the patch that PeterZ wrote a few weeks ago, it
was on the mm list about a month ago:

  Date: Thu, 29 Sep 2016 15:08:27 +0200
  From: Peter Zijlstra <peterz@infradead.org>
  Subject: Re: page_waitqueue() considered harmful
  Message-ID: <20160929130827.GX5016@twins.programming.kicks-ass.net>

and if you don't find it I can forward it to you (Peter had a few
versions, that latest one is the one that looked best).

                Linus

[toc] | [prev] | [next] | [standalone]

#1509712

----- Original Message -----
| On Wed, Oct 26, 2016 at 11:04 AM, Bob Peterson <rpeterso@redhat.com> wrote:
| >
| > I can test it for you, if you give me about an hour.

Sorry. I guess I underestimated the time it takes to build a kernel
on my test box. It will take a little longer, but it's compiling now.
 
| I can definitely wait an hour, it would be lovely to see more testing.
| Especially if you have a NUMA machine and an interesting workload.

I'll see what I can cook up.
 
| And if you actually have that NUMA machine and a load that shows the
| page_waietutu effects, it would also be lovely if you can then
| _additionally_ test the patch that PeterZ wrote a few weeks ago, it
| was on the mm list about a month ago:
| 
|   Date: Thu, 29 Sep 2016 15:08:27 +0200
|   From: Peter Zijlstra <peterz@infradead.org>
|   Subject: Re: page_waitqueue() considered harmful
|   Message-ID: <20160929130827.GX5016@twins.programming.kicks-ass.net>
| 
| and if you don't find it I can forward it to you (Peter had a few
| versions, that latest one is the one that looked best).

I'll see what I can do, but first I'll check basic functionality
and report back.
 
Bob Peterson

[toc] | [prev] | [next] | [standalone]

#1509822

----- Original Message -----
| On Wed, Oct 26, 2016 at 11:04 AM, Bob Peterson <rpeterso@redhat.com> wrote:
| >
| > I can test it for you, if you give me about an hour.
| 
| I can definitely wait an hour, it would be lovely to see more testing.
| Especially if you have a NUMA machine and an interesting workload.
| 
| And if you actually have that NUMA machine and a load that shows the
| page_waietutu effects, it would also be lovely if you can then
| _additionally_ test the patch that PeterZ wrote a few weeks ago, it
| was on the mm list about a month ago:
| 
|   Date: Thu, 29 Sep 2016 15:08:27 +0200
|   From: Peter Zijlstra <peterz@infradead.org>
|   Subject: Re: page_waitqueue() considered harmful
|   Message-ID: <20160929130827.GX5016@twins.programming.kicks-ass.net>
| 
| and if you don't find it I can forward it to you (Peter had a few
| versions, that latest one is the one that looked best).
| 
|                 Linus
| 

Hm. It didn't even boot, at least on my amd box in the lab.
I've made no attempt to debug this.

[    2.368403] NetLabel:  unlabeled traffic allowed by default
[    2.374271] ------------[ cut here ]------------
[    2.378877] kernel BUG at arch/x86/mm/physaddr.c:26!
[    2.383829] invalid opcode: 0000 [#1] SMP
[    2.387826] Modules linked in:
[    2.390882] CPU: 11 PID: 1 Comm: swapper/0 Not tainted 4.9.0-rc2+ #1
[    2.397219] Hardware name: Dell Inc. PowerEdge R815/06JC9T, BIOS 1.2.1 08/02/2010
[    2.404683] task: ffff947136548000 task.stack: ffffb36043130000
[    2.410588] RIP: 0010:[<ffffffffbe06848c>]  [<ffffffffbe06848c>] __phys_addr+0x3c/0x50
[    2.418500] RSP: 0018:ffffb36043133e10  EFLAGS: 00010287
[    2.423798] RAX: fffff39132a822fc RBX: 0000000000000000 RCX: 0000000000000000
[    2.430915] RDX: ffffffff00000001 RSI: 0000000000000000 RDI: ffff8800b2a822fc
[    2.438032] RBP: ffffb36043133e10 R08: ffff9475364026f8 R09: 0000000000000000
[    2.445151] R10: 0000000000000004 R11: 0000000000000000 R12: ffffffffbef8ce3d
[    2.452269] R13: ffffffffbf0e0428 R14: ffffffffbef7a8cd R15: 0000000000000000
[    2.459387] FS:  0000000000000000(0000) GS:ffff94773fa40000(0000) knlGS:0000000000000000
[    2.467458] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    2.473188] CR2: 0000000000000000 CR3: 000000057de07000 CR4: 00000000000006e0
[    2.480306] Stack:
[    2.482312]  ffffb36043133e38 ffffffffbef8d543 ffffb36043133e38 ffffffffbe34689b
[    2.489729]  000000007f4214f8 ffffb36043133e48 ffffffffbef8ce79 ffffb36043133ec0
[    2.497144]  ffffffffbe002190 0000000000000000 0000000000000000 ffffffffbed42f50
[    2.504561] Call Trace:
[    2.507005]  [<ffffffffbef8d543>] save_microcode_in_initrd_amd+0x31/0x106
[    2.513778]  [<ffffffffbe34689b>] ? debugfs_create_u64+0x2b/0x30
[    2.519769]  [<ffffffffbef8ce79>] save_microcode_in_initrd+0x3c/0x45
[    2.526110]  [<ffffffffbe002190>] do_one_initcall+0x50/0x180
[    2.531756]  [<ffffffffbef7a8cd>] ? set_debug_rodata+0x12/0x12
[    2.537573]  [<ffffffffbef7b17b>] kernel_init_freeable+0x194/0x230
[    2.543740]  [<ffffffffbe7f3430>] ? rest_init+0x80/0x80
[    2.548952]  [<ffffffffbe7f343e>] kernel_init+0xe/0x100
[    2.554164]  [<ffffffffbe800c55>] ret_from_fork+0x25/0x30
[    2.559548] Code: 48 89 f8 72 28 48 2b 05 7b a0 dc 00 48 05 00 00 00 80 48 39 c7 72 14 0f b6 0d 6a 75 ee 00 48 89 c2 48 d3 ea 48 85 d2 75 02 5d c3 <0f> 0b 48 03 05 7b 5b da 00 48 81 ff ff ff ff 3f 76 ec 0f 0b 0f 
[    2.579022] RIP  [<ffffffffbe06848c>] __phys_addr+0x3c/0x50
[    2.584590]  RSP <ffffb36043133e10>
[    2.588117] ---[ end trace 5c9b40c31651bd33 ]---
[    2.592745] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
[    2.592745] 
[    2.601900] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b

Regards,

Bob Peterson

[toc] | [prev] | [next] | [standalone]

#1509865

On Wed, Oct 26, 2016 at 2:01 PM, Bob Peterson <rpeterso@redhat.com> wrote:
>
> Hm. It didn't even boot, at least on my amd box in the lab.
> I've made no attempt to debug this.

Hmm. Looks like a completely independent issue from the patch. Did you
try booting that machine without the patch?

> [    2.378877] kernel BUG at arch/x86/mm/physaddr.c:26!

Ok, similar issue, I think - passing a non-1:1 address to __phys_addr().

But the call trace has nothing to do with gfs2 or the bitlocks:

> [    2.504561] Call Trace:
> [    2.507005]   save_microcode_in_initrd_amd+0x31/0x106
> [    2.513778]   save_microcode_in_initrd+0x3c/0x45
> [    2.526110]   do_one_initcall+0x50/0x180
> [    2.531756]   ? set_debug_rodata+0x12/0x12
> [    2.537573]   kernel_init_freeable+0x194/0x230
> [    2.543740]   ? rest_init+0x80/0x80
> [    2.548952]   kernel_init+0xe/0x100
> [    2.554164]   ret_from_fork+0x25/0x30

I think this might be the

        cont    = __pa(container);

line in save_microcode_in_initrd_amd().

I see that Borislav is busy with some x86/microcode patches, I suspect
he already hit this. Adding Borislav to the cc.

Can you re-try without the AMD microcode driver for now? This seems to
be a separate issue from the gfs2 one.

               Linus

[toc] | [prev] | [next] | [standalone]

#1509913

On Wed, Oct 26, 2016 at 02:30:49PM -0700, Linus Torvalds wrote:
> Ok, similar issue, I think - passing a non-1:1 address to __phys_addr().
> 
> But the call trace has nothing to do with gfs2 or the bitlocks:
> 
> > [    2.504561] Call Trace:
> > [    2.507005]   save_microcode_in_initrd_amd+0x31/0x106
> > [    2.513778]   save_microcode_in_initrd+0x3c/0x45
> > [    2.526110]   do_one_initcall+0x50/0x180
> > [    2.531756]   ? set_debug_rodata+0x12/0x12
> > [    2.537573]   kernel_init_freeable+0x194/0x230
> > [    2.543740]   ? rest_init+0x80/0x80
> > [    2.548952]   kernel_init+0xe/0x100
> > [    2.554164]   ret_from_fork+0x25/0x30
> 
> I think this might be the
> 
>         cont    = __pa(container);
> 
> line in save_microcode_in_initrd_amd().
> 
> I see that Borislav is busy with some x86/microcode patches, I suspect
> he already hit this. Adding Borislav to the cc.

Hmm, I guess that fires because that container thing is a static pointer
so it is >= PAGE_OFFSET. But I might be wrong, it is too late here for
brain to work.

In any case, looking at his Code:

   0:   48 89 f8                mov    %rdi,%rax
   3:   72 28                   jb     0x2d
   5:   48 2b 05 7b a0 dc 00    sub    0xdca07b(%rip),%rax        # 0xdca087
   c:   48 05 00 00 00 80       add    $0xffffffff80000000,%rax
  12:   48 39 c7                cmp    %rax,%rdi
  				^^^^^^^^^^^^^^^^

it could be this comparison here:

RAX: fffff39132a822fc, RDI: ffff8800b2a822fc

  15:   72 14                   jb     0x2b

... which sends us to the UD2.

  17:   0f b6 0d 6a 75 ee 00    movzbl 0xee756a(%rip),%ecx        # 0xee7588

We might end up at 0x2b from here too - that's !phys_addr_valid(x) - but
ECX is 0 while it should be 36...

  1e:   48 89 c2                mov    %rax,%rdx
  21:   48 d3 ea                shr    %cl,%rdx
  24:   48 85 d2                test   %rdx,%rdx
  27:   75 02                   jne    0x2b
  29:   5d                      pop    %rbp
  2a:   c3                      retq   
  2b:*  0f 0b                   ud2             <-- trapping instruction
  2d:   48 03 05 7b 5b da 00    add    0xda5b7b(%rip),%rax        # 0xda5baf
  34:   48 81 ff ff ff ff 3f    cmp    $0x3fffffff,%rdi
  3b:   76 ec                   jbe    0x29
  3d:   0f 0b                   ud2    
  3f:   0f                      .byte 0xf

But again, I could be already sleeping and this could be me talking in
my sleep so don't take it too seriously.

In any case, this code was flaky and fragile for many reasons and it is
why this whole wankery is gone in the microcode loader now.

> Can you re-try without the AMD microcode driver for now?

Yeah, just boot with "dis_ucode_ldr".

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

[toc] | [prev] | [next] | [standalone]

#1509931

On Wed, Oct 26, 2016 at 05:01:24PM -0400, Bob Peterson wrote:
> Hm. It didn't even boot, at least on my amd box in the lab.
> I've made no attempt to debug this.

Btw, can you send me your .config so that I can try to reproduce?

I'm assuming you're booting latest Linus' tree on it?

I'd need to take care of this for 4.9.

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [prev] | [next] | [standalone]

#1510165

On Wed, Oct 26, 2016 at 08:37:25PM -0400, Bob Peterson wrote:
> Attached, but as Linus suggested, I turned off the AMD microcode driver,
> so it should be the same if you turn it back on. If you want, I can
> do it and re-send so you have a more pristine .config. Let me know.

Thanks, but I was able to reproduce in a VM.

Here's a fix which works here - I'd appreciate it if you ran it and
checked the microcode was applied correctly, i.e.:

$ dmesg | grep -i microcode

before and after the patch. Please paste that output in a mail too.

Thanks!

---
From: Borislav Petkov <bp@suse.de>
Date: Thu, 27 Oct 2016 14:03:59 +0200
Subject: [PATCH] x86/microcode/AMD: Fix more fallout from CONFIG_RANDOMIZE_MEMORY

We needed the physical address of the container in order to compute the
offset within the relocated ramdisk. And we did this by doing __pa() on
the virtual address.

However, __pa() does checks whether the physical address is within
PAGE_OFFSET and __START_KERNEL_map - see __phys_addr() - which fail
if we have CONFIG_RANDOMIZE_MEMORY enabled: we feed a virtual address
which *doesn't* have the randomization offset into a function which uses
PAGE_OFFSET which *does* have that offset.

This makes this check fire:

	VIRTUAL_BUG_ON((x > y) || !phys_addr_valid(x));
			^^^^^^

due to the randomization offset.

The fix is as simple as using __pa_nodebug() because we do that
randomization offset accounting later in that function ourselves.

Reported-by: Bob Peterson <rpeterso@redhat.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: stable@vger.kernel.org # 4.9
---
 arch/x86/kernel/cpu/microcode/amd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 620ab06bcf45..017bda12caae 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -429,7 +429,7 @@ int __init save_microcode_in_initrd_amd(void)
 	 * We need the physical address of the container for both bitness since
 	 * boot_params.hdr.ramdisk_image is a physical address.
 	 */
-	cont    = __pa(container);
+	cont    = __pa_nodebug(container);
 	cont_va = container;
 #endif
 
-- 
2.10.0

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [prev] | [next] | [standalone]

#1510583

| Here's a fix which works here - I'd appreciate it if you ran it and
| checked the microcode was applied correctly, i.e.:
| 
| $ dmesg | grep -i microcode
| 
| before and after the patch. Please paste that output in a mail too.

Hi Borislav,

Sorry it's taken me so long. I've been having issues.
I couldn't recreate that first boot failure, even using .config.old,
and even after removing (rm -fR) my linux.git and untarring it from the
original tarball, doing a make clean, etc.
The output before and after your new patch are the same (except for the times):

# dmesg | grep -i microcode
[    5.291679] microcode: microcode updated early to new patch_level=0x010000d9
[    5.298761] microcode: CPU0: patch_level=0x010000d9
[    5.303648] microcode: CPU1: patch_level=0x010000d9
[    5.308529] microcode: CPU2: patch_level=0x010000d9
[    5.313414] microcode: CPU3: patch_level=0x010000d9
[    5.360834] microcode: CPU4: patch_level=0x010000d9
[    5.365719] microcode: CPU5: patch_level=0x010000d9
[    5.370602] microcode: CPU6: patch_level=0x010000d9
[    5.375486] microcode: CPU7: patch_level=0x010000d9
[    5.380372] microcode: CPU8: patch_level=0x010000d9
[    5.385256] microcode: CPU9: patch_level=0x010000d9
[    5.390142] microcode: CPU10: patch_level=0x010000d9
[    5.395102] microcode: CPU11: patch_level=0x010000d9
[    5.437813] microcode: CPU12: patch_level=0x010000d9
[    5.442785] microcode: CPU13: patch_level=0x010000d9
[    5.447755] microcode: CPU14: patch_level=0x010000d9
[    5.452724] microcode: CPU15: patch_level=0x010000d9
[    5.457756] microcode: Microcode Update Driver: v2.01 <tigran@aivazian.fsnet.co.uk>, Peter Oruba
# uname -a
Linux intec2 4.9.0-rc2+ #2 SMP Thu Oct 27 14:29:32 EDT 2016 x86_64 x86_64 x86_64 GNU/Linux

Regards,

Bob Peterson

[toc] | [prev] | [next] | [standalone]

#1510593

On Thu, Oct 27, 2016 at 02:51:30PM -0400, Bob Peterson wrote:
> I couldn't recreate that first boot failure, even using .config.old,
> and even after removing (rm -fR) my linux.git and untarring it from the
> original tarball, doing a make clean, etc.

Hmm, so it could also depend on the randomized offset as it is getting
generated anew each boot. So you could try to boot a couple of times
to see if the randomized offset is generated just right for the bug
condition to match.

I mean, it would be great if you try a couple times but even if you're
unsuccessful, that's fine too - the fix is obviously correct and I've
confirmed that it boots fine in my VM here.

> The output before and after your new patch are the same (except for the times):
> 
> # dmesg | grep -i microcode
> [    5.291679] microcode: microcode updated early to new patch_level=0x010000d9

That looks good.

Thanks!

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [prev] | [next] | [standalone]

#1510671

----- Original Message -----
| I mean, it would be great if you try a couple times but even if you're
| unsuccessful, that's fine too - the fix is obviously correct and I've
| confirmed that it boots fine in my VM here.

Hi Boris,

I rebooted the machine with and without your patch, about 15 times each,
and no failures. Not sure why I got it the first time. Must have been a one-off.

Bob Peterson

[toc] | [prev] | [next] | [standalone]

#1510676

On Thu, Oct 27, 2016 at 05:03:13PM -0400, Bob Peterson wrote:
> I rebooted the machine with and without your patch, about 15 times
> each, and no failures. Not sure why I got it the first time. Must have
> been a one-off.

Ok, thanks for giving it a try!

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

[toc] | [prev] | [next] | [standalone]

#1510960 — [tip:x86/urgent] x86/microcode/AMD: Fix more fallout from CONFIG_RANDOMIZE_MEMORY=y

Commit-ID:  1c27f646b18fb56308dff82784ca61951bad0b48
Gitweb:     http://git.kernel.org/tip/1c27f646b18fb56308dff82784ca61951bad0b48
Author:     Borislav Petkov <bp@suse.de>
AuthorDate: Thu, 27 Oct 2016 14:36:23 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 28 Oct 2016 10:29:59 +0200

x86/microcode/AMD: Fix more fallout from CONFIG_RANDOMIZE_MEMORY=y

We needed the physical address of the container in order to compute the
offset within the relocated ramdisk. And we did this by doing __pa() on
the virtual address.

However, __pa() does checks whether the physical address is within
PAGE_OFFSET and __START_KERNEL_map - see __phys_addr() - which fail
if we have CONFIG_RANDOMIZE_MEMORY enabled: we feed a virtual address
which *doesn't* have the randomization offset into a function which uses
PAGE_OFFSET which *does* have that offset.

This makes this check fire:

	VIRTUAL_BUG_ON((x > y) || !phys_addr_valid(x));
			^^^^^^

due to the randomization offset.

The fix is as simple as using __pa_nodebug() because we do that
randomization offset accounting later in that function ourselves.

Reported-by: Bob Peterson <rpeterso@redhat.com>
Tested-by: Bob Peterson <rpeterso@redhat.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: Andreas Gruenbacher <agruenba@redhat.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Whitehouse <swhiteho@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-mm <linux-mm@kvack.org>
Cc: stable@vger.kernel.org # 4.9
Link: http://lkml.kernel.org/r/20161027123623.j2jri5bandimboff@pd.tnic
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/cpu/microcode/amd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 620ab06..017bda1 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -429,7 +429,7 @@ int __init save_microcode_in_initrd_amd(void)
 	 * We need the physical address of the container for both bitness since
 	 * boot_params.hdr.ramdisk_image is a physical address.
 	 */
-	cont    = __pa(container);
+	cont    = __pa_nodebug(container);
 	cont_va = container;
 #endif
 

[toc] | [prev] | [next] | [standalone]

#1509796

On Wed, Oct 26, 2016 at 10:15:30AM -0700, Linus Torvalds wrote:
> On Wed, Oct 26, 2016 at 9:32 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > Quite frankly, I think the solution is to just rip out all the insane
> > zone crap.
> 
> IOW, something like the attached.
> 
> Advantage:
> 
>  - just look at the number of garbage lines removed!  21
> insertions(+), 182 deletions(-)
> 
>  - it will actually speed up even the current case for all common
> situations: no idiotic extra indirections that will take extra cache
> misses
> 
>  - because the bit_wait_table array is now denser (256 entries is
> about 6kB of data on 64-bit with no spinlock debugging, so ~100
> cachelines), maybe it gets fewer cache misses too
> 
>  - we know how to handle the page_waitqueue contention issue, and it
> has nothing to do with the stupid NUMA zones
> 
> The only case you actually get real page wait activity is IO, and I
> suspect that hashing it out over ~100 cachelines will be more than
> sufficient to avoid excessive contention, plus it's a cache-miss vs an
> IO, so nobody sane cares.
> 

IO wait activity is not all that matters. We hit the lock/unlock paths
during a lot of operations like reclaim.

False sharing is possible with either the new or old scheme so it's
irrelevant. There will be some remote NUMA cache misses which may be made
worse by false sharing. In the reclaim case, the bulk of those are hit
by kswapd. Kswapd itself doesn't care but there may be increased NUMA
traffic. By the time you hit direct reclaim, a remote cache miss is not
going to be the end of the world.

> Guys, holler if you hate this, but I think it's realistically the only
> sane solution to the "wait queue on stack" issue.
> 

Hate? No.

It's not clear cut that NUMA remote accesses will be a problem. A remote
cache miss may or may not be more expensive than multiple calculations,
virt->page calculations and chasing pointers to lookup the zone and the
table. It's multiple potential local misses versus one remote.

Even if NUMA conflicts are a problem then 256 entries gives 96 cache
lines. For pages only, the hash routine could partition table space into
max(96, nr_online_nodes) partitions. It wouldn't be perfect as wait_table_t
does not align well with cache line sizes so there would be collisions
on the boundary but it'd be close enough. It would require page_waitqueue
use a different hashing function so it's not simple but it's possible if
someone is sufficiently motivated and found a workload that matters.

There is some question whether the sizing will lead to more collisions and
spurious wakeups. There is no way to predict how much of an issue that is
but I suspect a lot of those happen during reclaim anyway. If collisions
are a problem then the table could be dynamically sized in the similar
way the inode and dcache hash tables are.

I didn't test this as I don't have a machine available right now.  The bulk
of what you removed was related to hotplug but the result looks hotplug safe.
So I've only Two minor nits only and a general caution to watch for increased
collisions and spurious wakeups with a minor caution about remote access
penalties.

> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index 7f2ae99e5daf..0f088f3a2fed 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -440,33 +440,7 @@ struct zone {
>  	seqlock_t		span_seqlock;
>  #endif
>  
> -	/*
> -	 * wait_table		-- the array holding the hash table
> -	 * wait_table_hash_nr_entries	-- the size of the hash table array
> -	 * wait_table_bits	-- wait_table_size == (1 << wait_table_bits)
> -	 *
> -	 * The purpose of all these is to keep track of the people
> -	 * waiting for a page to become available and make them
> -	 * runnable again when possible. The trouble is that this
> -	 * consumes a lot of space, especially when so few things
> -	 * wait on pages at a given time. So instead of using
> -	 * per-page waitqueues, we use a waitqueue hash table.
> -	 *
> -	 * The bucket discipline is to sleep on the same queue when
> -	 * colliding and wake all in that wait queue when removing.
> -	 * When something wakes, it must check to be sure its page is
> -	 * truly available, a la thundering herd. The cost of a
> -	 * collision is great, but given the expected load of the
> -	 * table, they should be so rare as to be outweighed by the
> -	 * benefits from the saved space.
> -	 *
> -	 * __wait_on_page_locked() and unlock_page() in mm/filemap.c, are the
> -	 * primary users of these fields, and in mm/page_alloc.c
> -	 * free_area_init_core() performs the initialization of them.
> -	 */
> -	wait_queue_head_t	*wait_table;
> -	unsigned long		wait_table_hash_nr_entries;
> -	unsigned long		wait_table_bits;
> +	int initialized;
>  
>  	/* Write-intensive fields used from the page allocator */
>  	ZONE_PADDING(_pad1_)

zone_is_initialized is mostly the domain of hotplug. A potential cleanup
is to use a page flag and shrink the size of zone slightly. Nothing to
panic over.

> @@ -546,7 +520,7 @@ static inline bool zone_spans_pfn(const struct zone *zone, unsigned long pfn)
>  
>  static inline bool zone_is_initialized(struct zone *zone)
>  {
> -	return !!zone->wait_table;
> +	return zone->initialized;
>  }
>  
>  static inline bool zone_is_empty(struct zone *zone)
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 94732d1ab00a..42d4027f9e26 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -7515,11 +7515,27 @@ static struct kmem_cache *task_group_cache __read_mostly;
>  DECLARE_PER_CPU(cpumask_var_t, load_balance_mask);
>  DECLARE_PER_CPU(cpumask_var_t, select_idle_mask);
>  
> +#define WAIT_TABLE_BITS 8
> +#define WAIT_TABLE_SIZE (1 << WAIT_TABLE_BITS)
> +static wait_queue_head_t bit_wait_table[WAIT_TABLE_SIZE] __cacheline_aligned;
> +
> +wait_queue_head_t *bit_waitqueue(void *word, int bit)
> +{
> +	const int shift = BITS_PER_LONG == 32 ? 5 : 6;
> +	unsigned long val = (unsigned long)word << shift | bit;
> +
> +	return bit_wait_table + hash_long(val, WAIT_TABLE_BITS);
> +}
> +EXPORT_SYMBOL(bit_waitqueue);
> +

Minor nit that it's unfortunate this moved to the scheduler core. It
wouldn't have been a complete disaster to add a page_waitqueue_init() or
something similar after sched_init.

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1509842

On Wed, Oct 26, 2016 at 1:31 PM, Mel Gorman <mgorman@techsingularity.net> wrote:
>
> IO wait activity is not all that matters. We hit the lock/unlock paths
> during a lot of operations like reclaim.

I doubt we do.

Yes, we hit the lock/unlock itself, but do we hit the *contention*?

The current code is nasty, and always ends up touching the wait-queue
regardless of whether it needs to or not, but we have a fix for that.

With that fixed, do we actually get contention on a per-page basis?
Because without contention, we'd never actually look up the wait-queue
at all.

I suspect that without IO, it's really really hard to actually get
that contention, because things like reclaim end up looking at the LRU
queue etc wioth their own locking, so it should look at various
individual pages one at a time, not have multiple queues look at the
same page.

>> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
>> index 7f2ae99e5daf..0f088f3a2fed 100644
>> --- a/include/linux/mmzone.h
>> +++ b/include/linux/mmzone.h
>> @@ -440,33 +440,7 @@ struct zone {
>> +     int initialized;
>>
>>       /* Write-intensive fields used from the page allocator */
>>       ZONE_PADDING(_pad1_)
>
> zone_is_initialized is mostly the domain of hotplug. A potential cleanup
> is to use a page flag and shrink the size of zone slightly. Nothing to
> panic over.

I really did that to make it very obvious that there was no semantic
change. I just set the "initialized" flag in the same place where it
used to initialize the wait_table, so that this:

>>  static inline bool zone_is_initialized(struct zone *zone)
>>  {
>> -     return !!zone->wait_table;
>> +     return zone->initialized;
>>  }

ends up being obviously equivalent.

Admittedly I didn't clear it when the code cleared the wait_table
pointer, because that _seemed_ a non-issue - the zone will remain
initialized until it can't be reached any more, so there didn't seem
to be an actual problem there.

>> +#define WAIT_TABLE_BITS 8
>> +#define WAIT_TABLE_SIZE (1 << WAIT_TABLE_BITS)
>> +static wait_queue_head_t bit_wait_table[WAIT_TABLE_SIZE] __cacheline_aligned;
>> +
>> +wait_queue_head_t *bit_waitqueue(void *word, int bit)
>> +{
>> +     const int shift = BITS_PER_LONG == 32 ? 5 : 6;
>> +     unsigned long val = (unsigned long)word << shift | bit;
>> +
>> +     return bit_wait_table + hash_long(val, WAIT_TABLE_BITS);
>> +}
>> +EXPORT_SYMBOL(bit_waitqueue);
>> +
>
> Minor nit that it's unfortunate this moved to the scheduler core. It
> wouldn't have been a complete disaster to add a page_waitqueue_init() or
> something similar after sched_init.

I considered that, but decided that "minimal patch" was better. Plus,
with that bit_waitqueue() actually also being used for the page
locking queues (which act _kind of_ but not quite, like a bitlock),
the bit_wait_table is actually more core than just the bit-wait code.

In fact, I considered just renaming it to "hashed_wait_queue", because
that's effectively how we use it now, rather than being particularly
specific to the bit-waiting. But again, that would have made the patch
bigger, which I wanted to avoid since this is a post-rc2 thing due to
the gfs2 breakage.

            Linus

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web