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 2 of 2 — ← Prev page 1 [2]

#1509887

On Wed, Oct 26, 2016 at 02:26:57PM -0700, Linus Torvalds wrote:
> 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.
> 

To be clear, are you referring to PeterZ's patch that avoids the lookup? If
so, I see your point.

> With that fixed, do we actually get contention on a per-page basis?

Reclaim would have to running parallel to migrations, faults, clearing
write-protect etc. I can't think of a situation where a normal workload
would hit it regularly and/or for long durations.

> 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.
> 

Except many direct reclaimers on small LRUs while a system is thrashing --
not a case that really matters, you've already lost.

> >> 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.
> 

No problem with that.

> >> +#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.
> 

No objection. Shuffling it around does not make it obviously better in
any way.

In the meantime, a machine freed up. FWIW, it survived booting on a 2-socket
and about 20 minutes of bashing on reclaim paths from multiple processes
to beat on lock/unlock. I didn't do a performance comparison or gather
profile data but I wouldn't expect anything interesting from profiles
other than some cycles saved.

-- 
Mel Gorman
SUSE Labs

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

#1509900

On Wed, Oct 26, 2016 at 3:03 PM, Mel Gorman <mgorman@techsingularity.net> wrote:
>
> To be clear, are you referring to PeterZ's patch that avoids the lookup? If
> so, I see your point.

Yup, that's the one. I think you tested it. In fact, I'm sure you did,
because I remember seeing performance numbers from  you ;)

So yes, I'd expect my patch on its own to quite possibly regress on
NUMA systems (although I wonder how much), but I consider PeterZ's
patch the fix to that, so I wouldn't worry about it.

              Linus

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

#1509923

On Wed, Oct 26, 2016 at 03:09:41PM -0700, Linus Torvalds wrote:
> On Wed, Oct 26, 2016 at 3:03 PM, Mel Gorman <mgorman@techsingularity.net> wrote:
> >
> > To be clear, are you referring to PeterZ's patch that avoids the lookup? If
> > so, I see your point.
> 
> Yup, that's the one. I think you tested it. In fact, I'm sure you did,
> because I remember seeing performance numbers from  you ;)
> 

Yeah and the figures were fine. IIRC, 32-bit support was the main thing
that was missing but who cares, 32-bit is not going to have the NUMA issues
in any way that matters.

> So yes, I'd expect my patch on its own to quite possibly regress on
> NUMA systems (although I wonder how much),

I doubt it's a lot. Even if it does, it's doesn't matter because it's a
functional fix.

> but I consider PeterZ's
> patch the fix to that, so I wouldn't worry about it.
> 

Agreed. Peter, do you plan to finish that patch?

-- 
Mel Gorman
SUSE Labs

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

#1510134

On Thu, Oct 27, 2016 at 10:08:52AM +0200, Peter Zijlstra wrote:
> On Thu, Oct 27, 2016 at 12:07:26AM +0100, Mel Gorman wrote:
> > > but I consider PeterZ's
> > > patch the fix to that, so I wouldn't worry about it.
> > > 
> > 
> > Agreed. Peter, do you plan to finish that patch?
> 
> I was waiting for you guys to hash out the 32bit issue. But if we're now
> OK with having this for 64bit only, I can certainly look at doing a new
> version.
> 

I've no problem with it being 64-bit only.

> I'll have to look at fixing Alpha's bitops for that first though,
> because as is that patch relies on atomics to the same word not needing
> ordering, but placing the contended/waiters bit in the high word for
> 64bit only sorta breaks that.
> 

I see the problem assuming you're referring to the requirement that locked
and waiter bits are on the same word. Without it, you need a per-arch helper
that forces ordering or takes a spinlock. I doubt it's worth the trouble.

> Hurm, we could of course play games with the layout, the 64bit only
> flags don't _have_ to be at the end.
> 
> Something like so could work I suppose, but then there's a slight
> regression in the page_unlock() path, where we now do an unconditional
> spinlock; iow. we loose the unlocked waitqueue_active() test.
> 

I can't convince myself it's worthwhile. At least, I can't see a penalty
of potentially moving one of the two bits to the high word. It's the
same cache line and the same op when it matters.

