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
