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

[Fabio M. De Francesco]

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 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. 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 about
what you are referring to when talking about "other things".

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

Thanks for helping and clarify,

Fabio