Re: lowmemory android driver not needed?

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

also quick review to lowmemorykiller.c

> #include <linux/module.h>
> #include <linux/kernel.h>
> #include <linux/mm.h>
> #include <linux/oom.h>
> #include <linux/sched.h>
>
> static int lowmem_shrink(int nr_to_scan, gfp_t gfp_mask);
>
> static struct shrinker lowmem_shrinker = {
>         .shrink = lowmem_shrink,
>         .seeks = DEFAULT_SEEKS * 16
> };

why do you choice *16?


> static uint32_t lowmem_debug_level = 2;
> static int lowmem_adj[6] = {

why do you choice [6]?

>         0,
>         1,
>         6,
>         12,
> };
>
> static int lowmem_adj_size = 4;
> static size_t lowmem_minfree[6] = {
>         3*512, // 6MB
>         2*1024, // 8MB
>         4*1024, // 16MB
>         16*1024, // 64MB
> };
> static int lowmem_minfree_size = 4;
>
> #define lowmem_print(level, x...) do { if(lowmem_debug_level >= (level)) printk(x); } while(0)
>
> module_param_named(cost, lowmem_shrinker.seeks, int, S_IRUGO | S_IWUSR);
> module_param_array_named(adj, lowmem_adj, int, &lowmem_adj_size, S_IRUGO | S_IWUSR);
> module_param_array_named(minfree, lowmem_minfree, uint, &lowmem_minfree_size, S_IRUGO | S_IWUSR);
> module_param_named(debug_level, lowmem_debug_level, uint, S_IRUGO | S_IWUSR);
>
> 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.


>         if(lowmem_adj_size < array_size)
>                 array_size = lowmem_adj_size;
>         if(lowmem_minfree_size < array_size)
>                 array_size = lowmem_minfree_size;
>         for(i = 0; i < array_size; i++) {
>                 if(other_free < lowmem_minfree[i]) {
>                         min_adj = lowmem_adj[i];
>                         break;
>                 }
>         }
>
>
>         if(nr_to_scan > 0)
>                 lowmem_print(3, "lowmem_shrink %d, %x, ofree %d, ma %d\n", nr_to_scan, gfp_mask, other_fr> ee, min_adj);
>         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.


>                 }
>         }
>         if(selected != NULL) {
>                 lowmem_print(1, "send sigkill to %d (%s), adj %d, size %d\n",
>                              selected->pid, selected->comm,
>                              selected->oomkilladj, selected_tasksize);
>                 force_sig(SIGKILL, selected);
>                 rem -= selected_tasksize;
>         }
>         lowmem_print(4, "lowmem_shrink %d, %x, return %d\n", nr_to_scan, gfp_mask, rem);
>         read_unlock(&tasklist_lock);
>         return rem;
> }