From: Ira Weiny <[email protected]>
Today, kmap_{local_page,atomic}() handle granting access to HIGHMEM
pages without the caller needing to know if the page is HIGHMEM, or not.
Use that existing infrastructure to grant access to PGMAP (PKS)
protected pages.
kmap_{local_page,atomic}() are both thread local mappings so they work
well with the thread specific protections available within PKS.
On the other hand, the kmap() call is not changed. kmap() allows for a
mapping to be shared with other threads, while PKS protections operate
on a thread local basis. For this reason, and the desire to move away
from mappings like this, kmap() is left unsupported.
This behavior is safe because neither of the 2 current DAX-capable
filesystems (ext4 and xfs) perform such global mappings. And known
device drivers that would handle devmap pages are not using kmap(). Any
future filesystems that gain DAX support, or device drivers wanting to
support devmap protected pages will need to use kmap_local_page().
Note: HIGHMEM support is mutually exclusive with PGMAP protection. The
rationale is mainly to reduce complexity, but also because direct-map
exposure is already mitigated by default on HIGHMEM systems because
by definition HIGHMEM systems do not have large capacities of memory
in the direct map.
Cc: Dan Williams <[email protected]>
Cc: Dave Hansen <[email protected]>
Signed-off-by: Ira Weiny <[email protected]>
---
Changes for V10
Include memremap.h because of upstream rework
Changes for V9
From Dan Williams
Update commit message
Clarify why kmap is not 'compatible' with PKS
Explain the HIGHMEM system exclusion more
Remove pgmap_protection_flag_invalid() from kmap
s/pks_mk*/pks_set*/
Changes for V8
Reword commit message
---
include/linux/highmem-internal.h | 6 ++++++
mm/Kconfig | 1 +
2 files changed, 7 insertions(+)
diff --git a/include/linux/highmem-internal.h b/include/linux/highmem-internal.h
index a77be5630209..32ed07c2994b 100644
--- a/include/linux/highmem-internal.h
+++ b/include/linux/highmem-internal.h
@@ -151,6 +151,8 @@ static inline void totalhigh_pages_add(long count)
#else /* CONFIG_HIGHMEM */
+#include <linux/memremap.h>
+
static inline struct page *kmap_to_page(void *addr)
{
return virt_to_page(addr);
@@ -174,6 +176,7 @@ static inline void kunmap(struct page *page)
static inline void *kmap_local_page(struct page *page)
{
+ pgmap_set_readwrite(page);
return page_address(page);
}
@@ -197,6 +200,7 @@ static inline void __kunmap_local(void *addr)
#ifdef ARCH_HAS_FLUSH_ON_KUNMAP
kunmap_flush_on_unmap(addr);
#endif
+ pgmap_set_noaccess(kmap_to_page(addr));
}
static inline void *kmap_atomic(struct page *page)
@@ -206,6 +210,7 @@ static inline void *kmap_atomic(struct page *page)
else
preempt_disable();
pagefault_disable();
+ pgmap_set_readwrite(page);
return page_address(page);
}
@@ -224,6 +229,7 @@ static inline void __kunmap_atomic(void *addr)
#ifdef ARCH_HAS_FLUSH_ON_KUNMAP
kunmap_flush_on_unmap(addr);
#endif
+ pgmap_set_noaccess(kmap_to_page(addr));
pagefault_enable();
if (IS_ENABLED(CONFIG_PREEMPT_RT))
migrate_enable();
diff --git a/mm/Kconfig b/mm/Kconfig
index fe1752e6e76c..616baee3f62d 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -800,6 +800,7 @@ config ZONE_DEVICE
config DEVMAP_ACCESS_PROTECTION
bool "Access protection for memremap_pages()"
depends on NVDIMM_PFN
+ depends on !HIGHMEM
depends on ARCH_HAS_SUPERVISOR_PKEYS
select ARCH_ENABLE_PKS_CONSUMER
default n
--
2.35.1
On Tue, Apr 19, 2022 at 10:06:38AM -0700, [email protected] wrote:
> index a77be5630209..32ed07c2994b 100644
> --- a/include/linux/highmem-internal.h
> +++ b/include/linux/highmem-internal.h
> @@ -151,6 +151,8 @@ static inline void totalhigh_pages_add(long count)
>
> #else /* CONFIG_HIGHMEM */
>
> +#include <linux/memremap.h>
Uuuh, I'd really prefer to avoid having to pull in memremap.h into
basically the whole kernel build.
On Thu, Apr 28, 2022 at 08:50:34AM -0700, Christoph Hellwig wrote:
> On Tue, Apr 19, 2022 at 10:06:38AM -0700, [email protected] wrote:
> > index a77be5630209..32ed07c2994b 100644
> > --- a/include/linux/highmem-internal.h
> > +++ b/include/linux/highmem-internal.h
> > @@ -151,6 +151,8 @@ static inline void totalhigh_pages_add(long count)
> >
> > #else /* CONFIG_HIGHMEM */
> >
> > +#include <linux/memremap.h>
>
> Uuuh, I'd really prefer to avoid having to pull in memremap.h into
> basically the whole kernel build.
>
Fair enough. What should I call a header to store the new pgmap_set_*() calls?
devmap_prot.h
?
Or I could just add the calls into highmem-internal.h. That might be more
appropriate given the new direction of that interface.
I think I'm leaning toward that vs adding another header.
Ira
On Wed, May 11, 2022 at 06:25:19PM -0700, Ira wrote:
> On Thu, Apr 28, 2022 at 08:50:34AM -0700, Christoph Hellwig wrote:
> > On Tue, Apr 19, 2022 at 10:06:38AM -0700, [email protected] wrote:
> > > index a77be5630209..32ed07c2994b 100644
> > > --- a/include/linux/highmem-internal.h
> > > +++ b/include/linux/highmem-internal.h
> > > @@ -151,6 +151,8 @@ static inline void totalhigh_pages_add(long count)
> > >
> > > #else /* CONFIG_HIGHMEM */
> > >
> > > +#include <linux/memremap.h>
> >
> > Uuuh, I'd really prefer to avoid having to pull in memremap.h into
> > basically the whole kernel build.
> >
>
> Fair enough. What should I call a header to store the new pgmap_set_*() calls?
>
> devmap_prot.h
>
> ?
>
> Or I could just add the calls into highmem-internal.h. That might be more
> appropriate given the new direction of that interface.
>
> I think I'm leaning toward that vs adding another header.
Ok there is a trade off here which I'm not sure is a good idea.
In order to make kmap faster I carefully placed the devmap_protected() call
completely inline.[1] This specifically checks the PGMAP_PROTECTION flag in
dev_pagemap.[1]
I see only 2 ways of not including memremap.h in highmem-internal.h.
1) Make this check a function call and place it in memremap.c
2) Move struct dev_pagemap (and it's dependencies) to another header.
Number 2 negates any positive effect of splitting the header file.
Number 1 is going to force some overhead on all ZONE_DEVICE pages if PMEM is in
the system.
Do you think the extra run time overhead is a reasonable trade off to
eliminating the header?
Ira
[1] https://lore.kernel.org/lkml/[email protected]/
On Tue, May 17, 2022 at 03:46:55PM -0700, Ira Weiny wrote:
> Ok there is a trade off here which I'm not sure is a good idea.
>
> In order to make kmap faster I carefully placed the devmap_protected() call
> completely inline.[1] This specifically checks the PGMAP_PROTECTION flag in
> dev_pagemap.[1]
>
> I see only 2 ways of not including memremap.h in highmem-internal.h.
>
> 1) Make this check a function call and place it in memremap.c
> 2) Move struct dev_pagemap (and it's dependencies) to another header.
>
> Number 2 negates any positive effect of splitting the header file.
>
> Number 1 is going to force some overhead on all ZONE_DEVICE pages if PMEM is in
> the system.
>
> Do you think the extra run time overhead is a reasonable trade off to
> eliminating the header?
Well, given how big these devmap helpes are it seems like they should be
out of line anyway due to the code size impact. It would be great to
compare the code size for the cases of:
1) baseline kernel
2) devmap protection inline
3) devmap protection out of line
And maybe you need to add linux-mm and Thomas to get a few more
opinions.
On Wed, May 18, 2022 at 12:33:44AM -0700, Christoph Hellwig wrote:
> On Tue, May 17, 2022 at 03:46:55PM -0700, Ira Weiny wrote:
> > Ok there is a trade off here which I'm not sure is a good idea.
> >
> > In order to make kmap faster I carefully placed the devmap_protected() call
> > completely inline.[1] This specifically checks the PGMAP_PROTECTION flag in
> > dev_pagemap.[1]
> >
> > I see only 2 ways of not including memremap.h in highmem-internal.h.
> >
> > 1) Make this check a function call and place it in memremap.c
> > 2) Move struct dev_pagemap (and it's dependencies) to another header.
> >
> > Number 2 negates any positive effect of splitting the header file.
> >
> > Number 1 is going to force some overhead on all ZONE_DEVICE pages if PMEM is in
> > the system.
> >
> > Do you think the extra run time overhead is a reasonable trade off to
> > eliminating the header?
>
> Well, given how big these devmap helpes are it seems like they should be
> out of line anyway due to the code size impact. It would be great to
> compare the code size for the cases of:
Fair enough. I did a few experiments with all yes config modified only to
remove LOCKDEP because those numbers were a bit skewed IMO.
>
> 1) baseline kernel
Text Data bss
293450036 337712849 19907196
> 2) devmap protection inline
This is V10
Text Data bss
293461422 337759561 19882620
From Baseline:
+ 11386 + 46712
0.0038% 0.014%
2.5) devmap partly inline (This cleans up the headers by adding a new header
for devmap protection and not using memremap.h in highmem-internal.h)
Text Data bss
293460722 337759593 19882620
From Baseline:
+ 10686 + 46744
0.0036% 0.014%
> 3) devmap protection out of line
Text Data bss
293458303 337734401 19890812
From Baseline:
+ 8267 + 21552
0.0028% 0.0064%
From these numbers it seems reasonable to have some of devmap_protected() out
of line to clean up the header dependencies.
Thanks,
Ira