Message-ID: <YlSqHf8Kv23iNp0E@iweiny-desk3>
Date:   Mon, 11 Apr 2022 15:22:21 -0700
From:   Ira Weiny <ira.weiny@...el.com>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     "Fabio M. De Francesco" <fmdefrancesco@...il.com>,
        Jonathan Corbet <corbet@....net>, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
        peterz@...radead.org
Subject: Re: [PATCH] Documentation/vm: Extend "Temporary Virtual Mappings" in
 highmem.rst

On Sun, Apr 10, 2022 at 04:21:07AM +0100, Matthew Wilcox wrote:
> On Sat, Apr 09, 2022 at 08:49:07PM +0200, Fabio M. De Francesco wrote:
> > @@ -52,25 +52,65 @@ Temporary Virtual Mappings
> >  
> >  The kernel contains several ways of creating temporary mappings:
> >  
> > -* vmap().  This can be used to make a long duration mapping of multiple
> > -  physical pages into a contiguous virtual space.  It needs global
> > -  synchronization to unmap.
> > +* vmap().  This can be used to make a long duration mapping of multiple physical
> > +  pages into a contiguous virtual space. It needs global synchronization to unmap.
> 
> Did you change any words here?  If so, I can't see them.  Please don't
> gratuitously reformat paragraphs; it obscures the real changes.  Also,
> 75 characters is a good limit for line length, and you're well past
> that.  If in doubt, use `fmt`.
> 
> > -* kmap().  This permits a short duration mapping of a single page.  It needs
> > -  global synchronization, but is amortized somewhat.  It is also prone to
> > -  deadlocks when using in a nested fashion, and so it is not recommended for
> > -  new code.
> > +* kmap().  This can be used to make long duration mapping of a single page with
> 
> kmap() really isn't for long duration.  But the pointer returned from
> kmap() is valid across all CPUs, unlike kmap_local() or kmap_atomic().

I think the problem is in how kmap() is being used now for the long duration
maps in a couple of places.

That said, I agree with Matt that we should not document this fact.  Rather we
should steer people away from making use of kmap() at all.

That said if kmap() goes away what replaces it in the areas of the code which
require a long term access to a VA?

The addition of PKS like protections on the direct map complicate this.

[snip]

> > +
> > +* kmap_local_*().  These provide a set of functions similar to kmap_atomic() and
> > +  are used to require short term mappings. They can be invoked from any context
> > +  (including interrupts).
> > +
> > +  The mapping can only be used in the context which acquired it, it is per thread,
> > +  CPU local (i.e., migration from one CPU to another is disabled - this is why
> > +  they are called "local"), but they don't disable preemption. It's valid to take
> > +  pagefaults in a local kmap region, unless the context in which the local mapping
> > +  is acquired does not allow it for other reasons.
> > +
> > +  If a task holding local kmaps is preempted, the maps are removed on context
> > +  switch and restored when the task comes back on the CPU. As the maps are strictly
> > +  CPU local, it is guaranteed that the task stays on the CPU and that the CPU
> > +  cannot be unplugged until the local kmaps are released.
> > +
> > +  Nesting kmap_local.*() and kmap_atomic.*() mappings is allowed to a certain
> > +  extent (up to KMAP_TYPE_NR). Nested kmap_local.*() and kunmap_local.*()
> > +  invocations have to be strictly ordered because the map implementation is stack
> > +  based.
> 
> I think the original layout of all this is flawed.  We should start by
> describing the interface we want people to use first -- kmap_local*(),
> then say "But if you can't use that, there's kmap_atomic()" and "If
> you can't use kmap_atomic(), you can use kmap()".

...  If, and only if, one absolutely has to use kmap(), then ok...

Ira

Powered by blists - more mailing lists