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

Groups > linux.kernel > #1711598 > unrolled thread

[PATCH 1/2] sched/wait: Break up long wake list walk

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/2] sched/wait: Break up long wake list walk Tim Chen <tim.c.chen@linux.intel.com> - 2017-08-15 03:20 +0200
    Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-15 03:50 +0200
      Re: [PATCH 1/2] sched/wait: Break up long wake list walk Andi Kleen <ak@linux.intel.com> - 2017-08-15 04:30 +0200
        Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-15 05:00 +0200
          Re: [PATCH 1/2] sched/wait: Break up long wake list walk Andi Kleen <ak@linux.intel.com> - 2017-08-15 05:20 +0200
            Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-15 05:30 +0200
              Re: [PATCH 1/2] sched/wait: Break up long wake list walk Tim Chen <tim.c.chen@linux.intel.com> - 2017-08-15 21:10 +0200
                Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-15 21:50 +0200
                Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-15 21:50 +0200
              Re: [PATCH 1/2] sched/wait: Break up long wake list walk Davidlohr Bueso <dave@stgolabs.net> - 2017-08-16 00:50 +0200
                Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-16 01:00 +0200
                  Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-16 02:00 +0200
                    Re: [PATCH 1/2] sched/wait: Break up long wake list walk ebiederm@xmission.com (Eric W. Biederman) - 2017-08-17 01:30 +0200
                Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-16 01:00 +0200
      RE: [PATCH 1/2] sched/wait: Break up long wake list walk "Liang, Kan" <kan.liang@intel.com> - 2017-08-17 18:20 +0200
        Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-17 18:30 +0200
          RE: [PATCH 1/2] sched/wait: Break up long wake list walk "Liang, Kan" <kan.liang@intel.com> - 2017-08-17 22:20 +0200
            Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-17 22:50 +0200
              Re: [PATCH 1/2] sched/wait: Break up long wake list walk Mel Gorman <mgorman@techsingularity.net> - 2017-08-18 14:30 +0200
                RE: [PATCH 1/2] sched/wait: Break up long wake list walk "Liang, Kan" <kan.liang@intel.com> - 2017-08-18 16:30 +0200
                  Re: [PATCH 1/2] sched/wait: Break up long wake list walk Mel Gorman <mgorman@techsingularity.net> - 2017-08-18 16:50 +0200
                    Re: [PATCH 1/2] sched/wait: Break up long wake list walk Tim Chen <tim.c.chen@linux.intel.com> - 2017-08-18 18:40 +0200
                      Re: [PATCH 1/2] sched/wait: Break up long wake list walk Andi Kleen <ak@linux.intel.com> - 2017-08-18 18:50 +0200
                    RE: [PATCH 1/2] sched/wait: Break up long wake list walk "Liang, Kan" <kan.liang@intel.com> - 2017-08-18 19:00 +0200
                      Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-18 19:50 +0200
                        Re: [PATCH 1/2] sched/wait: Break up long wake list walk Mel Gorman <mgorman@techsingularity.net> - 2017-08-18 21:00 +0200
                          Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-18 21:20 +0200
                            Re: [PATCH 1/2] sched/wait: Break up long wake list walk Andi Kleen <ak@linux.intel.com> - 2017-08-18 22:00 +0200
                              Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-18 22:20 +0200
                            Re: [PATCH 1/2] sched/wait: Break up long wake list walk Mel Gorman <mgorman@techsingularity.net> - 2017-08-21 20:40 +0200
                              RE: [PATCH 1/2] sched/wait: Break up long wake list walk "Liang, Kan" <kan.liang@intel.com> - 2017-08-21 21:00 +0200
                                RE: [PATCH 1/2] sched/wait: Break up long wake list walk "Liang, Kan" <kan.liang@intel.com> - 2017-08-22 19:30 +0200
                                  Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-22 20:20 +0200
                                    Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-22 20:30 +0200
                                    Re: [PATCH 1/2] sched/wait: Break up long wake list walk Peter Zijlstra <peterz@infradead.org> - 2017-08-22 21:00 +0200
                                      Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-22 21:20 +0200
                                    Re: [PATCH 1/2] sched/wait: Break up long wake list walk Peter Zijlstra <peterz@infradead.org> - 2017-08-22 21:10 +0200
                                      Re: [PATCH 1/2] sched/wait: Break up long wake list walk Andi Kleen <ak@linux.intel.com> - 2017-08-22 21:40 +0200
                                        Re: [PATCH 1/2] sched/wait: Break up long wake list walk Christopher Lameter <cl@linux.com> - 2017-08-22 23:10 +0200
                                          Re: [PATCH 1/2] sched/wait: Break up long wake list walk Andi Kleen <ak@linux.intel.com> - 2017-08-22 23:30 +0200
                                            Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-23 01:00 +0200
                                              Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-23 01:20 +0200
                                              RE: [PATCH 1/2] sched/wait: Break up long wake list walk "Liang, Kan" <kan.liang@intel.com> - 2017-08-23 17:00 +0200
                                      Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-22 21:40 +0200
                                    RE: [PATCH 1/2] sched/wait: Break up long wake list walk "Liang, Kan" <kan.liang@intel.com> - 2017-08-22 22:00 +0200
                                      Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-22 22:50 +0200
                                        Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-22 23:00 +0200
                                        Re: [PATCH 1/2] sched/wait: Break up long wake list walk Peter Zijlstra <peterz@infradead.org> - 2017-08-22 23:00 +0200
                                        RE: [PATCH 1/2] sched/wait: Break up long wake list walk "Liang, Kan" <kan.liang@intel.com> - 2017-08-23 16:50 +0200
                                          Re: [PATCH 1/2] sched/wait: Break up long wake list walk Tim Chen <tim.c.chen@linux.intel.com> - 2017-08-23 18:00 +0200
                                            Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-23 20:20 +0200
                                              RE: [PATCH 1/2] sched/wait: Break up long wake list walk "Liang, Kan" <kan.liang@intel.com> - 2017-08-23 23:00 +0200
                                              Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 01:40 +0200
                                                Re: [PATCH 1/2] sched/wait: Break up long wake list walk Tim Chen <tim.c.chen@linux.intel.com> - 2017-08-24 19:50 +0200
                                                  Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 20:20 +0200
                                                    Re: [PATCH 1/2] sched/wait: Break up long wake list walk Mel Gorman <mgorman@techsingularity.net> - 2017-08-24 22:50 +0200
                                                      Re: [PATCH 1/2] sched/wait: Break up long wake list walk Tim Chen <tim.c.chen@linux.intel.com> - 2017-08-25 18:50 +0200
                                    Re: [PATCH 1/2] sched/wait: Break up long wake list walk Mel Gorman <mgorman@techsingularity.net> - 2017-08-23 18:10 +0200
                        Re: [PATCH 1/2] sched/wait: Break up long wake list walk Andi Kleen <ak@linux.intel.com> - 2017-08-18 22:10 +0200
                          Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-18 22:40 +0200
                        RE: [PATCH 1/2] sched/wait: Break up long wake list walk "Liang, Kan" <kan.liang@intel.com> - 2017-08-18 22:30 +0200
                          Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-18 22:40 +0200
                Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-18 19:00 +0200
              RE: [PATCH 1/2] sched/wait: Break up long wake list walk "Liang, Kan" <kan.liang@intel.com> - 2017-08-18 15:10 +0200

