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, &tokill, 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(&tokill, forcekill, trapno,
> >- ret != SWAP_SUCCESS, p, pfn, flags);
> >+ kill_procs(&tokill, 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(&tokill, 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 majordomo@xxxxxxxxx. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
