Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1335220 > unrolled thread

[PATCH] mm: add MM_SWAPENTS and page table when calculate tasksize in lowmem_scan()

Back to article view | Back to linux.kernel

Contents

  [PATCH] mm: add MM_SWAPENTS and page table when calculate tasksize  in lowmem_scan() Xishi Qiu <qiuxishi@huawei.com> - 2016-02-16 10:40 +0100
    Re: [PATCH] mm: add MM_SWAPENTS and page table when calculate  tasksize in lowmem_scan() Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-02-16 18:40 +0100
      Re: [PATCH] mm: add MM_SWAPENTS and page table when calculate tasksize  in lowmem_scan() David Rientjes <rientjes@google.com> - 2016-02-17 01:40 +0100
        Re: [PATCH] mm: add MM_SWAPENTS and page table when calculate tasksize  in lowmem_scan() Xishi Qiu <qiuxishi@huawei.com> - 2016-02-17 09:50 +0100
          Re: [PATCH] mm: add MM_SWAPENTS and page table when calculate tasksize  in lowmem_scan() David Rientjes <rientjes@google.com> - 2016-02-17 23:50 +0100
        Re: [PATCH] mm: add MM_SWAPENTS and page table when calculate  tasksize in lowmem_scan() Michal Hocko <mhocko@kernel.org> - 2016-02-17 19:20 +0100
        Re: [PATCH] mm: add MM_SWAPENTS and page table when calculate tasksize  in lowmem_scan() Xishi Qiu <qiuxishi@huawei.com> - 2016-02-18 08:00 +0100
        Re: [PATCH] mm: add MM_SWAPENTS and page table when calculate tasksize  in lowmem_scan() Xishi Qiu <qiuxishi@huawei.com> - 2016-02-18 11:30 +0100

#1335220 — [PATCH] mm: add MM_SWAPENTS and page table when calculate tasksize in lowmem_scan()

Currently tasksize in lowmem_scan() only calculate rss, and not include swap.
But usually smart phones enable zram, so swap space actually use ram.

Signed-off-by: Xishi Qiu <qiuxishi@huawei.com>
---
 drivers/staging/android/lowmemorykiller.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/android/lowmemorykiller.c b/drivers/staging/android/lowmemorykiller.c
index 8b5a4a8..718ab8e 100644
--- a/drivers/staging/android/lowmemorykiller.c
+++ b/drivers/staging/android/lowmemorykiller.c
@@ -139,7 +139,10 @@ static unsigned long lowmem_scan(struct shrinker *s, struct shrink_control *sc)
 			task_unlock(p);
 			continue;
 		}
-		tasksize = get_mm_rss(p->mm);
+		tasksize = get_mm_rss(p->mm) +
+			   get_mm_counter(p->mm, MM_SWAPENTS) +
+			   atomic_long_read(&p->mm->nr_ptes) +
+			   mm_nr_pmds(p->mm);
 		task_unlock(p);
 		if (tasksize <= 0)
 			continue;
-- 
1.8.3.1

[toc] | [next] | [standalone]

#1335668 — Re: [PATCH] mm: add MM_SWAPENTS and page table when calculate tasksize in lowmem_scan()

On Tue, Feb 16, 2016 at 05:37:05PM +0800, Xishi Qiu wrote:
> Currently tasksize in lowmem_scan() only calculate rss, and not include swap.
> But usually smart phones enable zram, so swap space actually use ram.

Yes, but does that matter for this type of calculation?  I need an ack
from the android team before I could ever take such a core change to
this code...

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1335960

On Tue, 16 Feb 2016, Greg Kroah-Hartman wrote:

> On Tue, Feb 16, 2016 at 05:37:05PM +0800, Xishi Qiu wrote:
> > Currently tasksize in lowmem_scan() only calculate rss, and not include swap.
> > But usually smart phones enable zram, so swap space actually use ram.
> 
> Yes, but does that matter for this type of calculation?  I need an ack
> from the android team before I could ever take such a core change to
> this code...
> 

