Re: [PATCH] lowmemorykiller: prevent multiple instances of lowmemory killer

[Oskar Andero]

On 08:19 Tue 16 Apr     , Dan Carpenter wrote:
> On Mon, Apr 15, 2013 at 04:11:18PM -0700, David Rientjes wrote:
> > On Mon, 15 Apr 2013, Greg Kroah-Hartman wrote:
> > 
> > > > The positive numbers are used to return information on the remaining
> > > > cache size (again, see the comment I pasted above). We could use
> > > > -EBUSY, but we'd have to change vmscan.c, which checks specifically
> > > > for -1. I can't see a technical reason why -EBUSY couldn't have been
> > > > chosen instead, but there's also no real reason to change it now.
> > > 
> > > If it's not the correct thing to do, sure we can change it, just send a
> > > patch.  It makes way more sense than some random -1 return value to me.
> > > 
> > > Care to send a series of patches fixing this up properly?
> > > 
> > 
> > The comment in shrinker.h is misleading, not the source code.
> > do_shrinker_shrink() will fail for anything negative and 0.
> 
> The comment is correct.  The only acceptable negative return is -1.
> Look at the second time do_shrinker_shrink() is called from
> shrink_slab().
> 
>    283                  while (total_scan >= batch_size) {
>    284                          int nr_before;
>    285  
>    286                          nr_before = do_shrinker_shrink(shrinker, shrink, 0);
>    287                          shrink_ret = do_shrinker_shrink(shrinker, shrink,
>    288                                                          batch_size);
>    289                          if (shrink_ret == -1)
>    290                                  break;
>    291                          if (shrink_ret < nr_before)
>    292                                  ret += nr_before - shrink_ret;
>    293                          count_vm_events(SLABS_SCANNED, batch_size);

Yes, the comment is correct with what is implemented in the code, but
that doesn't mean the code is right. IMHO, relaying on magical numbers is highly
questionable coding style.

If there are no objections I will prepare a patch-set and let's discuss it from there.

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