Re: [RFC PATCH 0/2] return value from shrinkers

[Glauber Costa]

On 05/15/2013 06:10 PM, Oskar Andero wrote:
> On 17:03 Tue 14 May     , Glauber Costa wrote:
>> On 05/13/2013 06:16 PM, Oskar Andero wrote:
>>> Hi,
>>>
>>> In a previous discussion on lkml it was noted that the shrinkers use the
>>> magic value "-1" to signal that something went wrong.
>>>
>>> This patch-set implements the suggestion of instead using errno.h values
>>> to return something more meaningful.
>>>
>>> The first patch simply changes the check from -1 to any negative value and
>>> updates the comment accordingly.
>>>
>>> The second patch updates the shrinkers to return an errno.h value instead
>>> of -1. Since this one spans over many different areas I need input on what is
>>> a meaningful return value. Right now I used -EBUSY on everything for consitency.
>>>
>>> What do you say? Is this a good idea or does it make no sense at all?
>>>
>>> Thanks!
>>>
>>
>> Right now me and Dave are completely reworking the way shrinkers
>> operate. I suggest, first of all, that you take a look at that cautiously.
> 
> Sounds good. Where can one find the code for that?
> 
linux-mm, linux-fsdevel

Subject is "kmemcg shrinkers", but only the second part is memcg related.

>> On the specifics of what you are doing here, what would be the benefit
>> of returning something other than -1 ? Is there anything we would do
>> differently for a return value lesser than 1?
> 
> Firstly, what bugs me is the magic and unintuitiveness of using -1 rather than a
> more descriptive error code. IMO, even a #define SHRINK_ERROR -1 in some header
> file would be better.
> 
> Expanding the test to <0 will open up for more granular error checks,
> like -EAGAIN, -EBUSY and so on. Currently, they would all be treated the same,
> but maybe in the future we would like to handle them differently?
> 

Then in the future we change it.
This is not a user visible API, we are free to change it at any time,
under any conditions. There is only value in supporting different error
codes if we intend to do something different about it. Otherwise, it is
just churn.

Moreover, -1 does not necessarily mean error. It means "stop shrinking".
There are many non-error conditions in which it could happen.

> Finally, looking at the code:
>                         if (shrink_ret == -1)
>                                 break;
>                         if (shrink_ret < nr_before)
>                                 ret += nr_before - shrink_ret;
> 
> This piece of code will only function if shrink_ret is either greater than zero
> or -1. If shrink_ret is -2 this will lead to undefined behaviour.
> 
Except it never is. But since we are touching this code anyway, I see no
problems in expanding the test. What I don't see the point for, is the
other patch in your series in which you return error codes.

>> So far, shrink_slab behaves the same, you are just expanding the test.
>> If you really want to push this through, I would suggest coming up with
>> a more concrete reason for why this is wanted.
> 
> I don't know how well this patch is aligned with your current rework, but
> based on my comments above, I don't see a reason for not taking it.
> 
I see no objections for PATCH #1 that expands the check, as a cautionary
measure. But I will oppose returning error codes from shrinkers without
a solid reason for doing so (meaning a use case in which we really
threat one of the errors differently)



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/