Re: [PATCH] firmware_loader: Replace kmap() with kmap_local_page()

[Fabio M. De Francesco]

On domenica 10 luglio 2022 12:24:41 CEST Greg Kroah-Hartman wrote:
> On Sun, Jul 10, 2022 at 12:11:56PM +0200, Fabio M. De Francesco wrote:
> > The use of kmap() is being deprecated in favor of kmap_local_page().
> > 
> > With kmap_local_page() the mappings are per thread, CPU local, can take
> > page faults, and can be called from any context (including interrupts).
> 
> But that is not the case here for this kmap() instance?

I'm not 100% sure to get what you are asking here :-)
Probably you mean that kmap() can work here and you don't see reason for 
converting? Am I understanding correctly?

OK, then...

kmap() is being deprecated in favor of kmap_local_page(). Please see 
highmem.rst which I have updated weeks ago (https://docs.kernel.org/vm/
highmem.html).

Two main problems with kmap(): (1) It comes with an overhead as mapping 
space is restricted and protected by a global lock for synchronization and 
(2) kmap() also requires global TLB invalidation when the kmap’s pool wraps 
and it might block when the mapping space is fully utilized until a slot 
becomes available.

kmap_local_page() should be preferred, where feasible, over all the others.

Furthermore, current work in progress to use PKS to protect PMEM from stray 
writes does not support the kmap()/kunmap() calls. Probably this is why 
I've been asked to (mechanically) convert all kmap() call sites (where 
feasible) or refactor the code and then convert them where it is not (for 
example wherever kmapped pages are reused in different contexts). 

Unless I'm missing your point, I cannot see any reason here which prevents 
these conversions. Can you please tell me if I'm overlooking anything? 

> If this is a
> simple search/replace, why is this not just done once and be done with
> it?

No, this job needs code inspection. After more than 25 conversions I can 
say that no more than 30% have been simple search and replace.

> > Call kmap_local_page() in firmware_loader wherever kmap() is currently
> > used. In firmware_rw() use the copy_{from,to}_page() helpers instead of
> > open coding the local mappings plus memcpy().
> 
> Isn't that just a different cleanup than the kmap() change?  Or is that
> tied to the fact that the other buffer is now allocated with
> kmap_local_page() instead of kmap()?

This kinds of changes have never been considered clean-ups by other 
maintainers (for an example please see commit e88a6a8fece9 ("binder: Use 
memcpy_{to,from}_page() in binder_alloc_do_buffer_copy()"). 

Using helpers has always been considered part of the conversions themselves 
and nobody has ever requested further patches for these.

> > Suggested-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@xxxxxxxxx>
> > ---
> >  drivers/base/firmware_loader/main.c  | 4 ++--
> >  drivers/base/firmware_loader/sysfs.c | 9 ++++-----
> >  2 files changed, 6 insertions(+), 7 deletions(-)
> 
> Did you run this through the firmware test framework?

No, sorry. I assumed (wrongly?) that this is one of those cases which don't 
need any tests. However I have nothing against testing. I've done them 
where they were absolutely needed for Btrfs conversions and kexec.

Do you see anything which may break the rules of use of kmap_local_page() 
here? If you confirm you are concerned it could not work, I'll happily 
research how to test these changes :-)

> thanks,
> 
> greg k-h
> 

Thanks for your comments,

Fabio