Re: [RFC 08/11] mm: make ttu's return boolean

[John Hubbard]

On 03/08/2017 10:37 PM, Minchan Kim wrote:
>[...]


I think it's the matter of taste.

if (try_to_unmap(xxx))
something
else
something

It's perfectly understandable to me. IOW, if try_to_unmap returns true,
it means it did unmap successfully. Otherwise, failed.

IMHO, SWAP_SUCCESS or TTU_RESULT_* seems to be an over-engineering.
If the user want it, user can do it by introducing right variable name
in his context. See below.


I'm OK with that approach. Just something to avoid the "what does !ret mean in this 
function call" is what I was looking for...




[...]

forcekill = PageDirty(hpage) || (flags & MF_MUST_KILL);
-   kill_procs(, forcekill, trapno,
- ret != SWAP_SUCCESS, p, pfn, flags);
+   kill_procs(, forcekill, trapno, !ret , p, pfn, flags);


The kill_procs() invocation was a little more readable before.


Indeed but I think it's not a problem of try_to_unmap but ret variable name
isn't good any more. How about this?

bool unmap_success;

unmap_success = try_to_unmap(hpage, ttu);

..

kill_procs(, forcekill, trapno, !unmap_success , p, pfn, flags);

..

return unmap_success;

My point is user can introduce whatever variable name depends on his
context. No need to make return variable complicated, IMHO.


Yes, the local variable basically achieves what I was hoping for, so sure, works for 
me.



[...]

-   case SWAP_FAIL:


Again: the SWAP_FAIL makes it crystal clear which case we're in.


To me, I don't feel it.
To me, below is perfectly understandable.

if (try_to_unmap())
do something

That's why I think it's matter of taste. Okay, I admit I might be
biased, too so I will consider what you suggested if others votes
it.


Yes, if it's really just a matter of taste, then not worth debating. Your change 
above is fine I think.


thanks
john h



Thanks.

Re: [RFC 08/11] mm: make ttu's return boolean

[John Hubbard]

On 03/08/2017 10:37 PM, Minchan Kim wrote:
>[...]


I think it's the matter of taste.

if (try_to_unmap(xxx))
something
else
something

It's perfectly understandable to me. IOW, if try_to_unmap returns true,
it means it did unmap successfully. Otherwise, failed.

IMHO, SWAP_SUCCESS or TTU_RESULT_* seems to be an over-engineering.
If the user want it, user can do it by introducing right variable name
in his context. See below.


I'm OK with that approach. Just something to avoid the "what does !ret mean in this 
function call" is what I was looking for...




[...]

forcekill = PageDirty(hpage) || (flags & MF_MUST_KILL);
-   kill_procs(, forcekill, trapno,
- ret != SWAP_SUCCESS, p, pfn, flags);
+   kill_procs(, forcekill, trapno, !ret , p, pfn, flags);


The kill_procs() invocation was a little more readable before.


Indeed but I think it's not a problem of try_to_unmap but ret variable name
isn't good any more. How about this?

bool unmap_success;

unmap_success = try_to_unmap(hpage, ttu);

..

kill_procs(, forcekill, trapno, !unmap_success , p, pfn, flags);

..

return unmap_success;

My point is user can introduce whatever variable name depends on his
context. No need to make return variable complicated, IMHO.


Yes, the local variable basically achieves what I was hoping for, so sure, works for 
me.



[...]

-   case SWAP_FAIL:


Again: the SWAP_FAIL makes it crystal clear which case we're in.


To me, I don't feel it.
To me, below is perfectly understandable.

if (try_to_unmap())
do something

That's why I think it's matter of taste. Okay, I admit I might be
biased, too so I will consider what you suggested if others votes
it.


Yes, if it's really just a matter of taste, then not worth debating. Your change 
above is fine I think.


thanks
john h



Thanks.

Re: [RFC 08/11] mm: make ttu's return boolean

[Minchan Kim]

Hi John,

