Re: [PATCH v2] mm/highmem: Add notes about conversions from kmap{,_atomic}()

[Fabio M. De Francesco]

On giovedì 8 dicembre 2022 05:22:37 CET Bagas Sanjaya wrote:
> On Wed, Dec 07, 2022 at 11:53:08PM +0100, Fabio M. De Francesco wrote:
> > diff --git a/Documentation/mm/highmem.rst b/Documentation/mm/highmem.rst
> > index 0f731d9196b0..59d1078f53df 100644
> > --- a/Documentation/mm/highmem.rst
> > +++ b/Documentation/mm/highmem.rst
> > @@ -57,7 +57,8 @@ list shows them in order of preference of use.
> > 
> >    It can be invoked from any context (including interrupts) but the
> >    mappings
> >    can only be used in the context which acquired them.
> > 
> > -  This function should be preferred, where feasible, over all the others.
> > +  This function should always be used. kmap_atomic() and kmap() have been
> > +  deprecated.
> 
> "... always be used, whereas ..."
> 
> > +  NOTE: Conversions to kmap_local_page() must take care to follow the
> > mapping +  restrictions imposed on kmap_local_page(). Furthermore, the 
code
> > between +  calls to kmap_atomic() and kunmap_atomic() may implicitly 
depend
> > on the side +  effects of atomic mappings, i.e. disabling page faults or
> > preemption, or both. +  In that case, explicit calls to 
pagefault_disable()
> > or preempt_disable() or +  both must be made in conjunction with the use 
of
> > kmap_local_page(). +
> > <snipped>...
> > +  NOTE: Conversions to kmap_local_page() must take care to follow the
> > mapping +  restrictions imposed on kmap_local_page(). In particular, it is
> > necessary to +  make sure that the kernel virtual memory pointer is only
> > valid in the thread +  that obtained it.
> > +
> 
> What about using note block to signify conversion notes above?
> 
> ---- >8 ----
> 
> diff --git a/Documentation/mm/highmem.rst b/Documentation/mm/highmem.rst
> index 59d1078f53df57..ef53eb580d4cda 100644
> --- a/Documentation/mm/highmem.rst
> +++ b/Documentation/mm/highmem.rst
> @@ -103,12 +103,14 @@ list shows them in order of preference of use.
> 
>  * kmap_atomic(). This function has been deprecated; use kmap_local_page().
> 
> -  NOTE: Conversions to kmap_local_page() must take care to follow the 
mapping
> -  restrictions imposed on kmap_local_page(). Furthermore, the code between 
-
>  calls to kmap_atomic() and kunmap_atomic() may implicitly depend on the 
side
> -  effects of atomic mappings, i.e. disabling page faults or preemption, or
> both. -  In that case, explicit calls to pagefault_disable() or
> preempt_disable() or -  both must be made in conjunction with the use of
> kmap_local_page(). +  .. note::
> +     Conversions to kmap_local_page() must take care to follow the mapping
> +     restrictions imposed on kmap_local_page(). Furthermore, the code 
between
> +     calls to kmap_atomic() and kunmap_atomic() may implicitly depend on 
the
> +     side effects of atomic mappings, i.e. disabling page faults or
> preemption, +     or both. In that case, explicit calls to
> pagefault_disable() or +     preempt_disable() or both must be made in
> conjunction with the use of +     kmap_local_page().
> 
>    [Legacy documentation]
> 
> @@ -129,10 +131,11 @@ list shows them in order of preference of use.
> 
>  * kmap(). This function has been deprecated; use kmap_local_page().
> 
> -  NOTE: Conversions to kmap_local_page() must take care to follow the 
mapping
> -  restrictions imposed on kmap_local_page(). In particular, it is necessary
> to -  make sure that the kernel virtual memory pointer is only valid in the
> thread -  that obtained it.
> +  .. note::
> +     Conversions to kmap_local_page() must take care to follow the mapping
> +     restrictions imposed on kmap_local_page(). In particular, it is
> necessary +     to make sure that the kernel virtual memory pointer is only
> valid in the +     thread that obtained it.
> 
>    [Legacy documentation]
> 
> Thanks.
> 
> --
> An old man doll... just what I always wanted! - Clara

You provided valid suggestions, thanks!
However, immediately after submitting this v2 patch, Andrew added it to the -
mm mm-unstable branch.
I'll do the changes that you suggested in a subsequent patch which will build 
on this.

Again thanks,

Fabio