A few days ago, Ying reported a problem.
http://marc.info/?l=linux-mm&m=130403310601663&w=2
After I and Ying dive in problem, We found deactive_page
has a problem. Apparently, It's a BUG so
[1/2] is fix of the problem and [2/2] is enhancement for
helping CPU.
Minchan Kim (2):
[1/2] Check PageUnevictable in lru_deactivate_fn
[2/2] Filter unevictable page out in deactivate_page
mm/swap.c | 12 ++++++++++++
1 files changed, 12 insertions(+), 0 deletions(-)
The lru_deactivate_fn should not move page which in on unevictable lru
into inactive list. Otherwise, we can meet BUG when we use isolate_lru_pages
as __isolate_lru_page could return -EINVAL.
It's really BUG and let's fix it.
Reported-by: Ying Han <[email protected]>
Tested-by: Ying Han <[email protected]>
Signed-off-by: Minchan Kim <[email protected]>
---
mm/swap.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/mm/swap.c b/mm/swap.c
index a83ec5a..2e9656d 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -429,6 +429,9 @@ static void lru_deactivate_fn(struct page *page, void *arg)
if (!PageLRU(page))
return;
+ if (PageUnevictable(page))
+ return;
+
/* Some processes are using the page */
if (page_mapped(page))
return;
--
1.7.1
It's pointless that deactive_page's pagevec operation about
unevictable page as it's nop.
This patch removes unnecessary overhead which might be a bit problem
in case that there are many unevictable page in system(ex, mprotect workload)
Signed-off-by: Minchan Kim <[email protected]>
---
mm/swap.c | 9 +++++++++
1 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/mm/swap.c b/mm/swap.c
index 2e9656d..b707694 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -511,6 +511,15 @@ static void drain_cpu_pagevecs(int cpu)
*/
void deactivate_page(struct page *page)
{
+
+ /*
+ * In workload which system has many unevictable page(ex, mprotect),
+ * unevictalge page deactivation for accelerating reclaim
+ * is pointless.
+ */
+ if (PageUnevictable(page))
+ return;
+
if (likely(get_page_unless_zero(page))) {
struct pagevec *pvec = &get_cpu_var(lru_deactivate_pvecs);
--
1.7.1
On Sun, May 1, 2011 at 8:03 AM, Minchan Kim <[email protected]> wrote:
> The lru_deactivate_fn should not move page which in on unevictable lru
> into inactive list. Otherwise, we can meet BUG when we use isolate_lru_pages
> as __isolate_lru_page could return -EINVAL.
> It's really BUG and let's fix it.
>
> Reported-by: Ying Han <[email protected]>
> Tested-by: Ying Han <[email protected]>
> Signed-off-by: Minchan Kim <[email protected]>
> ---
> ?mm/swap.c | ? ?3 +++
> ?1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/mm/swap.c b/mm/swap.c
> index a83ec5a..2e9656d 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -429,6 +429,9 @@ static void lru_deactivate_fn(struct page *page, void *arg)
> ? ? ? ?if (!PageLRU(page))
> ? ? ? ? ? ? ? ?return;
>
> + ? ? ? if (PageUnevictable(page))
> + ? ? ? ? ? ? ? return;
> +
> ? ? ? ?/* Some processes are using the page */
> ? ? ? ?if (page_mapped(page))
> ? ? ? ? ? ? ? ?return;
> --
> 1.7.1
Thanks Minchan for the fix, and i haven't been able to reproducing the
issue after applying the patch.
--Ying
>
On Mon, May 2, 2011 at 7:10 AM, Ying Han <[email protected]> wrote:
> On Sun, May 1, 2011 at 8:03 AM, Minchan Kim <[email protected]> wrote:
>> The lru_deactivate_fn should not move page which in on unevictable lru
>> into inactive list. Otherwise, we can meet BUG when we use isolate_lru_pages
>> as __isolate_lru_page could return -EINVAL.
>> It's really BUG and let's fix it.
>>
>> Reported-by: Ying Han <[email protected]>
>> Tested-by: Ying Han <[email protected]>
>> Signed-off-by: Minchan Kim <[email protected]>
>> ---
>> mm/swap.c | 3 +++
>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/mm/swap.c b/mm/swap.c
>> index a83ec5a..2e9656d 100644
>> --- a/mm/swap.c
>> +++ b/mm/swap.c
>> @@ -429,6 +429,9 @@ static void lru_deactivate_fn(struct page *page, void *arg)
>> if (!PageLRU(page))
>> return;
>>
>> + if (PageUnevictable(page))
>> + return;
>> +
>> /* Some processes are using the page */
>> if (page_mapped(page))
>> return;
>> --
>> 1.7.1
>
> Thanks Minchan for the fix, and i haven't been able to reproducing the
> issue after applying the patch.
Thanks for the help, Ying.
--
Kind regards,
Minchan Kim
> The lru_deactivate_fn should not move page which in on unevictable lru
> into inactive list. Otherwise, we can meet BUG when we use isolate_lru_pages
> as __isolate_lru_page could return -EINVAL.
> It's really BUG and let's fix it.
>
> Reported-by: Ying Han <[email protected]>
> Tested-by: Ying Han <[email protected]>
> Signed-off-by: Minchan Kim <[email protected]>
> ---
> mm/swap.c | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/mm/swap.c b/mm/swap.c
> index a83ec5a..2e9656d 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -429,6 +429,9 @@ static void lru_deactivate_fn(struct page *page, void *arg)
> if (!PageLRU(page))
> return;
>
> + if (PageUnevictable(page))
> + return;
> +
Reviewed-by: KOSAKI Motohiro <[email protected]>
> It's pointless that deactive_page's pagevec operation about
> unevictable page as it's nop.
> This patch removes unnecessary overhead which might be a bit problem
> in case that there are many unevictable page in system(ex, mprotect workload)
>
> Signed-off-by: Minchan Kim <[email protected]>
> ---
> mm/swap.c | 9 +++++++++
> 1 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/mm/swap.c b/mm/swap.c
> index 2e9656d..b707694 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -511,6 +511,15 @@ static void drain_cpu_pagevecs(int cpu)
> */
> void deactivate_page(struct page *page)
> {
> +
> + /*
> + * In workload which system has many unevictable page(ex, mprotect),
> + * unevictalge page deactivation for accelerating reclaim
> + * is pointless.
> + */
> + if (PageUnevictable(page))
> + return;
> +
Reviewed-by: KOSAKI Motohiro <[email protected]>
btw, I think we should check PageLRU too.
On 05/01/2011 11:03 AM, Minchan Kim wrote:
> The lru_deactivate_fn should not move page which in on unevictable lru
> into inactive list. Otherwise, we can meet BUG when we use isolate_lru_pages
> as __isolate_lru_page could return -EINVAL.
> It's really BUG and let's fix it.
>
> Reported-by: Ying Han<[email protected]>
> Tested-by: Ying Han<[email protected]>
> Signed-off-by: Minchan Kim<[email protected]>
Reviewed-by: Rik van Riel<[email protected]>
--
All rights reversed
On 05/01/2011 11:03 AM, Minchan Kim wrote:
> It's pointless that deactive_page's pagevec operation about
> unevictable page as it's nop.
> This patch removes unnecessary overhead which might be a bit problem
> in case that there are many unevictable page in system(ex, mprotect workload)
>
> Signed-off-by: Minchan Kim<[email protected]>
> ---
> mm/swap.c | 9 +++++++++
> 1 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/mm/swap.c b/mm/swap.c
> index 2e9656d..b707694 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -511,6 +511,15 @@ static void drain_cpu_pagevecs(int cpu)
> */
> void deactivate_page(struct page *page)
> {
> +
> + /*
> + * In workload which system has many unevictable page(ex, mprotect),
> + * unevictalge page deactivation for accelerating reclaim
Typo.
> + * is pointless.
> + */
> + if (PageUnevictable(page))
> + return;
> +
> if (likely(get_page_unless_zero(page))) {
> struct pagevec *pvec =&get_cpu_var(lru_deactivate_pvecs);
>
--
All rights reversed
Hi KOSAKI,
On Mon, May 2, 2011 at 7:37 PM, KOSAKI Motohiro
<[email protected]> wrote:
>> It's pointless that deactive_page's pagevec operation about
>> unevictable page as it's nop.
>> This patch removes unnecessary overhead which might be a bit problem
>> in case that there are many unevictable page in system(ex, mprotect workload)
>>
>> Signed-off-by: Minchan Kim <[email protected]>
>> ---
>> mm/swap.c | 9 +++++++++
>> 1 files changed, 9 insertions(+), 0 deletions(-)
>>
>> diff --git a/mm/swap.c b/mm/swap.c
>> index 2e9656d..b707694 100644
>> --- a/mm/swap.c
>> +++ b/mm/swap.c
>> @@ -511,6 +511,15 @@ static void drain_cpu_pagevecs(int cpu)
>> */
>> void deactivate_page(struct page *page)
>> {
>> +
>> + /*
>> + * In workload which system has many unevictable page(ex, mprotect),
>> + * unevictalge page deactivation for accelerating reclaim
>> + * is pointless.
>> + */
>> + if (PageUnevictable(page))
>> + return;
>> +
>
> Reviewed-by: KOSAKI Motohiro <[email protected]>
>
Thanks!
>
> btw, I think we should check PageLRU too.
>
Yes. I remember you advised it when we push this patch but I didn't.
That's because I think most of pages in such context would be LRU as
they are cached pages.
So IMO, PageLRU checking in deactivate_page couldn't help much.
--
Kind regards,
Minchan Kim
Hi Rik,
On Mon, May 2, 2011 at 11:57 PM, Rik van Riel <[email protected]> wrote:
> On 05/01/2011 11:03 AM, Minchan Kim wrote:
>>
>> It's pointless that deactive_page's pagevec operation about
>> unevictable page as it's nop.
>> This patch removes unnecessary overhead which might be a bit problem
>> in case that there are many unevictable page in system(ex, mprotect
>> workload)
>>
>> Signed-off-by: Minchan Kim<[email protected]>
>> ---
>> mm/swap.c | 9 +++++++++
>> 1 files changed, 9 insertions(+), 0 deletions(-)
>>
>> diff --git a/mm/swap.c b/mm/swap.c
>> index 2e9656d..b707694 100644
>> --- a/mm/swap.c
>> +++ b/mm/swap.c
>> @@ -511,6 +511,15 @@ static void drain_cpu_pagevecs(int cpu)
>> */
>> void deactivate_page(struct page *page)
>> {
>> +
>> + /*
>> + * In workload which system has many unevictable page(ex,
>> mprotect),
>> + * unevictalge page deactivation for accelerating reclaim
>
> Typo.
My bad. I will resend after work.
Thanks.
--
Kind regards,
Minchan Kim