The calculation proposed in this patch is the same as the generic oom 
killer, it's an estimate of the amount of memory that will be freed if it 
is killed and can exit.  This is better than simply get_mm_rss().

However, I think we seriously need to re-consider the implementation of 
the lowmem killer entirely.  It currently abuses the use of TIF_MEMDIE, 
which should ideally only be set for one thread on the system since it 
allows unbounded access to global memory reserves.

It also abuses the user-visible /proc/self/oom_score_adj tunable: this 
tunable is used by the generic oom killer to bias or discount a proportion 
of memory from a process's usage.  This is the only supported semantic of 
the tunable.  The lowmem killer uses it as a strict prioritization, so any 
process with oom_score_adj higher than another process is preferred for 
kill, REGARDLESS of memory usage.  This leads to priority inversion, the 
user is unable to always define the same process to be killed by the 
generic oom killer and the lowmem killer.  This is what happens when a 
tunable with a very clear and defined purpose is used for other reasons.

I'd seriously consider not accepting any additional hacks on top of this 
code until the implementation is rewritten.

[toc] | [prev] | [next] | [standalone]

#1336149

On 2016/2/17 8:35, David Rientjes wrote:

> On Tue, 16 Feb 2016, Greg Kroah-Hartman wrote:
> 
>> On Tue, Feb 16, 2016 at 05:37:05PM +0800, Xishi Qiu wrote:
>>> Currently tasksize in lowmem_scan() only calculate rss, and not include swap.
>>> But usually smart phones enable zram, so swap space actually use ram.
>>
>> Yes, but does that matter for this type of calculation?  I need an ack
>> from the android team before I could ever take such a core change to
>> this code...
>>
> 
> The calculation proposed in this patch is the same as the generic oom 
> killer, it's an estimate of the amount of memory that will be freed if it 
> is killed and can exit.  This is better than simply get_mm_rss().
> 
> However, I think we seriously need to re-consider the implementation of 
> the lowmem killer entirely.  It currently abuses the use of TIF_MEMDIE, 
> which should ideally only be set for one thread on the system since it 
> allows unbounded access to global memory reserves.
> 
> It also abuses the user-visible /proc/self/oom_score_adj tunable: this 
> tunable is used by the generic oom killer to bias or discount a proportion 
> of memory from a process's usage.  This is the only supported semantic of 
> the tunable.  The lowmem killer uses it as a strict prioritization, so any 
> process with oom_score_adj higher than another process is preferred for 
> kill, REGARDLESS of memory usage.  This leads to priority inversion, the 
> user is unable to always define the same process to be killed by the 
> generic oom killer and the lowmem killer.  This is what happens when a 
> tunable with a very clear and defined purpose is used for other reasons.
> 
> I'd seriously consider not accepting any additional hacks on top of this 
> code until the implementation is rewritten.
> 

Hi David,

Thanks for your advice.

I have a stupid question, what's the main difference between lmk and oom?
1) lmk is called when reclaim memory, and oom is called when alloc failed in slow path.
2) lmk has several lowmem thresholds and oom is not.
3) others?

Thanks,
Xishi Qiu

> .
> 

[toc] | [prev] | [next] | [standalone]

#1336851

On Wed, 17 Feb 2016, Xishi Qiu wrote:

> Hi David,
> 
> Thanks for your advice.
> 
> I have a stupid question, what's the main difference between lmk and oom?

Hi Xishi, it's not a stupid question at all!

Low memory killer appears to be implemented as a generic shrinker that 
iterates through the tasklist and tries to free memory before the generic 
oom killer.  It has two tunables, "adj" and "minfree": "minfree" describes 
what class of processes are eligible based on how many free pages are left 
on the system and "adj" defines that class by using oom_score_adj values.

