972130 - [tarako] tune the memory pressure threshold

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Performance, defect)

Description

[[:fabrice] Fabrice Desré]

Running 
watch -n 1 adb shell cat /sys/kernel/mm/lowmemkiller/notify_trigger_active

shows that it's always returning 0, so we never get memory pressure events. That's a vendor regression.

Comment 1

[Joe Cheng [:jcheng] (please needinfo)]

Triage: 1.3T+
Hi James, can you please look into this? thanks

Comment 2

[Danny Liang [:dliang]]

Notify_trigger_active can be 1 if free + cache memory is lower than threshold, now notifyLowMemUnderMB is 10MB.
In today's build, the free + cache is hard to lower than 10MB. It keeps around 20MB or more, so we won't see notify_trigger_active be 1.
I also configured hal.processPriorityManager.gonk.notifyLowMemUnderMB to 20MB and hal.processPriorityManager.gonk.BACKGROUND.KillUnderMB to 12MB, I can easily see the notify_trigger_active be 1 when I launched several applications.
So I can confirm lowmemkiller works normal, but maybe we need to discuss about memory threshold in current memory usage.

Comment 3

[[:fabrice] Fabrice Desré]

Thanks Danny. So we need to tune all these parameters, since for now we have apps getting killed by the LMK while we have no oportunity to act on memory pressure.

Comment 5

[Danny Liang [:dliang]]

I summarize the current setting and problem as below. 

The current specific memory setting on tarako as below:
pref("hal.processPriorityManager.gonk.enableOOBPKiller", true);
pref("hal.processPriorityManager.gonk.MASTER.KillUnderMB", 1);
pref("hal.processPriorityManager.gonk.FOREGROUND_HIGH.KillUnderMB", 2);
pref("hal.processPriorityManager.gonk.FOREGROUND.KillUnderMB", 4);
pref("hal.processPriorityManager.gonk.BACKGROUND_PERCEIVABLE.KillUnderMB", 6);
pref("hal.processPriorityManager.gonk.BACKGROUND.KillUnderMB", 20);
pref("hal.processPriorityManager.gonk.notifyLowMemUnderMB", 10);

1. With above setting, the memory pressure events are hard to be trigger due to free+cache is usually higher than 20MB.
2. If BACKGROUND.KillUnderMB is lower than 20MB, the performance of launch application is worse.
3. If notifyLowMemUnderMB is higher, there might be many memory pressure events and it will let performance bad.

Comment 6

[Ting-Yu Chou [:ting] (away)]

