Re: [PATCH v2] x86/sgx: Replace kmap/kunmap_atomic calls

[Ira Weiny]

On Wed, Nov 16, 2022 at 03:01:56PM +0100, Fabio M. De Francesco wrote:
> On mercoledì 16 novembre 2022 12:58:41 CET Thomas Gleixner wrote:
> > On Wed, Nov 16 2022 at 11:16, Fabio M. De Francesco wrote:
> > > On martedì 15 novembre 2022 17:16:26 CET Kristen Carlson Accardi wrote:
> > >> The use of kmap_atomic() in the SGX code was not an explicit design
> > >> choice to disable page faults or preemption, and there is no compelling
> > >> design reason to using kmap_atomic() vs. kmap_local_page().
> > >
> > > This is at the core of the reasons why you are converting, that is to
> avoid
> > > the potential overhead (in 32 bit architectures) of switching in atomic
> > > context where it is not required.
> >
> > No. The point is that kmap_atomic() is an historical artifact of 32bit
> > HIGHMEM support.
> 
> I just saw that the last part of my email is missing. In fact it's enough
> clear that what I was saying was still incomplete and that no signature was
> visible.
> 
> I meant that, before we had kmap_local_page(), the choice was between kmap()
> and kmap_atomic().
> 
> Despite I suppose that nobody should rely on those kmap_atomic() side effects.
> I have been observing that a large part of the users of the Highmem API used
> to call kmap_atomic() for its implicit pagefault_disable() and
> preempt_disable() side effects

Fabio I think you missed the point here.  Just because we have found _some_
places where the side effect was required does not mean that "a large part of
the users..." do.

While I value your review of these conversions I think Kristen did her home
work here.  She checked with Jarkko (the original author of the code) and
verified that the side effects are not necessary here.  That is all that needs
to be mentioned in the commit message.

> and so switching in atomic context only between
> mappings and unmappings. Others used to call the atomic variant of kmap() just
> because they wanted to default to it, instead of calling kmap().
> 
> Since 2019 we have kmap_local_page() and kmap() / kmap_atomic() have been
> recently deprecated but we can't know why the author of a piece of code
> decided to use kmap_atomic()...
> 
> 1) Are we already in atomic context when kmap_atomic() is invoked?
> 2) Do we call kmap_atomic() instead of kmap() only to switch from non atomic
> to atomic context between the mapping the unmapping (although I stress that we
> shouldn't ever rely on these side effects)?
> 
> I think that who converts should tell whether or not we must explicitly, for
> example, disable page faults and/or preemption along with converting to
> kmap_local_page().
> 
> This is why I think that Kristen did well with the inspection of what happens
> in those sections in between.
> 
> I expressed this concept in a bad way and furthermore the lines below that
> sentence disappeared, making the whole much more confusing.
> 
> I think that later I'm saying a large part of what you are detailing because I
> had to learn all these things for reworking highmem.rst and the kdocs of the
> relevant highmem-related files.
> 
> The new documentation is upstream since months and you've been Cd'ed. If
> anything is wrong please help me to correct what I wrote or do it yourself if
> you prefer that other way.
> 
> > The back then chosen implementation was to disable preemption and
> > pagefaults and use a temporary per CPU mapping. Disabling preemption was
> > required to protect the temporary mapping against a context switch.
> >
> > Disabling pagefaults was an implicit side effect of disabling
> > preemption. The separation of preempt_disable() and pagefault_disable()
> > happened way later.
> >
> > On 64bit and on 32bit systems with CONFIG_HIGHMEM=n this is not required
> > at all because the pages are already in the direct map.
> 
> Unfortunately, this is one of the things that were missing at the end of my
> previous email. I'm aware that with HIGHMEM=n all these kmap_local_page() are
> plain page_address(). I wrote this too in my email (and in the documentation)
> but it disappeared when I sent my message.
> 
> > That means support for 32bit highmem forces code to accomodate with the
> > preemption disabled section, where in the most cases this is absolutely
> > not required.
> 
> This is the core: Kristen knows that the code between mapping / unmapping does
> not need preemption and page faults disabling. Therefore kmap_local_page()
> will work with no need to run in atomic. We all agree to the necessity to
> convert but sometimes we are not sure about how to convert.

But we are sure about this conversion.

> Otherwise a script
> would be enough to convert to kmap_local_page(). :-)
> 
> > That results often in suboptimal and horrible code:
> >
> > again:
> >     kmap_atomic();
> >     remaining = copy_to_user_inatomic();
> >     kunmap_atomic();
> >     if (remaining) {
> >       if (try_to_handle_fault())
> >             goto again;
> >         ret = -EFAULT;
> >     }
> >
> > instead of:
> >
> >     kmap_local();
> >     ret = copy_to_user();
> >     kunmap_local();
> >
> > It obsiously does not allow to sleep or take sleeping locks in the
> > kmap_atomic() section which puts further restrictions on code just to
> > accomodate 32bit highmem.
> 
> I understand and agree. I'm sorry for the missing parts and for expressing
> with not so good English proficiency.
> 
> > So a few years ago we implemented kmap_local() and related interfaces to
> > replace kmap_atomic() completely, but we could not do a scripted
> > wholesale conversion because there are a few code pathes which rely on
> > the implicit preemption disable and of course something like the above
> > monstrosity needs manual conversion.
> >
> > kmap_local() puts a penalty exclusively on 32bit highmem systems. The
> > temporary mapping is still strict per CPU, which is guaranteed by an
> > implicit migrate_disable(), and it is context switched in case of
> > [un]voluntary scheduling.
> >
> > On plain 32bit and 64bit systems kmap_local() is pretty much a NOP. All
> > it does is to return the page address. It does not disable migration in
> > that case either. kunmap_local() is a complete NOP.
> 
> > The goal is to eliminate _all_ use cases of kmap_atomic*() and replace
> > them with kmap_local*(). This is a prerequisite for system protection
> > keys and other things.
> 
> Ira confirmed that system protection keys aren't affected.

I'm not sure what you mean here.  This work started because of system
protection keys.  But since they have yet to have a good use case they are not
yet upstream.  Therefore some _specific_ conversions have used page_address()
directly because those drivers know _exactly_ where and how the pages were
mapped.  That is a very specific use case which is different from what Thomas
is talking about.  General code which gets a page may still need to have
additional operations performed prior to using a kernel map.

> I'm not sure about
> what you are referring to when talking about "other things".

There are a number of other ideas where memory may not be in the direct map.

> 
> > Thanks,
> >
> >         tglx
> 
> Can you please say if you noticed other further misleading or plainly wrong
> sentences in my message? Ira and I are coordinating this conversions efforts
> (from kmap() and kmap_atomic() to kmap_local_page), so I'm much interested to
> know whether or not I'm wrong with regard to fundamental details.

I think the issue is you are trying to get way too much detail for patches
which don't require it.  If you review a patch like this and you find
kmap_atomic() was being used for the side effects note that and NAK the patch.
If you feel there is some evidence but you are unsure a quick email asking is
fine.  But if there is no evidence the patch is fine lets tag it with a review
and move on.

In this case people are getting frustrated with the 'bikesheding'[1].  We are
never going to complete all these conversions if we spend this much time on the
easy ones.

Ira

[1] https://en.wiktionary.org/wiki/bikeshedding

> 
> Thanks for helping and clarify,
> 
> Fabio