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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

Jan