Re: [PATCH V8 38/44] memremap_pages: Define pgmap_mk_{readwrite|noaccess}() calls

[Ira Weiny <ira.weiny@intel.com>]

On Fri, Feb 04, 2022 at 10:35:59AM -0800, Dan Williams wrote:
> On Thu, Jan 27, 2022 at 9:55 AM <ira.weiny@intel.com> wrote:
> >

[snip]

I'll address the other comments later but wanted to address the idea below.

> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index f5b2be39a78c..5020ed7e67b7 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -1492,6 +1492,13 @@ struct task_struct {
> >         struct callback_head            l1d_flush_kill;
> >  #endif
> >
> > +#ifdef CONFIG_DEVMAP_ACCESS_PROTECTION
> > +       /*
> > +        * NOTE: pgmap_prot_count is modified within a single thread of
> > +        * execution.  So it does not need to be atomic_t.
> > +        */
> > +       u32                             pgmap_prot_count;
> > +#endif
> 
> It's not at all clear why the task struct needs to be burdened with
> this accounting. Given that a devmap instance is needed to manage page
> protections, why not move the nested protection tracking to a percpu
> variable relative to an @pgmap arg? Something like:
> 
> void __pgmap_mk_readwrite(struct dev_pagemap *pgmap)
> {
>        migrate_disable();
>        preempt_disable();

Why burden threads like this?  kmap_local_page() is perfectly able to migrate
or be preempted?

I think this is way to restrictive.

>        if (this_cpu_add_return(pgmap->pgmap_prot_count, 1) == 1)
>                pks_mk_readwrite(PKS_KEY_PGMAP_PROTECTION);
> }
> EXPORT_SYMBOL_GPL(__pgmap_mk_readwrite);
> 
> void __pgmap_mk_noaccess(struct dev_pagemap *pgmap)
> {
>        if (!this_cpu_sub_return(pgmap->pgmap_prot_count, 1))
>                pks_mk_noaccess(PKS_KEY_PGMAP_PROTECTION);
>        preempt_enable();
>        migrate_enable();
> }
> EXPORT_SYMBOL_GPL(__pgmap_mk_noaccess);
> 
> The naming, which I had a hand in, is not aging well. When I see "mk"
> I expect it to be building some value like a page table entry that
> will be installed later. These helpers are directly enabling and
> disabling access and are meant to be called symmetrically. So I would
> expect symmetric names like:
> 
> pgmap_enable_access()
> pgmap_disable_access()

Names are easily changed.  I'll look at changing the names.

Ira

> 
> 
> >         /*
> >          * New fields for task_struct should be added above here, so that
> >          * they are included in the randomized portion of task_struct.
> > diff --git a/init/init_task.c b/init/init_task.c
> > index 73cc8f03511a..948b32cf8139 100644
> > --- a/init/init_task.c
> > +++ b/init/init_task.c
> > @@ -209,6 +209,9 @@ struct task_struct init_task
> >  #ifdef CONFIG_SECCOMP_FILTER
> >         .seccomp        = { .filter_count = ATOMIC_INIT(0) },
> >  #endif
> > +#ifdef CONFIG_DEVMAP_ACCESS_PROTECTION
> > +       .pgmap_prot_count = 0,
> > +#endif
> >  };
> >  EXPORT_SYMBOL(init_task);
> >
> > diff --git a/mm/memremap.c b/mm/memremap.c
> > index d3e6f328a711..b75c4f778c59 100644
> > --- a/mm/memremap.c
> > +++ b/mm/memremap.c
> > @@ -96,6 +96,20 @@ static void devmap_protection_disable(void)
> >         static_branch_dec(&dev_pgmap_protection_static_key);
> >  }
> >
> > +void __pgmap_mk_readwrite(struct dev_pagemap *pgmap)
> > +{
> > +       if (!current->pgmap_prot_count++)
> > +               pks_mk_readwrite(PKS_KEY_PGMAP_PROTECTION);
> > +}
> > +EXPORT_SYMBOL_GPL(__pgmap_mk_readwrite);
> > +
> > +void __pgmap_mk_noaccess(struct dev_pagemap *pgmap)
> > +{
> > +       if (!--current->pgmap_prot_count)
> > +               pks_mk_noaccess(PKS_KEY_PGMAP_PROTECTION);
> > +}
> > +EXPORT_SYMBOL_GPL(__pgmap_mk_noaccess);
> > +
> >  bool pgmap_protection_available(void)
> >  {
> >         return pks_available();
> > --
> > 2.31.1
> >