linux-kernel - Re: [PATCH 1/5] mm: vmscan: shrink all slab objects if tight on memory

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-Id: <20140115012541.ad302526.akpm@linux-foundation.org>
Date:	Wed, 15 Jan 2014 01:25:41 -0800
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Vladimir Davydov <vdavydov@...allels.com>
Cc:	<linux-kernel@...r.kernel.org>, <linux-mm@...ck.org>,
	<devel@...nvz.org>, Mel Gorman <mgorman@...e.de>,
	Michal Hocko <mhocko@...e.cz>,
	Johannes Weiner <hannes@...xchg.org>,
	Rik van Riel <riel@...hat.com>,
	Dave Chinner <dchinner@...hat.com>,
	Glauber Costa <glommer@...il.com>
Subject: Re: [PATCH 1/5] mm: vmscan: shrink all slab objects if tight on
 memory

On Wed, 15 Jan 2014 12:47:35 +0400 Vladimir Davydov <vdavydov@...allels.com> wrote:

> On 01/15/2014 02:14 AM, Andrew Morton wrote:
> > On Tue, 14 Jan 2014 11:23:30 +0400 Vladimir Davydov <vdavydov@...allels.com> wrote:
> >
> >> On 01/14/2014 03:05 AM, Andrew Morton wrote:
> >>> That being said, I think I'll schedule this patch as-is for 3.14.  Can
> >>> you please take a look at implementing the simpler approach, send me
> >>> something for 3.15-rc1?
> >> IMHO the simpler approach (Glauber's patch) is not suitable as is,
> >> because it, in fact, neglects the notion of batch_size when doing low
> >> prio scans, because it calls ->scan() for < batch_size objects even if
> >> the slab has >= batch_size objects while AFAIU it should accumulate a
> >> sufficient number of objects to scan in nr_deferred instead.
> > Well.  If you mean that when nr-objects=large and batch_size=32 and
> > total_scan=33, the patched code will scan 32 objects and then 1 object
> > then yes, that should be fixed.
> 
> I mean if nr_objects=large and batch_size=32 and shrink_slab() is called
> 8 times with total_scan=4, we can either call ->scan() 8 times with
> nr_to_scan=4 (Glauber's patch) or call it only once with nr_to_scan=32
> (that's how it works now). Frankly, after a bit of thinking I am
> starting to doubt that this can affect performance at all provided the
> shrinker is implemented in a sane way, because as you've mentioned
> shrink_slab() is already a slow path. It seems I misunderstood the
> purpose of batch_size initially: I though we need it to limit the number
> of calls to ->scan(), but now I guess the only purpose of it is limiting
> the number of objects scanned in one pass to avoid latency issues.

Actually, the intent of batching is to limit the number of calls to
->scan().  At least, that was the intent when I wrote it!  This is a
good principle and we should keep doing it.  If we're going to send the
CPU away to tread on a pile of cold cachelines, we should make sure
that it does a good amount of work while it's there.

> But
> then another question arises - why do you think the behavior you
> described above (scanning 32 and then 1 object if total_scan=33,
> batch_size=32) is bad?

Yes, it's a bit inefficient but it won't be too bad.  What would be bad
would be to scan a very small number of objects and then to advance to
the next shrinker.

> In other words why can't we make the scan loop
> look like this:
> 
>     while (total_scan > 0) {
>         unsigned long ret;
>         unsigned long nr_to_scan = min(total_scan, batch_size);
> 
>         shrinkctl->nr_to_scan = nr_to_scan;
>         ret = shrinker->scan_objects(shrinker, shrinkctl);
>         if (ret == SHRINK_STOP)
>             break;
>         freed += ret;
> 
>         count_vm_events(SLABS_SCANNED, nr_to_scan);
>         total_scan -= nr_to_scan;
> 
>         cond_resched();
>     }


Well, if we come in here with total_scan=1 then we defeat the original
intent of the batching, don't we?  We end up doing a lot of work just
to scan one object.  So perhaps add something like

	if (total_scan < batch_size && max_pass > batch_size)
		skip the while loop

If we do this, total_scan will be accumulated into nr_deferred, up to
the point where the threshold is exceeded, yes?

All the arithmetic in there hurts my brain and I don't know what values
total_scan typically ends up with.

btw. all that trickery with delta and lru_pages desperately needs
documenting.  What the heck is it intended to do??



We could avoid the "scan 32 then scan just 1" issue with something like

	if (total_scan > batch_size)
		total_scan %= batch_size;

before the loop.  But I expect the effects of that will be unmeasurable
- on average the number of objects which are scanned in the final pass
of the loop will be batch_size/2, yes?  That's still a decent amount.
--
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/