Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1438557 > unrolled thread

[PATCH 1/1] arm64/hugetlb: clear PG_dcache_clean if the page is dirty when munmap

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/1] arm64/hugetlb: clear PG_dcache_clean if the page is dirty when munmap Zhen Lei <thunder.leizhen@huawei.com> - 2016-07-07 14:20 +0200
    Re: [PATCH 1/1] arm64/hugetlb: clear PG_dcache_clean if the page is  dirty when munmap Catalin Marinas <catalin.marinas@arm.com> - 2016-07-07 17:40 +0200
      Re: [PATCH 1/1] arm64/hugetlb: clear PG_dcache_clean if the page is  dirty when munmap "Leizhen (ThunderTown)" <thunder.leizhen@huawei.com> - 2016-07-08 05:40 +0200
        Re: [PATCH 1/1] arm64/hugetlb: clear PG_dcache_clean if the page is  dirty when munmap Catalin Marinas <catalin.marinas@arm.com> - 2016-07-08 16:00 +0200
          Re: [PATCH 1/1] arm64/hugetlb: clear PG_dcache_clean if the page is  dirty when munmap "Leizhen (ThunderTown)" <thunder.leizhen@huawei.com> - 2016-07-08 17:30 +0200
            Re: [PATCH 1/1] arm64/hugetlb: clear PG_dcache_clean if the page is  dirty when munmap Catalin Marinas <catalin.marinas@arm.com> - 2016-07-08 18:20 +0200

#1438557 — [PATCH 1/1] arm64/hugetlb: clear PG_dcache_clean if the page is dirty when munmap

At present, PG_dcache_clean is only cleared when the related huge page
is about to be freed. But sometimes, there maybe a process is in charge
to copy binary codes into a shared memory, and notifies other processes
to execute base on that. For the first time, there is no problem, because
the default value of page->flags is PG_dcache_clean cleared. So the cache
will be maintained at the time of set_pte_at for other processes. But if
the content of the shared memory have been updated again, there is no
cache operations, because the PG_dcache_clean is still set.

For example:
Process A
	open a hugetlbfs file
	mmap it as a shared memory
	copy some binary codes into it
	munmap

Process B
	open the hugetlbfs file
	mmap it as a shared memory, executable
	invoke the functions in the shared memory
	munmap

repeat the above steps.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 arch/arm64/mm/hugetlbpage.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
index 2e49bd2..547b158 100644
--- a/arch/arm64/mm/hugetlbpage.c
+++ b/arch/arm64/mm/hugetlbpage.c
@@ -201,6 +201,7 @@ pte_t huge_ptep_get_and_clear(struct mm_struct *mm,
 			      unsigned long addr, pte_t *ptep)
 {
 	pte_t pte;
+	struct page *page;

 	if (pte_cont(*ptep)) {
 		int ncontig, i;
@@ -222,12 +223,21 @@ pte_t huge_ptep_get_and_clear(struct mm_struct *mm,
 			if (pte_dirty(ptep_get_and_clear(mm, addr, cpte)))
 				is_dirty = true;
 		}
-		if (is_dirty)
-			return pte_mkdirty(pte);
-		else
-			return pte;
+		if (is_dirty) {
+			pte = pte_mkdirty(pte);
+			page = pte_page(pte);
+			clear_bit(PG_dcache_clean, &page->flags);
+		}
+
+		return pte;
 	} else {
-		return ptep_get_and_clear(mm, addr, ptep);
+		pte = ptep_get_and_clear(mm, addr, ptep);
+		if (huge_pte_dirty(pte)) {
+			page = pte_page(pte);
+			clear_bit(PG_dcache_clean, &page->flags);
+		}
+
+		return pte;
 	}
 }

--
2.5.0

[toc] | [next] | [standalone]

#1438686 — Re: [PATCH 1/1] arm64/hugetlb: clear PG_dcache_clean if the page is dirty when munmap

