Re: [PATCH 4/5] mm: simplify reclaim path for MADV_FREE

[yalin wang]

> On Oct 27, 2015, at 16:52, yalin wang <yalin.wang2010@xxxxxxxxx> wrote:
> 
> 
>> On Oct 27, 2015, at 16:10, Minchan Kim <minchan@xxxxxxxxxx> wrote:
>> 
>> On Tue, Oct 27, 2015 at 03:39:16PM +0800, yalin wang wrote:
>>> 
>>>> On Oct 27, 2015, at 15:09, Minchan Kim <minchan@xxxxxxxxxx> wrote:
>>>> 
>>>> Hello Yalin,
>>>> 
>>>> Sorry for missing you in Cc list.
>>>> IIRC, mails to send your previous mail address(Yalin.Wang@xxxxxxxxxxxxxx)
>>>> were returned.
>>>> 
>>>> You added comment bottom line so I'm not sure what PageDirty you meant.
>>>> 
>>>>> it is wrong here if you only check PageDirty() to decide if the page is freezable or not .
>>>>> The Anon page are shared by multiple process, _mapcount > 1 ,
>>>>> so you must check all pt_dirty bit during page_referenced() function,
>>>>> see this mail thread:
>>>>> http://ns1.ske-art.com/lists/kernel/msg1934021.html
>>>> 
>>>> If one of pte among process sharing the page was dirty, the dirtiness should
>>>> be propagated from pte to PG_dirty by try_to_unmap_one.
>>>> IOW, if the page doesn't have PG_dirty flag, it means all of process did
>>>> MADV_FREE.
>>>> 
>>>> Am I missing something from you question?
>>>> If so, could you show exact scenario I am missing?
>>>> 
>>>> Thanks for the interest.
>>> oh, yeah , that is right , i miss that , pte_dirty will propagate to PG_dirty ,
>>> so that is correct .
>>> Generic to say this patch move set_page_dirty() from add_to_swap() to 
>>> try_to_unmap(), i think can change a little about this patch:
>>> 
>>> @@ -1476,6 +1446,8 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
>>> 				ret = SWAP_FAIL;
>>> 				goto out_unmap;
>>> 			}
>>> +			if (!PageDirty(page))
>>> +				SetPageDirty(page);
>>> 			if (list_empty(&mm->mmlist)) {
>>> 				spin_lock(&mmlist_lock);
>>> 				if (list_empty(&mm->mmlist))
>>> 
>>> i think this 2 lines can be removed ,
>>> since  pte_dirty have propagated to set_page_dirty() , we donât need this line here ,
>>> otherwise you will always dirty a AnonPage, even it is clean,
>>> then we will page out this clean page to swap partition one more , this is not needed.
>>> am i understanding correctly ?
>> 
>> Your understanding is correct.
>> I will fix it in next spin.
>> 
>>> 
>>> By the way, please change my mail address to yalin.wang2010@xxxxxxxxx in CC list .
>>> Thanks a lot. :) 
>> 
>> Thanks for the review!
> 
> i have a look at the old mail list , i recall the scenario that multiple processes share a AnonPage 
> special case :
> 
> for example Process A have a AnonPage map like this:
> 	! pte_dirty() && PageDirty()==1   (this is possible after read fault happened on swap entry, and try_to_free_swap() succeed.)
> Process A  do a fork() , New process is called B .
> Then A  syscall(MADV_FREE) on the page .
> At this time, page table like this:
> 
> A  ! pte_dirty() && PageDirty() == 0  && PageSwapCache() == 0
> 
> B ! pte_dirty() && PageDirty() == 0  && PageSwapCache() == 0
> 
> This means this page is freeable , and can be freed during page reclaim.
> This is not fair for Process B . Since B donât call syscall(MADV_FREE) ,
> its page should not be discard .  Will cause some strange behaviour if happened .
> 
> This is discussed by 
> http://www.serverphorums.com/read.php?12,1220840
> but i donât know why the patch is not merged .
> 
> Thanks 
oh, i have see 0b502297d1cc26e09b98955b4efa728be1c48921
this commit merged , then this problem should be fixed by this method.
ignore this mail. :)

Thanks a lot .





--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/