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

Groups > linux.kernel > #1595704

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

From Minchan Kim <minchan@kernel.org>
Newsgroups linux.kernel
Subject Re: [RFC 08/11] mm: make ttu's return boolean
Date 2017-03-09 07:40 +0100
Message-ID <tiZTY-1bv-11@gated-at.bofh.it> (permalink)
References <tgtbP-7hM-5@gated-at.bofh.it> <tgtbQ-7hM-29@gated-at.bofh.it> <tiFsf-3Rn-19@gated-at.bofh.it>
Organization linux.* mail to news gateway

Show all headers | View raw

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@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>

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[RFC 00/11] make try_to_unmap simple Minchan Kim <minchan@kernel.org> - 2017-03-02 08:20 +0100
  [RFC 01/11] mm: use SWAP_SUCCESS instead of 0 Minchan Kim <minchan@kernel.org> - 2017-03-02 08:20 +0100
    Re: [RFC 01/11] mm: use SWAP_SUCCESS instead of 0 Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-03-02 15:30 +0100
      Re: [RFC 01/11] mm: use SWAP_SUCCESS instead of 0 Minchan Kim <minchan@kernel.org> - 2017-03-03 04:20 +0100
        Re: [RFC 01/11] mm: use SWAP_SUCCESS instead of 0 Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-03-06 10:10 +0100
    Re: [RFC 01/11] mm: use SWAP_SUCCESS instead of 0 "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-03-07 16:00 +0100
      Re: [RFC 01/11] mm: use SWAP_SUCCESS instead of 0 Minchan Kim <minchan@kernel.org> - 2017-03-08 07:50 +0100
  [RFC 09/11] mm: make rmap_walk void function Minchan Kim <minchan@kernel.org> - 2017-03-02 08:20 +0100
  [RFC 08/11] mm: make ttu's return boolean Minchan Kim <minchan@kernel.org> - 2017-03-02 08:20 +0100
    Re: [RFC 08/11] mm: make ttu's return boolean John Hubbard <jhubbard@nvidia.com> - 2017-03-08 09:50 +0100
      Re: [RFC 08/11] mm: make ttu's return boolean Minchan Kim <minchan@kernel.org> - 2017-03-09 07:40 +0100
        Re: [RFC 08/11] mm: make ttu's return boolean John Hubbard <jhubbard@nvidia.com> - 2017-03-09 07:50 +0100
  [RFC 06/11] mm: remove SWAP_MLOCK in ttu Minchan Kim <minchan@kernel.org> - 2017-03-02 08:20 +0100
    Re: [RFC 06/11] mm: remove SWAP_MLOCK in ttu Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-03-03 15:20 +0100
      Re: [RFC 06/11] mm: remove SWAP_MLOCK in ttu Minchan Kim <minchan@kernel.org> - 2017-03-06 03:20 +0100
        Re: [RFC 06/11] mm: remove SWAP_MLOCK in ttu "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-03-07 19:30 +0100
          Re: [RFC 06/11] mm: remove SWAP_MLOCK in ttu Minchan Kim <minchan@kernel.org> - 2017-03-08 08:00 +0100
  [RFC 04/11] mm: remove SWAP_MLOCK check for SWAP_SUCCESS in ttu Minchan Kim <minchan@kernel.org> - 2017-03-02 08:20 +0100
    Re: [RFC 04/11] mm: remove SWAP_MLOCK check for SWAP_SUCCESS in ttu Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-03-02 18:40 +0100
      Re: [RFC 04/11] mm: remove SWAP_MLOCK check for SWAP_SUCCESS in ttu Minchan Kim <minchan@kernel.org> - 2017-03-03 05:30 +0100
    Re: [RFC 04/11] mm: remove SWAP_MLOCK check for SWAP_SUCCESS in ttu "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-03-07 15:30 +0100
      Re: [RFC 04/11] mm: remove SWAP_MLOCK check for SWAP_SUCCESS in ttu Minchan Kim <minchan@kernel.org> - 2017-03-08 07:50 +0100
  [RFC 07/11] mm: remove SWAP_AGAIN in ttu Minchan Kim <minchan@kernel.org> - 2017-03-02 08:20 +0100
    Re: [RFC 07/11] mm: remove SWAP_AGAIN in ttu Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-03-03 17:20 +0100
      Re: [RFC 07/11] mm: remove SWAP_AGAIN in ttu Minchan Kim <minchan@kernel.org> - 2017-03-06 03:40 +0100
  [RFC 02/11] mm: remove unncessary ret in page_referenced Minchan Kim <minchan@kernel.org> - 2017-03-02 08:20 +0100
    Re: [RFC 02/11] mm: remove unncessary ret in page_referenced Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-03-02 19:20 +0100
      Re: [RFC 02/11] mm: remove unncessary ret in page_referenced Minchan Kim <minchan@kernel.org> - 2017-03-03 04:50 +0100
    Re: [RFC 02/11] mm: remove unncessary ret in page_referenced "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-03-07 15:40 +0100
  [RFC 05/11] mm: make the try_to_munlock void function Minchan Kim <minchan@kernel.org> - 2017-03-02 08:20 +0100
    Re: [RFC 05/11] mm: make the try_to_munlock void function Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-03-03 12:50 +0100
      Re: [RFC 05/11] mm: make the try_to_munlock void function Minchan Kim <minchan@kernel.org> - 2017-03-06 03:10 +0100
        Re: [RFC 05/11] mm: make the try_to_munlock void function Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-03-06 13:20 +0100
          Re: [RFC 05/11] mm: make the try_to_munlock void function Minchan Kim <minchan@kernel.org> - 2017-03-07 09:30 +0100
            Re: [RFC 05/11] mm: make the try_to_munlock void function Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-03-07 15:50 +0100
    Re: [RFC 05/11] mm: make the try_to_munlock void function "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-03-07 16:30 +0100
      Re: [RFC 05/11] mm: make the try_to_munlock void function Minchan Kim <minchan@kernel.org> - 2017-03-08 07:50 +0100
  Re: [RFC 00/11] make try_to_unmap simple Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-03-02 15:30 +0100
    Re: [RFC 00/11] make try_to_unmap simple Minchan Kim <minchan@kernel.org> - 2017-03-03 03:50 +0100
      Re: [RFC 00/11] make try_to_unmap simple Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-03-03 10:10 +0100

csiph-web