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!
-Oskar
Oskar Andero (2):
mm: vmscan: let any negative return value from shrinker mean error
Clean-up shrinker return values
drivers/staging/android/ashmem.c | 2 +-
drivers/staging/zcache/zcache-main.c | 2 +-
fs/gfs2/glock.c | 2 +-
fs/gfs2/quota.c | 2 +-
fs/nfs/dir.c | 2 +-
fs/ubifs/shrinker.c | 2 +-
include/linux/shrinker.h | 5 +++--
mm/vmscan.c | 2 +-
net/sunrpc/auth.c | 2 +-
9 files changed, 11 insertions(+), 10 deletions(-)
--
1.8.1.5
Shrinkers return hardcoded -1 on error. Use errno.h values instead
to add more meaning to the errors.
Cc: Hugh Dickins <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Oskar Andero <[email protected]>
---
drivers/staging/android/ashmem.c | 2 +-
drivers/staging/zcache/zcache-main.c | 2 +-
fs/gfs2/glock.c | 2 +-
fs/gfs2/quota.c | 2 +-
fs/nfs/dir.c | 2 +-
fs/ubifs/shrinker.c | 2 +-
net/sunrpc/auth.c | 2 +-
7 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index e681bdd..1968d2f 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -359,7 +359,7 @@ static int ashmem_shrink(struct shrinker *s, struct shrink_control *sc)
/* We might recurse into filesystem code, so bail out if necessary */
if (sc->nr_to_scan && !(sc->gfp_mask & __GFP_FS))
- return -1;
+ return -EBUSY;
if (!sc->nr_to_scan)
return lru_count;
diff --git a/drivers/staging/zcache/zcache-main.c b/drivers/staging/zcache/zcache-main.c
index 522cb8e..a38532c 100644
--- a/drivers/staging/zcache/zcache-main.c
+++ b/drivers/staging/zcache/zcache-main.c
@@ -1144,7 +1144,7 @@ static int shrink_zcache_memory(struct shrinker *shrink,
struct shrink_control *sc)
{
static bool in_progress;
- int ret = -1;
+ int ret = -EBUSY;
int nr = sc->nr_to_scan;
int nr_evict = 0;
int nr_writeback = 0;
diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index 9435384..401b089 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -1459,7 +1459,7 @@ static int gfs2_shrink_glock_memory(struct shrinker *shrink,
{
if (sc->nr_to_scan) {
if (!(sc->gfp_mask & __GFP_FS))
- return -1;
+ return -EBUSY;
gfs2_scan_glock_lru(sc->nr_to_scan);
}
diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
index c7c840e..14acbb2 100644
--- a/fs/gfs2/quota.c
+++ b/fs/gfs2/quota.c
@@ -85,7 +85,7 @@ int gfs2_shrink_qd_memory(struct shrinker *shrink, struct shrink_control *sc)
goto out;
if (!(sc->gfp_mask & __GFP_FS))
- return -1;
+ return -EBUSY;
spin_lock(&qd_lru_lock);
while (nr_to_scan && !list_empty(&qd_lru_list)) {
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index e093e73..9fee9bc 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1968,7 +1968,7 @@ int nfs_access_cache_shrinker(struct shrinker *shrink,
gfp_t gfp_mask = sc->gfp_mask;
if ((gfp_mask & GFP_KERNEL) != GFP_KERNEL)
- return (nr_to_scan == 0) ? 0 : -1;
+ return (nr_to_scan == 0) ? nr_to_scan : -EBUSY;
spin_lock(&nfs_access_lru_lock);
list_for_each_entry_safe(nfsi, next, &nfs_access_lru_list, access_cache_inode_lru) {
diff --git a/fs/ubifs/shrinker.c b/fs/ubifs/shrinker.c
index 9e1d056..294e685 100644
--- a/fs/ubifs/shrinker.c
+++ b/fs/ubifs/shrinker.c
@@ -316,7 +316,7 @@ int ubifs_shrinker(struct shrinker *shrink, struct shrink_control *sc)
if (!freed && contention) {
dbg_tnc("freed nothing, but contention");
- return -1;
+ return -EBUSY;
}
out:
diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
index ed2fdd2..45faea0 100644
--- a/net/sunrpc/auth.c
+++ b/net/sunrpc/auth.c
@@ -461,7 +461,7 @@ rpcauth_cache_shrinker(struct shrinker *shrink, struct shrink_control *sc)
gfp_t gfp_mask = sc->gfp_mask;
if ((gfp_mask & GFP_KERNEL) != GFP_KERNEL)
- return (nr_to_scan == 0) ? 0 : -1;
+ return (nr_to_scan == 0) ? nr_to_scan : -EBUSY;
if (list_empty(&cred_unused))
return 0;
spin_lock(&rpc_credcache_lock);
--
1.8.1.5
The shrinkers must return -1 to indicate that it is busy. Instead of
relaying on magical numbers, let any negative value indicate error. This
opens up for using the errno.h error codes in the shrinker
implementations.
Cc: Hugh Dickins <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Oskar Andero <[email protected]>
---
include/linux/shrinker.h | 5 +++--
mm/vmscan.c | 2 +-
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
index ac6b8ee..31e9406 100644
--- a/include/linux/shrinker.h
+++ b/include/linux/shrinker.h
@@ -18,8 +18,9 @@ struct shrink_control {
* 'sc' is passed shrink_control which includes a count 'nr_to_scan'
* and a 'gfpmask'. It should look through the least-recently-used
* 'nr_to_scan' entries and attempt to free them up. It should return
- * the number of objects which remain in the cache. If it returns -1, it means
- * it cannot do any scanning at this time (eg. there is a risk of deadlock).
+ * the number of objects which remain in the cache. If it returns a
+ * negative error code, it means it cannot do any scanning at this time
+ * (eg. there is a risk of deadlock).
*
* The 'gfpmask' refers to the allocation we are currently trying to
* fulfil.
diff --git a/mm/vmscan.c b/mm/vmscan.c
index fa6a853..d6ac9a8 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -287,7 +287,7 @@ unsigned long shrink_slab(struct shrink_control *shrink,
nr_before = do_shrinker_shrink(shrinker, shrink, 0);
shrink_ret = do_shrinker_shrink(shrinker, shrink,
batch_size);
- if (shrink_ret == -1)
+ if (shrink_ret < 0)
break;
if (shrink_ret < nr_before)
ret += nr_before - shrink_ret;
--
1.8.1.5
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.
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?
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.
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?
> 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?
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.
> 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.
-Oskar
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)
On 16:18 Wed 15 May , Glauber Costa wrote:
> 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.
>
Sure, maybe errno.h is not the right way to go. So, why not add the #define
instead? E.g. STOP_SHRINKING or something better than -1.
> > 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)
Sorry for being over-zealous about the return codes and I understand
that it is really a minor thing and possibly also a philosophical
question. My only "solid" reasons are unintuiveness and readability.
That is how I came across it in the first place.
If no-one backs me up on this I will drop the second patch and resend
the first patch without RFC prefix.
-Oskar
On 05/15/2013 06:47 PM, Oskar Andero wrote:
> On 16:18 Wed 15 May , Glauber Costa wrote:
>> 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.
>>
>
> Sure, maybe errno.h is not the right way to go. So, why not add the #define
> instead? E.g. STOP_SHRINKING or something better than -1.
>
>>> 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)
>
> Sorry for being over-zealous about the return codes and I understand
> that it is really a minor thing and possibly also a philosophical
> question. My only "solid" reasons are unintuiveness and readability.
> That is how I came across it in the first place.
>
> If no-one backs me up on this I will drop the second patch and resend
> the first patch without RFC prefix.
>
If you are willing to wait a bit until it finally gets merged, please
send it against my memcg.git in kernel.org (branch
kmemcg-lru-shrinkers). I can carry your patch in our series.
On Mon, 13 May 2013 16:16:33 +0200 Oskar Andero <[email protected]> wrote:
> 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?
I don't see much point in it, really. Returning an errno implies that
the errno will eventually be returned to userspace. But that isn't the
case, so such a change is somewhat misleading.
If we want the capability to return more than a binary yes/no message
to callers then yes, we could/should enumerate the shrinker return
values. But as that is a different concept from errnos, it should be
done with a different and shrinker-specific namespace.
On Mon, May 13, 2013 at 04:16:34PM +0200, Oskar Andero wrote:
> The shrinkers must return -1 to indicate that it is busy. Instead of
> relaying on magical numbers, let any negative value indicate error. This
> opens up for using the errno.h error codes in the shrinker
> implementations.
Just what is the shrinker infrastructure supposed to do with a
random error code?
Cheers,
Dave.
--
Dave Chinner
[email protected]
On 01:05 Thu 16 May , Andrew Morton wrote:
> On Mon, 13 May 2013 16:16:33 +0200 Oskar Andero <[email protected]> wrote:
>
> > 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?
>
> I don't see much point in it, really. Returning an errno implies that
> the errno will eventually be returned to userspace. But that isn't the
> case, so such a change is somewhat misleading.
Yes. Glauber Costa pointed that out and I agree - errno.h is probably not
the right way to go.
> If we want the capability to return more than a binary yes/no message
> to callers then yes, we could/should enumerate the shrinker return
> values. But as that is a different concept from errnos, it should be
> done with a different and shrinker-specific namespace.
Agreed, but even if there right now is only a binary return message, is a
hardcoded -1 considered to be acceptable for an interface? IMHO, it is not
very readable nor intuitive for the users of the interface. Why not, as you
mention, add a define or enum in shrinker.h instead, e.g. SHRINKER_STOP or
something.
-Oskar
On 16:49 Wed 15 May , Glauber Costa wrote:
> On 05/15/2013 06:47 PM, Oskar Andero wrote:
> > On 16:18 Wed 15 May , Glauber Costa wrote:
> >> 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.
> >>
> >
> > Sure, maybe errno.h is not the right way to go. So, why not add the #define
> > instead? E.g. STOP_SHRINKING or something better than -1.
> >
> >>> 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)
> >
> > Sorry for being over-zealous about the return codes and I understand
> > that it is really a minor thing and possibly also a philosophical
> > question. My only "solid" reasons are unintuiveness and readability.
> > That is how I came across it in the first place.
> >
> > If no-one backs me up on this I will drop the second patch and resend
> > the first patch without RFC prefix.
> >
> If you are willing to wait a bit until it finally gets merged, please
> send it against my memcg.git in kernel.org (branch
> kmemcg-lru-shrinkers). I can carry your patch in our series.
Alright. I will apply PATCH 1/2 ontop of your kmemcg-lru-shrinker branch
and send it to you offline.
Thanks!
-Oskar
On 05/16/2013 12:20 PM, Oskar Andero wrote:
> On 16:49 Wed 15 May , Glauber Costa wrote:
>> On 05/15/2013 06:47 PM, Oskar Andero wrote:
>>> On 16:18 Wed 15 May , Glauber Costa wrote:
>>>> 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.
>>>>
>>>
>>> Sure, maybe errno.h is not the right way to go. So, why not add the #define
>>> instead? E.g. STOP_SHRINKING or something better than -1.
>>>
>>>>> 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)
>>>
>>> Sorry for being over-zealous about the return codes and I understand
>>> that it is really a minor thing and possibly also a philosophical
>>> question. My only "solid" reasons are unintuiveness and readability.
>>> That is how I came across it in the first place.
>>>
>>> If no-one backs me up on this I will drop the second patch and resend
>>> the first patch without RFC prefix.
>>>
>> If you are willing to wait a bit until it finally gets merged, please
>> send it against my memcg.git in kernel.org (branch
>> kmemcg-lru-shrinkers). I can carry your patch in our series.
>
> Alright. I will apply PATCH 1/2 ontop of your kmemcg-lru-shrinker branch
> and send it to you offline.
>
> Thanks!
>
> -Oskar
>
No need to send it offline.
On Thu, 16 May 2013 09:52:05 +0200 Oskar Andero <[email protected]> wrote:
> > If we want the capability to return more than a binary yes/no message
> > to callers then yes, we could/should enumerate the shrinker return
> > values. But as that is a different concept from errnos, it should be
> > done with a different and shrinker-specific namespace.
>
> Agreed, but even if there right now is only a binary return message, is a
> hardcoded -1 considered to be acceptable for an interface? IMHO, it is not
> very readable nor intuitive for the users of the interface. Why not, as you
> mention, add a define or enum in shrinker.h instead, e.g. SHRINKER_STOP or
> something.
That sounds OK to me.