So LMK is trying to free memory before all memory is depleted based on 
heuristics for systems that load the driver whereas the generic oom killer 
is called to kill a process when reclaim has failed to free any memory and 
there's no forward progress.

> 1) lmk is called when reclaim memory, and oom is called when alloc failed in slow path.

Yeah, and I don't think LMK provides any sort of guarantee against all 
memory being fully depleted before it can run, so it would probably be 
best effort.

> 2) lmk has several lowmem thresholds and oom is not.

Right, and it abuses oom_score_adj, which is a generic oom killer tunable 
to define priorities to kill at different levels of memory availability.

> 3) others?
> 

LMK also abuses TIF_MEMDIE which is used by the generic oom killer to 
allow a process to free memory.  Since the system is out of memory when it 
is called, a process often needs additional memory to even exit, so we set 
TIF_MEMDIE to ignore zone watermarks in the page allocator.  LMK should 
not be using this, there should already be memory available for it to 
allocate from.

To fix these issues with LMK, I think it should:

 - send SIGKILL to terminate a process in lowmem situations, but not
   set TIF_MEMDIE and implement its own way of determining when to kill
   additional processes, and

 - introduce its own tunable to define the priority of kill when it runs
   rather than oom_score_adj, which is a proportion of memory to bias
   against, not a priority at all.

[toc] | [prev] | [next] | [standalone]

#1336627 — Re: [PATCH] mm: add MM_SWAPENTS and page table when calculate tasksize in lowmem_scan()

On Tue 16-02-16 16:35:39, David Rientjes wrote:
> On Tue, 16 Feb 2016, Greg Kroah-Hartman wrote:
> 
> > On Tue, Feb 16, 2016 at 05:37:05PM +0800, Xishi Qiu wrote:
> > > Currently tasksize in lowmem_scan() only calculate rss, and not include swap.
> > > But usually smart phones enable zram, so swap space actually use ram.
> > 
> > Yes, but does that matter for this type of calculation?  I need an ack
> > from the android team before I could ever take such a core change to
> > this code...
> > 
> 
> The calculation proposed in this patch is the same as the generic oom 
> killer, it's an estimate of the amount of memory that will be freed if it 
> is killed and can exit.  This is better than simply get_mm_rss().
> 
> However, I think we seriously need to re-consider the implementation of 
> the lowmem killer entirely.  It currently abuses the use of TIF_MEMDIE, 
> which should ideally only be set for one thread on the system since it 
> allows unbounded access to global memory reserves.
> 
> It also abuses the user-visible /proc/self/oom_score_adj tunable: this 
> tunable is used by the generic oom killer to bias or discount a proportion 
> of memory from a process's usage.  This is the only supported semantic of 
> the tunable.  The lowmem killer uses it as a strict prioritization, so any 
> process with oom_score_adj higher than another process is preferred for 
> kill, REGARDLESS of memory usage.  This leads to priority inversion, the 
> user is unable to always define the same process to be killed by the 
> generic oom killer and the lowmem killer.  This is what happens when a 
> tunable with a very clear and defined purpose is used for other reasons.
> 
> I'd seriously consider not accepting any additional hacks on top of this 
> code until the implementation is rewritten.

Fully agreed!

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1337091

On 2016/2/17 8:35, David Rientjes wrote:

> On Tue, 16 Feb 2016, Greg Kroah-Hartman wrote:
> 
>> On Tue, Feb 16, 2016 at 05:37:05PM +0800, Xishi Qiu wrote:
>>> Currently tasksize in lowmem_scan() only calculate rss, and not include swap.
>>> But usually smart phones enable zram, so swap space actually use ram.
>>
>> Yes, but does that matter for this type of calculation?  I need an ack
>> from the android team before I could ever take such a core change to
>> this code...
>>
> 
> The calculation proposed in this patch is the same as the generic oom 
> killer, it's an estimate of the amount of memory that will be freed if it 
> is killed and can exit.  This is better than simply get_mm_rss().
> 
> However, I think we seriously need to re-consider the implementation of 
> the lowmem killer entirely.  It currently abuses the use of TIF_MEMDIE, 
> which should ideally only be set for one thread on the system since it 
> allows unbounded access to global memory reserves.
> 

