linux-kernel - Re: CONFIG_VMAP_STACK, on-stack struct, and wake_up

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 27 Oct 2016 10:07:42 +0100
From:   Mel Gorman <mgorman@...hsingularity.net>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Andy Lutomirski <luto@...capital.net>,
        Andreas Gruenbacher <agruenba@...hat.com>,
        Andy Lutomirski <luto@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Bob Peterson <rpeterso@...hat.com>,
        Steven Whitehouse <swhiteho@...hat.com>,
        linux-mm <linux-mm@...ck.org>
Subject: Re: CONFIG_VMAP_STACK, on-stack struct, and wake_up_bit

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