Page 3 of 4 — ← Prev page 1 2 [3] 4  Next page →

#1717881

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

On Tue, Aug 22, 2017 at 2:24 PM, Andi Kleen <ak@linux.intel.com> wrote:
>
> I believe in this case it's used by threads, so a reference count limit
> wouldn't help.

For the first migration try, yes. But if it's some kind of "try and
try again" pattern, the second time you try and there are people
waiting for the page, the page count (not the map count) would be
elevanted.

So it's possible that depending on exactly what the deeper problem is,
the "this page is very busy, don't migrate" case might be
discoverable, and the page count might be part of it.

However, after PeterZ made that comment that page migration should
have that should_numa_migrate_memory() filter, I am looking at that
mpol_misplaced() code.

And honestly, that MPOL_PREFERRED / MPOL_F_LOCAL case really looks
like complete garbage to me.

It looks like garbage exactly because it says "always migrate to the
current node", but that's crazy - if it's a group of threads all
running together on the same VM, that obviously will just bounce the
page around for absolute zero good ewason.

The *other* memory policies look fairly sane. They basically have a
fairly well-defined preferred node for the policy (although the
"MPOL_INTERLEAVE" looks wrong for a hugepage).  But
MPOL_PREFERRED/MPOL_F_LOCAL really looks completely broken.

Maybe people expected that anybody who uses MPOL_F_LOCAL will also
bind all threads to one single node?

Could we perhaps make that "MPOL_PREFERRED / MPOL_F_LOCAL" case just
do the MPOL_F_MORON policy, which *does* use that "should I migrate to
the local node" filter?