(In reply to Danny Liang [:dliang] from comment #5)
> 3. If notifyLowMemUnderMB is higher, there might be many memory pressure
> events and it will let performance bad.

Maybe we should not trigger memory pressure event when lowmem_shrink() is called with sc->nr_to_scan 0, which VM is queriying the cache size. Currently it is not checked:

  if (other_free < lowmem_minfree_notif_trigger &&
      other_file < lowmem_minfree_notif_trigger) {
    lowmem_notify_killzone_approach();
  }

Comment 7

[[:fabrice] Fabrice Desré]

Ting-Yu can we make progress here? Could we only consider free memory and not free+cache for the low memory pressure event?

Comment 8

[Ting-Yu Chou [:ting] (away)]

(In reply to Fabrice Desré [:fabrice] from comment #7)
> Ting-Yu can we make progress here? Could we only consider free memory and
> not free+cache for the low memory pressure event?

I think we could.

Usually the free stays around 2~3 MB when there's an app running, maybe set notifyLowMemUnderMB a bit lower, for example: 1.6 or 1.8 MB according to comment 5. Just I don't know how will this impact the performance when memory is already that tight.

Combine this with your patch of bug 974308, I guess the low memory pressure will reclaim memory mostly from b2g and Homescreen.

Comment 9

[[:fabrice] Fabrice Desré]

(In reply to Ting-Yu Chou [:ting] from comment #8)
> (In reply to Fabrice Desré [:fabrice] from comment #7)
> > Ting-Yu can we make progress here? Could we only consider free memory and
> > not free+cache for the low memory pressure event?
> 
> I think we could.
> 
> Usually the free stays around 2~3 MB when there's an app running, maybe set
> notifyLowMemUnderMB a bit lower, for example: 1.6 or 1.8 MB according to
> comment 5. Just I don't know how will this impact the performance when
> memory is already that tight.
> 
> Combine this with your patch of bug 974308, I guess the low memory pressure
> will reclaim memory mostly from b2g and Homescreen.

I think so too, and that's what we need - there's quite a bit of memory to reclaim.

Can you follow-up on the kernel changes or in userland?

Comment 10

[Ting-Yu Chou [:ting] (away)]

Attachment: lowmem_monitor_free.tar.gz

- Modified lowmemorykiller.c to trigger low memory pressure event when:
    a) sc->nr_to_scan > 0, and
    b) free < notify_trigger.

- Changed preference notifyLowMemUnderMB from 10 to 2. I'd like to make it a bit
  smaller, but the number is read as an integer.

Comment 11

[Gabriele Svelto [:gsvelto]]

(In reply to Ting-Yu Chou [:ting] from comment #10)
> - Changed preference notifyLowMemUnderMB from 10 to 2. I'd like to make it a
> bit
>   smaller, but the number is read as an integer.

We can change those parameters to KiB to give us better granularity, let me open a follow-up on that.

Comment 12

[Ting-Yu Chou [:ting] (away)]

Attachment: lowmem_monitor_free.tar.gz

Updated to align Gabriele's patch of bug 979205.

Comment 13

[Gabriele Svelto [:gsvelto]]

FYI delivering low-memory notifications reliably to the main b2g process and the current foreground app is paramount for keeping them alive when using large images or large amounts of images in the app/page. See bug 860818 and it's solution bug 862970.

Comment 14

[Ting-Yu Chou [:ting] (away)]

Attachment: lowmem_monitor_free.tar.gz

- Use nr_free_pages() instead of other_free. Note other_free is usually 0:

    other_free = nr_free_pages() - totalreserve_pages

- Change notify_trigger from 1844 to 1600 to lower the number of times
  triggering low memory pressure.

Comment 15

[James Zhang (Spreadtrum)]

Can I merge this patch on myside?

Comment 16

[James Zhang (Spreadtrum)]

We meet "memory pressure is over" bug when monkey test now. Maybe you change the LMK param from MB to KB in gecko, and I haven't change this param in device/sprd.

Comment 17

[[:fabrice] Fabrice Desré]

I don't think you need any gonk changes for the MB/KB modification. Gabriele, can you confirm?

Comment 18

[Gabriele Svelto [:gsvelto]]

(In reply to Fabrice Desré [:fabrice] from comment #17)
> I don't think you need any gonk changes for the MB/KB modification.
> Gabriele, can you confirm?

Yes, gonk is unaffected, the changes in bug 979205 are gecko-only. They've just not been uplifted yet though because I was unsure if we wanted this or not in the end, shall I nom it for v1.3t?

Comment 19

[[:fabrice] Fabrice Desré]

It's already 1.3t+ !

Comment 20

[Gabriele Svelto [:gsvelto]]

(In reply to Fabrice Desré [:fabrice] from comment #19)
> It's already 1.3t+ !

I meant bug 979205, it hasn't any flags set ATM from what I can see.

Comment 21

[James Zhang (Spreadtrum)]

Add Thomas, I think the patch of bug 972130 and bug 979205 should be landed.

Comment 22

[Ting-Yu Chou [:ting] (away)]

I just noticed /sys/kernel/mm/lowmemkiller/notify_trigger_active has to be 1 if memory pressure event is triggered, but I am not sure when it should be set to 0. Are there any spec or guideline for this?

# Current patch (attachment 8385854 [details]) does not reflect the status.

Comment 23

[thomas tsai]

James: please uplift bug 979205 to v1.3t.

Comment 24

[Gabriele Svelto [:gsvelto]]

(In reply to Ting-Yu Chou [:ting] from comment #22)
> I just noticed /sys/kernel/mm/lowmemkiller/notify_trigger_active has to be 1
> if memory pressure event is triggered, but I am not sure when it should be
> set to 0. Are there any spec or guideline for this?

AFAIK it should be set to 0 when the memory pressure is over (i.e. the amount of free memory goes above the notify_trigger value). Note that we don't rely on the value to fire the first memory pressure event but we just wait for the notification (poll() wakes up with a POLLPRI event, see the comment here:

http://hg.mozilla.org/mozilla-central/file/44ae8462d6ab/widget/gonk/GonkMemoryPressureMonitoring.cpp#l165

We do rely on the value of notify_trigger_active afterwards to decide whether the memory pressure is over.

Comment 25

[Ting-Yu Chou [:ting] (away)]

Attachment: lowmem_monitor_free.tar.gz

Fixes LMK per comment 24. Thanks, Gabriele.

Comment 26

[Ting-Yu Chou [:ting] (away)]

Fabrice, is there anything I should do before applying the changes to spreadtrum's repository?

Comment 27

[James Zhang (Spreadtrum)]

What can I do on my side?

Comment 28

[[:fabrice] Fabrice Desré]

Ting, please provide PR to James, you can set r=me on the patches.

Comment 29

[Ting-Yu Chou [:ting] (away)]

Yesterday Thinker suggested to use number of anonymous pages plus swap pages instead will make more sense. Fabrice, do you have any concerns? If not, I will try to work it out during the weekend.

BTW, I'm not sure what is PR, is it submitting a patch?

James, please allow me to work on it a bit more, I will let you know once the patch is ready.

Comment 30

[[:fabrice] Fabrice Desré]

(In reply to Ting-Yu Chou [:ting] from comment #29)
> Yesterday Thinker suggested to use number of anonymous pages plus swap pages
> instead will make more sense. Fabrice, do you have any concerns? If not, I
> will try to work it out during the weekend.

No objection on my side.

> BTW, I'm not sure what is PR, is it submitting a patch?

PR = pull request, but a patch is maybe the only solution for James.

Comment 31

[Ting-Yu Chou [:ting] (away)]

Attachment: [PATCH] mem_pressure_tuning.tar.gz

I found using number of anonymous and swap pages seems contrary to the preference name "notifyLowMemUnderKB", also, tuning is just needed for whatever number we monitor. So I decided not to change spreadtrum's criteria, but add nr_to_scan checking which I think it is a bug.

Increase the size from 10MB to 14MB, memory pressure will be triggered but not often.

James, the patch is ready. But please understand the number is tuned by myself, not from massive tests, it may still need to be changed.

# I am sorry that I have wasted everyone's time.

Comment 32

[James Zhang (Spreadtrum)]

(In reply to Ting-Yu Chou [:ting] from comment #31)
> Created attachment 8391877 [details]
> [PATCH] mem_pressure_tuning.tar.gz
> 
> I found using number of anonymous and swap pages seems contrary to the
> preference name "notifyLowMemUnderKB", also, tuning is just needed for
> whatever number we monitor. So I decided not to change spreadtrum's
> criteria, but add nr_to_scan checking which I think it is a bug.
> 
> Increase the size from 10MB to 14MB, memory pressure will be triggered but
> not often.
> 
> James, the patch is ready. But please understand the number is tuned by
> myself, not from massive tests, it may still need to be changed.
> 
> # I am sorry that I have wasted everyone's time.

Land this patch on myside.

Comment 33

[James Zhang (Spreadtrum)]

device/sprd

commit a455a5a1f33f628242f7552b70451bba64b4b805
Author: james.zhang <[e-mail]>
Date:   Mon Mar 17 09:17:53 2014 +0800

    Bug#269156 merge patch from mozilla, Bug 972130 - [tarako] tune the memory pressure threshold
    
    Change-Id: I4735c748937b6cbb30af9c2760ac14c5a6ba6a2c

kernel

commit 09d752004b3c43c4e1b4a9d4b3654a9c3cd6ec7d
Author: james.zhang <[e-mail]>
Date:   Mon Mar 17 09:16:58 2014 +0800

    Bug#269156 merge patch from mozilla, Bug 972130 - [tarako] tune the memory pressure threshold
    
    Change-Id: If61f5492749bf484b235eb7a9f2c1a23c2d82bab

Comment 34

[ying.xu]

(In reply to Ting-Yu Chou [:ting] from comment #31)
> Created attachment 8391877 [details]
> [PATCH] mem_pressure_tuning.tar.gz
> 
> I found using number of anonymous and swap pages seems contrary to the
> preference name "notifyLowMemUnderKB", also, tuning is just needed for
> whatever number we monitor. So I decided not to change spreadtrum's
> criteria, but add nr_to_scan checking which I think it is a bug.

It's ture that add nr_to_scan == 0 often happen.
If you want to reduce the amount of low memory event, that I can understand

Comment 35

[Gabriele Svelto [:gsvelto]]

(In reply to Ting-Yu Chou [:ting] from comment #31)
> Increase the size from 10MB to 14MB, memory pressure will be triggered but
> not often.
> 
> James, the patch is ready. But please understand the number is tuned by
> myself, not from massive tests, it may still need to be changed.

Try the Pinterest app / page, it's a typical scenario where the foreground application will OOM if memory pressure notifications are not working correctly. Considering you reverted the trigger parameter back to its original default I think this should work correctly.

Comment 36

[Ting-Yu Chou [:ting] (away)]

(In reply to Gabriele Svelto [:gsvelto] from comment #35)
> Try the Pinterest app / page, it's a typical scenario where the foreground
> application will OOM if memory pressure notifications are not working
> correctly. Considering you reverted the trigger parameter back to its
> original default I think this should work correctly.

Yes, I tried Pinterest page per your comment 13, I didn't know it is default value though.