Re: CONFIG_VMAP_STACK, on-stack struct, and wake_up_bit

[Mel Gorman <mgorman@techsingularity.net>]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>