[tarako] tune the memory pressure threshold

RESOLVED FIXED

Status

Firefox OS
Performance
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: fabrice, Assigned: ting)

Tracking

unspecified
All
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:1.3T+, b2g-v1.3T fixed)

Details

(Whiteboard: [POVB][demo])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Whiteboard: [POVB]
Triage: 1.3T+
Hi James, can you please look into this? thanks
blocking-b2g: 1.3T? → 1.3T+
Flags: needinfo?(james.zhang)
Whiteboard: [POVB] → [POVB][demo]
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.
(Reporter)

Comment 3

4 years ago
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.
Flags: needinfo?(james.zhang)
Summary: [tarako] memory pressure is not triggered by kernel anymore → [tarako] tune the memory pressure threshold

Updated

4 years ago
Duplicate of this bug: 972256
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.
(Assignee)

Comment 6

4 years ago
(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();
  }
(Reporter)

Comment 7

4 years ago
Ting-Yu can we make progress here? Could we only consider free memory and not free+cache for the low memory pressure event?
(Assignee)

Comment 8

4 years ago
(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.
(Reporter)

Comment 9

4 years ago
(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?
(Assignee)

Comment 10

4 years ago
Created attachment 8385131 [details]
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.
(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.
Depends on: 979205
(Assignee)

Comment 12

4 years ago
Created attachment 8385232 [details]
lowmem_monitor_free.tar.gz

Updated to align Gabriele's patch of bug 979205.
Attachment #8385131 - Attachment is obsolete: true
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.
See Also: → bug 862970, bug 860818
(Assignee)

Comment 14

4 years ago
Created attachment 8385854 [details]
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.
Attachment #8385232 - Attachment is obsolete: true
Assignee: nobody → tchou
Component: General → Runtime
Can I merge this patch on myside?
Flags: needinfo?(tchou)
Flags: needinfo?(fabrice)
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.
(Reporter)

Comment 17

4 years ago
I don't think you need any gonk changes for the MB/KB modification. Gabriele, can you confirm?
Flags: needinfo?(fabrice) → needinfo?(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?
Flags: needinfo?(gsvelto)
(Reporter)

Comment 19

4 years ago
It's already 1.3t+ !
(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.
Add Thomas, I think the patch of bug 972130 and bug 979205 should be landed.
Flags: needinfo?(ttsai)
(Assignee)

Comment 22

4 years ago
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.
Flags: needinfo?(tchou)

Comment 23

4 years ago
James: please uplift bug 979205 to v1.3t.
Flags: needinfo?(ttsai)
(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.
(Reporter)

Updated

4 years ago
Component: Runtime → Performance
(Assignee)

Comment 25

4 years ago
Created attachment 8390258 [details]
lowmem_monitor_free.tar.gz

Fixes LMK per comment 24. Thanks, Gabriele.
Attachment #8385854 - Attachment is obsolete: true
(Assignee)

Comment 26

4 years ago
Fabrice, is there anything I should do before applying the changes to spreadtrum's repository?
Flags: needinfo?(fabrice)
What can I do on my side?
Flags: needinfo?(tchou)
(Reporter)

Comment 28

4 years ago
Ting, please provide PR to James, you can set r=me on the patches.
Flags: needinfo?(fabrice)
(Assignee)

Comment 29

4 years ago
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.
Flags: needinfo?(tchou) → needinfo?(fabrice)
(Reporter)

Comment 30

4 years ago
(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.
Flags: needinfo?(fabrice)
(Assignee)

Comment 31

4 years ago
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.
Attachment #8390258 - Attachment is obsolete: true
Flags: needinfo?(james.zhang)
(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.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: needinfo?(james.zhang)
Resolution: --- → FIXED
device/sprd

commit a455a5a1f33f628242f7552b70451bba64b4b805
Author: james.zhang <james.zhang@spreadtrum.com>
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 <james.zhang@spreadtrum.com>
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

4 years ago
(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
(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.
(Assignee)

Comment 36

4 years ago
(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.
(Reporter)

Updated

4 years ago
status-b2g-v1.3T: --- → fixed
You need to log in before you can comment on or make changes to this bug.