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

[John Hubbard <jhubbard@nvidia.com>]

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

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