Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1593963 > unrolled thread

[PATCH] mm: Do not use double negation for testing page flags

Back to article view | Back to linux.kernel

Contents

  [PATCH] mm: Do not use double negation for testing page flags Minchan Kim <minchan@kernel.org> - 2017-03-07 09:10 +0100
    Re: [PATCH] mm: Do not use double negation for testing page flags Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-03-07 17:10 +0100
      Re: [PATCH] mm: Do not use double negation for testing page flags Minchan Kim <minchan@kernel.org> - 2017-03-08 06:50 +0100
        Re: [PATCH] mm: Do not use double negation for testing page flags Vlastimil Babka <vbabka@suse.cz> - 2017-03-08 10:10 +0100
          Re: [PATCH] mm: Do not use double negation for testing page flags Michal Hocko <mhocko@kernel.org> - 2017-03-08 11:00 +0100
          Re: [PATCH] mm: Do not use double negation for testing page flags Minchan Kim <minchan@kernel.org> - 2017-03-09 07:50 +0100
    Re: [PATCH] mm: Do not use double negation for testing page flags Johannes Weiner <hannes@cmpxchg.org> - 2017-03-07 18:50 +0100

#1593963 — [PATCH] mm: Do not use double negation for testing page flags

With the discussion[1], I found it seems there are every PageFlags
functions return bool at this moment so we don't need double
negation any more.
Although it's not a problem to keep it, it makes future users
confused to use dobule negation for them, too.

Remove such possibility.

[1] https://marc.info/?l=linux-kernel&m=148881578820434

Frankly sepaking, I like every PageFlags return bool instead of int.
It will make it clear. AFAIR, Chen Gang had tried it but don't know
why it was not merged at that time.

http://lkml.kernel.org/r/1469336184-1904-1-git-send-email-chengang@emindsoft.com.cn

