[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 2/8] x86/paging: fold most HAP and shadow final teardown
On 05.01.2023 18:56, Andrew Cooper wrote: > On 22/12/2022 7:51 am, Jan Beulich wrote: >> On 21.12.2022 18:16, Andrew Cooper wrote: >>> On 21/12/2022 1:25 pm, Jan Beulich wrote: >>>> + d, d->arch.paging.total_pages, >>>> + d->arch.paging.free_pages, d->arch.paging.p2m_pages); >>>> + >>>> + if ( hap ) >>>> hap_final_teardown(d); >>>> + >>>> + /* >>>> + * Double-check that the domain didn't have any paging memory. >>>> + * It is possible for a domain that never got domain_kill()ed >>>> + * to get here with its paging allocation intact. >>> I know you're mostly just moving this comment, but it's misleading. >>> >>> This path is used for the domain_create() error path, and there will be >>> a nonzero allocation for HVM guests. >>> >>> I think we do want to rework this eventually - we will simplify things >>> massively by splitting the things can can only happen for a domain which >>> has run into relinquish_resources. >>> >>> At a minimum, I'd suggest dropping the first sentence. "double check" >>> implies it's an extraordinary case, which isn't warranted here IMO. >> Instead of dropping I think I would prefer to make it start "Make sure >> ...". > > That's still awkward grammar, and a bit misleading IMO. How about > rewriting it entirely? > > /* Remove remaining paging memory. This can be nonzero on certain error > paths. */ Fine with me, changed. >>>> + */ >>>> + if ( d->arch.paging.total_pages ) >>>> + { >>>> + if ( hap ) >>>> + hap_teardown(d, NULL); >>>> + else >>>> + shadow_teardown(d, NULL); >>>> + } >>>> + >>>> + /* It is now safe to pull down the p2m map. */ >>>> + p2m_teardown(p2m_get_hostp2m(d), true, NULL); >>>> + >>>> + /* Free any paging memory that the p2m teardown released. */ >>> I don't think this isn't true any more. 410 also made HAP/shadow free >>> pages fully for a dying domain, so p2m_teardown() at this point won't >>> add to the free memory pool. >>> >>> I think the subsequent *_set_allocation() can be dropped, and the >>> assertions left. >> I agree, but if anything this will want to be a separate patch then. > > I'd be happy putting that in this patch, but if you don't want to > combine it, then it should be a prerequisite IMO, so we don't have a > "clean up $X" patch which is shuffling buggy code around. Well, doing it the other way around (as I already had it before your reply) also has its advantages. Are you feeling very strong about the order? Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |