All of lore.kernel.org
 help / color / mirror / Atom feed
From: Nicholas Piggin <npiggin@gmail.com>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Dave Hansen <dave.hansen@linux.intel.com>,
	Bob Peterson <rpeterso@redhat.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Steven Whitehouse <swhiteho@redhat.com>,
	Andrew Lutomirski <luto@kernel.org>,
	Andreas Gruenbacher <agruenba@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	linux-mm <linux-mm@kvack.org>,
	Mel Gorman <mgorman@techsingularity.net>
Subject: Re: [PATCH 2/2] mm: add PageWaiters indicating tasks are waiting for a page bit
Date: Thu, 29 Dec 2016 15:26:15 +1000	[thread overview]
Message-ID: <20161229152615.2dad5402@roar.ozlabs.ibm.com> (raw)
In-Reply-To: <CA+55aFxGz8R8J9jLvKpLUgyhWVYcgtObhbHBP7eZzZyc05AODw@mail.gmail.com>

On Wed, 28 Dec 2016 20:16:56 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Wed, Dec 28, 2016 at 8:08 PM, Nicholas Piggin <npiggin@gmail.com> wrote:
> >
> > Okay. The name could be a bit better though I think, for readability.
> > Just a BUILD_BUG_ON if it is not constant and correct bit numbers?  
> 
> I have a slightly edited patch - moved the comments around and added
> some new comments (about both the sign bit, but also about how the
> smp_mb() shouldn't be necessary even for the non-atomic fallback).

That's a good point -- they're in the same byte, so all architectures
will be able to avoid the extra barrier regardless of how the
primitives are implemented. Good.

> 
> I also did a BUILD_BUG_ON(), except the other way around - keeping it
> about the sign bit in the byte, just just verifying that yes,
> PG_waiters is that sign bit.

Yep. I still don't like the name, but now you've got PG_waiters
commented there at least. I'll have to live with it.

If we get more cases that want to use a similar function, we might make
a more general primitive for architectures that can optimize these multi
bit ops better than x86. Actually even x86 would prefer to do load ;
cmpxchg rather than bitop ; load for the cases where condition code can't
be used to check result.

> 
> > BTW. I just notice in your patch too that you didn't use "nr" in the
> > generic version.  
> 
> And I fixed that too.
> 
> Of course, I didn't test the changes (apart from building it). But
> I've been running the previous version since yesterday, so far no
> issues.

It looks good to me.

Thanks,
Nick

WARNING: multiple messages have this Message-ID (diff)
From: Nicholas Piggin <npiggin@gmail.com>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Dave Hansen <dave.hansen@linux.intel.com>,
	Bob Peterson <rpeterso@redhat.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Steven Whitehouse <swhiteho@redhat.com>,
	Andrew Lutomirski <luto@kernel.org>,
	Andreas Gruenbacher <agruenba@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	linux-mm <linux-mm@kvack.org>,
	Mel Gorman <mgorman@techsingularity.net>
Subject: Re: [PATCH 2/2] mm: add PageWaiters indicating tasks are waiting for a page bit
Date: Thu, 29 Dec 2016 15:26:15 +1000	[thread overview]
Message-ID: <20161229152615.2dad5402@roar.ozlabs.ibm.com> (raw)
In-Reply-To: <CA+55aFxGz8R8J9jLvKpLUgyhWVYcgtObhbHBP7eZzZyc05AODw@mail.gmail.com>

On Wed, 28 Dec 2016 20:16:56 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Wed, Dec 28, 2016 at 8:08 PM, Nicholas Piggin <npiggin@gmail.com> wrote:
> >
> > Okay. The name could be a bit better though I think, for readability.
> > Just a BUILD_BUG_ON if it is not constant and correct bit numbers?  
> 
> I have a slightly edited patch - moved the comments around and added
> some new comments (about both the sign bit, but also about how the
> smp_mb() shouldn't be necessary even for the non-atomic fallback).

That's a good point -- they're in the same byte, so all architectures
will be able to avoid the extra barrier regardless of how the
primitives are implemented. Good.

> 
> I also did a BUILD_BUG_ON(), except the other way around - keeping it
> about the sign bit in the byte, just just verifying that yes,
> PG_waiters is that sign bit.

Yep. I still don't like the name, but now you've got PG_waiters
commented there at least. I'll have to live with it.

If we get more cases that want to use a similar function, we might make
a more general primitive for architectures that can optimize these multi
bit ops better than x86. Actually even x86 would prefer to do load ;
cmpxchg rather than bitop ; load for the cases where condition code can't
be used to check result.

> 
> > BTW. I just notice in your patch too that you didn't use "nr" in the
> > generic version.  
> 
> And I fixed that too.
> 
> Of course, I didn't test the changes (apart from building it). But
> I've been running the previous version since yesterday, so far no
> issues.

It looks good to me.

Thanks,
Nick

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

  reply	other threads:[~2016-12-29  5:26 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-12-25  3:00 [PATCH 0/2] PageWaiters again Nicholas Piggin
2016-12-25  3:00 ` Nicholas Piggin
2016-12-25  3:00 ` [PATCH 1/2] mm: Use owner_priv bit for PageSwapCache, valid when PageSwapBacked Nicholas Piggin
2016-12-25  3:00   ` Nicholas Piggin
2016-12-25  5:13   ` Hugh Dickins
2016-12-25  5:13     ` Hugh Dickins
2016-12-25  3:00 ` [PATCH 2/2] mm: add PageWaiters indicating tasks are waiting for a page bit Nicholas Piggin
2016-12-25  3:00   ` Nicholas Piggin
2016-12-25 21:51   ` Linus Torvalds
2016-12-25 21:51     ` Linus Torvalds
2016-12-26  1:16     ` Nicholas Piggin
2016-12-26  1:16       ` Nicholas Piggin
2016-12-26 19:07       ` Linus Torvalds
2016-12-26 19:07         ` Linus Torvalds
2016-12-27 11:19         ` Nicholas Piggin
2016-12-27 11:19           ` Nicholas Piggin
2016-12-27 18:58           ` Linus Torvalds
2016-12-27 18:58             ` Linus Torvalds
2016-12-27 19:23             ` Linus Torvalds
2016-12-27 19:23               ` Linus Torvalds
2016-12-27 19:24               ` Linus Torvalds
2016-12-27 19:40                 ` Linus Torvalds
2016-12-27 20:17                   ` Linus Torvalds
2016-12-27 20:17                     ` Linus Torvalds
2016-12-28  3:53             ` Nicholas Piggin
2016-12-28  3:53               ` Nicholas Piggin
2016-12-28 19:17               ` Linus Torvalds
2016-12-28 19:17                 ` Linus Torvalds
2016-12-29  4:08                 ` Nicholas Piggin
2016-12-29  4:08                   ` Nicholas Piggin
2016-12-29  4:16                   ` Linus Torvalds
2016-12-29  5:26                     ` Nicholas Piggin [this message]
2016-12-29  5:26                       ` Nicholas Piggin
2017-01-03 10:24                       ` Mel Gorman
2017-01-03 10:24                         ` Mel Gorman
2017-01-03 12:29                         ` Nicholas Piggin
2017-01-03 12:29                           ` Nicholas Piggin
2017-01-03 17:18                           ` Mel Gorman
2017-01-03 17:18                             ` Mel Gorman
2016-12-29 22:16                     ` [PATCH] mm/filemap: fix parameters to test_bit() Olof Johansson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20161229152615.2dad5402@roar.ozlabs.ibm.com \
    --to=npiggin@gmail.com \
    --cc=agruenba@redhat.com \
    --cc=dave.hansen@linux.intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=luto@kernel.org \
    --cc=mgorman@techsingularity.net \
    --cc=peterz@infradead.org \
    --cc=rpeterso@redhat.com \
    --cc=swhiteho@redhat.com \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.