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/[email protected]
Cc: Vlastimil Vlastimil Babka <[email protected]>
Cc: Michal Hocko <[email protected]>
Cc: Kirill A. Shutemov <[email protected]>
Cc: Johannes Weiner <[email protected]>
Cc: Chen Gang <[email protected]>
Signed-off-by: Minchan Kim <[email protected]>
---
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
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.
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/[email protected]
>
> Cc: Vlastimil Vlastimil Babka <[email protected]>
> Cc: Michal Hocko <[email protected]>
> Cc: Kirill A. Shutemov <[email protected]>
> Cc: Johannes Weiner <[email protected]>
> Cc: Chen Gang <[email protected]>
> Signed-off-by: Minchan Kim <[email protected]>
Acked-by: Johannes Weiner <[email protected]>
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.
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 <[email protected]>
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 <[email protected]>
--
Michal Hocko
SUSE Labs
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 <[email protected]>
Thanks!