Cc: Vlastimil Vlastimil Babka <vbabka@suse.cz>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Kirill A. Shutemov <kirill@shutemov.name>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Chen Gang <gang.chen.5i5j@gmail.com>
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 mm/khugepaged.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 88e4b17..7cb9c88 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -548,7 +548,7 @@ static int __collapse_huge_page_isolate(struct vm_area_struct *vma,
 		 * The page must only be referenced by the scanned process
 		 * and page swap cache.
 		 */
-		if (page_count(page) != 1 + !!PageSwapCache(page)) {
+		if (page_count(page) != 1 + PageSwapCache(page)) {
 			unlock_page(page);
 			result = SCAN_PAGE_COUNT;
 			goto out;
@@ -1181,7 +1181,7 @@ static int khugepaged_scan_pmd(struct mm_struct *mm,
 		 * The page must only be referenced by the scanned process
 		 * and page swap cache.
 		 */
-		if (page_count(page) != 1 + !!PageSwapCache(page)) {
+		if (page_count(page) != 1 + PageSwapCache(page)) {
 			result = SCAN_PAGE_COUNT;
 			goto out_unmap;
 		}
-- 
2.7.4

[toc] | [next] | [standalone]

#1594389

On 03/07/2017 12:06 PM, Minchan Kim wrote:
> With the discussion[1], I found it seems there are every PageFlags
> functions return bool at this moment so we don't need double
> negation any more.
> Although it's not a problem to keep it, it makes future users
> confused to use dobule negation for them, too.
> 
> Remove such possibility.

A quick search of '!!Page' in the source tree does not show any other
place having this double negation. So I guess this is all which need
to be fixed.

[toc] | [prev] | [next] | [standalone]

#1594867

Hi Anshuman,

On Tue, Mar 07, 2017 at 09:31:18PM +0530, Anshuman Khandual wrote:
> On 03/07/2017 12:06 PM, Minchan Kim wrote:
> > With the discussion[1], I found it seems there are every PageFlags
> > functions return bool at this moment so we don't need double
> > negation any more.
> > Although it's not a problem to keep it, it makes future users
> > confused to use dobule negation for them, too.
> > 
> > Remove such possibility.
> 
> A quick search of '!!Page' in the source tree does not show any other
> place having this double negation. So I guess this is all which need
> to be fixed.

Yeb. That's the why my patch includes only khugepagd part but my
concern is PageFlags returns int type not boolean so user might
be confused easily and tempted to use dobule negation.

Other side is they who create new custom PageXXX(e.g., PageMovable)
should keep it in mind that they should return 0 or 1 although
fucntion prototype's return value is int type. It shouldn't be
documented nowhere. Although we can add a little description
somewhere in page-flags.h, I believe changing to boolean is more
clear/not-error-prone so Chen's work is enough worth, I think.

[toc] | [prev] | [next] | [standalone]

#1594957

On 03/08/2017 06:25 AM, Minchan Kim wrote:
> Hi Anshuman,
> 
> On Tue, Mar 07, 2017 at 09:31:18PM +0530, Anshuman Khandual wrote:
>> On 03/07/2017 12:06 PM, Minchan Kim wrote:
>>> With the discussion[1], I found it seems there are every PageFlags
>>> functions return bool at this moment so we don't need double
>>> negation any more.
>>> Although it's not a problem to keep it, it makes future users
>>> confused to use dobule negation for them, too.
>>>
>>> Remove such possibility.
>>
>> A quick search of '!!Page' in the source tree does not show any other
>> place having this double negation. So I guess this is all which need
>> to be fixed.
> 
> Yeb. That's the why my patch includes only khugepagd part but my
> concern is PageFlags returns int type not boolean so user might
> be confused easily and tempted to use dobule negation.
> 
> Other side is they who create new custom PageXXX(e.g., PageMovable)
> should keep it in mind that they should return 0 or 1 although
> fucntion prototype's return value is int type.

> It shouldn't be
> documented nowhere.

Was this double negation intentional? :P

> Although we can add a little description
> somewhere in page-flags.h, I believe changing to boolean is more
> clear/not-error-prone so Chen's work is enough worth, I think.

Agree, unless some arches benefit from the int by performance
for some reason (no idea if it's possible).

Anyway, to your original patch:

Acked-by: Vlastimil Babka <vbabka@suse.cz>

[toc] | [prev] | [next] | [standalone]

#1595005

On Wed 08-03-17 08:51:23, Vlastimil Babka wrote:
> On 03/08/2017 06:25 AM, Minchan Kim wrote:
[...]
> > Although we can add a little description
> > somewhere in page-flags.h, I believe changing to boolean is more
> > clear/not-error-prone so Chen's work is enough worth, I think.
> 
> Agree, unless some arches benefit from the int by performance
> for some reason (no idea if it's possible).

I have a vague recollection somebody tried to change this to bool and
the resulting code was larger on some architecture. Do not remember any
details though

Btw. feel free to add
Acked-by: Michal Hocko <mhocko@suse.com>

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1595707

Hi Vlastimil,

On Wed, Mar 08, 2017 at 08:51:23AM +0100, Vlastimil Babka wrote:
> On 03/08/2017 06:25 AM, Minchan Kim wrote:
> > Hi Anshuman,
> > 
> > On Tue, Mar 07, 2017 at 09:31:18PM +0530, Anshuman Khandual wrote:
> >> On 03/07/2017 12:06 PM, Minchan Kim wrote:
> >>> With the discussion[1], I found it seems there are every PageFlags
> >>> functions return bool at this moment so we don't need double
> >>> negation any more.
> >>> Although it's not a problem to keep it, it makes future users
> >>> confused to use dobule negation for them, too.
> >>>
> >>> Remove such possibility.
> >>
> >> A quick search of '!!Page' in the source tree does not show any other
> >> place having this double negation. So I guess this is all which need
> >> to be fixed.
> > 
> > Yeb. That's the why my patch includes only khugepagd part but my
> > concern is PageFlags returns int type not boolean so user might
> > be confused easily and tempted to use dobule negation.
> > 
> > Other side is they who create new custom PageXXX(e.g., PageMovable)
> > should keep it in mind that they should return 0 or 1 although
> > fucntion prototype's return value is int type.
> 
> > It shouldn't be
> > documented nowhere.
> 
> Was this double negation intentional? :P

Nice catch!
It seems you have a crystal ball. ;-)

> 
> > Although we can add a little description
> > somewhere in page-flags.h, I believe changing to boolean is more
> > clear/not-error-prone so Chen's work is enough worth, I think.
> 
> Agree, unless some arches benefit from the int by performance
> for some reason (no idea if it's possible).
> 
> Anyway, to your original patch:
> 
> Acked-by: Vlastimil Babka <vbabka@suse.cz>

Thanks!

[toc] | [prev] | [next] | [standalone]

#1594461

On Tue, Mar 07, 2017 at 03:36:37PM +0900, Minchan Kim wrote:
> With the discussion[1], I found it seems there are every PageFlags
> functions return bool at this moment so we don't need double
> negation any more.
> Although it's not a problem to keep it, it makes future users
> confused to use dobule negation for them, too.
> 
> Remove such possibility.
> 
> [1] https://marc.info/?l=linux-kernel&m=148881578820434
> 
> Frankly sepaking, I like every PageFlags return bool instead of int.
> It will make it clear. AFAIR, Chen Gang had tried it but don't know
> why it was not merged at that time.
> 
> http://lkml.kernel.org/r/1469336184-1904-1-git-send-email-chengang@emindsoft.com.cn
> 
> Cc: Vlastimil Vlastimil Babka <vbabka@suse.cz>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Kirill A. Shutemov <kirill@shutemov.name>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Chen Gang <gang.chen.5i5j@gmail.com>
> Signed-off-by: Minchan Kim <minchan@kernel.org>

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web