> We could re-instate this with an #ifndef CONFIG_NUMA I suppose.. not
> pretty though.
> 
> Also did the s/contended/waiters/ rename per popular request.
> 
> ---
>  include/linux/page-flags.h     |   19 ++++++++
>  include/linux/pagemap.h        |   25 ++++++++--
>  include/trace/events/mmflags.h |    7 +++
>  mm/filemap.c                   |   94 +++++++++++++++++++++++++++++++++++++----
>  4 files changed, 130 insertions(+), 15 deletions(-)
> 
> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
> @@ -73,6 +73,14 @@
>   */
>  enum pageflags {
>  	PG_locked,		/* Page is locked. Don't touch. */
> +#ifdef CONFIG_NUMA
> +	/*
> +	 * This bit must end up in the same word as PG_locked (or any other bit
> +	 * we're waiting on), as per all architectures their bitop
> +	 * implementations.
> +	 */
> +	PG_waiters,		/* The hashed waitqueue has waiters */
> +#endif
>  	PG_error,
>  	PG_referenced,
>  	PG_uptodate,

I don't see why it should be NUMA-specific even though with Linus'
patch, NUMA is a concern. Even then, you still need a 64BIT check
because 32BIT && NUMA is allowed on a number of architectures.

Otherwise, nothing jumped out at me but glancing through it looked very
similar to the previous patch.

-- 
Mel Gorman
SUSE Labs

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

#1510329

On Thu, Oct 27, 2016 at 11:44:49AM +0200, Peter Zijlstra wrote:
> On Thu, Oct 27, 2016 at 10:07:42AM +0100, Mel Gorman wrote:
> > > Something like so could work I suppose, but then there's a slight
> > > regression in the page_unlock() path, where we now do an unconditional
> > > spinlock; iow. we loose the unlocked waitqueue_active() test.
> > > 
> > 
> > I can't convince myself it's worthwhile. At least, I can't see a penalty
> > of potentially moving one of the two bits to the high word. It's the
> > same cache line and the same op when it matters.
> 
> I'm having trouble connecting these here two paragraphs. Or were you
> replying to something else?
> 
> So the current unlock code does:
> 
>   wake_up_page()
>     if (waitqueue_active())
>       __wake_up() /* takes waitqueue spinlocks here */
> 
> While the new one does:
> 
>   spin_lock(&q->lock);
>   if (waitqueue_active()) {
>     __wake_up_common()
>   }
>   spin_unlock(&q->lock);
> 
> Which is an unconditional atomic op (which go for about ~20 cycles each,
> when uncontended).
> 

Ok, we were thinking about different things but I'm not sure I get your
concern. With your patch, in the uncontended case we check the waiters
bit and if there is no contention, we carry on. In the contended case,
the lock is taken. Given that contention is likely to be due to IO being
completed, I don't think the atomic op on top is going to make that much
of a difference.

About the only hazard I can think of is when unrelated pages hash to the
same queue and so there is an extra op for the "fake contended" case. I
don't think it's worth worrying about given that a false contention and
atomic op might hurt some workload but the common case is avoiding a
lookup.

> > I don't see why it should be NUMA-specific even though with Linus'
> > patch, NUMA is a concern. Even then, you still need a 64BIT check
> > because 32BIT && NUMA is allowed on a number of architectures.
> 
> Oh, I thought we killed 32bit NUMA and didn't check. I can make it
> CONFIG_64BIT and be done with it. s/CONFIG_NUMA/CONFIG_64BIT/ on the
> patch should do :-)
> 

Sounds good.

-- 
Mel Gorman
SUSE Labs

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

#1510385

On Thu, Oct 27, 2016 at 10:07:42AM +0100, Mel Gorman wrote:
> > Something like so could work I suppose, but then there's a slight
> > regression in the page_unlock() path, where we now do an unconditional
> > spinlock; iow. we loose the unlocked waitqueue_active() test.
> > 
> 
> I can't convince myself it's worthwhile. At least, I can't see a penalty
> of potentially moving one of the two bits to the high word. It's the
> same cache line and the same op when it matters.

I'm having trouble connecting these here two paragraphs. Or were you
replying to something else?

So the current unlock code does:

  wake_up_page()
    if (waitqueue_active())
      __wake_up() /* takes waitqueue spinlocks here */

While the new one does:

  spin_lock(&q->lock);
  if (waitqueue_active()) {
    __wake_up_common()
  }
  spin_unlock(&q->lock);

Which is an unconditional atomic op (which go for about ~20 cycles each,
when uncontended).


