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....@citrix.com> 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.