IOW, we've been looking at the waiters (because the problem shows up
due to the excessive wait queues), but maybe the source of the problem
comes from the numa balancing code just insanely bouncing pages
back-and-forth if you use that "always balance to local node" thing.

Untested (as always) patch attached.

              Linus

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

#1717888

On Tue, Aug 22, 2017 at 3:52 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> The *other* memory policies look fairly sane. They basically have a
> fairly well-defined preferred node for the policy (although the
> "MPOL_INTERLEAVE" looks wrong for a hugepage).  But
> MPOL_PREFERRED/MPOL_F_LOCAL really looks completely broken.

Of course, I don't know if that customer test-case actually triggers
that MPOL_PREFERRED/MPOL_F_LOCAL case at all.

So again, that issue may not even be what is going on.

               Linus

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

#1718437

> Subject: Re: [PATCH 1/2] sched/wait: Break up long wake list walk
> 
> On Tue, Aug 22, 2017 at 2:24 PM, Andi Kleen <ak@linux.intel.com> wrote:
> >
> > I believe in this case it's used by threads, so a reference count
> > limit wouldn't help.
> 
> For the first migration try, yes. But if it's some kind of "try and try again"
> pattern, the second time you try and there are people waiting for the page,
> the page count (not the map count) would be elevanted.
> 
> So it's possible that depending on exactly what the deeper problem is, the
> "this page is very busy, don't migrate" case might be discoverable, and the
> page count might be part of it.
> 
> However, after PeterZ made that comment that page migration should have
> that should_numa_migrate_memory() filter, I am looking at that
> mpol_misplaced() code.
> 
> And honestly, that MPOL_PREFERRED / MPOL_F_LOCAL case really looks like
> complete garbage to me.
> 
> It looks like garbage exactly because it says "always migrate to the current
> node", but that's crazy - if it's a group of threads all running together on the
> same VM, that obviously will just bounce the page around for absolute zero
> good ewason.
> 
> The *other* memory policies look fairly sane. They basically have a fairly
> well-defined preferred node for the policy (although the
> "MPOL_INTERLEAVE" looks wrong for a hugepage).  But
> MPOL_PREFERRED/MPOL_F_LOCAL really looks completely broken.
> 
> Maybe people expected that anybody who uses MPOL_F_LOCAL will also
> bind all threads to one single node?
> 
> Could we perhaps make that "MPOL_PREFERRED / MPOL_F_LOCAL" case just
> do the MPOL_F_MORON policy, which *does* use that "should I migrate to
> the local node" filter?
> 
> IOW, we've been looking at the waiters (because the problem shows up due
> to the excessive wait queues), but maybe the source of the problem comes
> from the numa balancing code just insanely bouncing pages back-and-forth if
> you use that "always balance to local node" thing.
> 
> Untested (as always) patch attached.

The patch doesn’t work.

Thanks,
Kan

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

#1717771

On Tue, Aug 22, 2017 at 12:08 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>
> So that migration stuff has a filter on, we need two consecutive numa
> faults from the same page_cpupid 'hash', see
> should_numa_migrate_memory().

Hmm. That is only called for MPOL_F_MORON.

We don't actually know what policy the problem space uses, since tthis
is some specialized load.

I could easily see somebody having set MPOL_PREFERRED with
MPOL_F_LOCAL and then touch it from every single node. Isn't that even
the default?

> And since this appears to be anonymous memory (no THP) this is all a
> single address space. However, we don't appear to invalidate TLBs when
> we upgrade the PTE protection bits (not strictly required of course), so
> we can have multiple CPUs trip over the same 'old' NUMA PTE.
>
> Still, generating such a migration storm would be fairly tricky I think.

Well, Mel seems to have been unable to generate a load that reproduces
the long page waitqueues. And I don't think we've had any other
reports of this either.

So "quite tricky" may well be exactly what it needs.

Likely also with a user load that does something that the people
involved in the automatic numa migration would have considered
completely insane and never tested or even thought about.

Users sometimes do completely insane things. It may have started as a
workaround for some particular case where they did something wrong "on
purpose", and then they entirely forgot about it, and five years later
it's running their whole infrastructure and doing insane things
because the "particular case" it was tested with was on some broken
preproduction machine with totally broken firmware tables for memory
node layout.

             Linus

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

#1717795

 
> So I propose testing the attached trivial patch. 

