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

[Fabio M. De Francesco]

On mercoledì 16 novembre 2022 18:21:50 CET Ira Weiny wrote:
> 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.

You are right. Numbers don't support that "a large part of the users..." but
that it happened and will happen again. Let's delete "large". However this is
not the point. The real point is below...

> While I value your review of these conversions I think Kristen did her home
> work here.

I agree with you: she did her homework and the patch is good.

I don't know why I have not been able to convey that I appreciated her
homework. I especially appreciated that she cared of checking that the
code between mappings / un-mappings does not rely on any of the disabling side
effects of kmap_atomic so she could simply replace it with kmap_local_page().

I'm not sure why the message that this patch is good didn't pass. I suspect
that I stressed too much what I was considering minor inaccuracies.

In the while I missed to talk about what matters for real, i.e. that the
changes are good and that they will work properly.

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

I should avoid that kind of excessive focus on less relevant things.
Instead the overall end product is what matters the most.

[snip]

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

Yes we are. Did you note that I wrote it above?

I supposed that that was enough to see that I'm aware that Kristen knows what
she did and that she did it correctly. My objections were only about
minor details
in the commit message. I didn't ask for another version, but I admit
that several
parts of my message were ambiguous.

[snip]

> > 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 was referring to an email from you to Jonathan Corbet.[1]
Did I misunderstand it?

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

OK, I'm not yet aware of them.

[snip]

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

You are right. I'm sorry especially because we recently talked about
this topic.
But I fell again into the same trap :-(

Thanks,

Fabio

[1] https://lore.kernel.org/lkml/Ytny132kWjXvu1Ql@iweiny-desk3/