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

John Hubbard
2017-03-09T06:46:49Z

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.