> > +++ b/include/linux/page-flags.h
> > @@ -73,6 +73,14 @@
> >   */
> >  enum pageflags {
> >  	PG_locked,		/* Page is locked. Don't touch. */
> > +#ifdef CONFIG_NUMA
> > +	/*
> > +	 * This bit must end up in the same word as PG_locked (or any other bit
> > +	 * we're waiting on), as per all architectures their bitop
> > +	 * implementations.
> > +	 */
> > +	PG_waiters,		/* The hashed waitqueue has waiters */
> > +#endif
> >  	PG_error,
> >  	PG_referenced,
> >  	PG_uptodate,
> 
> I don't see why it should be NUMA-specific even though with Linus'
> patch, NUMA is a concern. Even then, you still need a 64BIT check
> because 32BIT && NUMA is allowed on a number of architectures.

Oh, I thought we killed 32bit NUMA and didn't check. I can make it
CONFIG_64BIT and be done with it. s/CONFIG_NUMA/CONFIG_64BIT/ on the
patch should do :-)

> Otherwise, nothing jumped out at me but glancing through it looked very
> similar to the previous patch.

Right, all the difference was in the bit being conditional and having a
different name.

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

#1510211

On Thu, 27 Oct 2016 10:08:52 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, Oct 27, 2016 at 12:07:26AM +0100, Mel Gorman wrote:
> > > but I consider PeterZ's
> > > patch the fix to that, so I wouldn't worry about it.
> > >   
> > 
> > Agreed. Peter, do you plan to finish that patch?  
> 
> I was waiting for you guys to hash out the 32bit issue. But if we're now
> OK with having this for 64bit only, I can certainly look at doing a new
> version.
> 
> I'll have to look at fixing Alpha's bitops for that first though,
> because as is that patch relies on atomics to the same word not needing
> ordering, but placing the contended/waiters bit in the high word for
> 64bit only sorta breaks that.

I got mine working too. Haven't removed the bitops barrier (that's
for another day), or sorted the page flags in this one. But the core
code is there.

It's a bit more intrusive than your patch, but I like the end result
better. It just stops using the generic bit waiter code completely
and uses its own keys. Ends up being making things easier, and we
could wait for other page details too if that was ever required.

It uses the same wait bit logic for all page waiters.
It keeps PageWaiters manipulation entirely under waitqueue lock, so no
additional data races beyond existing unlocked waitqueue_active tests.
And it checks to clear the waiter bit when no waiters for that page are
in the queue, so hash collisions with long waiters don't end up dragging
us into the slowpath always.

Also didn't uninline unlock_page yet. Still causes some text expansion,
but we should revisit that.

Thanks,
Nick

---
 include/linux/page-flags.h     |   2 +
 include/linux/pagemap.h        |  23 +++---
 include/trace/events/mmflags.h |   1 +
 mm/filemap.c                   | 157 ++++++++++++++++++++++++++++++++---------
 mm/swap.c                      |   2 +
 5 files changed, 138 insertions(+), 47 deletions(-)

diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index 74e4dda..8059c04 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -73,6 +73,7 @@
  */
 enum pageflags {
 	PG_locked,		/* Page is locked. Don't touch. */
+	PG_waiters,		/* Page has waiters, check its waitqueue */
 	PG_error,
 	PG_referenced,
 	PG_uptodate,
@@ -253,6 +254,7 @@ static inline int TestClearPage##uname(struct page *page) { return 0; }
 	TESTSETFLAG_FALSE(uname) TESTCLEARFLAG_FALSE(uname)
 
 __PAGEFLAG(Locked, locked, PF_NO_TAIL)
+PAGEFLAG(Waiters, waiters, PF_NO_COMPOUND) __CLEARPAGEFLAG(Waiters, waiters, PF_NO_COMPOUND)
 PAGEFLAG(Error, error, PF_NO_COMPOUND) TESTCLEARFLAG(Error, error, PF_NO_COMPOUND)
 PAGEFLAG(Referenced, referenced, PF_HEAD)
 	TESTCLEARFLAG(Referenced, referenced, PF_HEAD)
diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index dd15d39..97f2d0b 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -477,22 +477,14 @@ static inline int lock_page_or_retry(struct page *page, struct mm_struct *mm,
  * and for filesystems which need to wait on PG_private.
  */
 extern void wait_on_page_bit(struct page *page, int bit_nr);
-
 extern int wait_on_page_bit_killable(struct page *page, int bit_nr);
-extern int wait_on_page_bit_killable_timeout(struct page *page,
-					     int bit_nr, unsigned long timeout);
-
-static inline int wait_on_page_locked_killable(struct page *page)
-{
-	if (!PageLocked(page))
-		return 0;
-	return wait_on_page_bit_killable(compound_head(page), PG_locked);
-}
+extern void wake_up_page_bit(struct page *page, int bit_nr);
 
-extern wait_queue_head_t *page_waitqueue(struct page *page);
 static inline void wake_up_page(struct page *page, int bit)
 {
-	__wake_up_bit(page_waitqueue(page), &page->flags, bit);
+	if (!PageWaiters(page))
+		return;
+	wake_up_page_bit(page, bit);
 }
 
 /* 
@@ -508,6 +500,13 @@ static inline void wait_on_page_locked(struct page *page)
 		wait_on_page_bit(compound_head(page), PG_locked);
 }
 
+static inline int wait_on_page_locked_killable(struct page *page)
+{
+	if (!PageLocked(page))
+		return 0;
+	return wait_on_page_bit_killable(compound_head(page), PG_locked);
+}
+
 /* 
  * Wait for a page to complete writeback
  */
diff --git a/include/trace/events/mmflags.h b/include/trace/events/mmflags.h
index 5a81ab4..7ac8c0a 100644
--- a/include/trace/events/mmflags.h
+++ b/include/trace/events/mmflags.h
@@ -81,6 +81,7 @@
 
 #define __def_pageflag_names						\
 	{1UL << PG_locked,		"locked"	},		\
+	{1UL << PG_waiters,		"waiters"	},		\
 	{1UL << PG_error,		"error"		},		\
 	{1UL << PG_referenced,		"referenced"	},		\
 	{1UL << PG_uptodate,		"uptodate"	},		\
diff --git a/mm/filemap.c b/mm/filemap.c
index 849f459..cab1f87 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -788,47 +788,137 @@ EXPORT_SYMBOL(__page_cache_alloc);
  * at a cost of "thundering herd" phenomena during rare hash
  * collisions.
  */
-wait_queue_head_t *page_waitqueue(struct page *page)
+static wait_queue_head_t *page_waitqueue(struct page *page)
 {
 	const struct zone *zone = page_zone(page);
 
 	return &zone->wait_table[hash_ptr(page, zone->wait_table_bits)];
 }
-EXPORT_SYMBOL(page_waitqueue);
 
-void wait_on_page_bit(struct page *page, int bit_nr)
+struct wait_page_key {
+	struct page *page;
+	int bit_nr;
+	int page_match;
+};
+
+struct wait_page_queue {
+	struct page *page;
+	int bit_nr;
+	wait_queue_t wait;
+};
+
+static int wake_page_function(wait_queue_t *wait, unsigned mode, int sync, void *arg)
 {
-	DEFINE_WAIT_BIT(wait, &page->flags, bit_nr);
+	struct wait_page_key *key = arg;
+	struct wait_page_queue *wait_page
+		= container_of(wait, struct wait_page_queue, wait);
+
+	if (wait_page->page != key->page)
+	       return 0;
+	key->page_match = 1;
 
-	if (test_bit(bit_nr, &page->flags))
-		__wait_on_bit(page_waitqueue(page), &wait, bit_wait_io,
-							TASK_UNINTERRUPTIBLE);
+	if (wait_page->bit_nr != key->bit_nr)
+		return 0;
+	if (test_bit(key->bit_nr, &key->page->flags))
+		return 0;
+
+	return autoremove_wake_function(wait, mode, sync, key);
 }
-EXPORT_SYMBOL(wait_on_page_bit);
 
-int wait_on_page_bit_killable(struct page *page, int bit_nr)
+void wake_up_page_bit(struct page *page, int bit_nr)
 {
-	DEFINE_WAIT_BIT(wait, &page->flags, bit_nr);
+	wait_queue_head_t *q = page_waitqueue(page);
+	struct wait_page_key key;
+	unsigned long flags;
 
-	if (!test_bit(bit_nr, &page->flags))
-		return 0;
+	key.page = page;
+	key.bit_nr = bit_nr;
+	key.page_match = 0;
 
-	return __wait_on_bit(page_waitqueue(page), &wait,
-			     bit_wait_io, TASK_KILLABLE);
+	spin_lock_irqsave(&q->lock, flags);
+	__wake_up_locked_key(q, TASK_NORMAL, &key);
+	if (!waitqueue_active(q) || !key.page_match) {
+		ClearPageWaiters(page);
+		/*
+		 * It's possible to miss clearing Waiters here, when we woke
+		 * our page waiters, but the hashed waitqueue has waiters for
+		 * other pages on it.
+		 *
+		 * That's okay, it's a rare case. The next waker will clear it.
+		 */
+	}
+	spin_unlock_irqrestore(&q->lock, flags);
 }
+EXPORT_SYMBOL(wake_up_page_bit);
 
-int wait_on_page_bit_killable_timeout(struct page *page,
-				       int bit_nr, unsigned long timeout)
+static inline int wait_on_page_bit_common(wait_queue_head_t *q,
+		struct page *page, int bit_nr, int state, bool lock)
 {
-	DEFINE_WAIT_BIT(wait, &page->flags, bit_nr);
+	struct wait_page_queue wait_page;
+	wait_queue_t *wait = &wait_page.wait;
+	int ret = 0;
 
-	wait.key.timeout = jiffies + timeout;
-	if (!test_bit(bit_nr, &page->flags))
-		return 0;
-	return __wait_on_bit(page_waitqueue(page), &wait,
-			     bit_wait_io_timeout, TASK_KILLABLE);
+	init_wait(wait);
+	wait->func = wake_page_function;
+	wait_page.page = page;
+	wait_page.bit_nr = bit_nr;
+
+	for (;;) {
+		spin_lock_irq(&q->lock);
+
+		if (likely(list_empty(&wait->task_list))) {
+			if (lock)
+				__add_wait_queue_tail_exclusive(q, wait);
+			else
+				__add_wait_queue(q, wait);
+			SetPageWaiters(page);
+		}
+
+		set_current_state(state);
+
+		spin_unlock_irq(&q->lock);
+
+		if (likely(test_bit(bit_nr, &page->flags))) {
+			io_schedule();
+			if (unlikely(signal_pending_state(state, current))) {
+				ret = -EINTR;
+				break;
+			}
+		}
+
+		if (lock) {
+			if (!test_and_set_bit_lock(bit_nr, &page->flags))
+				break;
+		} else {
+			if (!test_bit(bit_nr, &page->flags))
+				break;
+		}
+	}
+
+	finish_wait(q, wait);
+
+	/*
+	 * A signal could leave PageWaiters set. Clearing it here if
+	 * !waitqueue_active would be possible, but still fail to catch it in
+	 * the case of wait hash collision. We already can fail to clear wait
+	 * hash collision cases, so don't bother with signals either.
+	 */
+
+	return ret;
+}
+
+void wait_on_page_bit(struct page *page, int bit_nr)
+{
+	wait_queue_head_t *q = page_waitqueue(page);
+	wait_on_page_bit_common(q, page, bit_nr, TASK_UNINTERRUPTIBLE, false);
+}
+EXPORT_SYMBOL(wait_on_page_bit);
+
+int wait_on_page_bit_killable(struct page *page, int bit_nr)
+{
+	wait_queue_head_t *q = page_waitqueue(page);
+	return wait_on_page_bit_common(q, page, bit_nr, TASK_KILLABLE, false);
 }
-EXPORT_SYMBOL_GPL(wait_on_page_bit_killable_timeout);
 
 /**
  * add_page_wait_queue - Add an arbitrary waiter to a page's wait queue
@@ -844,6 +934,7 @@ void add_page_wait_queue(struct page *page, wait_queue_t *waiter)
 
 	spin_lock_irqsave(&q->lock, flags);
 	__add_wait_queue(q, waiter);
+	SetPageWaiters(page);
 	spin_unlock_irqrestore(&q->lock, flags);
 }
 EXPORT_SYMBOL_GPL(add_page_wait_queue);
@@ -925,23 +1016,19 @@ EXPORT_SYMBOL_GPL(page_endio);
  * __lock_page - get a lock on the page, assuming we need to sleep to get it
  * @page: the page to lock
  */
-void __lock_page(struct page *page)
+void __lock_page(struct page *__page)
 {
-	struct page *page_head = compound_head(page);
-	DEFINE_WAIT_BIT(wait, &page_head->flags, PG_locked);
-
-	__wait_on_bit_lock(page_waitqueue(page_head), &wait, bit_wait_io,
-							TASK_UNINTERRUPTIBLE);
+	struct page *page = compound_head(__page);
+	wait_queue_head_t *q = page_waitqueue(page);
+	wait_on_page_bit_common(q, page, PG_locked, TASK_UNINTERRUPTIBLE, true);
 }
 EXPORT_SYMBOL(__lock_page);
 
-int __lock_page_killable(struct page *page)
+int __lock_page_killable(struct page *__page)
 {
-	struct page *page_head = compound_head(page);
-	DEFINE_WAIT_BIT(wait, &page_head->flags, PG_locked);
-
-	return __wait_on_bit_lock(page_waitqueue(page_head), &wait,
-					bit_wait_io, TASK_KILLABLE);
+	struct page *page = compound_head(__page);
+	wait_queue_head_t *q = page_waitqueue(page);
+	return wait_on_page_bit_common(q, page, PG_locked, TASK_KILLABLE, true);
 }
 EXPORT_SYMBOL_GPL(__lock_page_killable);
 
diff --git a/mm/swap.c b/mm/swap.c
index 4dcf852..844baed 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -69,6 +69,7 @@ static void __page_cache_release(struct page *page)
 		del_page_from_lru_list(page, lruvec, page_off_lru(page));
 		spin_unlock_irqrestore(zone_lru_lock(zone), flags);
 	}
+	__ClearPageWaiters(page);
 	mem_cgroup_uncharge(page);
 }
 
