On giovedì 28 aprile 2022 11:02:10 CEST Sebastian Andrzej Siewior wrote: > 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. > … > > > > -* 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. > > + These mappings are thread-local and 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. > > So if you replace this block with > > 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 the > CPU while the mapping is active. Even if the thread is preempted (since > preemption is never disabled by the function) the CPU can not be > unplugged from the system via CPU-hotplug until the mapping is disposed. OK, I'm too wordy here :( > The you could drop the latter block > > > 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. > > > + kmap_local_page() always returns a valid virtual address and it is assumed > > + that kunmap_local() will never fail. > > from here > > > + 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. The maps are > > + strictly thread-local and CPU-local, therefore it is guaranteed that the > > + task stays on the CPU and the CPU cannot be unplugged until the local kmaps > > + are released. > > to here since it mostly the same thing. I agree, this is redundant. > > > + Nesting kmap_local_page() and kmap_atomic() mappings is allowed to a certain > > + extent (up to KMAP_TYPE_NR) but their invocations have to be strictly ordered > > + because the map implementation is stack based. See kmap_local_page () kdocs > > kmap_local_page () => kmap_local_page() Sure, it's just a typo. > > + (included in the "Functions" section) for details on how to manage nested > > + mappings. > > > > * kmap_atomic(). 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, but 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 is does not > > - sleep and the caller may not sleep until after kunmap_atomic() is called. > > + 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 > > + called. > > + > > + Each call of kmap_atomic() in the kernel creates a non-preemptible section > > + and disable pagefaults. This could be a source of unwanted latency, so it > > + should be only used if it is absolutely required, otherwise kmap_local_page() > > + should be used where it is feasible. > > 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, Fabio
