linux-kernel - Re: [PATCH] lowmemorykiller: prevent multiple instances of low memory killer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 16 Apr 2013 12:59:51 +0200
From:	Oskar Andero <oskar.andero@...ymobile.com>
To:	Dan Carpenter <dan.carpenter@...cle.com>
CC:	David Rientjes <rientjes@...gle.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
	Brian Swetland <swetland@...gle.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Dolkow, Snild" <Snild.Dolkow@...ymobile.com>,
	"Lekanovic, Radovan" <Radovan.Lekanovic@...ymobile.com>
Subject: Re: [PATCH] lowmemorykiller: prevent multiple instances of low
 memory killer

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/