@@ -784,6 +785,7 @@ void release_pages(struct page **pages, int nr, bool cold)
 
 		/* Clear Active bit in case of parallel mark_page_accessed */
 		__ClearPageActive(page);
+		__ClearPageWaiters(page);
 
 		list_add(&page->lru, &pages_to_free);
 	}
-- 
2.9.3

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

#1510339

On Thu, Oct 27, 2016 at 12:07:26AM +0100, Mel Gorman wrote:
> > but I consider PeterZ's
> > patch the fix to that, so I wouldn't worry about it.
> > 
> 
> Agreed. Peter, do you plan to finish that patch?

I was waiting for you guys to hash out the 32bit issue. But if we're now
OK with having this for 64bit only, I can certainly look at doing a new
version.

I'll have to look at fixing Alpha's bitops for that first though,
because as is that patch relies on atomics to the same word not needing
ordering, but placing the contended/waiters bit in the high word for
64bit only sorta breaks that.

Hurm, we could of course play games with the layout, the 64bit only
flags don't _have_ to be at the end.

Something like so could work I suppose, but then there's a slight
regression in the page_unlock() path, where we now do an unconditional
spinlock; iow. we loose the unlocked waitqueue_active() test.

We could re-instate this with an #ifndef CONFIG_NUMA I suppose.. not
pretty though.

