Re: lowmemory android driver not needed?

[KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>]

Hi

> On Fri, Jan 16, 2009 at 5:18 AM, KOSAKI Motohiro
> <kosaki.motohiro@jp.fujitsu.com> wrote:
> > also quick review to lowmemorykiller.c
> 
> >> static struct shrinker lowmem_shrinker = {
> >>         .shrink = lowmem_shrink,
> >>         .seeks = DEFAULT_SEEKS * 16
> >> };
> >
> > why do you choice *16?
> 
> To indicate that it is more expensive to restart a killed process than
> a regular cache page.

I think you already know this answer isn't actual answer.
anybody know >1 mean expensive. a reviewer want to know why DEFAULT_SEEKS * 16
is best, not *10 nor *20.



> >> static uint32_t lowmem_debug_level = 2;
> >> static int lowmem_adj[6] = {
> >
> > why do you choice [6]?
> 
> We use six levels.

if you don't consider other user and other usage case,
this file can't merge forever.

The top problem is, this file stay on non proper place.
then, MM folks don't review at all.

I think this patch need to receive MM folks review.
Greg, please drop this file from stagin awhile.
(of cource, my intention is this one file only, other hardware depended 
driver file is needed to staging)

	Naked-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>


> >> static int lowmem_shrink(int nr_to_scan, gfp_t gfp_mask)
> >> {
> >>         struct task_struct *p;
> >>         struct task_struct *selected = NULL;
> >>         int rem = 0;
> >>         int tasksize;
> >>         int i;
> >>         int min_adj = OOM_ADJUST_MAX + 1;
> >>         int selected_tasksize = 0;
> >>         int array_size = ARRAY_SIZE(lowmem_adj);
> >>         int other_free = global_page_state(NR_FREE_PAGES) + global_page_state(NR_FILE_PAGES);
> >
> > I think you don't consider mlocked page (and/or other unevictable page).
> > if much mlocked file page exist, other_free can become large value.
> > then, this routine don't kill any process.
> 
> Yes, this is a problem we have seen, but I did not find counter for
> pinned cache pages. I may be able to use NR_UNEVICTABLE if
> CONFIG_UNEVICTABLE_LRU is enabled now though.

hm hom.


> Another problem we run into is that allocations of contiguous pages
> can cause kswapd to empty all the caches. This will not trigger this
> lowmemorykiller or the regular oom killer.

strange usage.
you shouldn't use high order allocation on embedded machine.



> >>         read_lock(&tasklist_lock);
> >>         for_each_process(p) {
> >
> > Oops. too long locking.
> > shrink_slab() is freqentlly called function. I don't like costly operation.
> >
> >>                 if(p->oomkilladj >= 0 && p->mm) {
> >>                         tasksize = get_mm_rss(p->mm);
> >>                         if(nr_to_scan > 0 && tasksize > 0 && p->oomkilladj >= min_adj) {
> >>                                 if(selected == NULL ||
> >>                                    p->oomkilladj > selected->oomkilladj ||
> >>                                    (p->oomkilladj == selected->oomkilladj &&
> >>                                     tasksize > selected_tasksize)) {
> >>                                         selected = p;
> >>                                         selected_tasksize = tasksize;
> >>                                         lowmem_print(2, "select %d (%s), adj %d, size %d, to kill\n",
> >>                                                      p->pid, p->comm, p->oomkilladj, tasksize);
> >>                                 }
> >>                         }
> >>                         rem += tasksize;
> >
> > this code mean,
> > shrinker->shrink(0, gfp_mask) indicate to recalculate total rss every time.
> > it is too CPU and battery wasting.
> >
> 
> How about this:
> ----
> diff --git a/drivers/misc/lowmemorykiller.c b/drivers/misc/lowmemorykiller.c
> index 3715d56..3f4e41a 100644
> --- a/drivers/misc/lowmemorykiller.c
> +++ b/drivers/misc/lowmemorykiller.c
> @@ -71,6 +71,12 @@ static int lowmem_shrink(int nr_to_scan, gfp_t gfp_mask)
>         }
>         if(nr_to_scan > 0)
>                 lowmem_print(3, "lowmem_shrink %d, %x, ofree %d, ma
> %d\n", nr_to_scan, gfp_mask, other_free, min_adj);
> +       rem = global_page_state(NR_ACTIVE_ANON) +
> global_page_state(NR_ACTIVE_FILE);
> +       if (!nr_to_scan || min_adj == OOM_ADJUST_MAX + 1) {
> +               lowmem_print(5, "lowmem_shrink %d, %x, return %d\n",
> nr_to_scan, gfp_mask, rem);
> +               return rem;
> +       }
> +

I don't understand your intention.
if file server machine has tons NR_ACTIVE_FILE, this logic kill many 
process.

Do I misunderstand anything?

>         read_lock(&tasklist_lock);
>         for_each_process(p) {
>                 if(p->oomkilladj >= 0 && p->mm) {
> @@ -86,7 +92,6 @@ static int lowmem_shrink(int nr_to_scan, gfp_t gfp_mask)
>                                                      p->pid, p->comm,
> p->oomkilladj, tasksize);
>                                 }
>                         }
> -                       rem += tasksize;
>                 }
>         }
>         if(selected != NULL) {