Closed Bug 972130 Opened 6 years ago Closed 6 years ago

[tarako] tune the memory pressure threshold

Categories

(Firefox OS Graveyard :: Performance, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

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

RESOLVED FIXED
blocking-b2g 1.3T+
Tracking Status
b2g-v1.3T --- fixed

People

(Reporter: fabrice, Assigned: ting)

References

Details

(Whiteboard: [POVB][demo])

Attachments

(1 file, 4 obsolete files)

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.
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.
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
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.
(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();
  }
Ting-Yu can we make progress here? Could we only consider free memory and not free+cache for the low memory pressure event?
(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.
(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?
Attached file lowmem_monitor_free.tar.gz (obsolete) —
- 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
Attached file lowmem_monitor_free.tar.gz (obsolete) —
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: → 862970, 860818
Attached file lowmem_monitor_free.tar.gz (obsolete) —
- 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.
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)
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)
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)
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.
Component: Runtime → Performance
Attached file lowmem_monitor_free.tar.gz (obsolete) —
Fixes LMK per comment 24. Thanks, Gabriele.
Attachment #8385854 - Attachment is obsolete: true
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)
Ting, please provide PR to James, you can set r=me on the patches.
Flags: needinfo?(fabrice)
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)
(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)
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
Closed: 6 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
(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.
(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.
You need to log in before you can comment on or make changes to this bug.