Re: [PATCH 1/2 v2] mm: vmscan: do not pass reclaimed slab to vmpressure
On Tue, Jan 31, 2017 at 5:26 AM, Minchan Kim wrote:
> On Fri, Jan 27, 2017 at 01:43:36PM +0530, Vinayak Menon wrote:
>> It is noticed that during a global reclaim the memory
>> reclaimed via shrinking the slabs can sometimes result
>> in reclaimed pages being greater than the scanned pages
>> in shrink_node. When this is passed to vmpressure, the
>> unsigned arithmetic results in the pressure value to be
>> huge, thus resulting in a critical event being sent to
>> root cgroup. While this can be fixed by underflow checks
>> in vmpressure, adding reclaimed slab without a corresponding
>> increment of nr_scanned results in incorrect vmpressure
>> reporting. So do not consider reclaimed slab pages in
>> vmpressure calculation.
>
> I belive we could enhance the description better.
>
> problem
>
> VM include nr_reclaimed of slab but not nr_scanned so pressure
> calculation can be underflow.
>
> solution
>
> do not consider reclaimed slab pages for vmpressure
>
> why
>
> Freeing a page by slab shrinking depends on each slab's object
> population so the cost model(i.e., scan:free) is not fair with
> LRU pages. Also, every shrinker doesn't account reclaimed pages.
> Lastly, this regression happens since 6b4f7799c6a5
>
Done. Sending an updated one.
>>
>> Signed-off-by: Vinayak Menon
>> ---
>> mm/vmscan.c | 10 +-
>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index 947ab6f..37c4486 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -2594,16 +2594,16 @@ static bool shrink_node(pg_data_t *pgdat, struct
>> scan_control *sc)
>> sc->nr_scanned - nr_scanned,
>> node_lru_pages);
>>
>> - if (reclaim_state) {
>> - sc->nr_reclaimed += reclaim_state->reclaimed_slab;
>> - reclaim_state->reclaimed_slab = 0;
>> - }
>> -
>> /* Record the subtree's reclaim efficiency */
>> vmpressure(sc->gfp_mask, sc->target_mem_cgroup, true,
>> sc->nr_scanned - nr_scanned,
>> sc->nr_reclaimed - nr_reclaimed);
>>
>
> Please add comment about "vmpressure excludes reclaimed pages via slab
> because blah blah blah" so upcoming patches doesn't make mistake again.
>
> Thanks!
>
Done. Thanks Minchan.
Re: [PATCH 1/2 v2] mm: vmscan: do not pass reclaimed slab to vmpressure
On Fri, Jan 27, 2017 at 01:43:36PM +0530, Vinayak Menon wrote:
> It is noticed that during a global reclaim the memory
> reclaimed via shrinking the slabs can sometimes result
> in reclaimed pages being greater than the scanned pages
> in shrink_node. When this is passed to vmpressure, the
> unsigned arithmetic results in the pressure value to be
> huge, thus resulting in a critical event being sent to
> root cgroup. While this can be fixed by underflow checks
> in vmpressure, adding reclaimed slab without a corresponding
> increment of nr_scanned results in incorrect vmpressure
> reporting. So do not consider reclaimed slab pages in
> vmpressure calculation.
I belive we could enhance the description better.
problem
VM include nr_reclaimed of slab but not nr_scanned so pressure
calculation can be underflow.
solution
do not consider reclaimed slab pages for vmpressure
why
Freeing a page by slab shrinking depends on each slab's object
population so the cost model(i.e., scan:free) is not fair with
LRU pages. Also, every shrinker doesn't account reclaimed pages.
Lastly, this regression happens since 6b4f7799c6a5
>
> Signed-off-by: Vinayak Menon
> ---
> mm/vmscan.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 947ab6f..37c4486 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -2594,16 +2594,16 @@ static bool shrink_node(pg_data_t *pgdat, struct
> scan_control *sc)
> sc->nr_scanned - nr_scanned,
> node_lru_pages);
>
> - if (reclaim_state) {
> - sc->nr_reclaimed += reclaim_state->reclaimed_slab;
> - reclaim_state->reclaimed_slab = 0;
> - }
> -
> /* Record the subtree's reclaim efficiency */
> vmpressure(sc->gfp_mask, sc->target_mem_cgroup, true,
> sc->nr_scanned - nr_scanned,
> sc->nr_reclaimed - nr_reclaimed);
>
Please add comment about "vmpressure excludes reclaimed pages via slab
because blah blah blah" so upcoming patches doesn't make mistake again.
Thanks!
> + if (reclaim_state) {
> + sc->nr_reclaimed += reclaim_state->reclaimed_slab;
> + reclaim_state->reclaimed_slab = 0;
> + }
> +
> if (sc->nr_reclaimed - nr_reclaimed)
> reclaimable = true;
>
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
> member of the Code Aurora Forum, hosted by The Linux Foundation
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majord...@kvack.org. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: mailto:"d...@kvack.org";> em...@kvack.org
[PATCH 1/2 v2] mm: vmscan: do not pass reclaimed slab to vmpressure
It is noticed that during a global reclaim the memory
reclaimed via shrinking the slabs can sometimes result
in reclaimed pages being greater than the scanned pages
in shrink_node. When this is passed to vmpressure, the
unsigned arithmetic results in the pressure value to be
huge, thus resulting in a critical event being sent to
root cgroup. While this can be fixed by underflow checks
in vmpressure, adding reclaimed slab without a corresponding
increment of nr_scanned results in incorrect vmpressure
reporting. So do not consider reclaimed slab pages in
vmpressure calculation.
Signed-off-by: Vinayak Menon
---
mm/vmscan.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 947ab6f..37c4486 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2594,16 +2594,16 @@ static bool shrink_node(pg_data_t *pgdat, struct
scan_control *sc)
sc->nr_scanned - nr_scanned,
node_lru_pages);
- if (reclaim_state) {
- sc->nr_reclaimed += reclaim_state->reclaimed_slab;
- reclaim_state->reclaimed_slab = 0;
- }
-
/* Record the subtree's reclaim efficiency */
vmpressure(sc->gfp_mask, sc->target_mem_cgroup, true,
sc->nr_scanned - nr_scanned,
sc->nr_reclaimed - nr_reclaimed);
+ if (reclaim_state) {
+ sc->nr_reclaimed += reclaim_state->reclaimed_slab;
+ reclaim_state->reclaimed_slab = 0;
+ }
+
if (sc->nr_reclaimed - nr_reclaimed)
reclaimable = true;
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of the Code Aurora Forum, hosted by The Linux Foundation