Also did the s/contended/waiters/ rename per popular request.

---
 include/linux/page-flags.h     |   19 ++++++++
 include/linux/pagemap.h        |   25 ++++++++--
 include/trace/events/mmflags.h |    7 +++
 mm/filemap.c                   |   94 +++++++++++++++++++++++++++++++++++++----
 4 files changed, 130 insertions(+), 15 deletions(-)

--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -73,6 +73,14 @@
  */
 enum pageflags {
 	PG_locked,		/* Page is locked. Don't touch. */
+#ifdef CONFIG_NUMA
+	/*
+	 * This bit must end up in the same word as PG_locked (or any other bit
+	 * we're waiting on), as per all architectures their bitop
+	 * implementations.
+	 */
+	PG_waiters,		/* The hashed waitqueue has waiters */
+#endif
 	PG_error,
 	PG_referenced,
 	PG_uptodate,
@@ -231,6 +239,9 @@ static __always_inline int TestClearPage
 #define TESTPAGEFLAG_FALSE(uname)					\
 static inline int Page##uname(const struct page *page) { return 0; }
 
+#define TESTPAGEFLAG_TRUE(uname)					\
+static inline int Page##uname(const struct page *page) { return 1; }
+
 #define SETPAGEFLAG_NOOP(uname)						\
 static inline void SetPage##uname(struct page *page) {  }
 
@@ -249,10 +260,18 @@ static inline int TestClearPage##uname(s
 #define PAGEFLAG_FALSE(uname) TESTPAGEFLAG_FALSE(uname)			\
 	SETPAGEFLAG_NOOP(uname) CLEARPAGEFLAG_NOOP(uname)
 
+#define PAGEFLAG_TRUE(uname) TESTPAGEFLAG_TRUE(uname)			\
+	SETPAGEFLAG_NOOP(uname) CLEARPAGEFLAG_NOOP(uname)
+
 #define TESTSCFLAG_FALSE(uname)						\
 	TESTSETFLAG_FALSE(uname) TESTCLEARFLAG_FALSE(uname)
 
 __PAGEFLAG(Locked, locked, PF_NO_TAIL)
+#ifdef CONFIG_NUMA
+PAGEFLAG(Waiters, waiters, PF_NO_TAIL)
+#else
+PAGEFLAG_TRUE(Waiters);
+#endif
 PAGEFLAG(Error, error, PF_NO_COMPOUND) TESTCLEARFLAG(Error, error, PF_NO_COMPOUND)
 PAGEFLAG(Referenced, referenced, PF_HEAD)
 	TESTCLEARFLAG(Referenced, referenced, PF_HEAD)
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -427,7 +427,7 @@ extern void __lock_page(struct page *pag
 extern int __lock_page_killable(struct page *page);
 extern int __lock_page_or_retry(struct page *page, struct mm_struct *mm,
 				unsigned int flags);
-extern void unlock_page(struct page *page);
+extern void __unlock_page(struct page *page);
 
 static inline int trylock_page(struct page *page)
 {
@@ -458,6 +458,20 @@ static inline int lock_page_killable(str
 	return 0;
 }
 
+static inline void unlock_page(struct page *page)
+{
+	page = compound_head(page);
+	VM_BUG_ON_PAGE(!PageLocked(page), page);
+	clear_bit_unlock(PG_locked, &page->flags);
+	/*
+	 * Since PG_locked and PG_waiters are in the same word, Program-Order
+	 * ensures the load of PG_waiters must not observe a value earlier
+	 * than our clear_bit() store.
+	 */
+	if (PageWaiters(page))
+		__unlock_page(page);
+}
+
 /*
  * lock_page_or_retry - Lock the page, unless this would block and the
  * caller indicated that it can handle a retry.
@@ -482,11 +496,11 @@ extern int wait_on_page_bit_killable(str
 extern int wait_on_page_bit_killable_timeout(struct page *page,
 					     int bit_nr, unsigned long timeout);
 
+extern int wait_on_page_lock(struct page *page, int mode);
+
 static inline int wait_on_page_locked_killable(struct page *page)
 {
-	if (!PageLocked(page))
-		return 0;
-	return wait_on_page_bit_killable(compound_head(page), PG_locked);
+	return wait_on_page_lock(page, TASK_KILLABLE);
 }
 
 extern wait_queue_head_t *page_waitqueue(struct page *page);
@@ -504,8 +518,7 @@ static inline void wake_up_page(struct p
  */
 static inline void wait_on_page_locked(struct page *page)
 {
-	if (PageLocked(page))
-		wait_on_page_bit(compound_head(page), PG_locked);
+	wait_on_page_lock(page, TASK_UNINTERRUPTIBLE);
 }
 
 /* 
--- a/include/trace/events/mmflags.h
+++ b/include/trace/events/mmflags.h
@@ -55,6 +55,12 @@
 	__def_gfpflag_names						\
 	) : "none"
 
+#ifdef CONFIG_NUMA
+#define IF_HAVE_PG_WAITERS(flag,string) ,{1UL << flag, string}
+#else
+#define IF_HAVE_PG_WAITERS(flag,string)
+#endif
+
 #ifdef CONFIG_MMU
 #define IF_HAVE_PG_MLOCK(flag,string) ,{1UL << flag, string}
 #else
@@ -81,6 +87,7 @@
 
 #define __def_pageflag_names						\
 	{1UL << PG_locked,		"locked"	},		\
+IF_HAVE_PG_WAITERS(PG_waiters,		"waiters"	)		\
 	{1UL << PG_error,		"error"		},		\
 	{1UL << PG_referenced,		"referenced"	},		\
 	{1UL << PG_uptodate,		"uptodate"	},		\
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -860,15 +860,30 @@ EXPORT_SYMBOL_GPL(add_page_wait_queue);
  * The mb is necessary to enforce ordering between the clear_bit and the read
  * of the waitqueue (to avoid SMP races with a parallel wait_on_page_locked()).
  */
-void unlock_page(struct page *page)
+void __unlock_page(struct page *page)
 {
-	page = compound_head(page);
-	VM_BUG_ON_PAGE(!PageLocked(page), page);
-	clear_bit_unlock(PG_locked, &page->flags);
-	smp_mb__after_atomic();
-	wake_up_page(page, PG_locked);
+	wait_queue_head_t *wq = page_waitqueue(page);
+	unsigned long flags;
+
+	spin_lock_irqsave(&wq->lock, flags);
+	if (waitqueue_active(wq)) {
+		struct wait_bit_key key =
+			__WAIT_BIT_KEY_INITIALIZER(&page->flags, PG_locked);
+
+		__wake_up_locked_key(wq, TASK_NORMAL, &key);
+	} else {
+		/*
+		 * We need to do ClearPageWaiters() under wq->lock such that
+		 * we serialize against prepare_to_wait() adding waiters and
+		 * setting task_struct::state.
+		 *
+		 * See lock_page_wait().
+		 */
+		ClearPageWaiters(page);
+	}
+	spin_unlock_irqrestore(&wq->lock, flags);
 }
-EXPORT_SYMBOL(unlock_page);
+EXPORT_SYMBOL(__unlock_page);
 
 /**
  * end_page_writeback - end writeback against a page
@@ -921,6 +936,55 @@ void page_endio(struct page *page, bool
 }
 EXPORT_SYMBOL_GPL(page_endio);
 
+static int lock_page_wait(struct wait_bit_key *word, int mode)
+{
+	struct page *page = container_of(word->flags, struct page, flags);
+
+	/*
+	 * We cannot go sleep without having PG_waiters set. This would mean
+	 * nobody would issue a wakeup and we'd be stuck.
+	 */
+	if (!PageWaiters(page)) {
+
+		/*
+		 * There are two orderings of importance:
+		 *
+		 * 1)
+		 *
+		 *  [unlock]			[wait]
+		 *
+		 *  clear PG_locked		set PG_waiters
+		 *  test  PG_waiters		test (and-set) PG_locked
+		 *
+		 * Since these are in the same word, and the clear/set
+		 * operation are atomic, they are ordered against one another.
+		 * Program-Order further constraints a CPU from speculating the
+		 * later load to not be earlier than the RmW. So this doesn't
+		 * need an explicit barrier. Also see unlock_page().
+		 *
+		 * 2)
+		 *
+		 *  [unlock]			[wait]
+		 *
+		 *  LOCK wq->lock		LOCK wq->lock
+		 *    __wake_up_locked ||	  list-add
+		 *    clear PG_waiters		set_current_state()
+		 *  UNLOCK wq->lock		UNLOCK wq->lock
+		 *				set PG_waiters
+		 *
+		 * Since we're added to the waitqueue, we cannot get
+		 * PG_waiters cleared without also getting TASK_RUNNING set,
+		 * which will then void the schedule() call and we'll loop.
+		 * Here wq->lock is sufficient ordering. See __unlock_page().
+		 */
+		SetPageWaiters(page);
+
+		return 0;
+	}
+
+	return bit_wait_io(word, mode);
+}
+
 /**
  * __lock_page - get a lock on the page, assuming we need to sleep to get it
  * @page: the page to lock
@@ -930,7 +994,7 @@ void __lock_page(struct page *page)
 	struct page *page_head = compound_head(page);
 	DEFINE_WAIT_BIT(wait, &page_head->flags, PG_locked);
 
-	__wait_on_bit_lock(page_waitqueue(page_head), &wait, bit_wait_io,
+	__wait_on_bit_lock(page_waitqueue(page_head), &wait, lock_page_wait,
 							TASK_UNINTERRUPTIBLE);
 }
 EXPORT_SYMBOL(__lock_page);
@@ -941,10 +1005,22 @@ int __lock_page_killable(struct page *pa
 	DEFINE_WAIT_BIT(wait, &page_head->flags, PG_locked);
 
 	return __wait_on_bit_lock(page_waitqueue(page_head), &wait,
-					bit_wait_io, TASK_KILLABLE);
+					lock_page_wait, TASK_KILLABLE);
 }
 EXPORT_SYMBOL_GPL(__lock_page_killable);
 
+int wait_on_page_lock(struct page *page, int mode)
+{
+	struct page __always_unused *__page = (page = compound_head(page));
+	DEFINE_WAIT_BIT(wait, &page->flags, PG_locked);
+
+	if (!PageLocked(page))
+		return 0;
+
+	return __wait_on_bit(page_waitqueue(page), &wait, lock_page_wait, mode);
+}
+EXPORT_SYMBOL(wait_on_page_lock);
+
 /*
  * Return values:
  * 1 - page is locked; mmap_sem is still held.

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web