Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221206070029.7342-1-fmdefrancesco@gmail.com> <Y472ipY908pHip+B@linutronix.de>
In-Reply-To: <Y472ipY908pHip+B@linutronix.de>
From:   "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
Date:   Tue, 6 Dec 2022 20:14:52 +0100
Message-ID: <CAPj211tzOzk7JBmCOaNUekx4APv+khnKk0pxLqOJppUAp0A+8A@mail.gmail.com>
Subject: Re: [PATCH] mm/highmem: Add notes about conversions from kmap{,_atomic}()
To:     Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc:     Jonathan Corbet <corbet@lwn.net>,
        Andrew Morton <akpm@linux-foundation.org>,
        Ira Weiny <ira.weiny@intel.com>,
        Mike Rapoport <rppt@kernel.org>, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        Mike Rapoport <rppt@linux.ibm.com>,
        Peter Zijlstra <a.p.zijlstra@chello.nl>,
        Thomas Gleixner <tglx@linutronix.de>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On marted=C3=AC 6 dicembre 2022 09:00:10 CET Sebastian Andrzej Siewior wrot=
e:
> On 2022-12-06 08:00:29 [+0100], Fabio M. De Francesco wrote:
> > diff --git a/Documentation/mm/highmem.rst b/Documentation/mm/highmem.rs=
t
> > index 0f731d9196b0..9523e92299f6 100644
> > --- a/Documentation/mm/highmem.rst
> > +++ b/Documentation/mm/highmem.rst
> > @@ -100,10 +101,21 @@ list shows them in order of preference of use.
> >
> >    (included in the "Functions" section) for details on how to manage
nested
> >    mappings.
> >
> > -* kmap_atomic().  This permits a very short duration mapping of a sing=
le
> > -  page.  Since the mapping is restricted to the CPU that issued it, it
> > -  performs well, but the issuing task is therefore required to stay on
that
> > -  CPU until it has finished, lest some other task displace its mapping=
s.
> > +* 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, code
> > between the +  map/unmap operations may implicitly depended on the side
> > effects of +  kmap_atomic(), such as disabling pagefaults, migration,
> > and/or preemption. +  Such conversions should be changed to make explic=
it
> > calls for those +  requirements.

Sebastian, thanks for taking a look at my patch and replying.

>   Furthermore, code between the kmap_atomic() and kunmap_atomic()
>   functions may implicitly depended

I suppose it should be "depend"? Shouldn't it?

>   on the side effects of kmap_atomic()
>   namely disabling pagefaults or preemption or both.

I agree with you for rephrasing, mainly because it is
written in poor English.

However, I still have doubts about why you deleted "migration".
AFAIK, __kmap_local_pfn_prot() always takes care of disabling migration for
HIGHMEM enabled kernels.

How about !HIGHMEM, where kmap_local_page() is an indirect call to
page_address()? Did you mean that, if the code between kmap_atomic() and
kunmap_atomic() depended on migrate_disable() (in PREEMPT_RT) we should alw=
ays
just stay safe and call preempt_disable() together with conversion to
kmap_local_page()?

If so, I understand and I again agree with you. If not, I'm missing somethi=
ng;
so please let me understand properly.

Aside from the above, I'm not sure whether you deleted the last phrase befo=
re
your suggestion. What about making it to become "For the above-mentioned
cases, conversions should also explicitly disable page-faults and/or
preemption"?

Thanks again for noticing my mistakes.


Fabio

>
> > +  [Legacy documentation]
> > +
> > +  This permits a very short duration mapping of a single page.  Since =
the
> > +  mapping is restricted to the CPU that issued it, it performs well, b=
ut
> > +  the issuing task is therefore required to stay on that CPU until it =
has
> > +  finished, lest some other task displace its mappings.
> >
> >    kmap_atomic() may also be used by interrupt contexts, since it does =
not
> >    sleep and the callers too may not sleep until after kunmap_atomic() =
is
>
> Sebastian