On Thu, Jul 07, 2016 at 08:09:04PM +0800, Zhen Lei wrote:
> At present, PG_dcache_clean is only cleared when the related huge page
> is about to be freed. But sometimes, there maybe a process is in charge
> to copy binary codes into a shared memory, and notifies other processes
> to execute base on that. For the first time, there is no problem, because
> the default value of page->flags is PG_dcache_clean cleared. So the cache
> will be maintained at the time of set_pte_at for other processes. But if
> the content of the shared memory have been updated again, there is no
> cache operations, because the PG_dcache_clean is still set.
> 
> For example:
> Process A
> 	open a hugetlbfs file
> 	mmap it as a shared memory
> 	copy some binary codes into it
> 	munmap
> 
> Process B
> 	open the hugetlbfs file
> 	mmap it as a shared memory, executable
> 	invoke the functions in the shared memory
> 	munmap
> 
> repeat the above steps.

Does this work as you would expect with small pages (and for example
shared file mmap)? I don't want to have a different behaviour between
small and huge pages.

-- 
Catalin

[toc] | [prev] | [next] | [standalone]

#1439088 — Re: [PATCH 1/1] arm64/hugetlb: clear PG_dcache_clean if the page is dirty when munmap

On 2016/7/7 23:37, Catalin Marinas wrote:
> On Thu, Jul 07, 2016 at 08:09:04PM +0800, Zhen Lei wrote:
>> At present, PG_dcache_clean is only cleared when the related huge page
>> is about to be freed. But sometimes, there maybe a process is in charge
>> to copy binary codes into a shared memory, and notifies other processes
>> to execute base on that. For the first time, there is no problem, because
>> the default value of page->flags is PG_dcache_clean cleared. So the cache
>> will be maintained at the time of set_pte_at for other processes. But if
>> the content of the shared memory have been updated again, there is no
>> cache operations, because the PG_dcache_clean is still set.
>>
>> For example:
>> Process A
>> 	open a hugetlbfs file
>> 	mmap it as a shared memory
>> 	copy some binary codes into it
>> 	munmap
>>
>> Process B
>> 	open the hugetlbfs file
>> 	mmap it as a shared memory, executable
>> 	invoke the functions in the shared memory
>> 	munmap
>>
>> repeat the above steps.
> 
> Does this work as you would expect with small pages (and for example
> shared file mmap)? I don't want to have a different behaviour between
> small and huge pages.

The small pages also have this problem, I will try to fix it too.

> 

[toc] | [prev] | [next] | [standalone]

#1439471 — Re: [PATCH 1/1] arm64/hugetlb: clear PG_dcache_clean if the page is dirty when munmap

On Fri, Jul 08, 2016 at 11:36:57AM +0800, Leizhen (ThunderTown) wrote:
> On 2016/7/7 23:37, Catalin Marinas wrote:
> > On Thu, Jul 07, 2016 at 08:09:04PM +0800, Zhen Lei wrote:
> >> At present, PG_dcache_clean is only cleared when the related huge page
> >> is about to be freed. But sometimes, there maybe a process is in charge
> >> to copy binary codes into a shared memory, and notifies other processes
> >> to execute base on that. For the first time, there is no problem, because
> >> the default value of page->flags is PG_dcache_clean cleared. So the cache
> >> will be maintained at the time of set_pte_at for other processes. But if
> >> the content of the shared memory have been updated again, there is no
> >> cache operations, because the PG_dcache_clean is still set.
> >>
> >> For example:
> >> Process A
> >> 	open a hugetlbfs file
> >> 	mmap it as a shared memory
> >> 	copy some binary codes into it
> >> 	munmap
> >>
> >> Process B
> >> 	open the hugetlbfs file
> >> 	mmap it as a shared memory, executable
> >> 	invoke the functions in the shared memory
> >> 	munmap
> >>
> >> repeat the above steps.
> > 
> > Does this work as you would expect with small pages (and for example
> > shared file mmap)? I don't want to have a different behaviour between
> > small and huge pages.
> 
> The small pages also have this problem, I will try to fix it too.