On Tue, Mar 07, 2017 at 11:13:26PM -0800, John Hubbard wrote:
> On 03/01/2017 10:39 PM, Minchan Kim wrote:
> >try_to_unmap returns SWAP_SUCCESS or SWAP_FAIL so it's suitable for
> >boolean return. This patch changes it.
> 
> Hi Minchan,
> 
> So, up until this patch, I definitely like the cleanup, because as you
> observed, the return values didn't need so many different values. However,
> at this point, I think you should stop, and keep the SWAP_SUCCESS and
> SWAP_FAIL (or maybe even rename them to UNMAP_* or TTU_RESULT_*, to match
> their functions' names better), because removing them makes the code
> considerably less readable.
> 
> And since this is billed as a cleanup, we care here, even though this is a
> minor point. :)
> 
> Bool return values are sometimes perfect, such as when asking a question:
> 
>bool mode_changed = needs_modeset(crtc_state);
> 
> The above is very nice. However, for returning success or failure, bools are
> not as nice, because *usually* success == true, except when you use the
> errno-based system, in which success == 0 (which would translate to false,
> if you mistakenly treated it as a bool). That leads to the reader having to
> remember which system is in use, usually with no visual cues to help.

I think it's the matter of taste.

if (try_to_unmap(xxx))
something
else
something

It's perfectly understandable to me. IOW, if try_to_unmap returns true,
it means it did unmap successfully. Otherwise, failed.

IMHO, SWAP_SUCCESS or TTU_RESULT_* seems to be an over-engineering.
If the user want it, user can do it by introducing right variable name
in his context. See below.

> 
> >
> [...]
> > if (PageSwapCache(p)) {
> >@@ -971,7 +971,7 @@ static int hwpoison_user_mappings(struct page *p, 
> >unsigned long pfn,
> > collect_procs(hpage, , flags & MF_ACTION_REQUIRED);
> >
> > ret = try_to_unmap(hpage, ttu);
> >-if (ret != SWAP_SUCCESS)
> >+if (!ret)
> > pr_err("Memory failure: %#lx: failed to unmap page 
> > (mapcount=%d)\n",
> >pfn, page_mapcount(hpage));
> >
> >@@ -986,8 +986,7 @@ static int hwpoison_user_mappings(struct page *p, 
> >unsigned long pfn,
> >  * any accesses to the poisoned memory.
> >  */
> > forcekill = PageDirty(hpage) || (flags & MF_MUST_KILL);
> >-kill_procs(, forcekill, trapno,
> >-  ret != SWAP_SUCCESS, p, pfn, flags);
> >+kill_procs(, forcekill, trapno, !ret , p, pfn, flags);
> 
> The kill_procs() invocation was a little more readable before.

Indeed but I think it's not a problem of try_to_unmap but ret variable name
isn't good any more. How about this?

bool unmap_success;

unmap_success = try_to_unmap(hpage, ttu);

..

kill_procs(, forcekill, trapno, !unmap_success , p, pfn, flags);

..

return unmap_success;

My point is user can introduce whatever variable name depends on his
context. No need to make return variable complicated, IMHO.

> 
> >
> [...]
> >diff --git a/mm/vmscan.c b/mm/vmscan.c
> >index 170c61f..e4b74f1 100644
> >--- a/mm/vmscan.c
> >+++ b/mm/vmscan.c
> >@@ -966,7 +966,6 @@ static unsigned long shrink_page_list(struct list_head 
> >*page_list,
> > int may_enter_fs;
> > enum page_references references = PAGEREF_RECLAIM_CLEAN;
> > bool dirty, writeback;
> >-int ret = SWAP_SUCCESS;
> >
> > cond_resched();
> >
> >@@ -1139,13 +1138,9 @@ static unsigned long shrink_page_list(struct 
> >list_head *page_list,
> >  * processes. Try to unmap it here.
> >  */
> > if (page_mapped(page)) {
> >-switch (ret = try_to_unmap(page,
> >-ttu_flags | TTU_BATCH_FLUSH)) {
> >-case SWAP_FAIL:
> 
> Again: the SWAP_FAIL makes it crystal clear which case we're in.

To me, I don't feel it.
To me, below is perfectly understandable.

if (try_to_unmap())
do something
 
That's why I think it's matter of taste. Okay, I admit I might be
biased, too so I will consider what you suggested if others votes
it.

Thanks.

> 
> I also wonder if UNMAP_FAIL or TTU_RESULT_FAIL is a better name?
> 
> thanks,
> John Hubbard
> NVIDIA
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majord...@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: mailto:"d...@kvack.org;> em...@kvack.org 

Re: [RFC 08/11] mm: make ttu's return boolean

[Minchan Kim]

Hi John,

On Tue, Mar 07, 2017 at 11:13:26PM -0800, John Hubbard wrote:
> On 03/01/2017 10:39 PM, Minchan Kim wrote:
> >try_to_unmap returns SWAP_SUCCESS or SWAP_FAIL so it's suitable for
> >boolean return. This patch changes it.
> 
> Hi Minchan,
> 
> So, up until this patch, I definitely like the cleanup, because as you
> observed, the return values didn't need so many different values. However,
> at this point, I think you should stop, and keep the SWAP_SUCCESS and
> SWAP_FAIL (or maybe even rename them to UNMAP_* or TTU_RESULT_*, to match
> their functions' names better), because removing them makes the code
> considerably less readable.
> 
> And since this is billed as a cleanup, we care here, even though this is a
> minor point. :)
> 
> Bool return values are sometimes perfect, such as when asking a question:
> 
>bool mode_changed = needs_modeset(crtc_state);
> 
> The above is very nice. However, for returning success or failure, bools are
> not as nice, because *usually* success == true, except when you use the
> errno-based system, in which success == 0 (which would translate to false,
> if you mistakenly treated it as a bool). That leads to the reader having to
> remember which system is in use, usually with no visual cues to help.

I think it's the matter of taste.

if (try_to_unmap(xxx))
something
else
something

It's perfectly understandable to me. IOW, if try_to_unmap returns true,
it means it did unmap successfully. Otherwise, failed.

IMHO, SWAP_SUCCESS or TTU_RESULT_* seems to be an over-engineering.
If the user want it, user can do it by introducing right variable name
in his context. See below.

> 
> >
> [...]
> > if (PageSwapCache(p)) {
> >@@ -971,7 +971,7 @@ static int hwpoison_user_mappings(struct page *p, 
> >unsigned long pfn,
> > collect_procs(hpage, , flags & MF_ACTION_REQUIRED);
> >
> > ret = try_to_unmap(hpage, ttu);
> >-if (ret != SWAP_SUCCESS)
> >+if (!ret)
> > pr_err("Memory failure: %#lx: failed to unmap page 
> > (mapcount=%d)\n",
> >pfn, page_mapcount(hpage));
> >
> >@@ -986,8 +986,7 @@ static int hwpoison_user_mappings(struct page *p, 
> >unsigned long pfn,
> >  * any accesses to the poisoned memory.
> >  */
> > forcekill = PageDirty(hpage) || (flags & MF_MUST_KILL);
> >-kill_procs(, forcekill, trapno,
> >-  ret != SWAP_SUCCESS, p, pfn, flags);
> >+kill_procs(, forcekill, trapno, !ret , p, pfn, flags);
> 
> The kill_procs() invocation was a little more readable before.

Indeed but I think it's not a problem of try_to_unmap but ret variable name
isn't good any more. How about this?

bool unmap_success;

unmap_success = try_to_unmap(hpage, ttu);

..

kill_procs(, forcekill, trapno, !unmap_success , p, pfn, flags);

..

return unmap_success;

My point is user can introduce whatever variable name depends on his
context. No need to make return variable complicated, IMHO.

> 
> >
> [...]
> >diff --git a/mm/vmscan.c b/mm/vmscan.c
> >index 170c61f..e4b74f1 100644
> >--- a/mm/vmscan.c
> >+++ b/mm/vmscan.c
> >@@ -966,7 +966,6 @@ static unsigned long shrink_page_list(struct list_head 
> >*page_list,
> > int may_enter_fs;
> > enum page_references references = PAGEREF_RECLAIM_CLEAN;
> > bool dirty, writeback;
> >-int ret = SWAP_SUCCESS;
> >
> > cond_resched();
> >
> >@@ -1139,13 +1138,9 @@ static unsigned long shrink_page_list(struct 
> >list_head *page_list,
> >  * processes. Try to unmap it here.
> >  */
> > if (page_mapped(page)) {
> >-switch (ret = try_to_unmap(page,
> >-ttu_flags | TTU_BATCH_FLUSH)) {
> >-case SWAP_FAIL:
> 
> Again: the SWAP_FAIL makes it crystal clear which case we're in.

To me, I don't feel it.
To me, below is perfectly understandable.

if (try_to_unmap())
do something
 
That's why I think it's matter of taste. Okay, I admit I might be
biased, too so I will consider what you suggested if others votes
it.

Thanks.

> 
> I also wonder if UNMAP_FAIL or TTU_RESULT_FAIL is a better name?
> 
> thanks,
> John Hubbard
> NVIDIA
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majord...@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: mailto:"d...@kvack.org;> em...@kvack.org 

Re: [RFC 08/11] mm: make ttu's return boolean

[John Hubbard]

On 03/01/2017 10:39 PM, Minchan Kim wrote:

try_to_unmap returns SWAP_SUCCESS or SWAP_FAIL so it's suitable for
boolean return. This patch changes it.


Hi Minchan,

So, up until this patch, I definitely like the cleanup, because as you observed, the 
return values didn't need so many different values. However, at this point, I think 
you should stop, and keep the SWAP_SUCCESS and SWAP_FAIL (or maybe even rename them 
to UNMAP_* or TTU_RESULT_*, to match their functions' names better), because 
removing them makes the code considerably less readable.


And since this is billed as a cleanup, we care here, even though this is a minor 
point. :)


Bool return values are sometimes perfect, such as when asking a question:

   bool mode_changed = needs_modeset(crtc_state);

The above is very nice. However, for returning success or failure, bools are not as 
nice, because *usually* success == true, except when you use the errno-based system, 
in which success == 0 (which would translate to false, if you mistakenly treated it 
as a bool). That leads to the reader having to remember which system is in use, 
usually with no visual cues to help.





[...]

if (PageSwapCache(p)) {
@@ -971,7 +971,7 @@ static int hwpoison_user_mappings(struct page *p, unsigned 
long pfn,
collect_procs(hpage, , flags & MF_ACTION_REQUIRED);

ret = try_to_unmap(hpage, ttu);
-   if (ret != SWAP_SUCCESS)
+   if (!ret)
pr_err("Memory failure: %#lx: failed to unmap page 
(mapcount=%d)\n",
   pfn, page_mapcount(hpage));

@@ -986,8 +986,7 @@ static int hwpoison_user_mappings(struct page *p, unsigned 
long pfn,
 * any accesses to the poisoned memory.
 */
forcekill = PageDirty(hpage) || (flags & MF_MUST_KILL);
-   kill_procs(, forcekill, trapno,
- ret != SWAP_SUCCESS, p, pfn, flags);
+   kill_procs(, forcekill, trapno, !ret , p, pfn, flags);


The kill_procs() invocation was a little more readable before.




[...]

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 170c61f..e4b74f1 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -966,7 +966,6 @@ static unsigned long shrink_page_list(struct list_head 
*page_list,
int may_enter_fs;
enum page_references references = PAGEREF_RECLAIM_CLEAN;
bool dirty, writeback;
-   int ret = SWAP_SUCCESS;

cond_resched();

@@ -1139,13 +1138,9 @@ static unsigned long shrink_page_list(struct list_head 
*page_list,
 * processes. Try to unmap it here.
 */
if (page_mapped(page)) {
-   switch (ret = try_to_unmap(page,
-   ttu_flags | TTU_BATCH_FLUSH)) {
-   case SWAP_FAIL:


Again: the SWAP_FAIL makes it crystal clear which case we're in.

I also wonder if UNMAP_FAIL or TTU_RESULT_FAIL is a better name?

thanks,
John Hubbard
NVIDIA

Re: [RFC 08/11] mm: make ttu's return boolean

[John Hubbard]

On 03/01/2017 10:39 PM, Minchan Kim wrote:

try_to_unmap returns SWAP_SUCCESS or SWAP_FAIL so it's suitable for
boolean return. This patch changes it.


Hi Minchan,

So, up until this patch, I definitely like the cleanup, because as you observed, the 
return values didn't need so many different values. However, at this point, I think 
you should stop, and keep the SWAP_SUCCESS and SWAP_FAIL (or maybe even rename them 
to UNMAP_* or TTU_RESULT_*, to match their functions' names better), because 
removing them makes the code considerably less readable.


And since this is billed as a cleanup, we care here, even though this is a minor 
point. :)


Bool return values are sometimes perfect, such as when asking a question:

   bool mode_changed = needs_modeset(crtc_state);

The above is very nice. However, for returning success or failure, bools are not as 
nice, because *usually* success == true, except when you use the errno-based system, 
in which success == 0 (which would translate to false, if you mistakenly treated it 
as a bool). That leads to the reader having to remember which system is in use, 
usually with no visual cues to help.





[...]

if (PageSwapCache(p)) {
@@ -971,7 +971,7 @@ static int hwpoison_user_mappings(struct page *p, unsigned 
long pfn,
collect_procs(hpage, , flags & MF_ACTION_REQUIRED);

ret = try_to_unmap(hpage, ttu);
-   if (ret != SWAP_SUCCESS)
+   if (!ret)
pr_err("Memory failure: %#lx: failed to unmap page 
(mapcount=%d)\n",
   pfn, page_mapcount(hpage));

@@ -986,8 +986,7 @@ static int hwpoison_user_mappings(struct page *p, unsigned 
long pfn,
 * any accesses to the poisoned memory.
 */
forcekill = PageDirty(hpage) || (flags & MF_MUST_KILL);
-   kill_procs(, forcekill, trapno,
- ret != SWAP_SUCCESS, p, pfn, flags);
+   kill_procs(, forcekill, trapno, !ret , p, pfn, flags);


The kill_procs() invocation was a little more readable before.




[...]

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 170c61f..e4b74f1 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -966,7 +966,6 @@ static unsigned long shrink_page_list(struct list_head 
*page_list,
int may_enter_fs;
enum page_references references = PAGEREF_RECLAIM_CLEAN;
bool dirty, writeback;
-   int ret = SWAP_SUCCESS;

cond_resched();

@@ -1139,13 +1138,9 @@ static unsigned long shrink_page_list(struct list_head 
*page_list,
 * processes. Try to unmap it here.
 */
if (page_mapped(page)) {
-   switch (ret = try_to_unmap(page,
-   ttu_flags | TTU_BATCH_FLUSH)) {
-   case SWAP_FAIL:


Again: the SWAP_FAIL makes it crystal clear which case we're in.

I also wonder if UNMAP_FAIL or TTU_RESULT_FAIL is a better name?

thanks,
John Hubbard
NVIDIA

[RFC 08/11] mm: make ttu's return boolean

[Minchan Kim]

try_to_unmap returns SWAP_SUCCESS or SWAP_FAIL so it's suitable for
boolean return. This patch changes it.

Cc: "Kirill A. Shutemov" 
Cc: Naoya Horiguchi 
Signed-off-by: Minchan Kim 
---
 include/linux/rmap.h |  4 ++--
 mm/huge_memory.c |  4 ++--
 mm/memory-failure.c  | 22 ++
 mm/rmap.c|  8 +++-
 mm/vmscan.c  |  7 +--
 5 files changed, 18 insertions(+), 27 deletions(-)

diff --git a/include/linux/rmap.h b/include/linux/rmap.h
index 3630d4d..6028c38 100644
--- a/include/linux/rmap.h
+++ b/include/linux/rmap.h
@@ -191,7 +191,7 @@ static inline void page_dup_rmap(struct page *page, bool 
compound)
 int page_referenced(struct page *, int is_locked,
struct mem_cgroup *memcg, unsigned long *vm_flags);
 
-int try_to_unmap(struct page *, enum ttu_flags flags);
+bool try_to_unmap(struct page *, enum ttu_flags flags);
 
 /* Avoid racy checks */
 #define PVMW_SYNC  (1 << 0)
@@ -281,7 +281,7 @@ static inline int page_referenced(struct page *page, int 
is_locked,
return 0;
 }
 
-#define try_to_unmap(page, refs) SWAP_FAIL
+#define try_to_unmap(page, refs) false
 
 static inline int page_mkclean(struct page *page)
 {
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index fe2ccd4..79ea769 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -2106,7 +2106,7 @@ static void freeze_page(struct page *page)
 {
enum ttu_flags ttu_flags = TTU_IGNORE_MLOCK | TTU_IGNORE_ACCESS |
TTU_RMAP_LOCKED | TTU_SPLIT_HUGE_PMD;
-   int ret;
+   bool ret;
 
VM_BUG_ON_PAGE(!PageHead(page), page);
 
@@ -2114,7 +2114,7 @@ static void freeze_page(struct page *page)
ttu_flags |= TTU_MIGRATION;
 
ret = try_to_unmap(page, ttu_flags);
-   VM_BUG_ON_PAGE(ret != SWAP_SUCCESS, page);
+   VM_BUG_ON_PAGE(!ret, page);
 }
 
 static void unfreeze_page(struct page *page)
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index b78d080..75fcbd8 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -321,7 +321,7 @@ static void add_to_kill(struct task_struct *tsk, struct 
page *p,
  * wrong earlier.
  */
 static void kill_procs(struct list_head *to_kill, int forcekill, int trapno,
- int fail, struct page *page, unsigned long pfn,
+ bool fail, struct page *page, unsigned long pfn,
  int flags)
 {
struct to_kill *tk, *next;
@@ -903,13 +903,13 @@ EXPORT_SYMBOL_GPL(get_hwpoison_page);
  * Do all that is necessary to remove user space mappings. Unmap
  * the pages and send SIGBUS to the processes if the data was dirty.
  */
-static int hwpoison_user_mappings(struct page *p, unsigned long pfn,
+static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
  int trapno, int flags, struct page **hpagep)
 {
enum ttu_flags ttu = TTU_IGNORE_MLOCK | TTU_IGNORE_ACCESS;
struct address_space *mapping;
LIST_HEAD(tokill);
-   int ret;
+   bool ret;
int kill = 1, forcekill;
struct page *hpage = *hpagep;
 
@@ -918,20 +918,20 @@ static int hwpoison_user_mappings(struct page *p, 
unsigned long pfn,
 * other types of pages.
 */
if (PageReserved(p) || PageSlab(p))
-   return SWAP_SUCCESS;
+   return true;
if (!(PageLRU(hpage) || PageHuge(p)))
-   return SWAP_SUCCESS;
+   return true;
 
/*
 * This check implies we don't kill processes if their pages
 * are in the swap cache early. Those are always late kills.
 */
if (!page_mapped(hpage))
-   return SWAP_SUCCESS;
+   return true;
 
if (PageKsm(p)) {
pr_err("Memory failure: %#lx: can't handle KSM pages.\n", pfn);
-   return SWAP_FAIL;
+   return false;
}
 
if (PageSwapCache(p)) {
@@ -971,7 +971,7 @@ static int hwpoison_user_mappings(struct page *p, unsigned 
long pfn,
collect_procs(hpage, , flags & MF_ACTION_REQUIRED);
 
ret = try_to_unmap(hpage, ttu);
-   if (ret != SWAP_SUCCESS)
+   if (!ret)
pr_err("Memory failure: %#lx: failed to unmap page 
(mapcount=%d)\n",
   pfn, page_mapcount(hpage));
 
@@ -986,8 +986,7 @@ static int hwpoison_user_mappings(struct page *p, unsigned 
long pfn,
 * any accesses to the poisoned memory.
 */
forcekill = PageDirty(hpage) || (flags & MF_MUST_KILL);
-   kill_procs(, forcekill, trapno,
- ret != SWAP_SUCCESS, p, pfn, flags);
+   kill_procs(, forcekill, trapno, !ret , p, pfn, flags);
 
return ret;
 }
@@ -1229,8 +1228,7 @@ int memory_failure(unsigned long pfn, int trapno, int 
flags)
 * When the raw error page is thp tail page, hpage points to the 

[RFC 08/11] mm: make ttu's return boolean

[Minchan Kim]

try_to_unmap returns SWAP_SUCCESS or SWAP_FAIL so it's suitable for
boolean return. This patch changes it.

Cc: "Kirill A. Shutemov" 
Cc: Naoya Horiguchi 
Signed-off-by: Minchan Kim 
---
 include/linux/rmap.h |  4 ++--
 mm/huge_memory.c |  4 ++--
 mm/memory-failure.c  | 22 ++
 mm/rmap.c|  8 +++-
 mm/vmscan.c  |  7 +--
 5 files changed, 18 insertions(+), 27 deletions(-)

diff --git a/include/linux/rmap.h b/include/linux/rmap.h
index 3630d4d..6028c38 100644
--- a/include/linux/rmap.h
+++ b/include/linux/rmap.h
@@ -191,7 +191,7 @@ static inline void page_dup_rmap(struct page *page, bool 
compound)
 int page_referenced(struct page *, int is_locked,
struct mem_cgroup *memcg, unsigned long *vm_flags);
 
-int try_to_unmap(struct page *, enum ttu_flags flags);
+bool try_to_unmap(struct page *, enum ttu_flags flags);
 
 /* Avoid racy checks */
 #define PVMW_SYNC  (1 << 0)
@@ -281,7 +281,7 @@ static inline int page_referenced(struct page *page, int 
is_locked,
return 0;
 }
 
-#define try_to_unmap(page, refs) SWAP_FAIL
+#define try_to_unmap(page, refs) false
 
 static inline int page_mkclean(struct page *page)
 {
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index fe2ccd4..79ea769 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -2106,7 +2106,7 @@ static void freeze_page(struct page *page)
 {
enum ttu_flags ttu_flags = TTU_IGNORE_MLOCK | TTU_IGNORE_ACCESS |
TTU_RMAP_LOCKED | TTU_SPLIT_HUGE_PMD;
-   int ret;
+   bool ret;
 
VM_BUG_ON_PAGE(!PageHead(page), page);
 
@@ -2114,7 +2114,7 @@ static void freeze_page(struct page *page)
ttu_flags |= TTU_MIGRATION;
 
ret = try_to_unmap(page, ttu_flags);
-   VM_BUG_ON_PAGE(ret != SWAP_SUCCESS, page);
+   VM_BUG_ON_PAGE(!ret, page);
 }
 
 static void unfreeze_page(struct page *page)
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index b78d080..75fcbd8 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -321,7 +321,7 @@ static void add_to_kill(struct task_struct *tsk, struct 
page *p,
  * wrong earlier.
  */
 static void kill_procs(struct list_head *to_kill, int forcekill, int trapno,
- int fail, struct page *page, unsigned long pfn,
+ bool fail, struct page *page, unsigned long pfn,
  int flags)
 {
struct to_kill *tk, *next;
@@ -903,13 +903,13 @@ EXPORT_SYMBOL_GPL(get_hwpoison_page);
  * Do all that is necessary to remove user space mappings. Unmap
  * the pages and send SIGBUS to the processes if the data was dirty.
  */
-static int hwpoison_user_mappings(struct page *p, unsigned long pfn,
+static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
  int trapno, int flags, struct page **hpagep)
 {
enum ttu_flags ttu = TTU_IGNORE_MLOCK | TTU_IGNORE_ACCESS;
struct address_space *mapping;
LIST_HEAD(tokill);
-   int ret;
+   bool ret;
int kill = 1, forcekill;
struct page *hpage = *hpagep;
 
@@ -918,20 +918,20 @@ static int hwpoison_user_mappings(struct page *p, 
unsigned long pfn,
 * other types of pages.
 */
if (PageReserved(p) || PageSlab(p))
-   return SWAP_SUCCESS;
+   return true;
if (!(PageLRU(hpage) || PageHuge(p)))
-   return SWAP_SUCCESS;
+   return true;
 
/*
 * This check implies we don't kill processes if their pages
 * are in the swap cache early. Those are always late kills.
 */
if (!page_mapped(hpage))
-   return SWAP_SUCCESS;
+   return true;
 
if (PageKsm(p)) {
pr_err("Memory failure: %#lx: can't handle KSM pages.\n", pfn);
-   return SWAP_FAIL;
+   return false;
}
 
if (PageSwapCache(p)) {
@@ -971,7 +971,7 @@ static int hwpoison_user_mappings(struct page *p, unsigned 
long pfn,
collect_procs(hpage, , flags & MF_ACTION_REQUIRED);
 
ret = try_to_unmap(hpage, ttu);
-   if (ret != SWAP_SUCCESS)
+   if (!ret)
pr_err("Memory failure: %#lx: failed to unmap page 
(mapcount=%d)\n",
   pfn, page_mapcount(hpage));
 
@@ -986,8 +986,7 @@ static int hwpoison_user_mappings(struct page *p, unsigned 
long pfn,
 * any accesses to the poisoned memory.
 */
forcekill = PageDirty(hpage) || (flags & MF_MUST_KILL);
-   kill_procs(, forcekill, trapno,
- ret != SWAP_SUCCESS, p, pfn, flags);
+   kill_procs(, forcekill, trapno, !ret , p, pfn, flags);
 
return ret;
 }
@@ -1229,8 +1228,7 @@ int memory_failure(unsigned long pfn, int trapno, int 
flags)
 * When the raw error page is thp tail page, hpage points to the raw
 * page after thp split.
 */
-   if