So I made a note to myself a while back to have a look at what this struct
reclaim_state thing was all about. It turns out it's referenced in WAY too
many places for what it does.
The idea is that during memory reclaim, we want to keep track of how many
pages get freed up by the various slab shrinkers. The memory is eventually
freed in kmem_freepages() and there is not convenient place to store the
number of slab pages we freed. So, at some point in history, we decided to
store it in the current task's task structure. Unfortunately we decided to
store it in a really obfuscated way that made my brain hurt. So I ripped
it out.
The only place that *ever* updates current->reclaim_state->reclaimed_slab
to be anything but 0 is kmem_freepages(). The only places that give a
woozy wombat's bottom about the value of reclaimed_slab are
try_to_free_pages() & balance_pgdat(). These two functions zero
current->reclaim_state->reclaimed_slab, call shrink_slab(), then look at
the value. Since shrink_slab() currently returns 0 no matter what happens,
I changed it to return the number of slab pages freed. This saves us 30
lines of code, makes the what's going on much more clear, and may even be a
smidge faster. It builds, boots, and survives kernbench runs on i386.
mcd@arrakis:~/linux/source $ diffstat
~/linux/patches/remove-reclaim_state.patch
include/linux/sched.h | 3 +--
include/linux/swap.h | 8 --------
mm/page_alloc.c | 6 ------
mm/slab.c | 3 +--
mm/vmscan.c | 26 ++++++--------------------
5 files changed, 8 insertions(+), 38 deletions(-)
-Matt
Matthew Dobson <[email protected]> wrote:
>
> Since shrink_slab() currently returns 0 no matter what happens,
> I changed it to return the number of slab pages freed.
A sane cleanup, but it conflicts with vmscan-notice-slab-shrinking.patch,
which returns a different thing from shrink_slab() in order to account for
slab reclaim only causing internal fragmentation and not actually freeing
pages yet.
vmscan-notice-slab-shrinking.patch isn't quite complete yet, but we do need
to do something along these lines. I need to get back onto it.
Andrew Morton wrote:
> Matthew Dobson <[email protected]> wrote:
>
>>Since shrink_slab() currently returns 0 no matter what happens,
>> I changed it to return the number of slab pages freed.
>
>
> A sane cleanup, but it conflicts with vmscan-notice-slab-shrinking.patch,
> which returns a different thing from shrink_slab() in order to account for
> slab reclaim only causing internal fragmentation and not actually freeing
> pages yet.
>
> vmscan-notice-slab-shrinking.patch isn't quite complete yet, but we do need
> to do something along these lines. I need to get back onto it.
I hadn't seen that patch... These can coexist fairly easily, though. I
can change my patch to zero and then read current->reclaimed_slab *outside*
the call to shrink_slab. It unfortunately shrinks less lines of code, and
leaves the hack that is current->reclaimed_slab exposed to more than one
function, but...
How's this look?
mcd@arrakis:~/linux/patches $ diffstat remove-reclaim_state.patch
include/linux/sched.h | 3 +--
include/linux/swap.h | 8 --------
mm/page_alloc.c | 6 ------
mm/slab.c | 3 +--
mm/vmscan.c | 21 ++++-----------------
5 files changed, 6 insertions(+), 35 deletions(-)
-Matt
Matthew Dobson wrote:
> Andrew Morton wrote:
>
>>Matthew Dobson <[email protected]> wrote:
>>
>>
>>>Since shrink_slab() currently returns 0 no matter what happens,
>>>I changed it to return the number of slab pages freed.
>>
>>
>>A sane cleanup, but it conflicts with vmscan-notice-slab-shrinking.patch,
>>which returns a different thing from shrink_slab() in order to account for
>>slab reclaim only causing internal fragmentation and not actually freeing
>>pages yet.
>>
>>vmscan-notice-slab-shrinking.patch isn't quite complete yet, but we do need
>>to do something along these lines. I need to get back onto it.
>
>
> I hadn't seen that patch... These can coexist fairly easily, though. I
> can change my patch to zero and then read current->reclaimed_slab *outside*
> the call to shrink_slab. It unfortunately shrinks less lines of code, and
> leaves the hack that is current->reclaimed_slab exposed to more than one
> function, but...
>
> How's this look?
>
> mcd@arrakis:~/linux/patches $ diffstat remove-reclaim_state.patch
> include/linux/sched.h | 3 +--
> include/linux/swap.h | 8 --------
> mm/page_alloc.c | 6 ------
> mm/slab.c | 3 +--
> mm/vmscan.c | 21 ++++-----------------
> 5 files changed, 6 insertions(+), 35 deletions(-)
>
> -Matt
Ok. Now I'm getting REALLY crazy. I noticed that try_to_free_pages() &
balance_pgdat() (the two callers of shrink_slab()) do something really
dumb: they set sc.nr_scanned to 0, call shrink_slab() with sc.nr_scanned as
a pass-by-value parameter, then do total_scanned += sc.nr_scanned, despite
knowing damn well that sc.nr_scanned = 0, since _they just set it to that_.
I doubt we really want to be constantly incrementing total_scanned by 0.
We either don't want to update it, or we want to update it by the value
scanned gets in shrink_slab().
So, you're probably asking yourself, what is the point of all this
rambling? What if we change shrink_slab() to take a struct scan_control *
instead of an unsigned long for it's first argument? Then we can
_actually_ update nr_scanned AND nr_reclaimed directly inside of
shrink_slab() instead of trying to pass all this data back to the two
partially brain-dead callers.
Updated to 2.6.12-rc3-mm2, which includes
vmscan-notice-slab-shrinking.patch. Look at all those -'s!! :)
mcd@arrakis:~/linux/patches $ diffstat remove-reclaim_state.patch
include/linux/sched.h | 3 +--
include/linux/swap.h | 8 --------
mm/page_alloc.c | 6 ------
mm/slab.c | 4 ++--
mm/vmscan.c | 36 ++++++++++++------------------------
5 files changed, 15 insertions(+), 42 deletions(-)
Running it through a build & boot test now. I'll let you know if it barfs.
-Matt