Re: [PATCH 2/2] x86/hap: Resolve mm-lock order violations when forking VMs with nested p2m

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Thu, Jan 7, 2021 at 7:56 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 07.01.2021 13:43, Tamas K Lengyel wrote:
> > On Thu, Jan 7, 2021 at 7:26 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >> On 06.01.2021 17:26, Tamas K Lengyel wrote:
> >>> On Wed, Jan 6, 2021 at 11:11 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >>>> On 06.01.2021 16:29, Tamas K Lengyel wrote:
> >>>>> On Wed, Jan 6, 2021 at 7:03 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >>>>>> On 04.01.2021 18:41, Tamas K Lengyel wrote:
> >>>>>>> --- a/xen/arch/x86/mm/p2m.c
> >>>>>>> +++ b/xen/arch/x86/mm/p2m.c
> >>>>>>> @@ -1598,8 +1598,17 @@ void
> >>>>>>>  p2m_flush_nestedp2m(struct domain *d)
> >>>>>>>  {
> >>>>>>>      int i;
> >>>>>>> +    struct p2m_domain *p2m;
> >>>>>>> +
> >>>>>>>      for ( i = 0; i < MAX_NESTEDP2M; i++ )
> >>>>>>> -        p2m_flush_table(d->arch.nested_p2m[i]);
> >>>>>>> +    {
> >>>>>>> +        p2m = d->arch.nested_p2m[i];
> >>>>>>
> >>>>>> Please move the declaration here, making this the variable's
> >>>>>> initializer (unless line length constraints make the latter
> >>>>>> undesirable).
> >>>>>
> >>>>> I really don't get what difference this would make.
> >>>>
> >>>> Both choice of (generally) inappropriate types (further up)
> >>>> and placement of declarations (here) (and of course also
> >>>> other style violations) can set bad precedents even if in a
> >>>> specific case it may not matter much. So yes, it may be
> >>>> good enough here, but it would violate our desire to
> >>>> - use unsigned types when a variable will hold only non-
> >>>>   negative values (which in the general case may improve
> >>>>   generated code in particular on x86-64),
> >>>> - limit the scopes of variables as much as possible, to
> >>>>   more easily spot inappropriate uses (like bypassing
> >>>>   initialization).
> >>>>
> >>>> This code here actually demonstrates such a bad precedent,
> >>>> using plain int for the loop induction variable. While I
> >>>> can't be any way near sure, there's a certain chance you
> >>>> actually took it and copied it to
> >>>> __mem_sharing_unshare_page(). The chance of such happening
> >>>> is what we'd like to reduce over time.
> >>>
> >>> Yes, I copied it from p2m.c. All I meant was that such minor changes
> >>> are generally speaking not worth a round-trip of sending new patches.
> >>> I obviously don't care whether this is signed or unsigned. Minor stuff
> >>> like that could be changed on commit and is not even worth having a
> >>> discussion about.
> >>
> >> I'm sorry, but no. Committing ought to be a purely mechanical
> >> thing. It is a _courtesy_ of the committer if they offer to
> >> make adjustments. If us offering this regularly gets taken as
> >> "expected behavior", I'm afraid I personally would stop making
> >> such an offer, and instead insist on further round trips.
> >
> > So you requested changes I consider purely cosmetic, no objections to
> > any of them. It would be faster if you just made those changes -
> > literally 2 seconds - instead of insisting on this back and forth. I
> > really have no idea under what metric this saves *you* time. Now you
> > have to write emails to point out the issues and review the patch
> > twice, including the lag time of when the sender has time to do the
> > changes and resend the patches.
>
> For one, I didn't talk about either process saving time, I don't
> think. Then I had comments beyond these purely cosmetic ones.
> Therefore I didn't think it was justified to offer making the
> mechanical adjustments at commit time. Making such an offer will
> please remain subject to the individual's judgement, without
> having to justify _at all_ when not making such an offer.
>
> As to time savings - even if I had offered making these changes,
> I'd still have to give the respective comments. Both for your
> awareness (after all I'd be changing your patch, and you might
> not like me doing so), and to hopefully have the effect that in
> future submissions you'd take care of such aspects yourself
> right away (plus same for any possible observers of the thread).
>
> > I think this process is just bad for
> > everyone involved. And now you are saying out of principle you would
> > be insisting on more of this just to prove a point? Yea, that would
> > certainly solve the problem of commit lag and backlog of reviewing
> > patches. But it's your call, I really don't care enough to argue any
> > more about it.
>
> I have to admit that I find this odd: If there's disagreement,
> wouldn't it generally be better to get it resolved?
>

I don't see where the disagreement was, I had no objections to the
changes you requested. I don't like this unnecessary back and forth on
trivia. But v2 is sent. I'm moving on.

Tamas