It doesn’t work. 
The call stack is the same.

   100.00%  (ffffffff821af140)
            |
            ---wait_on_page_bit
               __migration_entry_wait
               migration_entry_wait
               do_swap_page
               __handle_mm_fault
               handle_mm_fault
               __do_page_fault
               do_page_fault
               page_fault
               |
               |--40.62%--0x123a2
               |          start_thread
               |


> It may not do anything at all.
> But the existing code is actually doing extra work just to be fragile, in case the
> scenario above can happen.
> 
> Comments?
> 
>                 Linus

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

#1717824

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

On Tue, Aug 22, 2017 at 12:55 PM, Liang, Kan <kan.liang@intel.com> wrote:
>
>> So I propose testing the attached trivial patch.
>
> It doesn’t work.
> The call stack is the same.

So I would have expected the stack trace to be the same, and I would
even expect the CPU usage to be fairly similar, because you'd see
repeating from the callers (taking the fault again if the page is -
once again - being migrated).

But I was hoping that the wait queues would be shorter because the
loop for the retry would be bigger.

Oh well.

I'm slightly out of ideas. Apparently the yield() worked ok (apart
from not catching all cases), and maybe we could do a version that
waits on the page bit in the non-contended case, but yields under
contention?

IOW, maybe this is the best we can do for now? Introducing that
"wait_on_page_migration()" helper might allow us to tweak this a bit
as people come up with better ideas..

And then add Tim's patch for the general worst-case just in case?

             Linus

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

#1717832

On Tue, Aug 22, 2017 at 1:53 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>
> So _the_ problem with yield() is when you hit this with a RT task it
> will busy spin and possibly not allow the task that actually has the
> lock to make progress at all.

I thought we had explicitly defined yield() to not do that.

But I guess we could make this yielding behavior depend on a few more
heuristics. So do the yield only when there is contention, and when
it's a non-RT task.

          Linus

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

#1717834

On Tue, Aug 22, 2017 at 01:42:13PM -0700, Linus Torvalds wrote:
> +void wait_on_page_bit_or_yield(struct page *page, int bit_nr)
> +{
> +	if (PageWaiters(page)) {
> +		yield();
> +		return;
> +	}
> +	wait_on_page_bit(page, bit_nr);
> +}

So _the_ problem with yield() is when you hit this with a RT task it
will busy spin and possibly not allow the task that actually has the
lock to make progress at all.

So ideally there'd be a timeout or other limit on the amount of yield().

This being bit-spinlocks leaves us very short on state to play with
though :/

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

#1718428

> On Tue, Aug 22, 2017 at 12:55 PM, Liang, Kan <kan.liang@intel.com> wrote:
> >
> >> So I propose testing the attached trivial patch.
> >
> > It doesn’t work.
> > The call stack is the same.
> 
> So I would have expected the stack trace to be the same, and I would even
> expect the CPU usage to be fairly similar, because you'd see repeating from
> the callers (taking the fault again if the page is - once again - being migrated).
> 
> But I was hoping that the wait queues would be shorter because the loop for
> the retry would be bigger.
> 
> Oh well.
> 
> I'm slightly out of ideas. Apparently the yield() worked ok (apart from not
> catching all cases), and maybe we could do a version that waits on the page
> bit in the non-contended case, but yields under contention?
> 
> IOW, maybe this is the best we can do for now? Introducing that
> "wait_on_page_migration()" helper might allow us to tweak this a bit as
> people come up with better ideas..

The "wait_on_page_migration()" helper works well in the overnight testing.

Thanks,
Kan

> 
> And then add Tim's patch for the general worst-case just in case?
> 
>              Linus

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

#1718478

On 08/23/2017 07:49 AM, Liang, Kan wrote:
>> On Tue, Aug 22, 2017 at 12:55 PM, Liang, Kan <kan.liang@intel.com> wrote:
>>>
>>>> So I propose testing the attached trivial patch.
>>>
>>> It doesn’t work.
>>> The call stack is the same.
>>
>> So I would have expected the stack trace to be the same, and I would even
>> expect the CPU usage to be fairly similar, because you'd see repeating from
>> the callers (taking the fault again if the page is - once again - being migrated).
>>
>> But I was hoping that the wait queues would be shorter because the loop for
>> the retry would be bigger.
>>
>> Oh well.
>>
>> I'm slightly out of ideas. Apparently the yield() worked ok (apart from not
>> catching all cases), and maybe we could do a version that waits on the page
>> bit in the non-contended case, but yields under contention?
>>
>> IOW, maybe this is the best we can do for now? Introducing that
>> "wait_on_page_migration()" helper might allow us to tweak this a bit as
>> people come up with better ideas..
> 
> The "wait_on_page_migration()" helper works well in the overnight testing.
> 


