Re: [PATCH 1/2] x86: Perform mem_sharing teardown before paging teardown
On Thu, Feb 16, 2023 at 10:24 AM Jan Beulich wrote:
>
> On 16.02.2023 16:10, Tamas K Lengyel wrote:
> > On Thu, Feb 16, 2023 at 9:27 AM Jan Beulich wrote:
> >>
> >> On 16.02.2023 13:22, Tamas K Lengyel wrote:
> >>> On Thu, Feb 16, 2023 at 3:31 AM Jan Beulich wrote:
>
> On 15.02.2023 17:29, Tamas K Lengyel wrote:
> > On Wed, Feb 15, 2023 at 5:27 AM Jan Beulich
wrote:
> >> Did you consider the alternative of adjusting the ASSERT() in
> > question
> >> instead? It could reasonably become
> >>
> >> #ifdef CONFIG_MEM_SHARING
> >> ASSERT(!p2m_is_hostp2m(p2m) || !remove_root ||
> > !atomic_read(>shr_pages));
> >> #endif
> >>
> >> now, I think. That would be less intrusive a change (helpful for
> >> backporting), but there may be other (so far unnamed) benefits of
> >>> pulling
> >> ahead the shared memory teardown.
> >
> > I have a hard time understanding this proposed ASSERT.
>
> It accounts for the various ways p2m_teardown() can (now) be called,
> limiting the actual check for no remaining shared pages to the last
> of all these invocations (on the host p2m with remove_root=true).
>
> Maybe
>
> /* Limit the check to the final invocation. */
> if ( p2m_is_hostp2m(p2m) && remove_root )
> ASSERT(!atomic_read(>shr_pages));
>
> would make this easier to follow? Another option might be to move
> the assertion to paging_final_teardown(), ahead of that specific call
> to p2m_teardown().
> >>>
> >>> AFAICT d->shr_pages would still be more then 0 when this is called
> > before
> >>> sharing is torn down so the rearrangement is necessary even if we do
> > this
> >>> assert only on the final invocation. I did a printk in place of this
> > assert
> >>> without the rearrangement and afaict it was always != 0.
> >>
> >> Was your printk() in an if() as above? paging_final_teardown() runs
really
> >> late during cleanup (when we're about to free struct domain), so I
would
> >> be somewhat concerned if by that time the count was still non-zero.
> >
> > Just replaced the assert with a printk. Without calling relinquish
shared
> > pages I don't find it odd that the count is non-zero, it only gets
> > decremented when you do call relinquish. Once the order is corrected it
is
> > zero.
>
> I may be getting you wrong, but it feels like you're still talking about
> early invocations of p2m_teardown() (from underneath domain_kill()) when
> I'm talking about the final one. No matter what ordering inside
> domain_relinquish_resources() (called from domain_kill()), the freeing
> will have happened by the time that process completes. Which is before
> the (typically last) last domain ref would be put (near the end of
> domain_kill()), and hence before the domain freeing process might be
> invoked (which is where paging_final_teardown() gets involved; see
> {,arch_}domain_destroy()).
I don't recall seeing a count with 0 before I reordered things but the
output on the serial may also have been truncated due to it printing a ton
of lines very quickly, so it could be the last one was zero just didn't
make it to my screen.
Tamas
Re: [PATCH 1/2] x86: Perform mem_sharing teardown before paging teardown
On 16.02.2023 16:10, Tamas K Lengyel wrote:
> On Thu, Feb 16, 2023 at 9:27 AM Jan Beulich wrote:
>>
>> On 16.02.2023 13:22, Tamas K Lengyel wrote:
>>> On Thu, Feb 16, 2023 at 3:31 AM Jan Beulich wrote:
On 15.02.2023 17:29, Tamas K Lengyel wrote:
> On Wed, Feb 15, 2023 at 5:27 AM Jan Beulich wrote:
>> Did you consider the alternative of adjusting the ASSERT() in
> question
>> instead? It could reasonably become
>>
>> #ifdef CONFIG_MEM_SHARING
>> ASSERT(!p2m_is_hostp2m(p2m) || !remove_root ||
> !atomic_read(>shr_pages));
>> #endif
>>
>> now, I think. That would be less intrusive a change (helpful for
>> backporting), but there may be other (so far unnamed) benefits of
>>> pulling
>> ahead the shared memory teardown.
>
> I have a hard time understanding this proposed ASSERT.
It accounts for the various ways p2m_teardown() can (now) be called,
limiting the actual check for no remaining shared pages to the last
of all these invocations (on the host p2m with remove_root=true).
Maybe
/* Limit the check to the final invocation. */
if ( p2m_is_hostp2m(p2m) && remove_root )
ASSERT(!atomic_read(>shr_pages));
would make this easier to follow? Another option might be to move
the assertion to paging_final_teardown(), ahead of that specific call
to p2m_teardown().
>>>
>>> AFAICT d->shr_pages would still be more then 0 when this is called
> before
>>> sharing is torn down so the rearrangement is necessary even if we do
> this
>>> assert only on the final invocation. I did a printk in place of this
> assert
>>> without the rearrangement and afaict it was always != 0.
>>
>> Was your printk() in an if() as above? paging_final_teardown() runs really
>> late during cleanup (when we're about to free struct domain), so I would
>> be somewhat concerned if by that time the count was still non-zero.
>
> Just replaced the assert with a printk. Without calling relinquish shared
> pages I don't find it odd that the count is non-zero, it only gets
> decremented when you do call relinquish. Once the order is corrected it is
> zero.
I may be getting you wrong, but it feels like you're still talking about
early invocations of p2m_teardown() (from underneath domain_kill()) when
I'm talking about the final one. No matter what ordering inside
domain_relinquish_resources() (called from domain_kill()), the freeing
will have happened by the time that process completes. Which is before
the (typically last) last domain ref would be put (near the end of
domain_kill()), and hence before the domain freeing process might be
invoked (which is where paging_final_teardown() gets involved; see
{,arch_}domain_destroy()).
Jan
Re: [PATCH 1/2] x86: Perform mem_sharing teardown before paging teardown
On Thu, Feb 16, 2023 at 9:27 AM Jan Beulich wrote: > > On 16.02.2023 13:22, Tamas K Lengyel wrote: > > On Thu, Feb 16, 2023 at 3:31 AM Jan Beulich wrote: > >> > >> On 15.02.2023 17:29, Tamas K Lengyel wrote: > >>> On Wed, Feb 15, 2023 at 5:27 AM Jan Beulich wrote: > Did you consider the alternative of adjusting the ASSERT() in question > instead? It could reasonably become > > #ifdef CONFIG_MEM_SHARING > ASSERT(!p2m_is_hostp2m(p2m) || !remove_root || > >>> !atomic_read(>shr_pages)); > #endif > > now, I think. That would be less intrusive a change (helpful for > backporting), but there may be other (so far unnamed) benefits of > > pulling > ahead the shared memory teardown. > >>> > >>> I have a hard time understanding this proposed ASSERT. > >> > >> It accounts for the various ways p2m_teardown() can (now) be called, > >> limiting the actual check for no remaining shared pages to the last > >> of all these invocations (on the host p2m with remove_root=true). > >> > >> Maybe > >> > >> /* Limit the check to the final invocation. */ > >> if ( p2m_is_hostp2m(p2m) && remove_root ) > >> ASSERT(!atomic_read(>shr_pages)); > >> > >> would make this easier to follow? Another option might be to move > >> the assertion to paging_final_teardown(), ahead of that specific call > >> to p2m_teardown(). > > > > AFAICT d->shr_pages would still be more then 0 when this is called before > > sharing is torn down so the rearrangement is necessary even if we do this > > assert only on the final invocation. I did a printk in place of this assert > > without the rearrangement and afaict it was always != 0. > > Was your printk() in an if() as above? paging_final_teardown() runs really > late during cleanup (when we're about to free struct domain), so I would > be somewhat concerned if by that time the count was still non-zero. Just replaced the assert with a printk. Without calling relinquish shared pages I don't find it odd that the count is non-zero, it only gets decremented when you do call relinquish. Once the order is corrected it is zero. Tamas
Re: [PATCH 1/2] x86: Perform mem_sharing teardown before paging teardown
On 16.02.2023 13:22, Tamas K Lengyel wrote: > On Thu, Feb 16, 2023 at 3:31 AM Jan Beulich wrote: >> >> On 15.02.2023 17:29, Tamas K Lengyel wrote: >>> On Wed, Feb 15, 2023 at 5:27 AM Jan Beulich wrote: Did you consider the alternative of adjusting the ASSERT() in question instead? It could reasonably become #ifdef CONFIG_MEM_SHARING ASSERT(!p2m_is_hostp2m(p2m) || !remove_root || >>> !atomic_read(>shr_pages)); #endif now, I think. That would be less intrusive a change (helpful for backporting), but there may be other (so far unnamed) benefits of > pulling ahead the shared memory teardown. >>> >>> I have a hard time understanding this proposed ASSERT. >> >> It accounts for the various ways p2m_teardown() can (now) be called, >> limiting the actual check for no remaining shared pages to the last >> of all these invocations (on the host p2m with remove_root=true). >> >> Maybe >> >> /* Limit the check to the final invocation. */ >> if ( p2m_is_hostp2m(p2m) && remove_root ) >> ASSERT(!atomic_read(>shr_pages)); >> >> would make this easier to follow? Another option might be to move >> the assertion to paging_final_teardown(), ahead of that specific call >> to p2m_teardown(). > > AFAICT d->shr_pages would still be more then 0 when this is called before > sharing is torn down so the rearrangement is necessary even if we do this > assert only on the final invocation. I did a printk in place of this assert > without the rearrangement and afaict it was always != 0. Was your printk() in an if() as above? paging_final_teardown() runs really late during cleanup (when we're about to free struct domain), so I would be somewhat concerned if by that time the count was still non-zero. Jan
Re: [PATCH 1/2] x86: Perform mem_sharing teardown before paging teardown
On Thu, Feb 16, 2023 at 3:31 AM Jan Beulich wrote: > > On 15.02.2023 17:29, Tamas K Lengyel wrote: > > On Wed, Feb 15, 2023 at 5:27 AM Jan Beulich wrote: > >> Did you consider the alternative of adjusting the ASSERT() in question > >> instead? It could reasonably become > >> > >> #ifdef CONFIG_MEM_SHARING > >> ASSERT(!p2m_is_hostp2m(p2m) || !remove_root || > > !atomic_read(>shr_pages)); > >> #endif > >> > >> now, I think. That would be less intrusive a change (helpful for > >> backporting), but there may be other (so far unnamed) benefits of pulling > >> ahead the shared memory teardown. > > > > I have a hard time understanding this proposed ASSERT. > > It accounts for the various ways p2m_teardown() can (now) be called, > limiting the actual check for no remaining shared pages to the last > of all these invocations (on the host p2m with remove_root=true). > > Maybe > > /* Limit the check to the final invocation. */ > if ( p2m_is_hostp2m(p2m) && remove_root ) > ASSERT(!atomic_read(>shr_pages)); > > would make this easier to follow? Another option might be to move > the assertion to paging_final_teardown(), ahead of that specific call > to p2m_teardown(). AFAICT d->shr_pages would still be more then 0 when this is called before sharing is torn down so the rearrangement is necessary even if we do this assert only on the final invocation. I did a printk in place of this assert without the rearrangement and afaict it was always != 0. Tamas
Re: [PATCH 1/2] x86: Perform mem_sharing teardown before paging teardown
On 15.02.2023 17:29, Tamas K Lengyel wrote: > On Wed, Feb 15, 2023 at 5:27 AM Jan Beulich wrote: >> Did you consider the alternative of adjusting the ASSERT() in question >> instead? It could reasonably become >> >> #ifdef CONFIG_MEM_SHARING >> ASSERT(!p2m_is_hostp2m(p2m) || !remove_root || > !atomic_read(>shr_pages)); >> #endif >> >> now, I think. That would be less intrusive a change (helpful for >> backporting), but there may be other (so far unnamed) benefits of pulling >> ahead the shared memory teardown. > > I have a hard time understanding this proposed ASSERT. It accounts for the various ways p2m_teardown() can (now) be called, limiting the actual check for no remaining shared pages to the last of all these invocations (on the host p2m with remove_root=true). Maybe /* Limit the check to the final invocation. */ if ( p2m_is_hostp2m(p2m) && remove_root ) ASSERT(!atomic_read(>shr_pages)); would make this easier to follow? Another option might be to move the assertion to paging_final_teardown(), ahead of that specific call to p2m_teardown(). Jan
Re: [PATCH 1/2] x86: Perform mem_sharing teardown before paging teardown
On Wed, Feb 15, 2023 at 5:27 AM Jan Beulich wrote:
>
> On 14.02.2023 06:07, Tamas K Lengyel wrote:
> > An assert failure has been observed at p2m-basic.c:173 when performing
vm
>
> Please can you at least also name the function here? The line number is
> going to change, and when coming back to this change later, it'll be
> more troublesome to figure out which precise assertion was meant. Yes,
> ...
>
> > forking and then destroying the forked VM. The assert checks whether the
> > domain's shared pages counter is 0.
>
> ... you verbally describe it here, but still.
Sure.
>
> > 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
> > ---
> > Note: it is unclear why this assert hasn't tripped in the past. It
hasn't
> > been observed with 4.17-rc1 but it is in RELEASE-4.17.0
>
> That's almost certainly a result of e7aa55c0aab3 ("x86/p2m: free the
paging
> memory pool preemptively"), which added calls to p2m_teardown() to
> hap_teardown(). If you agree, this wants recording in a Fixes: tag, as
> - being an XSA fix - that change has been backported to everywhere, and
> hence the issue now also exists everywhere.
Ough.. In that case we'll need this patch backported too. Will add the
Fixes: tag.
>
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -2310,6 +2310,32 @@ int domain_relinquish_resources(struct domain *d)
> > if ( ret )
> > return ret;
> >
> > +#ifdef CONFIG_MEM_SHARING
> > +PROGRESS(shared):
>
> If we go with the re-ordering as you suggest, then please also move the
> enumerator near the top of the switch() body.
Sure.
>
> Did you consider the alternative of adjusting the ASSERT() in question
> instead? It could reasonably become
>
> #ifdef CONFIG_MEM_SHARING
> ASSERT(!p2m_is_hostp2m(p2m) || !remove_root ||
!atomic_read(>shr_pages));
> #endif
>
> now, I think. That would be less intrusive a change (helpful for
> backporting), but there may be other (so far unnamed) benefits of pulling
> ahead the shared memory teardown.
I have a hard time understanding this proposed ASSERT.
>
> > +if ( is_hvm_domain(d) )
> > +{
> > +/* If the domain has shared pages, relinquish them allowing
> > + * for preemption. */
>
> Similarly, if the code is to be moved, please correct style here at this
> occasion.
Sure.
>
> > +ret = relinquish_shared_pages(d);
> > +if ( ret )
> > +return ret;
>
> While I can easily agree with the movement ahead of this being okay, ...
>
> > +/*
> > + * If the domain is forked, decrement the parent's pause
count
> > + * and release the domain.
> > + */
> > +if ( mem_sharing_is_fork(d) )
> > +{
> > +struct domain *parent = d->parent;
> > +
> > +d->parent = NULL;
> > +domain_unpause(parent);
> > +put_domain(parent);
> > +}
>
> ... I can only trust you on being sure that moving ahead of this is
> okay, too.
That's fine, we are in the teardown of the fork so it doesn't matter where
you are releasing the parent as long as its after the fork is unlinked from
it.
Thanks,
Tamas
Re: [PATCH 1/2] x86: Perform mem_sharing teardown before paging teardown
On 14.02.2023 06:07, Tamas K Lengyel wrote:
> An assert failure has been observed at p2m-basic.c:173 when performing vm
Please can you at least also name the function here? The line number is
going to change, and when coming back to this change later, it'll be
more troublesome to figure out which precise assertion was meant. Yes,
...
> forking and then destroying the forked VM. The assert checks whether the
> domain's shared pages counter is 0.
... you verbally describe it here, but still.
> 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
> ---
> Note: it is unclear why this assert hasn't tripped in the past. It hasn't
> been observed with 4.17-rc1 but it is in RELEASE-4.17.0
That's almost certainly a result of e7aa55c0aab3 ("x86/p2m: free the paging
memory pool preemptively"), which added calls to p2m_teardown() to
hap_teardown(). If you agree, this wants recording in a Fixes: tag, as
- being an XSA fix - that change has been backported to everywhere, and
hence the issue now also exists everywhere.
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -2310,6 +2310,32 @@ int domain_relinquish_resources(struct domain *d)
> if ( ret )
> return ret;
>
> +#ifdef CONFIG_MEM_SHARING
> +PROGRESS(shared):
If we go with the re-ordering as you suggest, then please also move the
enumerator near the top of the switch() body.
Did you consider the alternative of adjusting the ASSERT() in question
instead? It could reasonably become
#ifdef CONFIG_MEM_SHARING
ASSERT(!p2m_is_hostp2m(p2m) || !remove_root || !atomic_read(>shr_pages));
#endif
now, I think. That would be less intrusive a change (helpful for
backporting), but there may be other (so far unnamed) benefits of pulling
ahead the shared memory teardown.
> +if ( is_hvm_domain(d) )
> +{
> +/* If the domain has shared pages, relinquish them allowing
> + * for preemption. */
Similarly, if the code is to be moved, please correct style here at this
occasion.
> +ret = relinquish_shared_pages(d);
> +if ( ret )
> +return ret;
While I can easily agree with the movement ahead of this being okay, ...
> +/*
> + * If the domain is forked, decrement the parent's pause count
> + * and release the domain.
> + */
> +if ( mem_sharing_is_fork(d) )
> +{
> +struct domain *parent = d->parent;
> +
> +d->parent = NULL;
> +domain_unpause(parent);
> +put_domain(parent);
> +}
... I can only trust you on being sure that moving ahead of this is
okay, too.
Jan