Hi David,

Does somebody do the work of re-implementation of the lowmem killer entirely
now? Could you give me some details? e.g. when and how?

Here are another two questions.
1) lmk has several lowmem thresholds, it's "lowmem_minfree[]", and the value is
static definition, so is it reasonable for different memory size(e.g. 2G/3G/4G...)
of smart phones?
2) There are many adjustable arguments in /proc/sys/vm/, and the default value
maybe not benefit for smart phones, so any suggestions?

Thanks,
Xishi Qiu

> It also abuses the user-visible /proc/self/oom_score_adj tunable: this 
> tunable is used by the generic oom killer to bias or discount a proportion 
> of memory from a process's usage.  This is the only supported semantic of 
> the tunable.  The lowmem killer uses it as a strict prioritization, so any 
> process with oom_score_adj higher than another process is preferred for 
> kill, REGARDLESS of memory usage.  This leads to priority inversion, the 
> user is unable to always define the same process to be killed by the 
> generic oom killer and the lowmem killer.  This is what happens when a 
> tunable with a very clear and defined purpose is used for other reasons.
> 
> I'd seriously consider not accepting any additional hacks on top of this 
> code until the implementation is rewritten.
> 
> .
> 

[toc] | [prev] | [next] | [standalone]

#1337228

On 2016/2/18 15:55, Figo.zhang wrote:

> 
> 
> 2016-02-17 8:35 GMT+08:00 David Rientjes <rientjes@google.com <mailto:rientjes@google.com>>:
> 
>     On Tue, 16 Feb 2016, Greg Kroah-Hartman wrote:
> 
>     > On Tue, Feb 16, 2016 at 05:37:05PM +0800, Xishi Qiu wrote:
>     > > Currently tasksize in lowmem_scan() only calculate rss, and not include swap.
>     > > But usually smart phones enable zram, so swap space actually use ram.
>     >
>     > Yes, but does that matter for this type of calculation?  I need an ack
>     > from the android team before I could ever take such a core change to
>     > this code...
>     >
> 
>     The calculation proposed in this patch is the same as the generic oom
>     killer, it's an estimate of the amount of memory that will be freed if it
>     is killed and can exit.  This is better than simply get_mm_rss().
> 
>     However, I think we seriously need to re-consider the implementation of
>     the lowmem killer entirely.  It currently abuses the use of TIF_MEMDIE,
>     which should ideally only be set for one thread on the system since it
>     allows unbounded access to global memory reserves.
> 
> 
> 
> i don't understand why it need wait 1 second:
> 

Hi David,

How about kill more processes at one time?

Usually loading camera will alloc 300-500M memory immediately, so call lmk
repeatedly is a waste of time.

And can we reclaim memory at one time instead of reclaim-alloc-reclaim-alloc...
in this situation? e.g. use try_to_free_pages(), set nr_to_reclaim=300M

Thanks,
Xishi Qiu

> if (test_tsk_thread_flag(p, TIF_MEMDIE) &&
>    time_before_eq(jiffies, lowmem_deathpending_timeout)) {
> task_unlock(p);
> rcu_read_unlock();
> return 0;                             <= why return rather than continue?
> }
> 
> and it will retry and wait many CPU times if one task holding the TIF_MEMDI.
>    shrink_slab_node()   
>        while()
>            shrinker->scan_objects();
>                      lowmem_scan()
>                                  if (test_tsk_thread_flag(p, TIF_MEMDIE) &&
>                                        time_before_eq(jiffies, lowmem_deathpending_timeout)) 
> 
>  
> 
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web