From mboxrd@z Thu Jan 1 00:00:00 1970 From: Glauber Costa Subject: Re: [PATCH v2 02/28] vmscan: take at least one pass with shrinkers Date: Mon, 8 Apr 2013 13:05:59 +0400 Message-ID: <51628877.5000701@parallels.com> References: <1364548450-28254-1-git-send-email-glommer@parallels.com> <1364548450-28254-3-git-send-email-glommer@parallels.com> <20130408084202.GA21654@lge.com> <51628412.6050803@parallels.com> <20130408090131.GB21654@lge.com> Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Cc: , , , Michal Hocko , Johannes Weiner , , Andrew Morton , Dave Shrinnker , Greg Thelen , , , Theodore Ts'o , Al Viro To: Joonsoo Kim Return-path: Received: from mx2.parallels.com ([199.115.105.18]:39665 "EHLO mx2.parallels.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934547Ab3DHJF0 (ORCPT ); Mon, 8 Apr 2013 05:05:26 -0400 In-Reply-To: <20130408090131.GB21654@lge.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On 04/08/2013 01:01 PM, Joonsoo Kim wrote: > On Mon, Apr 08, 2013 at 12:47:14PM +0400, Glauber Costa wrote: >> On 04/08/2013 12:42 PM, Joonsoo Kim wrote: >>> Hello, Glauber. >>> >>> On Fri, Mar 29, 2013 at 01:13:44PM +0400, Glauber Costa wrote: >>>> In very low free kernel memory situations, it may be the case that we >>>> have less objects to free than our initial batch size. If this is the >>>> case, it is better to shrink those, and open space for the new workload >>>> then to keep them and fail the new allocations. >>>> >>>> More specifically, this happens because we encode this in a loop with >>>> the condition: "while (total_scan >= batch_size)". So if we are in such >>>> a case, we'll not even enter the loop. >>>> >>>> This patch modifies turns it into a do () while {} loop, that will >>>> guarantee that we scan it at least once, while keeping the behaviour >>>> exactly the same for the cases in which total_scan > batch_size. >>> >>> Current user of shrinker not only use their own condition, but also >>> use batch_size and seeks to throttle their behavior. So IMHO, >>> this behavior change is very dangerous to some users. >>> >>> For example, think lowmemorykiller. >>> With this patch, he always kill some process whenever shrink_slab() is >>> called and their low memory condition is satisfied. >>> Before this, total_scan also prevent us to go into lowmemorykiller, so >>> killing innocent process is limited as much as possible. >>> >> shrinking is part of the normal operation of the Linux kernel and >> happens all the time. Not only the call to shrink_slab, but actual >> shrinking of unused objects. >> >> I don't know therefore about any code that would kill process only >> because they have reached shrink_slab. >> >> In normal systems, this loop will be executed many, many times. So we're >> not shrinking *more*, we're just guaranteeing that at least one pass >> will be made. > > This one pass guarantee is a problem for lowmemory killer. > >> Also, anyone looking at this to see if we should kill processes, is a >> lot more likely to kill something if we tried to shrink but didn't, than >> if we successfully shrunk something. > > lowmemory killer is hacky user of shrink_slab interface. Well, it says it all =) In special, I really can't see how, hacky or not, it makes sense to kill a process if we *actually* shrunk memory. Moreover, I don't see the code in drivers/staging/android/lowmemory.c doing anything even remotely close to that. Could you point me to some code that does it ?