Re: [PATCH] export the function kmap_flush_unused.

Re: [PATCH] export the function kmap_flush_unused.

[Chintan Pandya]

On 08/08/2014 02:46 PM, Sha, Ruibin wrote:
> export the function kmap_flush_unused.
>
> Scenario: When graphic driver need high memory spece, we use alloc_pages()
> to allocate. But if the allocated page has just been
> mapped in the KMAP space(like first kmap then kunmap) and
> no flush page happened on PKMAP, the page virtual address is
> not NULL.Then when we get that page and set page attribute like
> set_memory_uc and set_memory_wc, we hit error.

Could you explain your scenario with more details ? set_memory_* should 
be applied on mapped address. And in attempt to map your page (which was 
just kmap and kunmap'ed), it will overwrite the previous mappings.

Moreover, in my view, kmap_flush_unused is just helping us in keeping 
the cache clean for kmap virtual addresses if they are unmapped. Is it 
serving any more purpose here ?

>
> fix: For that scenario,when we get the allocated page and its virtual
> address is not NULL, we would like first flush that page.
> So need export that function kmap_flush_unused.
>
> Signed-off-by: sha, ruibin <ruibin.sha@intel.com>
>
> ---
> mm/highmem.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/mm/highmem.c b/mm/highmem.c
> index b32b70c..511299b 100644
> --- a/mm/highmem.c
> +++ b/mm/highmem.c
> @@ -156,6 +156,7 @@ void kmap_flush_unused(void)
> flush_all_zero_pkmaps();
> unlock_kmap();
> }
> +EXPORT_SYMBOL(kmap_flush_unused);
This symbol is already extern'ed. Is it not sufficient for your case ?
>
> static inline unsigned long map_new_virtual(struct page *page)
> {
> --
> 1.7.9.5
>
> Best Regards
>
> ---------------------------------------------------------------
>
> Sha, Rui bin ( Robin )
>
> +86 13817890945
>
> Android System Integration Shanghai
>


-- 
Chintan Pandya

QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of the Code Aurora Forum, hosted by The Linux Foundation

RE: [PATCH] export the function kmap_flush_unused.

[Sha, Ruibin]

Hi Chintan,
Thank you very much for your timely and kindly response and comments.

Here is more detail about our Scenario:

    We have a big driver on Android product. The driver allocates lots of
    DDR pages. When applications mmap a file exported from the driver,
    driver would mmap the pages to the application space, usually with
    uncachable prot.
    On ia32/x86_64 arch, we have to avoid page cache alias issue. When
    driver allocates the pages, it would change page original mapping in
    page table with uncachable prot. Sometimes, the allocated page was
    used by kmap/kunmap. After kunmap, the page is still mapped in KMAP
    space. The entries in KMAP page table are not cleaned up until a
    kernel thread flushes the freed KMAP pages(usually it is woken up by kunmap).
    It means the driver need  force to flush the KMAP page table entries before mapping pages to
    application space to be used. Otherwise, there is a race to create
    cache alias.

    To resolve this issue, we need export function kmap_flush_unused as
    the driver is compiled as module. Then, the driver calls
    kmap_flush_unused if the allocated pages are in HIGHMEM and being
    used by kmap.

Thanks again!

Best Regards
---------------------------------------------------------------
Sha, Rui bin ( Robin )
+86 13817890945
Android System Integration Shanghai

-----Original Message-----
From: Chintan Pandya [mailto:cpandya@codeaurora.org] 
Sent: Friday, August 8, 2014 9:40 PM
To: Sha, Ruibin
Cc: linux-kernel@vger.kernel.org; linux-mm@kvack.org; mel@csn.ul.ie; a.p.zijlstra@chello.nl; mgorman@suse.de; mingo@redhat.com; Zhang, Yanmin; He, Bo
Subject: Re: [PATCH] export the function kmap_flush_unused.

On 08/08/2014 02:46 PM, Sha, Ruibin wrote:
> export the function kmap_flush_unused.
>
> Scenario: When graphic driver need high memory spece, we use 
> alloc_pages() to allocate. But if the allocated page has just been 
> mapped in the KMAP space(like first kmap then kunmap) and no flush 
> page happened on PKMAP, the page virtual address is not NULL.Then when 
> we get that page and set page attribute like set_memory_uc and 
> set_memory_wc, we hit error.

Could you explain your scenario with more details ? set_memory_* should be applied on mapped address. And in attempt to map your page (which was just kmap and kunmap'ed), it will overwrite the previous mappings.

Moreover, in my view, kmap_flush_unused is just helping us in keeping the cache clean for kmap virtual addresses if they are unmapped. Is it serving any more purpose here ?

>
> fix: For that scenario,when we get the allocated page and its virtual
> address is not NULL, we would like first flush that page.
> So need export that function kmap_flush_unused.
>
> Signed-off-by: sha, ruibin <ruibin.sha@intel.com>
>
> ---
> mm/highmem.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/mm/highmem.c b/mm/highmem.c
> index b32b70c..511299b 100644
> --- a/mm/highmem.c
> +++ b/mm/highmem.c
> @@ -156,6 +156,7 @@ void kmap_flush_unused(void)
> flush_all_zero_pkmaps();
> unlock_kmap();
> }
> +EXPORT_SYMBOL(kmap_flush_unused);
This symbol is already extern'ed. Is it not sufficient for your case ?
>
> static inline unsigned long map_new_virtual(struct page *page)
> {
> --
> 1.7.9.5
>
> Best Regards
>
> ---------------------------------------------------------------
>
> Sha, Rui bin ( Robin )
>
> +86 13817890945
>
> Android System Integration Shanghai
>


-- 
Chintan Pandya

QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of the Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] export the function kmap_flush_unused.

[Zhang, Yanmin]

On 2014/8/11 9:26, Sha, Ruibin wrote:
> Hi Chintan,
> Thank you very much for your timely and kindly response and comments.
>
> Here is more detail about our Scenario:
>
>      We have a big driver on Android product. The driver allocates lots of
>      DDR pages. When applications mmap a file exported from the driver,
>      driver would mmap the pages to the application space, usually with
>      uncachable prot.
>      On ia32/x86_64 arch, we have to avoid page cache alias issue. When
>      driver allocates the pages, it would change page original mapping in
>      page table with uncachable prot. Sometimes, the allocated page was
>      used by kmap/kunmap. After kunmap, the page is still mapped in KMAP
>      space. The entries in KMAP page table are not cleaned up until a
>      kernel thread flushes the freed KMAP pages(usually it is woken up by kunmap).
>      It means the driver need  force to flush the KMAP page table entries before mapping pages to
>      application space to be used. Otherwise, there is a race to create
>      cache alias.
>
>      To resolve this issue, we need export function kmap_flush_unused as
>      the driver is compiled as module. Then, the driver calls
>      kmap_flush_unused if the allocated pages are in HIGHMEM and being
>      used by kmap.
>
> Thanks again!
>
> Best Regards
> ---------------------------------------------------------------
> Sha, Rui bin ( Robin )
> +86 13817890945
> Android System Integration Shanghai
>
> -----Original Message-----
> From: Chintan Pandya [mailto:cpandya@codeaurora.org]
> Sent: Friday, August 8, 2014 9:40 PM
> To: Sha, Ruibin
> Cc: linux-kernel@vger.kernel.org; linux-mm@kvack.org; mel@csn.ul.ie; a.p.zijlstra@chello.nl; mgorman@suse.de; mingo@redhat.com; Zhang, Yanmin; He, Bo
> Subject: Re: [PATCH] export the function kmap_flush_unused.
>
> On 08/08/2014 02:46 PM, Sha, Ruibin wrote:
>> export the function kmap_flush_unused.
>>
>> Scenario: When graphic driver need high memory spece, we use
>> alloc_pages() to allocate. But if the allocated page has just been
>> mapped in the KMAP space(like first kmap then kunmap) and no flush
>> page happened on PKMAP, the page virtual address is not NULL.Then when
>> we get that page and set page attribute like set_memory_uc and
>> set_memory_wc, we hit error.
> Could you explain your scenario with more details ? set_memory_* should be applied on mapped address. And in attempt to map your page (which was just kmap and kunmap'ed), it will overwrite the previous mappings.
>
> Moreover, in my view, kmap_flush_unused is just helping us in keeping the cache clean for kmap virtual addresses if they are unmapped. Is it serving any more purpose here ?

It depends on how to define 'clean' here. It resets pkmap_count[i] to 0,
and cleans up page table entries used by PKMAP. Here, our scenario is
caused by the late page table entry cleanup as driver need avoid page
cache alias.

>
>> fix: For that scenario,when we get the allocated page and its virtual
>> address is not NULL, we would like first flush that page.
>> So need export that function kmap_flush_unused.
>>
>> Signed-off-by: sha, ruibin <ruibin.sha@intel.com>
>>
>> ---
>> mm/highmem.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/mm/highmem.c b/mm/highmem.c
>> index b32b70c..511299b 100644
>> --- a/mm/highmem.c
>> +++ b/mm/highmem.c
>> @@ -156,6 +156,7 @@ void kmap_flush_unused(void)
>> flush_all_zero_pkmaps();
>> unlock_kmap();
>> }
>> +EXPORT_SYMBOL(kmap_flush_unused);
> This symbol is already extern'ed. Is it not sufficient for your case ?

We want to call it in driver module. extern is not enough.

Thanks,
Yanmin

Re: [PATCH] export the function kmap_flush_unused.

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 1704 bytes --]

