Message-ID: <Ysq+rpkTU1/nquHo@kroah.com>
Date:   Sun, 10 Jul 2022 13:57:34 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     "Fabio M. De Francesco" <fmdefrancesco@...il.com>
Cc:     Luis Chamberlain <mcgrof@...nel.org>,
        Russ Weight <russell.h.weight@...el.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Nick Terrell <terrelln@...com>, linux-kernel@...r.kernel.org,
        Ira Weiny <ira.weiny@...el.com>
Subject: Re: [PATCH] firmware_loader: Replace kmap() with kmap_local_page()

On Sun, Jul 10, 2022 at 01:18:16PM +0200, Fabio M. De Francesco wrote:
> 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?

Yes, that is what I am saying, why is this conversion needed here?  A
justification would be nice.

> 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.

Ok, that is good to know, thanks for the pointer, you should put this in
the changelog text for maintainers who did not know this (like myself)
as it makes it easier to review.

> > 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@...el.com>
> > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@...il.com>
> > > ---
> > >  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.

Running the kernel selftests for firmware would be great, please do so
for your next version of this patch that fixes the
ktest-robot-found-issues.

thanks,

greg k-h

Powered by blists - more mailing lists