Linus,

Will you still consider the original patch as a fail safe mechanism?

Thanks.

Tim

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

#1718573

On Wed, Aug 23, 2017 at 8:58 AM, Tim Chen <tim.c.chen@linux.intel.com> wrote:
>
> Will you still consider the original patch as a fail safe mechanism?

I don't think we have much choice, although I would *really* want to
get this root-caused rather than just papering over the symptoms.

Maybe still worth testing that "sched/numa: Scale scan period with
tasks in group and shared/private" patch that Mel mentioned.

In fact, looking at that patch description, it does seem to match this
particular load a lot. Quoting from the commit message:

  "Running 80 tasks in the same group, or as threads of the same process,
   results in the memory getting scanned 80x as fast as it would be if a
   single task was using the memory.

   This really hurts some workloads"

So if 80 threads causes 80x as much scanning, a few thousand threads
might indeed be really really bad.

So once more unto the breach, dear friends, once more.

Please.

The patch got applied to -tip as commit b5dd77c8bdad, and can be
downloaded here:

  https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?id=b5dd77c8bdada7b6262d0cba02a6ed525bf4e6e1

(Hmm. It says it's cc'd to me, but I never noticed that patch simply
because it was in a big group of other -tip commits.. Oh well).

          Linus

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

#1718643

> 
> On Wed, Aug 23, 2017 at 8:58 AM, Tim Chen <tim.c.chen@linux.intel.com>
> wrote:
> >
> > Will you still consider the original patch as a fail safe mechanism?
> 
> I don't think we have much choice, although I would *really* want to get this
> root-caused rather than just papering over the symptoms.
> 
> Maybe still worth testing that "sched/numa: Scale scan period with tasks in
> group and shared/private" patch that Mel mentioned.

The patch doesn’t help on our load.

Thanks,
Kan
> 
> In fact, looking at that patch description, it does seem to match this particular
> load a lot. Quoting from the commit message:
> 
>   "Running 80 tasks in the same group, or as threads of the same process,
>    results in the memory getting scanned 80x as fast as it would be if a
>    single task was using the memory.
> 
>    This really hurts some workloads"
> 
> So if 80 threads causes 80x as much scanning, a few thousand threads might
> indeed be really really bad.
> 
> So once more unto the breach, dear friends, once more.
> 
> Please.
> 
> The patch got applied to -tip as commit b5dd77c8bdad, and can be
> downloaded here:
> 
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?id=b5dd
> 77c8bdada7b6262d0cba02a6ed525bf4e6e1
> 
> (Hmm. It says it's cc'd to me, but I never noticed that patch simply because it
> was in a big group of other -tip commits.. Oh well).
> 
>           Linus

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

#1718719

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

On Wed, Aug 23, 2017 at 11:17 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Aug 23, 2017 at 8:58 AM, Tim Chen <tim.c.chen@linux.intel.com> wrote:
>>
>> Will you still consider the original patch as a fail safe mechanism?
>
> I don't think we have much choice, although I would *really* want to
> get this root-caused rather than just papering over the symptoms.

Oh well. Apparently we're not making progress on that, so I looked at
the patch again.

Can we fix it up a bit? In particular, the "bookmark_wake_function()"
thing added no value, and definitely shouldn't have been exported.
Just use NULL instead.

And the WAITQUEUE_WALK_BREAK_CNT thing should be internal to
__wake_up_common(), not in some common header file. Again, there's no
value in exporting it to anybody else.

And doing

                if (curr->flags & WQ_FLAG_BOOKMARK)

looks odd, when we just did

                unsigned flags = curr->flags;

one line earlier, so that can be just simplified.

So can you test that simplified version of the patch? I'm attaching my
suggested edited patch, but you may just want to do those changes
directly to your tree instead.

Hmm?

               Linus

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

#1719473

