On Thu, Feb 16, 2023 at 3:31 AM Jan Beulich <[email protected]> wrote: > > On 15.02.2023 17:29, Tamas K Lengyel wrote: > > On Wed, Feb 15, 2023 at 5:27 AM Jan Beulich <[email protected]> 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(&d->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(&d->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
