Re: [PATCH v2] x86: Perform mem_sharing teardown before paging teardown

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.02.2023 16:59, Tamas K Lengyel wrote:
> On Tue, Feb 21, 2023 at 8:54 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> On 15.02.2023 18:07, Tamas K Lengyel wrote:
>>> An assert failure has been observed in p2m_teardown when performing vm
>>> forking and then destroying the forked VM (p2m-basic.c:173). The assert
>>> checks whether the domain's shared pages counter is 0. According to the
>>> patch that originally added the assert (7bedbbb5c31) the p2m_teardown
>>> should only happen after mem_sharing already relinquished all shared
> pages.
>>>
>>> In this patch we flip the order in which relinquish ops are called to
> avoid
>>> tripping the assert.
>>>
>>> Signed-off-by: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
>>> Fixes: e7aa55c0aab3 ("x86/p2m: free the paging memory pool
> preemptively")
>>
>> Especially considering the backporting requirement I'm disappointed to
>> find that you haven't extended the description to explain why this
>> heavier code churn is to be preferred over an adjustment to the offending
>> assertion. As said in reply to v1 - I'm willing to accept arguments
>> towards this being better, but I need to know those arguments.
> 
> The assert change as you proposed is hard to understand and will be thus
> harder to maintain. Conceptually sharing being torn down makes sense to
> happen before paging is torn down.

This is (perhaps) the crucial thing to say. Whereas ...

> Considering that's how it has been
> before the unfortunate regression I'm fixing here I don't think more
> justification is needed.

... I disagree here - that's _not_ how it has been before: paging_teardown()
has always been called first. The regression was with how much of teardown
is performed from there vs at the final stage of domain cleanup.

I'd be fine adding the "Conceptually ..." sentence to the description when
committing, of course provided you agree.

> The code-churn is minimal anyway.

Perspectives here may vary. Plus, as said, being the one to backport this
eventually, the larger the code change (even if pure movement), the larger
the chance of there being a conflict somewhere.

Jan