From: Linus Torvalds <torvalds@linux-foundation.org> To: Nicholas Piggin <npiggin@gmail.com> 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: Wed, 28 Dec 2016 11:17:00 -0800 [thread overview] Message-ID: <CA+55aFz-evT+NiZY0GhO719M+=u==TbCqxTJTjp+pJevhDnRrw@mail.gmail.com> (raw) In-Reply-To: <20161228135358.59f47204@roar.ozlabs.ibm.com> On Tue, Dec 27, 2016 at 7:53 PM, Nicholas Piggin <npiggin@gmail.com> wrote: >> >> Yeah, that patch is disgusting, and doesn't even help x86. > > No, although it would help some cases (but granted the bitops tend to > be problematic in this regard). To be clear I'm not asking to merge it, > just wondered your opinion. (We need something more for unlock_page > anyway because the memory barrier in the way). The thing is, the patch seems pointless anyway. The "add_return()" kind of cases already return the value, so any code that cares can just use that. And the other cases are downright incorrect, like the removal of "volatile" from the bit test ops. >> It also >> depends on the compiler doing the right thing in ways that are not >> obviously true. > > Can you elaborate on this? GCC will do the optimization (modulo a > regression https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77647) So the removal of volatile is just one example of that. You're basically forcing magical side effects. I've never seen this trick _documented_, and quite frankly, the gcc people have had a history of changing their own documentation when it came to their own extensions (ie they've changed how inline functions work etc). But I also worry about interactions with different gcc versions, or with the LLVM people who try to build the kernel with non-gcc compilers. Finally, it fundamentally can't work on x86 anyway, except for the add-return type of operations, which by definitions are pointless (see above). So everything just screams "this is a horrible approach" to me. > Patch seems okay, but it's kind of a horrible primitive. What if you > did clear_bit_unlock_and_test_bit, which does a __builtin_constant_p > test on the bit numbers and if they are < 7 and == 7, then do the > fastpath? So the problem with that is that it makes no sense *except* in the case where the bit is 7. So why add a "generic" function for something that really isn't generic? I agree that it's a hacky interface, but I also happen to believe that being explicit about what you are actually doing causes less pain. It's not magical, and it's not subtle. There's no question about what it does behind your back, and people won't use it by mistake in the wrong context where it doesn't actually work any better than just doing the obvious thing. Linus
WARNING: multiple messages have this Message-ID (diff)
From: Linus Torvalds <torvalds@linux-foundation.org> To: Nicholas Piggin <npiggin@gmail.com> 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: Wed, 28 Dec 2016 11:17:00 -0800 [thread overview] Message-ID: <CA+55aFz-evT+NiZY0GhO719M+=u==TbCqxTJTjp+pJevhDnRrw@mail.gmail.com> (raw) In-Reply-To: <20161228135358.59f47204@roar.ozlabs.ibm.com> On Tue, Dec 27, 2016 at 7:53 PM, Nicholas Piggin <npiggin@gmail.com> wrote: >> >> Yeah, that patch is disgusting, and doesn't even help x86. > > No, although it would help some cases (but granted the bitops tend to > be problematic in this regard). To be clear I'm not asking to merge it, > just wondered your opinion. (We need something more for unlock_page > anyway because the memory barrier in the way). The thing is, the patch seems pointless anyway. The "add_return()" kind of cases already return the value, so any code that cares can just use that. And the other cases are downright incorrect, like the removal of "volatile" from the bit test ops. >> It also >> depends on the compiler doing the right thing in ways that are not >> obviously true. > > Can you elaborate on this? GCC will do the optimization (modulo a > regression https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77647) So the removal of volatile is just one example of that. You're basically forcing magical side effects. I've never seen this trick _documented_, and quite frankly, the gcc people have had a history of changing their own documentation when it came to their own extensions (ie they've changed how inline functions work etc). But I also worry about interactions with different gcc versions, or with the LLVM people who try to build the kernel with non-gcc compilers. Finally, it fundamentally can't work on x86 anyway, except for the add-return type of operations, which by definitions are pointless (see above). So everything just screams "this is a horrible approach" to me. > Patch seems okay, but it's kind of a horrible primitive. What if you > did clear_bit_unlock_and_test_bit, which does a __builtin_constant_p > test on the bit numbers and if they are < 7 and == 7, then do the > fastpath? So the problem with that is that it makes no sense *except* in the case where the bit is 7. So why add a "generic" function for something that really isn't generic? I agree that it's a hacky interface, but I also happen to believe that being explicit about what you are actually doing causes less pain. It's not magical, and it's not subtle. There's no question about what it does behind your back, and people won't use it by mistake in the wrong context where it doesn't actually work any better than just doing the obvious thing. Linus -- 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>
next prev parent reply other threads:[~2016-12-28 19:17 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 [this message] 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 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='CA+55aFz-evT+NiZY0GhO719M+=u==TbCqxTJTjp+pJevhDnRrw@mail.gmail.com' \ --to=torvalds@linux-foundation.org \ --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=npiggin@gmail.com \ --cc=peterz@infradead.org \ --cc=rpeterso@redhat.com \ --cc=swhiteho@redhat.com \ /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: linkBe 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.