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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
