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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Wed, Feb 22, 2023 at 5:48 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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.

Perfectly fine by me.

Thanks,

Tamas