[PATCH -mm] split_lru: fix pagevec_move_tail() doesn't treat unevictable page

[PATCH -mm] split_lru: fix pagevec_move_tail() doesn't treat unevictable page

[KOSAKI Motohiro]

even under writebacking, page can move to unevictable list.
so shouldn't pagevec_move_tail() check unevictable?


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

---
 mm/swap.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: b/mm/swap.c
===================================================================
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -116,7 +116,7 @@ static void pagevec_move_tail(struct pag
 			zone = pagezone;
 			spin_lock(&zone->lru_lock);
 		}
-		if (PageLRU(page) && !PageActive(page)) {
+		if (PageLRU(page) && !PageActive(page) && !PageUnevictable(page)) {
 			int lru = page_is_file_cache(page);
 			list_move_tail(&page->lru, &zone->lru[lru].list);
 			pgmoved++;


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -mm] split_lru: fix pagevec_move_tail() doesn't treat unevictable page

[MinChan Kim]

On Tue, Jul 1, 2008 at 4:01 PM, KOSAKI Motohiro
<kosaki.motohiro@jp.fujitsu.com> wrote:
> even under writebacking, page can move to unevictable list.
> so shouldn't pagevec_move_tail() check unevictable?
>
Hi, Kosaki-san.

I can't understand this race situation.
How the page can move to unevictable list while it is under writeback?

Could you explain for me ? :)

> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>
> ---
>  mm/swap.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Index: b/mm/swap.c
> ===================================================================
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -116,7 +116,7 @@ static void pagevec_move_tail(struct pag
>                        zone = pagezone;
>                        spin_lock(&zone->lru_lock);
>                }
> -               if (PageLRU(page) && !PageActive(page)) {
> +               if (PageLRU(page) && !PageActive(page) && !PageUnevictable(page)) {
>                        int lru = page_is_file_cache(page);
>                        list_move_tail(&page->lru, &zone->lru[lru].list);
>                        pgmoved++;
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>



-- 
Kinds regards,
MinChan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -mm] split_lru: fix pagevec_move_tail() doesn't treat unevictable page

[KOSAKI Motohiro]

Hi Kim-san,

Thank you for good question.

> > even under writebacking, page can move to unevictable list.
> > so shouldn't pagevec_move_tail() check unevictable?
> >
> Hi, Kosaki-san.
> 
> I can't understand this race situation.
> How the page can move to unevictable list while it is under writeback?
> 
> Could you explain for me ? :)

Actually, I added below assertion and tested on stress workload.
then system crashed after 4H runnings.

----------------------------------------------
static void pagevec_move_tail(struct pagevec *pvec)
{
(snip)
                if (PageLRU(page) && !PageActive(page)) {
                        int lru = page_is_file_cache(page);
                        list_move_tail(&page->lru, &zone->lru[lru].list);
                        BUG_ON(page_lru(page) != lru);  // !!here
                        pgmoved++;
                }
        }
----------------------------------------------------

So, I guess below race exist (but I hope Rik's review)


    CPU1                                       CPU2
==================================================================
1. rotate_reclaimable_page()
2. PageUnevictable(page) return 0
3. local_irq_save()
4. pagevec_move_tail()
                                       SetPageUnevictable()   //mlock?
                                       move to unevictable list
5. spin_lock(&zone->lru_lock);
6. list_move_tail(); (move to inactive list)

then page have PageUnevictable() and is chained inactive lru.
Or, I misunderstand it?


abstraction of related function
------------------------------------------------------------
void  rotate_reclaimable_page(struct page *page)
{
        if (!PageLocked(page) && !PageDirty(page) && !PageActive(page) &&
            !PageUnevictable(page) && PageLRU(page)) {
                local_irq_save(flags);
                pagevec_move_tail(pvec);
                local_irq_restore(flags);
        }
}

pagevec_move_tail(){
	spin_lock(&zone->lru_lock);
	if (PageLRU(page) && !PageActive(page)) {
		list_move_tail(&page->lru, &zone->lru[lru].list);
	}
	spin_unlock(&zone->lru_lock);
}




--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH -mm] split_lru: fix pagevec_move_tail() doesn't treat unevictable page

[MinChan Kim]

On Tue, Jul 1, 2008 at 4:56 PM, KOSAKI Motohiro
<kosaki.motohiro@jp.fujitsu.com> wrote:
> Hi Kim-san,
>
> Thank you for good question.

Thanks for good explaining.
I guess your scenario have a possibility.

If I don't have a test HPC, I will dig in source. :)