On Mon, Aug 11, 2014 at 01:26:45AM +0000, Sha, Ruibin wrote:
> Hi Chintan,
> Thank you very much for your timely and kindly response and comments.
> 
> Here is more detail about our Scenario:
> 
>     We have a big driver on Android product. The driver allocates lots of
>     DDR pages. When applications mmap a file exported from the driver,
>     driver would mmap the pages to the application space, usually with
>     uncachable prot.
>     On ia32/x86_64 arch, we have to avoid page cache alias issue. When
>     driver allocates the pages, it would change page original mapping in
>     page table with uncachable prot. Sometimes, the allocated page was
>     used by kmap/kunmap. After kunmap, the page is still mapped in KMAP
>     space. The entries in KMAP page table are not cleaned up until a
>     kernel thread flushes the freed KMAP pages(usually it is woken up by kunmap).
>     It means the driver need  force to flush the KMAP page table entries before mapping pages to
>     application space to be used. Otherwise, there is a race to create
>     cache alias.
> 
>     To resolve this issue, we need export function kmap_flush_unused as
>     the driver is compiled as module. Then, the driver calls
>     kmap_flush_unused if the allocated pages are in HIGHMEM and being
>     used by kmap.

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

That said, it sounds like you want set_memory_() to call
kmap_flush_unused(). Because this race it not at all specific to your
usage, it could happen to any set_memory_() site, right?

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] export the function kmap_flush_unused.

[Zhang, Yanmin]

On 2014/8/11 19:54, Peter Zijlstra wrote:
> On Mon, Aug 11, 2014 at 01:26:45AM +0000, Sha, Ruibin wrote:
>> Hi Chintan,
>> Thank you very much for your timely and kindly response and comments.
>>
>> Here is more detail about our Scenario:
>>
>>      We have a big driver on Android product. The driver allocates lots of
>>      DDR pages. When applications mmap a file exported from the driver,
>>      driver would mmap the pages to the application space, usually with
>>      uncachable prot.
>>      On ia32/x86_64 arch, we have to avoid page cache alias issue. When
>>      driver allocates the pages, it would change page original mapping in
>>      page table with uncachable prot. Sometimes, the allocated page was
>>      used by kmap/kunmap. After kunmap, the page is still mapped in KMAP
>>      space. The entries in KMAP page table are not cleaned up until a
>>      kernel thread flushes the freed KMAP pages(usually it is woken up by kunmap).
>>      It means the driver need  force to flush the KMAP page table entries before mapping pages to
>>      application space to be used. Otherwise, there is a race to create
>>      cache alias.
>>
>>      To resolve this issue, we need export function kmap_flush_unused as
>>      the driver is compiled as module. Then, the driver calls
>>      kmap_flush_unused if the allocated pages are in HIGHMEM and being
>>      used by kmap.
> A: Because it messes up the order in which people normally read text.
> Q: Why is top-posting such a bad thing?
> A: Top-posting.
> Q: What is the most annoying thing in e-mail?

Sorry, Peter. Ruibin is a new guy in LKML community. He uses outlook
to send emails. He would improve that.

>
> That said, it sounds like you want set_memory_() to call
> kmap_flush_unused(). Because this race it not at all specific to your
> usage, it could happen to any set_memory_() site, right?
No. set_memory_() assumes the memory is not in HIGHMEM.
This scenario is driver allocates HIGHMEM pages, which are kmapped before.
Kernel uses a lazy method when kunmap a HIGHMEM page.
The pages are not unmapped from KMAP page table entries immediately.
When next kmap calling uses the same entry, kernel would change pte.
Or when change_page_attr_set_clr is called.

Our big driver doesn't call change_page_attr_set_clr when mmap the
pages with UNCACHABLE prot. It need call kmap_flush_unused directly after
allocating HIGHMEM pages.

Thanks for the kind comments.

Yanmin