Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221115161627.4169428-1-kristen@linux.intel.com>
 <CAPj211ugzBFJHgDNtSgR2zB7ZcGa_EqOAQGhFSu938718u+yMQ@mail.gmail.com>
 <87cz9nqff2.ffs@tglx> <CAPj211shRbJQcnhEpjs461nEMQw2x53f+2G46CXWJg6_Rc-x-g@mail.gmail.com>
 <Y3UcLudVzuS55Bp5@iweiny-mobl>
In-Reply-To: <Y3UcLudVzuS55Bp5@iweiny-mobl>
From:   "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
Date:   Wed, 16 Nov 2022 21:00:55 +0100
Message-ID: <CAPj211tkReuuqyi8w2dFvrz3MFgmVFgnheDJbJ25_gCBvZX7VA@mail.gmail.com>
Subject: Re: [PATCH v2] x86/sgx: Replace kmap/kunmap_atomic calls
To:     Ira Weiny <ira.weiny@intel.com>
Cc:     jarkko@kernel.org, dave.hansen@linux.intel.com,
        linux-sgx@vger.kernel.org, Ingo Molnar <mingo@redhat.com>,
        Borislav Petkov <bp@alien8.de>, x86@kernel.org,
        "H. Peter Anvin" <hpa@zytor.com>,
        Kristen Carlson Accardi <kristen@linux.intel.com>,
        linux-kernel@vger.kernel.org, Thomas Gleixner <tglx@linutronix.de>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On mercoled=C3=AC 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=C3=AC 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=C3=AC 15 novembre 2022 17:16:26 CET Kristen Carlson Accar=
di
wrote:
> > > >> The use of kmap_atomic() in the SGX code was not an explicit desig=
n
> > > >> 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 32bi=
t
> > > HIGHMEM support.
> >
> > I just saw that the last part of my email is missing. In fact it's enou=
gh
> > 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 _som=
e_
> places where the side effect was required does not mean that "a large par=
t
> of the users..." do.

You are right. Numbers don't support that "a large part of the users..." bu=
t
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 ho=
me
> 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 s=
ide
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 / unmappi=
ng
> > 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 wh=
at
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 ar=
e
not
> yet upstream.  Therefore some _specific_ conversions have used
page_address()
> directly because those drivers know _exactly_ where and how the pages wer=
e
> 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 m=
ap.

OK, I'm not yet aware of them.

[snip]

> I think the issue is you are trying to get way too much detail for patche=
s
> 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 askin=
g
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/