>> > even under writebacking, page can move to unevictable list.
>> > so shouldn't pagevec_move_tail() check unevictable?
>> >
>> Hi, Kosaki-san.
>>
>> I can't understand this race situation.
>> How the page can move to unevictable list while it is under writeback?
>>
>> Could you explain for me ? :)
>
> Actually, I added below assertion and tested on stress workload.
> then system crashed after 4H runnings.
>
> ----------------------------------------------
> static void pagevec_move_tail(struct pagevec *pvec)
> {
> (snip)
>                if (PageLRU(page) && !PageActive(page)) {
>                        int lru = page_is_file_cache(page);
>                        list_move_tail(&page->lru, &zone->lru[lru].list);
>                        BUG_ON(page_lru(page) != lru);  // !!here
>                        pgmoved++;
>                }
>        }
> ----------------------------------------------------
>
> So, I guess below race exist (but I hope Rik's review)
>
>
>    CPU1                                       CPU2
> ==================================================================
> 1. rotate_reclaimable_page()
> 2. PageUnevictable(page) return 0
> 3. local_irq_save()
> 4. pagevec_move_tail()
>                                       SetPageUnevictable()   //mlock?
>                                       move to unevictable list
> 5. spin_lock(&zone->lru_lock);
> 6. list_move_tail(); (move to inactive list)
>
> then page have PageUnevictable() and is chained inactive lru.
> Or, I misunderstand it?
>
>
> abstraction of related function
> ------------------------------------------------------------
> void  rotate_reclaimable_page(struct page *page)
> {
>        if (!PageLocked(page) && !PageDirty(page) && !PageActive(page) &&
>            !PageUnevictable(page) && PageLRU(page)) {
>                local_irq_save(flags);
>                pagevec_move_tail(pvec);
>                local_irq_restore(flags);
>        }
> }
>
> pagevec_move_tail(){
>        spin_lock(&zone->lru_lock);
>        if (PageLRU(page) && !PageActive(page)) {
>                list_move_tail(&page->lru, &zone->lru[lru].list);
>        }
>        spin_unlock(&zone->lru_lock);
> }
>
>
>
>
>



-- 
Kinds regards,
MinChan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[resend][PATCH -mm] split_lru: fix pagevec_move_tail() doesn't treat unevictable page

[KOSAKI Motohiro]

Agghh!
I am really stupid. I forgot CCed Andrew ;-)

So, I resend this.


--------------------------------------
even under writebacking, page can move to unevictable list.
so shouldn't pagevec_move_tail() check unevictable?

if pagevec_move_tail() doesn't PageUnevictable(), 
below race can occur.


    CPU1                                       CPU2
==================================================================
1. rotate_reclaimable_page()
2. PageUnevictable(page) return 0
3. local_irq_save()
4. pagevec_move_tail()
                                       SetPageUnevictable()   //mlock?
                                       move to unevictable list
5. spin_lock(&zone->lru_lock);
6. list_move_tail(); (move to inactive list)



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

---
 mm/swap.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: b/mm/swap.c
===================================================================
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -116,7 +116,7 @@ static void pagevec_move_tail(struct pag
 			zone = pagezone;
 			spin_lock(&zone->lru_lock);
 		}
