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;
From:   "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
To:     Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc:     Ira Weiny <ira.weiny@intel.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        "Matthew Wilcox (Oracle)" <willy@infradead.org>,
        Will Deacon <will@kernel.org>,
        Peter Collingbourne <pcc@google.com>,
        Vlastimil Babka <vbabka@suse.cz>, linux-kernel@vger.kernel.org,
        linux-doc@vger.kernel.org, outreachy@lists.linux.dev,
        Jonathan Corbet <corbet@lwn.net>,
        Thomas Gleixner <tglx@linutronix.de>,
        Peter Zijlstra <peterz@infradead.org>
Subject: Re: [PATCH v3 4/4] Documentation/vm: Rework "Temporary Virtual Mappings" section
Date:   Thu, 28 Apr 2022 13:14:30 +0200
Message-ID: <6442788.4vTCxPXJkl@leap>
In-Reply-To: <YmpYEkvbJX2JBPvW@linutronix.de>
References: <20220427183821.1979-1-fmdefrancesco@gmail.com> <20220427183821.1979-5-fmdefrancesco@gmail.com> <YmpYEkvbJX2JBPvW@linutronix.de>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="utf-8"
Precedence: bulk

On gioved=C3=AC 28 aprile 2022 11:02:10 CEST Sebastian Andrzej Siewior wrot=
e:
> On 2022-04-27 20:38:21 [+0200], Fabio M. De Francesco wrote:
> > index e05bf5524174..c8aff448612b 100644
> > --- a/Documentation/vm/highmem.rst
> > +++ b/Documentation/vm/highmem.rst
> > @@ -50,26 +50,78 @@ space when they use mm context tags.
> =E2=80=A6
> > =20
> > -* kmap().  This permits a short duration mapping of a single page.  It=
=20
needs
> > -  global synchronization, but is amortized somewhat.  It is also prone=
=20
to
> > -  deadlocks when using in a nested fashion, and so it is not=20
recommended for
> > -  new code.
> > +  These mappings are thread-local and CPU-local (i.e., migration from=
=20
one CPU
> > +  to another is disabled - this is why they are called "local"), but=20
they don't
> > +  disable preemption.=20
>=20
> So if you replace this block with
>=20
>    These mappings are thread-local and CPU-local meaning that the mapping
>    can only be accessed from within this thread and the thread is bound=20
the
>    CPU while the mapping is active. Even if the thread is preempted=20
(since
>    preemption is never disabled by the function) the CPU can not be
>    unplugged from the system via CPU-hotplug until the mapping is=20
disposed.

OK, I'm too wordy here :(

> The you could drop the latter block
>=20
> >                          It's valid to take pagefaults in a local kmap=
=20
region,
> > +  unless the context in which the local mapping is acquired does not=20
allow it
> > +  for other reasons.
>=20
> > +  kmap_local_page() always returns a valid virtual address and it is=20
assumed
> > +  that kunmap_local() will never fail.
>=20
> from here
>=20
> > +  If a task holding local kmaps is preempted, the maps are removed on=
=20
context
> > +  switch and restored when the task comes back on the CPU. The maps=20
are
> > +  strictly thread-local and CPU-local, therefore it is guaranteed that=
=20
the
> > +  task stays on the CPU and the CPU cannot be unplugged until the=20
local kmaps
> > +  are released.
>=20
> to here since it mostly the same thing.

I agree, this is redundant.

>=20
> > +  Nesting kmap_local_page() and kmap_atomic() mappings is allowed to a=
=20
certain
> > +  extent (up to KMAP_TYPE_NR) but their invocations have to be=20
strictly ordered
> > +  because the map implementation is stack based. See kmap_local_page=20
() kdocs
>=20
> kmap_local_page () =3D> kmap_local_page()

Sure, it's just a typo.

> > +  (included in the "Functions" section) for details on how to manage=20
nested
> > +  mappings.
> > =20
> >  * kmap_atomic().  This permits a very short duration mapping of a=20
single
> >    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=
=20
that
> >    CPU until it has finished, lest some other task displace its=20
mappings.
> > =20
> > -  kmap_atomic() may also be used by interrupt contexts, since it is=20
does not
> > -  sleep and the caller may not sleep until after kunmap_atomic() is=20
called.
> > +  kmap_atomic() may also be used by interrupt contexts, since it does=
=20
not
> > +  sleep and the callers too may not sleep until after kunmap_atomic()=
=20
is
> > +  called.
> > +
> > +  Each call of kmap_atomic() in the kernel creates a non-preemptible=20
section
> > +  and disable pagefaults. This could be a source of unwanted latency,=
=20
so it
> > +  should be only used if it is absolutely required, otherwise=20
kmap_local_page()
> > +  should be used where it is feasible.
>=20
> I'm not keen about the "absolutely required" wording and "feasible".
> That said, the other pieces look good, thank you for the work.

I'll rewrite the last part of this sentence as it follows:

+ should be only used if it is required, otherwise kmap_local_page()
+ should be preferred.

Thank you so much for the time you have spent for reviewing and helping,

=46abio