Re: [PATCH v2 4/4] Documentation/vm: Rework "Temporary Virtual Mappings" section

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
To: "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
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, Jonathan Corbet <corbet@lwn.net>,
	linux-doc@vger.kernel.org, outreachy@lists.linux.dev,
	Thomas Gleixner <tglx@linutronix.de>,
	Peter Zijlstra <peterz@infradead.org>
Subject: Re: [PATCH v2 4/4] Documentation/vm: Rework "Temporary Virtual Mappings" section
Date: Tue, 26 Apr 2022 13:47:34 +0200	[thread overview]
Message-ID: <Ymfb1saaHVuq4IUl@linutronix.de> (raw)
In-Reply-To: <3610796.MHq7AAxBmi@leap>

On 2022-04-26 12:45:12 [+0200], Fabio M. De Francesco wrote:
> > > +* kmap_local_page().  This function is used to require short term 
> mappings.
> > > +  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.
> > 
> > feasible? It should always be used. 
> 
> No, it cannot always be used. Please read again few lines above this that 
> "The mapping can only be used in the context which acquired them". We 
> cannot do blind s/kmap/kmap_local_page/.

I'm sorry it seems I slipped while reading and replying.
The kmap_atomic() part has "should be only used if it is absolutely
required" … " otherwise kmap_local_page()". The kmap_atomic() should be
used in new code. The alternative kmap() and kmap_local*() should be
enough.

> > Maybe "thread local" instead CPU local? Another thread on the same CPU
> > can not use this mapping.
> > 
> 
> Hmm, I might add "thread local" to convey that the local mappings should 
> stay in the same context that acquired them. 
> 
> However, kmap_local_page() also disable migration. This is how Thomas 
> Gleixner talks about kmap_local_page() in his patch where it introduced 
> this function: 
> 
> "The kmap_local.*() functions can be invoked from both preemptible and
> atomic context. kmap local sections disable migration to keep the resulting
> virtual mapping address correct, but disable neither pagefaults nor
> preemption.".
> 
> Therefore, if it "disable migration" it is "CPU local". I mean that I might 
> also add "thread local" but I think (at least at this moment) that I won't 
> remove "CPU local".

Hmm. It is thread-local in the end. There are slots 0 … KM_MAX_IDX for
the mappings. Slot 0 for task A can be different from slot 0 for task B
while both run on CPU0. So the same address, that is returned from
kmap_local(), will point to a different page for both tasks. Both tasks
can't be migrated to another CPU while the mapping is active.
"CPU local" sounds like something that is same to everyone on the same
CPU which is what this_cpu_read() for instance does.

> @Ira: what about this proposal?
> 
> Thanks,
> 
> Fabio

Sebastian