On 08/23/2017 04:30 PM, Linus Torvalds wrote:
> On Wed, Aug 23, 2017 at 11:17 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>> On Wed, Aug 23, 2017 at 8:58 AM, Tim Chen <tim.c.chen@linux.intel.com> wrote:
>>>
>>> Will you still consider the original patch as a fail safe mechanism?
>>
>> I don't think we have much choice, although I would *really* want to
>> get this root-caused rather than just papering over the symptoms.
> 
> Oh well. Apparently we're not making progress on that, so I looked at
> the patch again.
> 
> Can we fix it up a bit? In particular, the "bookmark_wake_function()"
> thing added no value, and definitely shouldn't have been exported.
> Just use NULL instead.
> 
> And the WAITQUEUE_WALK_BREAK_CNT thing should be internal to
> __wake_up_common(), not in some common header file. Again, there's no
> value in exporting it to anybody else.
> 
> And doing
> 
>                 if (curr->flags & WQ_FLAG_BOOKMARK)
> 
> looks odd, when we just did
> 
>                 unsigned flags = curr->flags;
> 
> one line earlier, so that can be just simplified.
> 
> So can you test that simplified version of the patch? I'm attaching my
> suggested edited patch, but you may just want to do those changes
> directly to your tree instead.

These changes look fine.  We are testing them now.
Does the second patch in the series look okay to you?

Thanks.

Tim

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

#1719496

On Thu, Aug 24, 2017 at 10:49 AM, Tim Chen <tim.c.chen@linux.intel.com> wrote:
>
> These changes look fine.  We are testing them now.
> Does the second patch in the series look okay to you?

I didn't really have any reaction to that one, as long as Mel&co are
ok with it, I'm fine with it.

            Linus

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

#1719555

On Thu, Aug 24, 2017 at 11:16:15AM -0700, Linus Torvalds wrote:
> On Thu, Aug 24, 2017 at 10:49 AM, Tim Chen <tim.c.chen@linux.intel.com> wrote:
> >
> > These changes look fine.  We are testing them now.
> > Does the second patch in the series look okay to you?
> 
> I didn't really have any reaction to that one, as long as Mel&co are
> ok with it, I'm fine with it.
> 

I've no strong objections or concerns. I'm disappointed that the
original root cause for this could not be found but hope that eventually a
reproducible test case will eventually be available. Despite having access
to a 4-socket box, I was still unable to create a workload that caused
large delays on wakeup. I'm going to have to stop as I don't think it's
possible to create on that particular machine for whatever reason.

-- 
Mel Gorman
SUSE Labs

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

#1720275

On 08/24/2017 01:44 PM, Mel Gorman wrote:
> On Thu, Aug 24, 2017 at 11:16:15AM -0700, Linus Torvalds wrote:
>> On Thu, Aug 24, 2017 at 10:49 AM, Tim Chen <tim.c.chen@linux.intel.com> wrote:
>>>
>>> These changes look fine.  We are testing them now.
>>> Does the second patch in the series look okay to you?
>>
>> I didn't really have any reaction to that one, as long as Mel&co are
>> ok with it, I'm fine with it.
>>
> 
> I've no strong objections or concerns. I'm disappointed that the
> original root cause for this could not be found but hope that eventually a
> reproducible test case will eventually be available. Despite having access
> to a 4-socket box, I was still unable to create a workload that caused
> large delays on wakeup. I'm going to have to stop as I don't think it's
> possible to create on that particular machine for whatever reason.
> 

Kan helped to test the updated patch 1 from Linus.  It worked fine.
I've refreshed the patch set that includes all the changes
and send a version 2 refresh of the patch set separately.

Thanks.

Tim

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

#1718486

On Tue, Aug 22, 2017 at 11:19:12AM -0700, Linus Torvalds wrote:
> On Tue, Aug 22, 2017 at 10:23 AM, Liang, Kan <kan.liang@intel.com> wrote:
> >
> > Although the patch doesn't trigger watchdog, the spin lock wait time
> > is not small (0.45s).
> > It may get worse again on larger systems.
> 
> Yeah, I don't think Mel's patch is great - because I think we could do
> so much better.
> 
> What I like about Mel's patch is that it recognizes that
> "wait_on_page_locked()" there is special, and replaces it with
> something else. I think that "something else" is worse than my
> "yield()" call, though.
> 

I only partially agree. yield() can be unbound if there are an indefinite
number of lock holders or frequent reacquisitions. yield() also some warnings
around it related to potentially never doing the actual yield. The latter
can cause lockup warnings. I was aiming for was the easiest path to "try
for a bit but give up in a reasonable amount of time". I picked waiting
on the page lock because at least it'll recover. I could have returned
and allowed the fault to retry but thought this may consume excessive CPU.

