Re: [PATCH 1/3] x86/mem_sharing: option to skip populating special pages during fork

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Mar 25, 2022 at 07:15:59AM -0400, Tamas K Lengyel wrote:
> On Fri, Mar 25, 2022, 6:59 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
> 
> > On Tue, Mar 22, 2022 at 01:41:37PM -0400, Tamas K Lengyel wrote:
> > > Add option to the fork memop to skip populating the fork with special
> > pages.
> > > These special pages are only necessary when setting up forks to be fully
> > > functional with a toolstack. For short-lived forks where no toolstack is
> > active
> > > these pages are uneccesary.
> >
> > Replying here because there's no cover letter AFAICT.
> >
> > For this kind of performance related changes it would be better if you
> > could provide some figures about the performance impact. It would help
> > if we knew whether avoiding mapping the vAPIC page means you can
> > create 0.1% more forks per-minute or 20%.
> >
> > If you really want to speed up the forking path it might be good to start
> > by perf sampling Xen in order to find the bottlenecks?
> >
> 
> Sure but for experiment systems I don't think its necessary to collect that
> data.

It helps weight whether the extra logic is worth the performance
benefit IMO. Here it might not matter that much since you say there's
also a non-performance reason for the change.

> There is also a non-performance reason why we want to keep special pages
> from being populated, in cases we really want the forks physmap to start
> empty for better control over its state. There was already a case where
> having special pages mapped in ended up triggering unexpected Xen behaviors
> leading to chain of events not easy to follow. For example if page 0 gets
> brought in while the vCPU is being created it ends up as a misconfigured
> ept entry if nested virtualization is enabled. That leads to ept
> misconfiguration exits instead of epf faults. Simply enforcing no entry in
> the physmap until forking is complete eliminates the chance of something
> like that happening again and makes reasoning about the VM's behavior from
> the start easier.

Could we have this added to the commit message then, and the option
renamed to 'empty_p2m' or some such. Then you should also ASSERT that
at the end of the fork process the p2m is indeed empty, not sure if
checking d->arch.paging.hap.p2m_pages == 0 would accomplish that?

Thanks, Roger.