-		if (PageLRU(page) && !PageActive(page)) {
+		if (PageLRU(page) && !PageActive(page) && !PageUnevictable(page)) {
 			int lru = page_is_file_cache(page);
 			list_move_tail(&page->lru, &zone->lru[lru].list);
 			pgmoved++;




--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [resend][PATCH -mm] split_lru: fix pagevec_move_tail() doesn't treat unevictable page

[Rik van Riel]

On Tue, 01 Jul 2008 17:26:51 +0900
KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:

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

Acked-by: Rik van Riel <riel@redhat.com>

Good catch!
 
> @@ -116,7 +116,7 @@ static void pagevec_move_tail(struct pag
>  			zone = pagezone;
>  			spin_lock(&zone->lru_lock);
>  		}
> -		if (PageLRU(page) && !PageActive(page)) {
> +		if (PageLRU(page) && !PageActive(page) && !PageUnevictable(page)) {
>  			int lru = page_is_file_cache(page);
>  			list_move_tail(&page->lru, &zone->lru[lru].list);
>  			pgmoved++;

-- 
All rights reversed.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [resend][PATCH -mm] split_lru: fix pagevec_move_tail() doesn't treat unevictable page

[MinChan Kim]

Hi, Rik and Kosaki-san

I want to know exact race situation for remaining git log.
As you know, git log is important for me who is newbie to understand source

There are many possibility in this race problem.

Did you use hugepage in this test ?
I think that If you used hugepage, it seems to happen following race.

--------------

CPU1                                                           CPU2

shm_unlock
scan_mapping_unevictable_pages
check_move_unevictable_page
ClearPageUnevictable                                 rotate_reclaimable_page

PageUnevictable(page) return 0
SetPageUnevictable
list_move(LRU_UNEVICTABLE)

local_irq_save

pagevec_move_tail

Do you think it is possible ?

On Tue, Jul 1, 2008 at 10:38 PM, Rik van Riel <riel@redhat.com> wrote:
> On Tue, 01 Jul 2008 17:26:51 +0900
> KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:
>
>> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>
> Acked-by: Rik van Riel <riel@redhat.com>
>
> Good catch!
>
>> @@ -116,7 +116,7 @@ static void pagevec_move_tail(struct pag
>>                       zone = pagezone;
>>                       spin_lock(&zone->lru_lock);
>>               }
>> -             if (PageLRU(page) && !PageActive(page)) {
>> +             if (PageLRU(page) && !PageActive(page) && !PageUnevictable(page)) {
>>                       int lru = page_is_file_cache(page);
>>                       list_move_tail(&page->lru, &zone->lru[lru].list);
>>                       pgmoved++;
>
> --
> All rights reversed.
>



-- 
Kinds regards,
MinChan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [resend][PATCH -mm] split_lru: fix pagevec_move_tail() doesn't treat unevictable page

[MinChan Kim]

Sorry, it seems mail sender problem.
Resend

 CPU1                                                           CPU2

 shm_unlock
 scan_mapping_unevictable_pages
 check_move_unevictable_page
 ClearPageUnevictable
                                                      rotate_reclaimable_page

PageUnevictable(page) return 0
 SetPageUnevictable
 list_move(LRU_UNEVICTABLE)

                                                      local_irq_save
                                                      pagevec_move_tail


On Wed, Jul 2, 2008 at 9:39 AM, MinChan Kim <minchan.kim@gmail.com> wrote:
> Hi, Rik and Kosaki-san
>
> I want to know exact race situation for remaining git log.
> As you know, git log is important for me who is newbie to understand source
>
> There are many possibility in this race problem.
>
> Did you use hugepage in this test ?
> I think that If you used hugepage, it seems to happen following race.
>
> --------------
>
> CPU1                                                           CPU2
>
> shm_unlock
> scan_mapping_unevictable_pages
> check_move_unevictable_page
> ClearPageUnevictable                                 rotate_reclaimable_page
>
> PageUnevictable(page) return 0
> SetPageUnevictable
> list_move(LRU_UNEVICTABLE)
>
> local_irq_save
>
> pagevec_move_tail
>
> Do you think it is possible ?
>
> On Tue, Jul 1, 2008 at 10:38 PM, Rik van Riel <riel@redhat.com> wrote:
>> On Tue, 01 Jul 2008 17:26:51 +0900
>> KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:
>>
>>> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>>
>> Acked-by: Rik van Riel <riel@redhat.com>
>>
>> Good catch!
>>
>>> @@ -116,7 +116,7 @@ static void pagevec_move_tail(struct pag
>>>                       zone = pagezone;
>>>                       spin_lock(&zone->lru_lock);
>>>               }
>>> -             if (PageLRU(page) && !PageActive(page)) {
>>> +             if (PageLRU(page) && !PageActive(page) && !PageUnevictable(page)) {
>>>                       int lru = page_is_file_cache(page);
>>>                       list_move_tail(&page->lru, &zone->lru[lru].list);
>>>                       pgmoved++;
>>
>> --
>> All rights reversed.
>>
>
>
>
> --
> Kinds regards,
> MinChan Kim
>



-- 
Kinds regards,
MinChan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [resend][PATCH -mm] split_lru: fix pagevec_move_tail() doesn't treat unevictable page

[MinChan Kim]

So sorry to make a noisy . :-<

Gmail is mangling my mail.
I hope this is last.

 CPU1                                                           CPU2

 shm_unlock
 scan_mapping_unevictable_pages
 check_move_unevictable_page
 ClearPageUnevictable
                                                      rotate_reclaimable_page
							PageUnevictable(page) return 0

 SetPageUnevictable
 list_move(LRU_UNEVICTABLE)

                                                      local_irq_save
                                                      pagevec_move_tail



On Wed, Jul 2, 2008 at 9:49 AM, MinChan Kim <minchan.kim@gmail.com> wrote:
> Sorry, it seems mail sender problem.
> Resend
>
>  CPU1                                                           CPU2
>
>  shm_unlock
>  scan_mapping_unevictable_pages
>  check_move_unevictable_page
>  ClearPageUnevictable
>                                                      rotate_reclaimable_page
>
> PageUnevictable(page) return 0
>  SetPageUnevictable
>  list_move(LRU_UNEVICTABLE)
>
>                                                      local_irq_save
>                                                      pagevec_move_tail
>
>
> On Wed, Jul 2, 2008 at 9:39 AM, MinChan Kim <minchan.kim@gmail.com> wrote:
>> Hi, Rik and Kosaki-san
>>
>> I want to know exact race situation for remaining git log.
>> As you know, git log is important for me who is newbie to understand source
>>
>> There are many possibility in this race problem.
>>
>> Did you use hugepage in this test ?
>> I think that If you used hugepage, it seems to happen following race.
>>
>> --------------
>>
>> CPU1                                                           CPU2
>>
>> shm_unlock
>> scan_mapping_unevictable_pages
>> check_move_unevictable_page
>> ClearPageUnevictable                                 rotate_reclaimable_page
>>
>> PageUnevictable(page) return 0
>> SetPageUnevictable
>> list_move(LRU_UNEVICTABLE)
>>
>> local_irq_save
>>
>> pagevec_move_tail
>>
>> Do you think it is possible ?
>>
>> On Tue, Jul 1, 2008 at 10:38 PM, Rik van Riel <riel@redhat.com> wrote:
>>> On Tue, 01 Jul 2008 17:26:51 +0900
>>> KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:
>>>
>>>> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>>>
>>> Acked-by: Rik van Riel <riel@redhat.com>
>>>
>>> Good catch!
>>>
>>>> @@ -116,7 +116,7 @@ static void pagevec_move_tail(struct pag
>>>>                       zone = pagezone;
>>>>                       spin_lock(&zone->lru_lock);
>>>>               }
>>>> -             if (PageLRU(page) && !PageActive(page)) {
>>>> +             if (PageLRU(page) && !PageActive(page) && !PageUnevictable(page)) {
>>>>                       int lru = page_is_file_cache(page);
>>>>                       list_move_tail(&page->lru, &zone->lru[lru].list);
>>>>                       pgmoved++;
>>>
>>> --
>>> All rights reversed.
>>>
>>
>>
>>
>> --
>> Kinds regards,
>> MinChan Kim
>>
>
>
>
> --
> Kinds regards,
> MinChan Kim
>



-- 
Kinds regards,
MinChan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [resend][PATCH -mm] split_lru: fix pagevec_move_tail() doesn't treat unevictable page

[KOSAKI Motohiro]

Hi Kim-san,

> Hi, Rik and Kosaki-san
> 
> I want to know exact race situation for remaining git log.
> As you know, git log is important for me who is newbie to understand source
> 
> There are many possibility in this race problem.
> 
> Did you use hugepage in this test ?
> I think that If you used hugepage, it seems to happen following race.

I don't use hugepage. but use SYSV-shmem.
so following scenario is very reasonable.

OK.
I resend my patch with following description.


> 
> --------------
> 
> CPU1                                                           CPU2
> 
> shm_unlock
> scan_mapping_unevictable_pages
> check_move_unevictable_page
> ClearPageUnevictable                                 rotate_reclaimable_page
> 
> PageUnevictable(page) return 0
> SetPageUnevictable
> list_move(LRU_UNEVICTABLE)
> 
> local_irq_save
> 
> pagevec_move_tail
> 
> Do you think it is possible ?


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [resend][PATCH -mm] split_lru: fix pagevec_move_tail() doesn't treat unevictable page

[MinChan Kim]

On Wed, Jul 2, 2008 at 12:30 PM, KOSAKI Motohiro
<kosaki.motohiro@jp.fujitsu.com> wrote:
> Hi Kim-san,
>
>> Hi, Rik and Kosaki-san
>>
>> I want to know exact race situation for remaining git log.
>> As you know, git log is important for me who is newbie to understand source
>>
>> There are many possibility in this race problem.
>>
>> Did you use hugepage in this test ?
>> I think that If you used hugepage, it seems to happen following race.
>
> I don't use hugepage. but use SYSV-shmem.
> so following scenario is very reasonable.

It is not reasonable if you don't use hugepage.
That's because file's address_space is still unevictable.
Am I missing your point?

I think following case is more reasonable rather than it,
Please, Let you review this scenario.
---

CPU1							CPU2

shrink_[in]active_list
cull_unevictable_page
putback_lru_page
TestClearPageUnevicetable
						rotate_reclaimable_page
						!PageUnevictable(page)
add_page_to_unevictable_list
						pagevec_move_tail



> OK.
> I resend my patch with following description.
>
>
>>
>> --------------
>>
>> CPU1                                                           CPU2
>>
>> shm_unlock
>> scan_mapping_unevictable_pages
>> check_move_unevictable_page
>> ClearPageUnevictable                                 rotate_reclaimable_page
>>
>> PageUnevictable(page) return 0
>> SetPageUnevictable
>> list_move(LRU_UNEVICTABLE)
>>
>> local_irq_save
>>
>> pagevec_move_tail
>>
>> Do you think it is possible ?
>
>
>



-- 
Kinds regards,
MinChan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>