I spent more time on the test case to try and get some sort of useful
data out of it and that took most of the time I had available again. The
current state of the test case still isn't hitting the worst patterns but
it can at least detect latency problems. It uses multiple threads bound to
different nodes to access thread-private data within a large buffer where
each thread's data is aligned. For example, an alignment of 64 would have
each thread access a private cache line while still sharing a page from
a NUMA balancing point of view. 4K would still share a page if THP was
used. I've a few patches running on a 4-socket machine with 144 I borrowed
for a few hours and hopefully something will fall out that.

> So if we do busy loops, I really think we should also make sure that
> the thing we're waiting for is not preempted.
> 
> HOWEVER, I'm actually starting to think that there is perhaps
> something else going on.
> 
> Let me walk you through my thinking:
> 
> This is the migration logic:
> 
>  (a) migration locks the page
> 
>  (b) migration is supposedly CPU-limited
> 
>  (c) migration then unlocks the page.
> 
> Ignore all the details, that's the 10.000 ft view. Right?
> 

Right.

> Now, if the above is right, then I have a question for people:
> 
>   HOW IN THE HELL DO WE HAVE TIME FOR THOUSANDS OF THREADS TO HIT THAT ONE PAGE?
> 

There are not many explanations. Given that it's thousands of threads, it
may be the case that some are waiting while there are many more contending
on CPU. The migration process can get scheduled out which might compound the
problem. While THP migration gives up quickly after a migration failure,
base page migration does not. If any part of the migration path returns
EAGAIN, it'll retry up to 10 times and depending where it is, that can
mean the migrating process is locking the page 10 times. If it's fast
enough reacquiring the lock, the waiting processes will wait for each of
those 10 attempts because they don't notice that base page migration has
already cleared the NUMA pte.

Given that NUMA balancing is best effort, the 10 attempts for numa balancing
is questionable. A patch to test should be straight-forward so I'll spit
it out after this mail and queue it up.

> That just sounds really sketchy to me. Even if all those thousands of
> threads are runnable, we need to schedule into them just to get them
> to wait on that one page.
> 

The same is true if they are just yielding.

> So that sounds really quite odd when migration is supposed to hold the
> page lock for a relatively short time and get out. Don't you agree?
> 

Yes. As part of that getting out, it shouldn't retry 10 times.

> Which is why I started thinking of what the hell could go on for that
> long wait-queue to happen.
> 
> One thing that strikes me is that the way wait_on_page_bit() works is
> that it will NOT wait until the next bit clearing, it will wait until
> it actively *sees* the page bit being clear.
> 
> Now, work with me on that. What's the difference?
> 
> What we could have is some bad NUMA balancing pattern that actually
> has a page that everybody touches.
> 
> And hey, we pretty much know that everybody touches that page, since
> people get stuck on that wait-queue, right?
> 
> And since everybody touches it, as a result everybody eventually
> thinks that page should be migrated to their NUMA node.
> 

Potentially yes. There is a two-pass filter as mentioned elsewhere in the
thread and the scanner has to update the PTEs for that to happen but it's not
completely impossible. Once a migration starts, other threads shouldn't try
again until the next window. That window can be small but with thousands
of threads potentially scanning (even at a very slow rate), the window
could be tiny. If many threads are doing the scanning one after the other,
it would potentially allow the two-pass check to pass sooner than expected.

Co-incidentally, Rik encountered this class of problem and there is a
patch in Andrew's tree "sched/numa: Scale scan period with tasks in group
and shared/private" that might have an impact on this problem.