Have you run the above tests on a standard file (with small pages)? It's
strange that we haven't hit this so far with gcc or something else
generating code (unless they don't use mmap but just sequential writes).

If both cases need solving, we might better move the fix in the
__sync_icache_dcache() function. Untested:

------------8<----------------
diff --git a/arch/arm64/mm/flush.c b/arch/arm64/mm/flush.c
index dbd12ea8ce68..c753fa804165 100644
--- a/arch/arm64/mm/flush.c
+++ b/arch/arm64/mm/flush.c
@@ -75,7 +75,8 @@ void __sync_icache_dcache(pte_t pte, unsigned long addr)
 	if (!page_mapping(page))
 		return;
 
-	if (!test_and_set_bit(PG_dcache_clean, &page->flags))
+	if (!test_and_set_bit(PG_dcache_clean, &page->flags) ||
+	    PageDirty(page))
 		sync_icache_aliases(page_address(page),
 				    PAGE_SIZE << compound_order(page));
 	else if (icache_is_aivivt())
----------------8<---------------------

BTW, can you make your tests (source) available somewhere?

Thanks.

-- 
Catalin

[toc] | [prev] | [next] | [standalone]

#1439568 — Re: [PATCH 1/1] arm64/hugetlb: clear PG_dcache_clean if the page is dirty when munmap

[Multipart message — attachments visible in raw view] — view raw

On 2016/7/8 21:54, Catalin Marinas wrote:
> On Fri, Jul 08, 2016 at 11:36:57AM +0800, Leizhen (ThunderTown) wrote:
>> On 2016/7/7 23:37, Catalin Marinas wrote:
>>> On Thu, Jul 07, 2016 at 08:09:04PM +0800, Zhen Lei wrote:
>>>> At present, PG_dcache_clean is only cleared when the related huge page
>>>> is about to be freed. But sometimes, there maybe a process is in charge
>>>> to copy binary codes into a shared memory, and notifies other processes
>>>> to execute base on that. For the first time, there is no problem, because
>>>> the default value of page->flags is PG_dcache_clean cleared. So the cache
>>>> will be maintained at the time of set_pte_at for other processes. But if
>>>> the content of the shared memory have been updated again, there is no
>>>> cache operations, because the PG_dcache_clean is still set.
>>>>
>>>> For example:
>>>> Process A
>>>> 	open a hugetlbfs file
>>>> 	mmap it as a shared memory
>>>> 	copy some binary codes into it
>>>> 	munmap
>>>>
>>>> Process B
>>>> 	open the hugetlbfs file
>>>> 	mmap it as a shared memory, executable
>>>> 	invoke the functions in the shared memory
>>>> 	munmap
>>>>
>>>> repeat the above steps.
>>>
>>> Does this work as you would expect with small pages (and for example
>>> shared file mmap)? I don't want to have a different behaviour between
>>> small and huge pages.
>>
>> The small pages also have this problem, I will try to fix it too.
> 
> Have you run the above tests on a standard file (with small pages)? It's
> strange that we haven't hit this so far with gcc or something else
> generating code (unless they don't use mmap but just sequential writes).
The test code should be randomly generated, to make sure the context
in ICache is always stale. I have attached the simplified testcase demo.

The main portion is picked as below:
	srand(time(NULL));
	ptr = (unsigned int *)share_mem;
	*ptr++ = 0xd2800000;				//mov x0, #0
	for (i = 0, total = 0; i < 100; i++) {
		value = 0xfff & rand();
		total += value;
		*ptr++ = 0xb1000000 | (value << 10);	//adds x0, x0, #value
	}
	*ptr = 0xd65f03c0;				//ret

> 
> If both cases need solving, we might better move the fix in the
> __sync_icache_dcache() function. Untested:
Yes.

At first I also want to fix it as below. But I'm not sure which time the PageDirty
will be cleared, and if two or more processes mmap it as executable, cache operations
will be duplicated. At present, I really have not found any good place to clear
PG_dcache_clean. So the below modification may be the best choice, concisely and clearly.

> 
> ------------8<----------------
> diff --git a/arch/arm64/mm/flush.c b/arch/arm64/mm/flush.c
> index dbd12ea8ce68..c753fa804165 100644
> --- a/arch/arm64/mm/flush.c
> +++ b/arch/arm64/mm/flush.c
> @@ -75,7 +75,8 @@ void __sync_icache_dcache(pte_t pte, unsigned long addr)
>  	if (!page_mapping(page))
>  		return;
>  
> -	if (!test_and_set_bit(PG_dcache_clean, &page->flags))
> +	if (!test_and_set_bit(PG_dcache_clean, &page->flags) ||
> +	    PageDirty(page))
>  		sync_icache_aliases(page_address(page),
>  				    PAGE_SIZE << compound_order(page));
>  	else if (icache_is_aivivt())
> ----------------8<---------------------
> 
> BTW, can you make your tests (source) available somewhere?
Both cases worked well with this patch.

> 
> Thanks.
> 

[toc] | [prev] | [next] | [standalone]

#1439619 — Re: [PATCH 1/1] arm64/hugetlb: clear PG_dcache_clean if the page is dirty when munmap

On Fri, Jul 08, 2016 at 11:24:26PM +0800, Leizhen (ThunderTown) wrote:
> On 2016/7/8 21:54, Catalin Marinas wrote:
> > On Fri, Jul 08, 2016 at 11:36:57AM +0800, Leizhen (ThunderTown) wrote:
> >> On 2016/7/7 23:37, Catalin Marinas wrote:
> >>> On Thu, Jul 07, 2016 at 08:09:04PM +0800, Zhen Lei wrote:
> >>>> At present, PG_dcache_clean is only cleared when the related huge page
> >>>> is about to be freed. But sometimes, there maybe a process is in charge
> >>>> to copy binary codes into a shared memory, and notifies other processes
> >>>> to execute base on that. For the first time, there is no problem, because
> >>>> the default value of page->flags is PG_dcache_clean cleared. So the cache
> >>>> will be maintained at the time of set_pte_at for other processes. But if
> >>>> the content of the shared memory have been updated again, there is no
> >>>> cache operations, because the PG_dcache_clean is still set.
> >>>>
> >>>> For example:
> >>>> Process A
> >>>> 	open a hugetlbfs file
> >>>> 	mmap it as a shared memory
> >>>> 	copy some binary codes into it
> >>>> 	munmap
> >>>>
> >>>> Process B
> >>>> 	open the hugetlbfs file
> >>>> 	mmap it as a shared memory, executable
> >>>> 	invoke the functions in the shared memory
> >>>> 	munmap
> >>>>
> >>>> repeat the above steps.
> >>>
> >>> Does this work as you would expect with small pages (and for example
> >>> shared file mmap)? I don't want to have a different behaviour between
> >>> small and huge pages.
> >>
> >> The small pages also have this problem, I will try to fix it too.
[...]
> > If both cases need solving, we might better move the fix in the
> > __sync_icache_dcache() function. Untested:
>
> At first I also want to fix it as below. But I'm not sure which time the PageDirty
> will be cleared, and if two or more processes mmap it as executable, cache operations
> will be duplicated. At present, I really have not found any good place to clear
> PG_dcache_clean. So the below modification may be the best choice, concisely and clearly.
> 
> > ------------8<----------------
> > diff --git a/arch/arm64/mm/flush.c b/arch/arm64/mm/flush.c
> > index dbd12ea8ce68..c753fa804165 100644
> > --- a/arch/arm64/mm/flush.c
> > +++ b/arch/arm64/mm/flush.c
> > @@ -75,7 +75,8 @@ void __sync_icache_dcache(pte_t pte, unsigned long addr)
> >  	if (!page_mapping(page))
> >  		return;
> >  
> > -	if (!test_and_set_bit(PG_dcache_clean, &page->flags))
> > +	if (!test_and_set_bit(PG_dcache_clean, &page->flags) ||
> > +	    PageDirty(page))
> >  		sync_icache_aliases(page_address(page),
> >  				    PAGE_SIZE << compound_order(page));
> >  	else if (icache_is_aivivt())
> > ----------------8<---------------------
> > 
> > BTW, can you make your tests (source) available somewhere?
>
> Both cases worked well with this patch.

Now I'm even more confused ;). IIUC, after an msync() in user space we
should flush the pages to disk via write_cache_pages(). This function
calls clear_page_dirty_for_io() after which PageDirty() is no longer
true. I can't tell how a subsequent mmap() can see the written pages as
dirty.

-- 
Catalin

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web