Re: [PATCH 2/3] staging: lowmemorykiller: count anon pages only when we have swap devices
2016-06-23 16:42 GMT+08:00 Sergey Senozhatsky: > On (06/22/16 11:27), Ganesh Mahendran wrote: > [..] >> > > Signed-off-by: Ganesh Mahendran >> > > --- >> > > drivers/staging/android/lowmemorykiller.c | 12 >> > > 1 file changed, 8 insertions(+), 4 deletions(-) >> > > >> > > diff --git a/drivers/staging/android/lowmemorykiller.c >> > > b/drivers/staging/android/lowmemorykiller.c >> > > index 6da9260..1d8de47 100644 >> > > --- a/drivers/staging/android/lowmemorykiller.c >> > > +++ b/drivers/staging/android/lowmemorykiller.c >> > > @@ -73,10 +73,14 @@ static unsigned long lowmem_deathpending_timeout; >> > > static unsigned long lowmem_count(struct shrinker *s, >> > > struct shrink_control *sc) >> > > { >> > > - return global_page_state(NR_ACTIVE_ANON) + >> > > - global_page_state(NR_ACTIVE_FILE) + >> > > - global_page_state(NR_INACTIVE_ANON) + >> > > - global_page_state(NR_INACTIVE_FILE); >> > > + unsigned long freeable = global_page_state(NR_ACTIVE_FILE) + >> > > + global_page_state(NR_INACTIVE_FILE); >> > > + >> > > + if (get_nr_swap_pages() > 0) >> > > + freeable += global_page_state(NR_ACTIVE_ANON) + >> > > + global_page_state(NR_INACTIVE_ANON); >> > > + >> > > + return freeable; >> > > } >> > > >> > > static unsigned long lowmem_scan(struct shrinker *s, struct >> > > shrink_control *sc) >> > >> > Shouldn't this be advertising the amount of memory that is freeable by >> > killing the process with the highest priority oom_score_adj? It's not >> > legitimate to say it can free all anon and file memory if nothing is oom >> > killable, so this function is wrong both originally and with your patched >> > version. >> >> Yes, so should we just simply return 1 to make do_shrink_slab() go ahead? >> Then lowmem_scan() will do the real job to scan all the process. > > hm, looking at ->scan (lowmem_scan) shouldn't it return SHRINK_STOP > when it has nothing to free instead of 0? Yes, you are right. It should return SHRINK_STOP. > > -ss
Re: [PATCH 2/3] staging: lowmemorykiller: count anon pages only when we have swap devices
2016-06-23 16:42 GMT+08:00 Sergey Senozhatsky : > On (06/22/16 11:27), Ganesh Mahendran wrote: > [..] >> > > Signed-off-by: Ganesh Mahendran >> > > --- >> > > drivers/staging/android/lowmemorykiller.c | 12 >> > > 1 file changed, 8 insertions(+), 4 deletions(-) >> > > >> > > diff --git a/drivers/staging/android/lowmemorykiller.c >> > > b/drivers/staging/android/lowmemorykiller.c >> > > index 6da9260..1d8de47 100644 >> > > --- a/drivers/staging/android/lowmemorykiller.c >> > > +++ b/drivers/staging/android/lowmemorykiller.c >> > > @@ -73,10 +73,14 @@ static unsigned long lowmem_deathpending_timeout; >> > > static unsigned long lowmem_count(struct shrinker *s, >> > > struct shrink_control *sc) >> > > { >> > > - return global_page_state(NR_ACTIVE_ANON) + >> > > - global_page_state(NR_ACTIVE_FILE) + >> > > - global_page_state(NR_INACTIVE_ANON) + >> > > - global_page_state(NR_INACTIVE_FILE); >> > > + unsigned long freeable = global_page_state(NR_ACTIVE_FILE) + >> > > + global_page_state(NR_INACTIVE_FILE); >> > > + >> > > + if (get_nr_swap_pages() > 0) >> > > + freeable += global_page_state(NR_ACTIVE_ANON) + >> > > + global_page_state(NR_INACTIVE_ANON); >> > > + >> > > + return freeable; >> > > } >> > > >> > > static unsigned long lowmem_scan(struct shrinker *s, struct >> > > shrink_control *sc) >> > >> > Shouldn't this be advertising the amount of memory that is freeable by >> > killing the process with the highest priority oom_score_adj? It's not >> > legitimate to say it can free all anon and file memory if nothing is oom >> > killable, so this function is wrong both originally and with your patched >> > version. >> >> Yes, so should we just simply return 1 to make do_shrink_slab() go ahead? >> Then lowmem_scan() will do the real job to scan all the process. > > hm, looking at ->scan (lowmem_scan) shouldn't it return SHRINK_STOP > when it has nothing to free instead of 0? Yes, you are right. It should return SHRINK_STOP. > > -ss
Re: [PATCH 2/3] staging: lowmemorykiller: count anon pages only when we have swap devices
On (06/22/16 11:27), Ganesh Mahendran wrote: [..] > > > Signed-off-by: Ganesh Mahendran> > > --- > > > drivers/staging/android/lowmemorykiller.c | 12 > > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/staging/android/lowmemorykiller.c > > > b/drivers/staging/android/lowmemorykiller.c > > > index 6da9260..1d8de47 100644 > > > --- a/drivers/staging/android/lowmemorykiller.c > > > +++ b/drivers/staging/android/lowmemorykiller.c > > > @@ -73,10 +73,14 @@ static unsigned long lowmem_deathpending_timeout; > > > static unsigned long lowmem_count(struct shrinker *s, > > > struct shrink_control *sc) > > > { > > > - return global_page_state(NR_ACTIVE_ANON) + > > > - global_page_state(NR_ACTIVE_FILE) + > > > - global_page_state(NR_INACTIVE_ANON) + > > > - global_page_state(NR_INACTIVE_FILE); > > > + unsigned long freeable = global_page_state(NR_ACTIVE_FILE) + > > > + global_page_state(NR_INACTIVE_FILE); > > > + > > > + if (get_nr_swap_pages() > 0) > > > + freeable += global_page_state(NR_ACTIVE_ANON) + > > > + global_page_state(NR_INACTIVE_ANON); > > > + > > > + return freeable; > > > } > > > > > > static unsigned long lowmem_scan(struct shrinker *s, struct > > > shrink_control *sc) > > > > Shouldn't this be advertising the amount of memory that is freeable by > > killing the process with the highest priority oom_score_adj? It's not > > legitimate to say it can free all anon and file memory if nothing is oom > > killable, so this function is wrong both originally and with your patched > > version. > > Yes, so should we just simply return 1 to make do_shrink_slab() go ahead? > Then lowmem_scan() will do the real job to scan all the process. hm, looking at ->scan (lowmem_scan) shouldn't it return SHRINK_STOP when it has nothing to free instead of 0? -ss
Re: [PATCH 2/3] staging: lowmemorykiller: count anon pages only when we have swap devices
On (06/22/16 11:27), Ganesh Mahendran wrote: [..] > > > Signed-off-by: Ganesh Mahendran > > > --- > > > drivers/staging/android/lowmemorykiller.c | 12 > > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/staging/android/lowmemorykiller.c > > > b/drivers/staging/android/lowmemorykiller.c > > > index 6da9260..1d8de47 100644 > > > --- a/drivers/staging/android/lowmemorykiller.c > > > +++ b/drivers/staging/android/lowmemorykiller.c > > > @@ -73,10 +73,14 @@ static unsigned long lowmem_deathpending_timeout; > > > static unsigned long lowmem_count(struct shrinker *s, > > > struct shrink_control *sc) > > > { > > > - return global_page_state(NR_ACTIVE_ANON) + > > > - global_page_state(NR_ACTIVE_FILE) + > > > - global_page_state(NR_INACTIVE_ANON) + > > > - global_page_state(NR_INACTIVE_FILE); > > > + unsigned long freeable = global_page_state(NR_ACTIVE_FILE) + > > > + global_page_state(NR_INACTIVE_FILE); > > > + > > > + if (get_nr_swap_pages() > 0) > > > + freeable += global_page_state(NR_ACTIVE_ANON) + > > > + global_page_state(NR_INACTIVE_ANON); > > > + > > > + return freeable; > > > } > > > > > > static unsigned long lowmem_scan(struct shrinker *s, struct > > > shrink_control *sc) > > > > Shouldn't this be advertising the amount of memory that is freeable by > > killing the process with the highest priority oom_score_adj? It's not > > legitimate to say it can free all anon and file memory if nothing is oom > > killable, so this function is wrong both originally and with your patched > > version. > > Yes, so should we just simply return 1 to make do_shrink_slab() go ahead? > Then lowmem_scan() will do the real job to scan all the process. hm, looking at ->scan (lowmem_scan) shouldn't it return SHRINK_STOP when it has nothing to free instead of 0? -ss
Re: [PATCH 2/3] staging: lowmemorykiller: count anon pages only when we have swap devices
Hi, David: On Tue, Jun 21, 2016 at 01:22:00PM -0700, David Rientjes wrote: > On Tue, 21 Jun 2016, Ganesh Mahendran wrote: > > > lowmem_count() should only count anon pages when we have swap device. > > > > Why? I make a mistake. I thought lowmem_count will return the shrinkalbe page of a process. > > > Signed-off-by: Ganesh Mahendran> > --- > > drivers/staging/android/lowmemorykiller.c | 12 > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/staging/android/lowmemorykiller.c > > b/drivers/staging/android/lowmemorykiller.c > > index 6da9260..1d8de47 100644 > > --- a/drivers/staging/android/lowmemorykiller.c > > +++ b/drivers/staging/android/lowmemorykiller.c > > @@ -73,10 +73,14 @@ static unsigned long lowmem_deathpending_timeout; > > static unsigned long lowmem_count(struct shrinker *s, > > struct shrink_control *sc) > > { > > - return global_page_state(NR_ACTIVE_ANON) + > > - global_page_state(NR_ACTIVE_FILE) + > > - global_page_state(NR_INACTIVE_ANON) + > > - global_page_state(NR_INACTIVE_FILE); > > + unsigned long freeable = global_page_state(NR_ACTIVE_FILE) + > > + global_page_state(NR_INACTIVE_FILE); > > + > > + if (get_nr_swap_pages() > 0) > > + freeable += global_page_state(NR_ACTIVE_ANON) + > > + global_page_state(NR_INACTIVE_ANON); > > + > > + return freeable; > > } > > > > static unsigned long lowmem_scan(struct shrinker *s, struct shrink_control > > *sc) > > Shouldn't this be advertising the amount of memory that is freeable by > killing the process with the highest priority oom_score_adj? It's not > legitimate to say it can free all anon and file memory if nothing is oom > killable, so this function is wrong both originally and with your patched > version. Yes, so should we just simply return 1 to make do_shrink_slab() go ahead? Then lowmem_scan() will do the real job to scan all the process. Thanks.
Re: [PATCH 2/3] staging: lowmemorykiller: count anon pages only when we have swap devices
Hi, David: On Tue, Jun 21, 2016 at 01:22:00PM -0700, David Rientjes wrote: > On Tue, 21 Jun 2016, Ganesh Mahendran wrote: > > > lowmem_count() should only count anon pages when we have swap device. > > > > Why? I make a mistake. I thought lowmem_count will return the shrinkalbe page of a process. > > > Signed-off-by: Ganesh Mahendran > > --- > > drivers/staging/android/lowmemorykiller.c | 12 > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/staging/android/lowmemorykiller.c > > b/drivers/staging/android/lowmemorykiller.c > > index 6da9260..1d8de47 100644 > > --- a/drivers/staging/android/lowmemorykiller.c > > +++ b/drivers/staging/android/lowmemorykiller.c > > @@ -73,10 +73,14 @@ static unsigned long lowmem_deathpending_timeout; > > static unsigned long lowmem_count(struct shrinker *s, > > struct shrink_control *sc) > > { > > - return global_page_state(NR_ACTIVE_ANON) + > > - global_page_state(NR_ACTIVE_FILE) + > > - global_page_state(NR_INACTIVE_ANON) + > > - global_page_state(NR_INACTIVE_FILE); > > + unsigned long freeable = global_page_state(NR_ACTIVE_FILE) + > > + global_page_state(NR_INACTIVE_FILE); > > + > > + if (get_nr_swap_pages() > 0) > > + freeable += global_page_state(NR_ACTIVE_ANON) + > > + global_page_state(NR_INACTIVE_ANON); > > + > > + return freeable; > > } > > > > static unsigned long lowmem_scan(struct shrinker *s, struct shrink_control > > *sc) > > Shouldn't this be advertising the amount of memory that is freeable by > killing the process with the highest priority oom_score_adj? It's not > legitimate to say it can free all anon and file memory if nothing is oom > killable, so this function is wrong both originally and with your patched > version. Yes, so should we just simply return 1 to make do_shrink_slab() go ahead? Then lowmem_scan() will do the real job to scan all the process. Thanks.
Re: [PATCH 2/3] staging: lowmemorykiller: count anon pages only when we have swap devices
On Tue, 21 Jun 2016, Ganesh Mahendran wrote: > lowmem_count() should only count anon pages when we have swap device. > Why? > Signed-off-by: Ganesh Mahendran> --- > drivers/staging/android/lowmemorykiller.c | 12 > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/android/lowmemorykiller.c > b/drivers/staging/android/lowmemorykiller.c > index 6da9260..1d8de47 100644 > --- a/drivers/staging/android/lowmemorykiller.c > +++ b/drivers/staging/android/lowmemorykiller.c > @@ -73,10 +73,14 @@ static unsigned long lowmem_deathpending_timeout; > static unsigned long lowmem_count(struct shrinker *s, > struct shrink_control *sc) > { > - return global_page_state(NR_ACTIVE_ANON) + > - global_page_state(NR_ACTIVE_FILE) + > - global_page_state(NR_INACTIVE_ANON) + > - global_page_state(NR_INACTIVE_FILE); > + unsigned long freeable = global_page_state(NR_ACTIVE_FILE) + > + global_page_state(NR_INACTIVE_FILE); > + > + if (get_nr_swap_pages() > 0) > + freeable += global_page_state(NR_ACTIVE_ANON) + > + global_page_state(NR_INACTIVE_ANON); > + > + return freeable; > } > > static unsigned long lowmem_scan(struct shrinker *s, struct shrink_control > *sc) Shouldn't this be advertising the amount of memory that is freeable by killing the process with the highest priority oom_score_adj? It's not legitimate to say it can free all anon and file memory if nothing is oom killable, so this function is wrong both originally and with your patched version.
Re: [PATCH 2/3] staging: lowmemorykiller: count anon pages only when we have swap devices
On Tue, 21 Jun 2016, Ganesh Mahendran wrote: > lowmem_count() should only count anon pages when we have swap device. > Why? > Signed-off-by: Ganesh Mahendran > --- > drivers/staging/android/lowmemorykiller.c | 12 > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/android/lowmemorykiller.c > b/drivers/staging/android/lowmemorykiller.c > index 6da9260..1d8de47 100644 > --- a/drivers/staging/android/lowmemorykiller.c > +++ b/drivers/staging/android/lowmemorykiller.c > @@ -73,10 +73,14 @@ static unsigned long lowmem_deathpending_timeout; > static unsigned long lowmem_count(struct shrinker *s, > struct shrink_control *sc) > { > - return global_page_state(NR_ACTIVE_ANON) + > - global_page_state(NR_ACTIVE_FILE) + > - global_page_state(NR_INACTIVE_ANON) + > - global_page_state(NR_INACTIVE_FILE); > + unsigned long freeable = global_page_state(NR_ACTIVE_FILE) + > + global_page_state(NR_INACTIVE_FILE); > + > + if (get_nr_swap_pages() > 0) > + freeable += global_page_state(NR_ACTIVE_ANON) + > + global_page_state(NR_INACTIVE_ANON); > + > + return freeable; > } > > static unsigned long lowmem_scan(struct shrinker *s, struct shrink_control > *sc) Shouldn't this be advertising the amount of memory that is freeable by killing the process with the highest priority oom_score_adj? It's not legitimate to say it can free all anon and file memory if nothing is oom killable, so this function is wrong both originally and with your patched version.
[PATCH 2/3] staging: lowmemorykiller: count anon pages only when we have swap devices
lowmem_count() should only count anon pages when we have swap device. Signed-off-by: Ganesh Mahendran--- drivers/staging/android/lowmemorykiller.c | 12 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/drivers/staging/android/lowmemorykiller.c b/drivers/staging/android/lowmemorykiller.c index 6da9260..1d8de47 100644 --- a/drivers/staging/android/lowmemorykiller.c +++ b/drivers/staging/android/lowmemorykiller.c @@ -73,10 +73,14 @@ static unsigned long lowmem_deathpending_timeout; static unsigned long lowmem_count(struct shrinker *s, struct shrink_control *sc) { - return global_page_state(NR_ACTIVE_ANON) + - global_page_state(NR_ACTIVE_FILE) + - global_page_state(NR_INACTIVE_ANON) + - global_page_state(NR_INACTIVE_FILE); + unsigned long freeable = global_page_state(NR_ACTIVE_FILE) + + global_page_state(NR_INACTIVE_FILE); + + if (get_nr_swap_pages() > 0) + freeable += global_page_state(NR_ACTIVE_ANON) + + global_page_state(NR_INACTIVE_ANON); + + return freeable; } static unsigned long lowmem_scan(struct shrinker *s, struct shrink_control *sc) -- 1.9.1
[PATCH 2/3] staging: lowmemorykiller: count anon pages only when we have swap devices
lowmem_count() should only count anon pages when we have swap device. Signed-off-by: Ganesh Mahendran --- drivers/staging/android/lowmemorykiller.c | 12 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/drivers/staging/android/lowmemorykiller.c b/drivers/staging/android/lowmemorykiller.c index 6da9260..1d8de47 100644 --- a/drivers/staging/android/lowmemorykiller.c +++ b/drivers/staging/android/lowmemorykiller.c @@ -73,10 +73,14 @@ static unsigned long lowmem_deathpending_timeout; static unsigned long lowmem_count(struct shrinker *s, struct shrink_control *sc) { - return global_page_state(NR_ACTIVE_ANON) + - global_page_state(NR_ACTIVE_FILE) + - global_page_state(NR_INACTIVE_ANON) + - global_page_state(NR_INACTIVE_FILE); + unsigned long freeable = global_page_state(NR_ACTIVE_FILE) + + global_page_state(NR_INACTIVE_FILE); + + if (get_nr_swap_pages() > 0) + freeable += global_page_state(NR_ACTIVE_ANON) + + global_page_state(NR_INACTIVE_ANON); + + return freeable; } static unsigned long lowmem_scan(struct shrinker *s, struct shrink_control *sc) -- 1.9.1