> But for all we know, the migration keeps on failing, because one of
> the points of that "lock page - try to move - unlock page" is that
> *TRY* in "try to move". There's a number of things that makes it not
> actually migrate. Like not being movable, or failing to isolate the
> page, or whatever.
> 
> So we could have some situation where we end up locking and unlocking
> the page over and over again (which admittedly is already a sign of
> something wrong in the NUMA balancing, but that's a separate issue).
> 

The retries are part of the picture in the migration side. Multiple
protection updates from large numbers of threads are another potential
source.

> And if we get into that situation, where everybody wants that one hot
> page, what happens to the waiters?
> 
> One of the thousands of waiters is unlucky (remember, this argument
> started with the whole "you shouldn't get that many waiters on one
> single page that isn't even locked for that long"), and goes:
> 
>  (a) Oh, the page is locked, I will wait for the lock bit to clear
> 
>  (b) go to sleep
> 
>  (c) the migration fails, the lock bit is cleared, the waiter is woken
> up but doesn't get the CPU immediately, and one of the other
> *thousands* of threads decides to also try to migrate (see above),
> 
>  (d) the guy waiting for the lock bit to clear will see the page
> "still" locked (really just "locked again") and continue to wait.
> 

Part c may be slightly inaccurate but I think a similar situation can occur
with multiple threads deciding to do change_prot_numa in quick succession
so it's functionally similar.

> In the meantime, one of the other threads happens to be unlucky, also
> hits the race, and now we have one more thread waiting for that page
> lock. It keeps getting unlocked, but it also keeps on getting locked,
> and so the queue can keep growing.
> 
> See where I'm going here? I think it's really odd how *thousands* of
> threads can hit that locked window that is supposed to be pretty
> small. But I think it's much more likely if we have some kind of
> repeated event going on.
> 

Agreed.

> So I'm starting to think that part of the problem may be how stupid
> that "wait_for_page_bit_common()" code is. It really shouldn't wait
> until it sees that the bit is clear. It could have been cleared and
> then re-taken.
> 
> And honestly, we actually have extra code for that "let's go round
> again". That seems pointless. If the bit has been cleared, we've been
> woken up, and nothing else would have done so anyway, so if we're not
> interested in locking, we're simply *done* after we've done the
> "io_scheduler()".
> 
> So I propose testing the attached trivial patch. It may not do
> anything at all. But the existing code is actually doing extra work
> just to be fragile, in case the scenario above can happen.
> 
> Comments?

Nothing useful to add on top of Peter's concerns but I haven't thought
about that aspect of the thread very much. I'm going to try see if a
patch that avoids multiple migration retries or Rik's patch have a
noticable impact in case they are enough on their own.

-- 
Mel Gorman
SUSE Labs

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

#1715429

> I was really hoping that we'd root-cause this and have a solution (and
> then apply Tim's patch as a "belt and suspenders" kind of thing), but

One thing I wanted to point out is that Tim's patch seems to make
several schedule intensive micro benchmarks faster. 

I think what's happening is that it allows more parallelism during wakeup:

Normally it's like

CPU 1                           CPU 2               CPU 3                    .....
 
LOCK
wake up tasks on other CPUs    woken up             woken up
UNLOCK                         SPIN on waitq lock   SPIN on waitq lock
                               LOCK
                               remove waitq
                               UNLOCk
                                                    LOCK
                                                    remove waitq
                                                    UNLOCK

So everything is serialized.

But with the bookmark patch the other CPUs can go through the "remove waitq" sequence
earlier because they have a chance to get a go at the lock and do it in parallel 
with the main wakeup.  

Tim used a 64 task threshold for the bookmark. That may be actually too large.
It may even be faster to use a shorter one.

So I think it's more than a bandaid, but likely a useful performance improvement
even for less extreme wait queues.

-Andi

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

#1715447

On Fri, Aug 18, 2017 at 1:05 PM, Andi Kleen <ak@linux.intel.com> wrote:
>
> I think what's happening is that it allows more parallelism during wakeup:
>
> Normally it's like
>
> CPU 1                           CPU 2               CPU 3                    .....
>
> LOCK
> wake up tasks on other CPUs    woken up             woken up
> UNLOCK                         SPIN on waitq lock   SPIN on waitq lock

Hmm. The processes that are woken up shouldn't need to touch the waitq
lock after wakeup. The default "autoremove_wake_function()" does the
wait list removal, so if you just use the normal wait/wakeup, you're
all done an don't need to do anythig more.

That's very much by design.

In fact, it's why "finish_wait()" uses that "list_empty_careful()"
thing on the entry - exactly so that it only needs to take the wait
queue lock if it is still on the wait list (ie it was woken up by
something else).

Now, it *is* racy, in the sense that the autoremove_wake_function()
will remove the entry *after* having successfully woken up the
process, so with bad luck and a quick wakeup, the woken process may
not see the good list_empty_careful() case.

So we really *should* do the remove earlier inside the pi_lock region
in ttwu(). We don't have that kind of interface, though. If you
actually do see tasks getting stuck on the waitqueue lock after being
woken up, it might be worth looking at, though.

The other possibility is that you were looking at cases that didn't
use "autoremove_wake_function()" at all, of course. Maybe they are
worth fixing. The autoremval really does make a difference, exactly
because of the issue you point to.

                       Linus

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

Page 3 of 4 — ← Prev page 1 2 [3] 4  Next page →

Back to top | Article view | linux.kernel

csiph-web