Re: [PATCH v2] ACPI / processor_idle: use ndelay instead of io port access for wait

[Yin, Fengwei]

On 10/18/2019 6:12 PM, Rafael J. Wysocki wrote:
> On Wednesday, October 16, 2019 7:56:17 AM CEST Yin, Fengwei wrote:
> > Hi David,
> >
> > On 10/15/2019 7:48 PM, David Laight wrote:
> > > From: Yin Fengwei
> > > > Sent: 15 October 2019 09:04
> > > > In function acpi_idle_do_entry(), an ioport access is used for dummy
> > > > wait to guarantee hardware behavior. But it could trigger unnecessary
> > > > vmexit in virtualization environment.
> > > >
> > > > If we run linux as guest and export all available native C state to
> > > > guest, we did see many PM timer access triggered VMexit when guest
> > > > enter deeper C state in our environment (We used ACRN hypervisor
> > > > instead of kvm or xen which has PM timer emulated and exports all
> > > > native C state to guest).
> > > >
> > > > According to the original comments of this part of code, io port
> > > > access is only for dummy wait. We could use busy wait instead of io
> > > > port access to guarantee hardware behavior and avoid unnecessary
> > > > VMexit.
> > >
> > > You need some hard synchronisation instruction(s) after the inb()
> > > and before any kind of delay to ensure your delay code is executed
> > > after the inb() completes.
> > >
> > > I'm pretty sure that inb() is only synchronised with memory reads.
> > Thanks a lot for the comments.
> >
> > I didn't find the common serializing instructions API in kernel (only
> > memory  barrier which is used to make sure of memory access). For Intel
> > x86, cpuid could be used as serializing instruction. But it's not
> > suitable for common code here. Do you have any suggestion?
>
> In the virt guest case you don't need to worry at all AFAICS, because the inb()
> itself will trap to the HV.
This is not always valid. If the physical cpu is totally owned by guest 
(not shared with other guest), it's possible we passthru the C state
port to guest. In that case, inb() which trigger C state transaction
doesn't trap to the HV.

> > > ...
> > > > +	/* profiling the time used for dummy wait op */
> > > > +	ktime_get_real_ts64(&ts0);
> > > > +	inl(acpi_gbl_FADT.xpm_timer_block.address);
> > > > +	ktime_get_real_ts64(&ts1);
>
> You may as well use ktime_get() for this, as it's almost the same code as
> ktime_get_real_ts64() AFAICS, only simpler.
>
> Plus, static vars need not be initialized to 0.
Thanks for pointing this out. Will update the patch accordingly.

> > > That could be dominated by the cost of ktime_get_real_ts64().
> > > It also need synchronising instructions.
> > I did some testing. ktime_get_real_ts64() takes much less time than io
> > port access.
> >
> > The test code is like:
> > 1.
> > 	local_irq_save(flag);
> > 	ktime_get_real_ts64(&ts0);
> > 	inl(acpi_gbl_FADT.xpm_timer_block.address);
> > 	ktime_get_real_ts64(&ts1);
> > 	local_irq_restore(flag);
> >
> > 2.
> > 	local_irq_save(flag);
> > 	ktime_get_real_ts64(&ts0);
> > 	ktime_get_real_ts64(&ts1);
> > 	local_irq_restore(flag);
> >
> > The delta in 1 is about 500000ns. And delta in 2 is about
> > 2000ns. The date is gotten on Intel(R) Core(TM) i7-8700 CPU @ 3.20GHz.
> > So I suppose the impact of ktime_get_real_ts64 is small.
>
> You may not be hitting the worst case for ktime_get_real_ts64(), though.
>
> I wonder if special casing the virt guest would be a better approach.
>
> Then, you could leave the code as is for non-virt and I'm not sure if the
> delay is needed in the virt guest case at all.
>
> So maybe do something like "if not in a virt guest, do the dummy inl()"
> and that would be it?
Yes. This is better. Which we could control the impact to non-virt env.

Regards
Yin, Fengwei

>