Subject: Re: [RFC 08/11] mm: make ttu's return boolean
To: Minchan Kim <minchan@kernel.org>
References: <1488436765-32350-1-git-send-email-minchan@kernel.org>
 <1488436765-32350-9-git-send-email-minchan@kernel.org>
 <70f60783-e098-c1a9-11b4-544530bcd809@nvidia.com> <20170309063721.GC854@bbox>
CC: Andrew Morton <akpm@linux-foundation.org>, <kernel-team@lge.com>,
        <linux-mm@kvack.org>, <linux-kernel@vger.kernel.org>,
        Johannes Weiner <hannes@cmpxchg.org>, Michal Hocko <mhocko@suse.com>,
        "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
From: John Hubbard <jhubbard@nvidia.com>
Message-ID: <49da6c96-387f-5931-eddb-cb6414631877@nvidia.com>
Date: Wed, 8 Mar 2017 22:46:40 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.7.1
MIME-Version: 1.0
In-Reply-To: <20170309063721.GC854@bbox>
Content-Type: text/plain; charset="windows-1252"; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2035
Lines: 74

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(&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.

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