* Re: [PATCH RFC v3 1/6] x86/paravirt: Add pv_idle_ops to paravirt ops
From: Quan Xu @ 2017-11-14 8:15 UTC (permalink / raw)
To: Wanpeng Li
Cc: Juergen Gross, Yang Zhang, Rusty Russell, kvm, linux-doc,
the arch/x86 maintainers, linux-kernel@vger.kernel.org,
virtualization, Ingo Molnar, Quan Xu, H. Peter Anvin,
open list:FILESYSTEMS (VFS and infrastructure), xen-devel,
Alok Kataria, Thomas Gleixner
In-Reply-To: <CANRm+CxirO0aSAQnOfPG0+KPCOnXf3SEASwBiprLmtncZE53pA@mail.gmail.com>
On 2017/11/14 15:12, Wanpeng Li wrote:
> 2017-11-14 15:02 GMT+08:00 Quan Xu <quan.xu0@gmail.com>:
>>
>> On 2017/11/13 18:53, Juergen Gross wrote:
>>> On 13/11/17 11:06, Quan Xu wrote:
>>>> From: Quan Xu <quan.xu0@gmail.com>
>>>>
>>>> So far, pv_idle_ops.poll is the only ops for pv_idle. .poll is called
>>>> in idle path which will poll for a while before we enter the real idle
>>>> state.
>>>>
>>>> In virtualization, idle path includes several heavy operations
>>>> includes timer access(LAPIC timer or TSC deadline timer) which will
>>>> hurt performance especially for latency intensive workload like message
>>>> passing task. The cost is mainly from the vmexit which is a hardware
>>>> context switch between virtual machine and hypervisor. Our solution is
>>>> to poll for a while and do not enter real idle path if we can get the
>>>> schedule event during polling.
>>>>
>>>> Poll may cause the CPU waste so we adopt a smart polling mechanism to
>>>> reduce the useless poll.
>>>>
>>>> Signed-off-by: Yang Zhang <yang.zhang.wz@gmail.com>
>>>> Signed-off-by: Quan Xu <quan.xu0@gmail.com>
>>>> Cc: Juergen Gross <jgross@suse.com>
>>>> Cc: Alok Kataria <akataria@vmware.com>
>>>> Cc: Rusty Russell <rusty@rustcorp.com.au>
>>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>>> Cc: Ingo Molnar <mingo@redhat.com>
>>>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>>>> Cc: x86@kernel.org
>>>> Cc: virtualization@lists.linux-foundation.org
>>>> Cc: linux-kernel@vger.kernel.org
>>>> Cc: xen-devel@lists.xenproject.org
>>> Hmm, is the idle entry path really so critical to performance that a new
>>> pvops function is necessary?
>> Juergen, Here is the data we get when running benchmark netperf:
>> 1. w/o patch and disable kvm dynamic poll (halt_poll_ns=0):
>> 29031.6 bit/s -- 76.1 %CPU
>>
>> 2. w/ patch and disable kvm dynamic poll (halt_poll_ns=0):
>> 35787.7 bit/s -- 129.4 %CPU
>>
>> 3. w/ kvm dynamic poll:
>> 35735.6 bit/s -- 200.0 %CPU
> Actually we can reduce the CPU utilization by sleeping a period of
> time as what has already been done in the poll logic of IO subsystem,
> then we can improve the algorithm in kvm instead of introduing another
> duplicate one in the kvm guest.
We really appreciate upstream's kvm dynamic poll mechanism, which is
really helpful for a lot of scenario..
However, as description said, in virtualization, idle path includes
several heavy operations includes timer access (LAPIC timer or TSC
deadline timer) which will hurt performance especially for latency
intensive workload like message passing task. The cost is mainly from
the vmexit which is a hardware context switch between virtual machine
and hypervisor.
for upstream's kvm dynamic poll mechanism, even you could provide a
better algorism, how could you bypass timer access (LAPIC timer or TSC
deadline timer), or a hardware context switch between virtual machine
and hypervisor. I know these is a tradeoff.
Furthermore, here is the data we get when running benchmark contextswitch
to measure the latency(lower is better):
1. w/o patch and disable kvm dynamic poll (halt_poll_ns=0):
3402.9 ns/ctxsw -- 199.8 %CPU
2. w/ patch and disable kvm dynamic poll:
1163.5 ns/ctxsw -- 205.5 %CPU
3. w/ kvm dynamic poll:
2280.6 ns/ctxsw -- 199.5 %CPU
so, these tow solution are quite similar, but not duplicate..
that's also why to add a generic idle poll before enter real idle path.
When a reschedule event is pending, we can bypass the real idle path.
Quan
Alibaba Cloud
> Regards,
> Wanpeng Li
>
>> 4. w/patch and w/ kvm dynamic poll:
>> 42225.3 bit/s -- 198.7 %CPU
>>
>> 5. idle=poll
>> 37081.7 bit/s -- 998.1 %CPU
>>
>>
>>
>> w/ this patch, we will improve performance by 23%.. even we could improve
>> performance by 45.4%, if we use w/patch and w/ kvm dynamic poll. also the
>> cost of CPU is much lower than 'idle=poll' case..
>>
>>> Wouldn't a function pointer, maybe guarded
>>> by a static key, be enough? A further advantage would be that this would
>>> work on other architectures, too.
>>
>> I assume this feature will be ported to other archs.. a new pvops makes code
>> clean and easy to maintain. also I tried to add it into existed pvops, but
>> it
>> doesn't match.
>>
>>
>>
>> Quan
>> Alibaba Cloud
>>>
>>> Juergen
>>>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH RFC v3 1/6] x86/paravirt: Add pv_idle_ops to paravirt ops
From: Wanpeng Li @ 2017-11-14 8:22 UTC (permalink / raw)
To: Quan Xu
Cc: Juergen Gross, Yang Zhang, Rusty Russell, kvm, linux-doc,
the arch/x86 maintainers, linux-kernel@vger.kernel.org,
virtualization, Ingo Molnar, Quan Xu, H. Peter Anvin,
open list:FILESYSTEMS (VFS and infrastructure), xen-devel,
Alok Kataria, Thomas Gleixner
In-Reply-To: <d233b179-9815-0134-ef04-c492f9523121@gmail.com>
2017-11-14 16:15 GMT+08:00 Quan Xu <quan.xu0@gmail.com>:
>
>
> On 2017/11/14 15:12, Wanpeng Li wrote:
>>
>> 2017-11-14 15:02 GMT+08:00 Quan Xu <quan.xu0@gmail.com>:
>>>
>>>
>>> On 2017/11/13 18:53, Juergen Gross wrote:
>>>>
>>>> On 13/11/17 11:06, Quan Xu wrote:
>>>>>
>>>>> From: Quan Xu <quan.xu0@gmail.com>
>>>>>
>>>>> So far, pv_idle_ops.poll is the only ops for pv_idle. .poll is called
>>>>> in idle path which will poll for a while before we enter the real idle
>>>>> state.
>>>>>
>>>>> In virtualization, idle path includes several heavy operations
>>>>> includes timer access(LAPIC timer or TSC deadline timer) which will
>>>>> hurt performance especially for latency intensive workload like message
>>>>> passing task. The cost is mainly from the vmexit which is a hardware
>>>>> context switch between virtual machine and hypervisor. Our solution is
>>>>> to poll for a while and do not enter real idle path if we can get the
>>>>> schedule event during polling.
>>>>>
>>>>> Poll may cause the CPU waste so we adopt a smart polling mechanism to
>>>>> reduce the useless poll.
>>>>>
>>>>> Signed-off-by: Yang Zhang <yang.zhang.wz@gmail.com>
>>>>> Signed-off-by: Quan Xu <quan.xu0@gmail.com>
>>>>> Cc: Juergen Gross <jgross@suse.com>
>>>>> Cc: Alok Kataria <akataria@vmware.com>
>>>>> Cc: Rusty Russell <rusty@rustcorp.com.au>
>>>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>>>> Cc: Ingo Molnar <mingo@redhat.com>
>>>>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>>>>> Cc: x86@kernel.org
>>>>> Cc: virtualization@lists.linux-foundation.org
>>>>> Cc: linux-kernel@vger.kernel.org
>>>>> Cc: xen-devel@lists.xenproject.org
>>>>
>>>> Hmm, is the idle entry path really so critical to performance that a new
>>>> pvops function is necessary?
>>>
>>> Juergen, Here is the data we get when running benchmark netperf:
>>> 1. w/o patch and disable kvm dynamic poll (halt_poll_ns=0):
>>> 29031.6 bit/s -- 76.1 %CPU
>>>
>>> 2. w/ patch and disable kvm dynamic poll (halt_poll_ns=0):
>>> 35787.7 bit/s -- 129.4 %CPU
>>>
>>> 3. w/ kvm dynamic poll:
>>> 35735.6 bit/s -- 200.0 %CPU
>>
>> Actually we can reduce the CPU utilization by sleeping a period of
>> time as what has already been done in the poll logic of IO subsystem,
>> then we can improve the algorithm in kvm instead of introduing another
>> duplicate one in the kvm guest.
>
> We really appreciate upstream's kvm dynamic poll mechanism, which is
> really helpful for a lot of scenario..
>
> However, as description said, in virtualization, idle path includes
> several heavy operations includes timer access (LAPIC timer or TSC
> deadline timer) which will hurt performance especially for latency
> intensive workload like message passing task. The cost is mainly from
> the vmexit which is a hardware context switch between virtual machine
> and hypervisor.
>
> for upstream's kvm dynamic poll mechanism, even you could provide a
> better algorism, how could you bypass timer access (LAPIC timer or TSC
> deadline timer), or a hardware context switch between virtual machine
> and hypervisor. I know these is a tradeoff.
>
> Furthermore, here is the data we get when running benchmark contextswitch
> to measure the latency(lower is better):
>
> 1. w/o patch and disable kvm dynamic poll (halt_poll_ns=0):
> 3402.9 ns/ctxsw -- 199.8 %CPU
>
> 2. w/ patch and disable kvm dynamic poll:
> 1163.5 ns/ctxsw -- 205.5 %CPU
>
> 3. w/ kvm dynamic poll:
> 2280.6 ns/ctxsw -- 199.5 %CPU
>
> so, these tow solution are quite similar, but not duplicate..
>
> that's also why to add a generic idle poll before enter real idle path.
> When a reschedule event is pending, we can bypass the real idle path.
>
There is a similar logic in the idle governor/driver, so how this
patchset influence the decision in the idle governor/driver when
running on bare-metal(power managment is not exposed to the guest so
we will not enter into idle driver in the guest)?
Regards,
Wanpeng Li
^ permalink raw reply
* Re: [PATCH RFC v3 1/6] x86/paravirt: Add pv_idle_ops to paravirt ops
From: Quan Xu @ 2017-11-14 9:38 UTC (permalink / raw)
To: Juergen Gross, Quan Xu, kvm, linux-doc, linux-fsdevel,
linux-kernel, virtualization, x86, xen-devel
Cc: Yang Zhang, Rusty Russell, Ingo Molnar, H. Peter Anvin,
Alok Kataria, Thomas Gleixner
In-Reply-To: <fe6eeed1-4eee-eaaa-df3b-8979af8a3891@suse.com>
On 2017/11/14 15:30, Juergen Gross wrote:
> On 14/11/17 08:02, Quan Xu wrote:
>>
>> On 2017/11/13 18:53, Juergen Gross wrote:
>>> On 13/11/17 11:06, Quan Xu wrote:
>>>> From: Quan Xu <quan.xu0@gmail.com>
>>>>
>>>> So far, pv_idle_ops.poll is the only ops for pv_idle. .poll is called
>>>> in idle path which will poll for a while before we enter the real idle
>>>> state.
>>>>
>>>> In virtualization, idle path includes several heavy operations
>>>> includes timer access(LAPIC timer or TSC deadline timer) which will
>>>> hurt performance especially for latency intensive workload like message
>>>> passing task. The cost is mainly from the vmexit which is a hardware
>>>> context switch between virtual machine and hypervisor. Our solution is
>>>> to poll for a while and do not enter real idle path if we can get the
>>>> schedule event during polling.
>>>>
>>>> Poll may cause the CPU waste so we adopt a smart polling mechanism to
>>>> reduce the useless poll.
>>>>
>>>> Signed-off-by: Yang Zhang <yang.zhang.wz@gmail.com>
>>>> Signed-off-by: Quan Xu <quan.xu0@gmail.com>
>>>> Cc: Juergen Gross <jgross@suse.com>
>>>> Cc: Alok Kataria <akataria@vmware.com>
>>>> Cc: Rusty Russell <rusty@rustcorp.com.au>
>>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>>> Cc: Ingo Molnar <mingo@redhat.com>
>>>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>>>> Cc: x86@kernel.org
>>>> Cc: virtualization@lists.linux-foundation.org
>>>> Cc: linux-kernel@vger.kernel.org
>>>> Cc: xen-devel@lists.xenproject.org
>>> Hmm, is the idle entry path really so critical to performance that a new
>>> pvops function is necessary?
>> Juergen, Here is the data we get when running benchmark netperf:
>> 1. w/o patch and disable kvm dynamic poll (halt_poll_ns=0):
>> 29031.6 bit/s -- 76.1 %CPU
>>
>> 2. w/ patch and disable kvm dynamic poll (halt_poll_ns=0):
>> 35787.7 bit/s -- 129.4 %CPU
>>
>> 3. w/ kvm dynamic poll:
>> 35735.6 bit/s -- 200.0 %CPU
>>
>> 4. w/patch and w/ kvm dynamic poll:
>> 42225.3 bit/s -- 198.7 %CPU
>>
>> 5. idle=poll
>> 37081.7 bit/s -- 998.1 %CPU
>>
>>
>>
>> w/ this patch, we will improve performance by 23%.. even we could improve
>> performance by 45.4%, if we use w/patch and w/ kvm dynamic poll. also the
>> cost of CPU is much lower than 'idle=poll' case..
> I don't question the general idea. I just think pvops isn't the best way
> to implement it.
>
>>> Wouldn't a function pointer, maybe guarded
>>> by a static key, be enough? A further advantage would be that this would
>>> work on other architectures, too.
>> I assume this feature will be ported to other archs.. a new pvops makes
sorry, a typo.. /other archs/other hypervisors/
it refers hypervisor like Xen, HyperV and VMware)..
>> code
>> clean and easy to maintain. also I tried to add it into existed pvops,
>> but it
>> doesn't match.
> You are aware that pvops is x86 only?
yes, I'm aware..
> I really don't see the big difference in maintainability compared to the
> static key / function pointer variant:
>
> void (*guest_idle_poll_func)(void);
> struct static_key guest_idle_poll_key __read_mostly;
>
> static inline void guest_idle_poll(void)
> {
> if (static_key_false(&guest_idle_poll_key))
> guest_idle_poll_func();
> }
thank you for your sample code :)
I agree there is no big difference.. I think we are discussion for two
things:
1) x86 VM on different hypervisors
2) different archs VM on kvm hypervisor
What I want to do is x86 VM on different hypervisors, such as kvm / xen
/ hyperv ..
> And KVM would just need to set guest_idle_poll_func and enable the
> static key. Works on non-x86 architectures, too.
>
.. referred to 'pv_mmu_ops', HyperV and Xen can implement their own
functions for 'pv_mmu_ops'.
I think it is the same to pv_idle_ops.
with above explaination, do you still think I need to define the static
key/function pointer variant?
btw, any interest to port it to Xen HVM guest? :)
Quan
Alibaba Cloud
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH] drm/virtio: add create_handle support.
From: Gerd Hoffmann @ 2017-11-14 10:11 UTC (permalink / raw)
To: lepton; +Cc: airlied, dri-devel, virtualization
In-Reply-To: <CALqoU4y=w8m5g_vCHTRmiBYg1g8W1xcJ5qih2pj+HQeQ2tNo2Q@mail.gmail.com>
On Mon, Nov 13, 2017 at 02:19:29PM -0800, lepton wrote:
> Ping.
>
> On Wed, Nov 8, 2017 at 10:42 AM, Lepton Wu <ytht.net@gmail.com> wrote:
> > Add create_handle support to virtio fb. Without this, screenshot tool
> > in chromium OS can't work.
Already queued up in drm-misc-next (the cirrus patch too).
cheers,
Gerd
^ permalink raw reply
* Re: [PATCH RFC v3 1/6] x86/paravirt: Add pv_idle_ops to paravirt ops
From: Quan Xu @ 2017-11-14 10:23 UTC (permalink / raw)
To: Wanpeng Li
Cc: Juergen Gross, Yang Zhang, Rusty Russell, kvm, linux-doc,
the arch/x86 maintainers, linux-kernel@vger.kernel.org,
virtualization, Ingo Molnar, Quan Xu, H. Peter Anvin,
open list:FILESYSTEMS (VFS and infrastructure), xen-devel,
Alok Kataria, Thomas Gleixner
In-Reply-To: <CANRm+Cycx3ewegOXR7c70kpdsaJA-=M5QztDt4J2L=VqpeCsfQ@mail.gmail.com>
On 2017/11/14 16:22, Wanpeng Li wrote:
> 2017-11-14 16:15 GMT+08:00 Quan Xu <quan.xu0@gmail.com>:
>>
>> On 2017/11/14 15:12, Wanpeng Li wrote:
>>> 2017-11-14 15:02 GMT+08:00 Quan Xu <quan.xu0@gmail.com>:
>>>>
>>>> On 2017/11/13 18:53, Juergen Gross wrote:
>>>>> On 13/11/17 11:06, Quan Xu wrote:
>>>>>> From: Quan Xu <quan.xu0@gmail.com>
>>>>>>
>>>>>> So far, pv_idle_ops.poll is the only ops for pv_idle. .poll is called
>>>>>> in idle path which will poll for a while before we enter the real idle
>>>>>> state.
>>>>>>
>>>>>> In virtualization, idle path includes several heavy operations
>>>>>> includes timer access(LAPIC timer or TSC deadline timer) which will
>>>>>> hurt performance especially for latency intensive workload like message
>>>>>> passing task. The cost is mainly from the vmexit which is a hardware
>>>>>> context switch between virtual machine and hypervisor. Our solution is
>>>>>> to poll for a while and do not enter real idle path if we can get the
>>>>>> schedule event during polling.
>>>>>>
>>>>>> Poll may cause the CPU waste so we adopt a smart polling mechanism to
>>>>>> reduce the useless poll.
>>>>>>
>>>>>> Signed-off-by: Yang Zhang <yang.zhang.wz@gmail.com>
>>>>>> Signed-off-by: Quan Xu <quan.xu0@gmail.com>
>>>>>> Cc: Juergen Gross <jgross@suse.com>
>>>>>> Cc: Alok Kataria <akataria@vmware.com>
>>>>>> Cc: Rusty Russell <rusty@rustcorp.com.au>
>>>>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>>>>> Cc: Ingo Molnar <mingo@redhat.com>
>>>>>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>>>>>> Cc: x86@kernel.org
>>>>>> Cc: virtualization@lists.linux-foundation.org
>>>>>> Cc: linux-kernel@vger.kernel.org
>>>>>> Cc: xen-devel@lists.xenproject.org
>>>>> Hmm, is the idle entry path really so critical to performance that a new
>>>>> pvops function is necessary?
>>>> Juergen, Here is the data we get when running benchmark netperf:
>>>> 1. w/o patch and disable kvm dynamic poll (halt_poll_ns=0):
>>>> 29031.6 bit/s -- 76.1 %CPU
>>>>
>>>> 2. w/ patch and disable kvm dynamic poll (halt_poll_ns=0):
>>>> 35787.7 bit/s -- 129.4 %CPU
>>>>
>>>> 3. w/ kvm dynamic poll:
>>>> 35735.6 bit/s -- 200.0 %CPU
>>> Actually we can reduce the CPU utilization by sleeping a period of
>>> time as what has already been done in the poll logic of IO subsystem,
>>> then we can improve the algorithm in kvm instead of introduing another
>>> duplicate one in the kvm guest.
>> We really appreciate upstream's kvm dynamic poll mechanism, which is
>> really helpful for a lot of scenario..
>>
>> However, as description said, in virtualization, idle path includes
>> several heavy operations includes timer access (LAPIC timer or TSC
>> deadline timer) which will hurt performance especially for latency
>> intensive workload like message passing task. The cost is mainly from
>> the vmexit which is a hardware context switch between virtual machine
>> and hypervisor.
>>
>> for upstream's kvm dynamic poll mechanism, even you could provide a
>> better algorism, how could you bypass timer access (LAPIC timer or TSC
>> deadline timer), or a hardware context switch between virtual machine
>> and hypervisor. I know these is a tradeoff.
>>
>> Furthermore, here is the data we get when running benchmark contextswitch
>> to measure the latency(lower is better):
>>
>> 1. w/o patch and disable kvm dynamic poll (halt_poll_ns=0):
>> 3402.9 ns/ctxsw -- 199.8 %CPU
>>
>> 2. w/ patch and disable kvm dynamic poll:
>> 1163.5 ns/ctxsw -- 205.5 %CPU
>>
>> 3. w/ kvm dynamic poll:
>> 2280.6 ns/ctxsw -- 199.5 %CPU
>>
>> so, these tow solution are quite similar, but not duplicate..
>>
>> that's also why to add a generic idle poll before enter real idle path.
>> When a reschedule event is pending, we can bypass the real idle path.
>>
> There is a similar logic in the idle governor/driver, so how this
> patchset influence the decision in the idle governor/driver when
> running on bare-metal(power managment is not exposed to the guest so
> we will not enter into idle driver in the guest)?
>
This is expected to take effect only when running as a virtual machine with
proper CONFIG_* enabled. This can not work on bare mental even with proper
CONFIG_* enabled.
Quan
Alibaba Cloud
^ permalink raw reply
* Re: [PATCH RFC v3 1/6] x86/paravirt: Add pv_idle_ops to paravirt ops
From: Juergen Gross @ 2017-11-14 10:27 UTC (permalink / raw)
To: Quan Xu, Quan Xu, kvm, linux-doc, linux-fsdevel, linux-kernel,
virtualization, x86, xen-devel
Cc: Yang Zhang, Rusty Russell, Ingo Molnar, H. Peter Anvin,
Alok Kataria, Thomas Gleixner
In-Reply-To: <79dad15c-2d26-bcf3-7283-293e42a161ea@gmail.com>
On 14/11/17 10:38, Quan Xu wrote:
>
>
> On 2017/11/14 15:30, Juergen Gross wrote:
>> On 14/11/17 08:02, Quan Xu wrote:
>>>
>>> On 2017/11/13 18:53, Juergen Gross wrote:
>>>> On 13/11/17 11:06, Quan Xu wrote:
>>>>> From: Quan Xu <quan.xu0@gmail.com>
>>>>>
>>>>> So far, pv_idle_ops.poll is the only ops for pv_idle. .poll is called
>>>>> in idle path which will poll for a while before we enter the real idle
>>>>> state.
>>>>>
>>>>> In virtualization, idle path includes several heavy operations
>>>>> includes timer access(LAPIC timer or TSC deadline timer) which will
>>>>> hurt performance especially for latency intensive workload like
>>>>> message
>>>>> passing task. The cost is mainly from the vmexit which is a hardware
>>>>> context switch between virtual machine and hypervisor. Our solution is
>>>>> to poll for a while and do not enter real idle path if we can get the
>>>>> schedule event during polling.
>>>>>
>>>>> Poll may cause the CPU waste so we adopt a smart polling mechanism to
>>>>> reduce the useless poll.
>>>>>
>>>>> Signed-off-by: Yang Zhang <yang.zhang.wz@gmail.com>
>>>>> Signed-off-by: Quan Xu <quan.xu0@gmail.com>
>>>>> Cc: Juergen Gross <jgross@suse.com>
>>>>> Cc: Alok Kataria <akataria@vmware.com>
>>>>> Cc: Rusty Russell <rusty@rustcorp.com.au>
>>>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>>>> Cc: Ingo Molnar <mingo@redhat.com>
>>>>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>>>>> Cc: x86@kernel.org
>>>>> Cc: virtualization@lists.linux-foundation.org
>>>>> Cc: linux-kernel@vger.kernel.org
>>>>> Cc: xen-devel@lists.xenproject.org
>>>> Hmm, is the idle entry path really so critical to performance that a
>>>> new
>>>> pvops function is necessary?
>>> Juergen, Here is the data we get when running benchmark netperf:
>>> 1. w/o patch and disable kvm dynamic poll (halt_poll_ns=0):
>>> 29031.6 bit/s -- 76.1 %CPU
>>>
>>> 2. w/ patch and disable kvm dynamic poll (halt_poll_ns=0):
>>> 35787.7 bit/s -- 129.4 %CPU
>>>
>>> 3. w/ kvm dynamic poll:
>>> 35735.6 bit/s -- 200.0 %CPU
>>>
>>> 4. w/patch and w/ kvm dynamic poll:
>>> 42225.3 bit/s -- 198.7 %CPU
>>>
>>> 5. idle=poll
>>> 37081.7 bit/s -- 998.1 %CPU
>>>
>>>
>>>
>>> w/ this patch, we will improve performance by 23%.. even we could
>>> improve
>>> performance by 45.4%, if we use w/patch and w/ kvm dynamic poll.
>>> also the
>>> cost of CPU is much lower than 'idle=poll' case..
>> I don't question the general idea. I just think pvops isn't the best way
>> to implement it.
>>
>>>> Wouldn't a function pointer, maybe guarded
>>>> by a static key, be enough? A further advantage would be that this
>>>> would
>>>> work on other architectures, too.
>>> I assume this feature will be ported to other archs.. a new pvops makes
>
> sorry, a typo.. /other archs/other hypervisors/
> it refers hypervisor like Xen, HyperV and VMware)..
>
>>> code
>>> clean and easy to maintain. also I tried to add it into existed pvops,
>>> but it
>>> doesn't match.
>> You are aware that pvops is x86 only?
>
> yes, I'm aware..
>
>> I really don't see the big difference in maintainability compared to the
>> static key / function pointer variant:
>>
>> void (*guest_idle_poll_func)(void);
>> struct static_key guest_idle_poll_key __read_mostly;
>>
>> static inline void guest_idle_poll(void)
>> {
>> if (static_key_false(&guest_idle_poll_key))
>> guest_idle_poll_func();
>> }
>
>
>
> thank you for your sample code :)
> I agree there is no big difference.. I think we are discussion for two
> things:
> 1) x86 VM on different hypervisors
> 2) different archs VM on kvm hypervisor
>
> What I want to do is x86 VM on different hypervisors, such as kvm / xen
> / hyperv ..
Why limit the solution to x86 if the more general solution isn't
harder?
As you didn't give any reason why the pvops approach is better other
than you don't care for non-x86 platforms you won't get an "Ack" from
me for this patch.
>
>> And KVM would just need to set guest_idle_poll_func and enable the
>> static key. Works on non-x86 architectures, too.
>>
>
> .. referred to 'pv_mmu_ops', HyperV and Xen can implement their own
> functions for 'pv_mmu_ops'.
> I think it is the same to pv_idle_ops.
>
> with above explaination, do you still think I need to define the static
> key/function pointer variant?
>
> btw, any interest to port it to Xen HVM guest? :)
Maybe. But this should work for Xen on ARM, too.
Juergen
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH RFC v3 1/6] x86/paravirt: Add pv_idle_ops to paravirt ops
From: Quan Xu @ 2017-11-14 11:43 UTC (permalink / raw)
To: Juergen Gross, Quan Xu, kvm, linux-doc, linux-fsdevel,
linux-kernel, virtualization, x86, xen-devel
Cc: Yang Zhang, Rusty Russell, Ingo Molnar, H. Peter Anvin,
Alok Kataria, Thomas Gleixner
In-Reply-To: <9a4f53f3-225f-0e99-d9b2-d89656b8fb31@suse.com>
On 2017/11/14 18:27, Juergen Gross wrote:
> On 14/11/17 10:38, Quan Xu wrote:
>>
>> On 2017/11/14 15:30, Juergen Gross wrote:
>>> On 14/11/17 08:02, Quan Xu wrote:
>>>> On 2017/11/13 18:53, Juergen Gross wrote:
>>>>> On 13/11/17 11:06, Quan Xu wrote:
>>>>>> From: Quan Xu <quan.xu0@gmail.com>
>>>>>>
>>>>>> So far, pv_idle_ops.poll is the only ops for pv_idle. .poll is called
>>>>>> in idle path which will poll for a while before we enter the real idle
>>>>>> state.
>>>>>>
>>>>>> In virtualization, idle path includes several heavy operations
>>>>>> includes timer access(LAPIC timer or TSC deadline timer) which will
>>>>>> hurt performance especially for latency intensive workload like
>>>>>> message
>>>>>> passing task. The cost is mainly from the vmexit which is a hardware
>>>>>> context switch between virtual machine and hypervisor. Our solution is
>>>>>> to poll for a while and do not enter real idle path if we can get the
>>>>>> schedule event during polling.
>>>>>>
>>>>>> Poll may cause the CPU waste so we adopt a smart polling mechanism to
>>>>>> reduce the useless poll.
>>>>>>
>>>>>> Signed-off-by: Yang Zhang <yang.zhang.wz@gmail.com>
>>>>>> Signed-off-by: Quan Xu <quan.xu0@gmail.com>
>>>>>> Cc: Juergen Gross <jgross@suse.com>
>>>>>> Cc: Alok Kataria <akataria@vmware.com>
>>>>>> Cc: Rusty Russell <rusty@rustcorp.com.au>
>>>>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>>>>> Cc: Ingo Molnar <mingo@redhat.com>
>>>>>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>>>>>> Cc: x86@kernel.org
>>>>>> Cc: virtualization@lists.linux-foundation.org
>>>>>> Cc: linux-kernel@vger.kernel.org
>>>>>> Cc: xen-devel@lists.xenproject.org
>>>>> Hmm, is the idle entry path really so critical to performance that a
>>>>> new
>>>>> pvops function is necessary?
>>>> Juergen, Here is the data we get when running benchmark netperf:
>>>> 1. w/o patch and disable kvm dynamic poll (halt_poll_ns=0):
>>>> 29031.6 bit/s -- 76.1 %CPU
>>>>
>>>> 2. w/ patch and disable kvm dynamic poll (halt_poll_ns=0):
>>>> 35787.7 bit/s -- 129.4 %CPU
>>>>
>>>> 3. w/ kvm dynamic poll:
>>>> 35735.6 bit/s -- 200.0 %CPU
>>>>
>>>> 4. w/patch and w/ kvm dynamic poll:
>>>> 42225.3 bit/s -- 198.7 %CPU
>>>>
>>>> 5. idle=poll
>>>> 37081.7 bit/s -- 998.1 %CPU
>>>>
>>>>
>>>>
>>>> w/ this patch, we will improve performance by 23%.. even we could
>>>> improve
>>>> performance by 45.4%, if we use w/patch and w/ kvm dynamic poll.
>>>> also the
>>>> cost of CPU is much lower than 'idle=poll' case..
>>> I don't question the general idea. I just think pvops isn't the best way
>>> to implement it.
>>>
>>>>> Wouldn't a function pointer, maybe guarded
>>>>> by a static key, be enough? A further advantage would be that this
>>>>> would
>>>>> work on other architectures, too.
>>>> I assume this feature will be ported to other archs.. a new pvops makes
>> sorry, a typo.. /other archs/other hypervisors/
>> it refers hypervisor like Xen, HyperV and VMware)..
>>
>>>> code
>>>> clean and easy to maintain. also I tried to add it into existed pvops,
>>>> but it
>>>> doesn't match.
>>> You are aware that pvops is x86 only?
>> yes, I'm aware..
>>
>>> I really don't see the big difference in maintainability compared to the
>>> static key / function pointer variant:
>>>
>>> void (*guest_idle_poll_func)(void);
>>> struct static_key guest_idle_poll_key __read_mostly;
>>>
>>> static inline void guest_idle_poll(void)
>>> {
>>> if (static_key_false(&guest_idle_poll_key))
>>> guest_idle_poll_func();
>>> }
>>
>>
>> thank you for your sample code :)
>> I agree there is no big difference.. I think we are discussion for two
>> things:
>> 1) x86 VM on different hypervisors
>> 2) different archs VM on kvm hypervisor
>>
>> What I want to do is x86 VM on different hypervisors, such as kvm / xen
>> / hyperv ..
> Why limit the solution to x86 if the more general solution isn't
> harder?
>
> As you didn't give any reason why the pvops approach is better other
> than you don't care for non-x86 platforms you won't get an "Ack" from
> me for this patch.
It just looks a little odder to me. I understand you care about no-x86 arch.
Are you aware 'pv_time_ops' for arm64/arm/x86 archs, defined in
- arch/arm64/include/asm/paravirt.h
- arch/x86/include/asm/paravirt_types.h
- arch/arm/include/asm/paravirt.h
I am unfamilar with arm code. IIUC, if you'd implement pv_idle_ops
for arm/arm64 arch, you'd define a same structure in
- arch/arm64/include/asm/paravirt.h or
- arch/arm/include/asm/paravirt.h
.. instead of static key / fuction.
then implement a real function in
- arch/arm/kernel/paravirt.c.
Also I wonder HOW/WHERE to define a static key/function, then to benifit
x86/no-x86 archs?
Quan
Alibaba Cloud
>>> And KVM would just need to set guest_idle_poll_func and enable the
>>> static key. Works on non-x86 architectures, too.
>>>
>> .. referred to 'pv_mmu_ops', HyperV and Xen can implement their own
>> functions for 'pv_mmu_ops'.
>> I think it is the same to pv_idle_ops.
>>
>> with above explaination, do you still think I need to define the static
>> key/function pointer variant?
>>
>> btw, any interest to port it to Xen HVM guest? :)
> Maybe. But this should work for Xen on ARM, too.
>
>
> Juergen
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH RFC v3 1/6] x86/paravirt: Add pv_idle_ops to paravirt ops
From: Juergen Gross @ 2017-11-14 11:58 UTC (permalink / raw)
To: Quan Xu, Quan Xu, kvm, linux-doc, linux-fsdevel, linux-kernel,
virtualization, x86, xen-devel
Cc: Yang Zhang, Rusty Russell, Ingo Molnar, H. Peter Anvin,
Alok Kataria, Thomas Gleixner
In-Reply-To: <41403bbb-bfa9-0618-abf7-dd871a7b783a@gmail.com>
On 14/11/17 12:43, Quan Xu wrote:
>
>
> On 2017/11/14 18:27, Juergen Gross wrote:
>> On 14/11/17 10:38, Quan Xu wrote:
>>>
>>> On 2017/11/14 15:30, Juergen Gross wrote:
>>>> On 14/11/17 08:02, Quan Xu wrote:
>>>>> On 2017/11/13 18:53, Juergen Gross wrote:
>>>>>> On 13/11/17 11:06, Quan Xu wrote:
>>>>>>> From: Quan Xu <quan.xu0@gmail.com>
>>>>>>>
>>>>>>> So far, pv_idle_ops.poll is the only ops for pv_idle. .poll is
>>>>>>> called
>>>>>>> in idle path which will poll for a while before we enter the real
>>>>>>> idle
>>>>>>> state.
>>>>>>>
>>>>>>> In virtualization, idle path includes several heavy operations
>>>>>>> includes timer access(LAPIC timer or TSC deadline timer) which will
>>>>>>> hurt performance especially for latency intensive workload like
>>>>>>> message
>>>>>>> passing task. The cost is mainly from the vmexit which is a hardware
>>>>>>> context switch between virtual machine and hypervisor. Our
>>>>>>> solution is
>>>>>>> to poll for a while and do not enter real idle path if we can get
>>>>>>> the
>>>>>>> schedule event during polling.
>>>>>>>
>>>>>>> Poll may cause the CPU waste so we adopt a smart polling
>>>>>>> mechanism to
>>>>>>> reduce the useless poll.
>>>>>>>
>>>>>>> Signed-off-by: Yang Zhang <yang.zhang.wz@gmail.com>
>>>>>>> Signed-off-by: Quan Xu <quan.xu0@gmail.com>
>>>>>>> Cc: Juergen Gross <jgross@suse.com>
>>>>>>> Cc: Alok Kataria <akataria@vmware.com>
>>>>>>> Cc: Rusty Russell <rusty@rustcorp.com.au>
>>>>>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>>>>>> Cc: Ingo Molnar <mingo@redhat.com>
>>>>>>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>>>>>>> Cc: x86@kernel.org
>>>>>>> Cc: virtualization@lists.linux-foundation.org
>>>>>>> Cc: linux-kernel@vger.kernel.org
>>>>>>> Cc: xen-devel@lists.xenproject.org
>>>>>> Hmm, is the idle entry path really so critical to performance that a
>>>>>> new
>>>>>> pvops function is necessary?
>>>>> Juergen, Here is the data we get when running benchmark netperf:
>>>>> 1. w/o patch and disable kvm dynamic poll (halt_poll_ns=0):
>>>>> 29031.6 bit/s -- 76.1 %CPU
>>>>>
>>>>> 2. w/ patch and disable kvm dynamic poll (halt_poll_ns=0):
>>>>> 35787.7 bit/s -- 129.4 %CPU
>>>>>
>>>>> 3. w/ kvm dynamic poll:
>>>>> 35735.6 bit/s -- 200.0 %CPU
>>>>>
>>>>> 4. w/patch and w/ kvm dynamic poll:
>>>>> 42225.3 bit/s -- 198.7 %CPU
>>>>>
>>>>> 5. idle=poll
>>>>> 37081.7 bit/s -- 998.1 %CPU
>>>>>
>>>>>
>>>>>
>>>>> w/ this patch, we will improve performance by 23%.. even we could
>>>>> improve
>>>>> performance by 45.4%, if we use w/patch and w/ kvm dynamic poll.
>>>>> also the
>>>>> cost of CPU is much lower than 'idle=poll' case..
>>>> I don't question the general idea. I just think pvops isn't the best
>>>> way
>>>> to implement it.
>>>>
>>>>>> Wouldn't a function pointer, maybe guarded
>>>>>> by a static key, be enough? A further advantage would be that this
>>>>>> would
>>>>>> work on other architectures, too.
>>>>> I assume this feature will be ported to other archs.. a new pvops
>>>>> makes
>>> sorry, a typo.. /other archs/other hypervisors/
>>> it refers hypervisor like Xen, HyperV and VMware)..
>>>
>>>>> code
>>>>> clean and easy to maintain. also I tried to add it into existed pvops,
>>>>> but it
>>>>> doesn't match.
>>>> You are aware that pvops is x86 only?
>>> yes, I'm aware..
>>>
>>>> I really don't see the big difference in maintainability compared to
>>>> the
>>>> static key / function pointer variant:
>>>>
>>>> void (*guest_idle_poll_func)(void);
>>>> struct static_key guest_idle_poll_key __read_mostly;
>>>>
>>>> static inline void guest_idle_poll(void)
>>>> {
>>>> if (static_key_false(&guest_idle_poll_key))
>>>> guest_idle_poll_func();
>>>> }
>>>
>>>
>>> thank you for your sample code :)
>>> I agree there is no big difference.. I think we are discussion for two
>>> things:
>>> 1) x86 VM on different hypervisors
>>> 2) different archs VM on kvm hypervisor
>>>
>>> What I want to do is x86 VM on different hypervisors, such as kvm / xen
>>> / hyperv ..
>> Why limit the solution to x86 if the more general solution isn't
>> harder?
>>
>> As you didn't give any reason why the pvops approach is better other
>> than you don't care for non-x86 platforms you won't get an "Ack" from
>> me for this patch.
>
>
> It just looks a little odder to me. I understand you care about no-x86
> arch.
>
> Are you aware 'pv_time_ops' for arm64/arm/x86 archs, defined in
> - arch/arm64/include/asm/paravirt.h
> - arch/x86/include/asm/paravirt_types.h
> - arch/arm/include/asm/paravirt.h
Yes, I know. This is just a hack to make it compile. Other than the
same names this has nothing to do with pvops, but is just a function
vector.
> I am unfamilar with arm code. IIUC, if you'd implement pv_idle_ops
> for arm/arm64 arch, you'd define a same structure in
> - arch/arm64/include/asm/paravirt.h or
> - arch/arm/include/asm/paravirt.h
>
> .. instead of static key / fuction.
>
> then implement a real function in
> - arch/arm/kernel/paravirt.c.
So just to use pvops you want to implement it in each arch instead
of using a mechanism available everywhere?
> Also I wonder HOW/WHERE to define a static key/function, then to benifit
> x86/no-x86 archs?
What? There are plenty of examples in the kernel.
Please stop wasting my time. Either write a patch which is acceptable
or let it be. I won't take your pvops approach without a really good
reason to do so. And so far you haven't given any reason other than
you are too lazy to write a proper patch, sorry.
Juergen
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v17 6/6] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_VQ
From: Wei Wang @ 2017-11-14 12:02 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: yang.zhang.wz, kvm, penguin-kernel, liliang.opensource,
qemu-devel, virtualization, linux-mm, aarcange, virtio-dev,
mawilcox, willy, quan.xu, Nitesh Narayan Lal, Rik van Riel,
cornelia.huck, mhocko, linux-kernel, amit.shah, pbonzini, akpm,
mgorman
In-Reply-To: <20171113192309-mutt-send-email-mst@kernel.org>
On 11/14/2017 01:32 AM, Michael S. Tsirkin wrote:
>> - guest2host_cmd: written by the guest to ACK to the host about the
>> commands that have been received. The host will clear the corresponding
>> bits on the host2guest_cmd register. The guest also uses this register
>> to send commands to the host (e.g. when finish free page reporting).
> I am not sure what is the role of guest2host_cmd. Reporting of
> the correct cmd id seems sufficient indication that guest
> received the start command. Not getting any more seems sufficient
> to detect stop.
>
I think the issue is when the host is waiting for the guest to report
pages, it does not know whether the guest is going to report more or the
report is done already. That's why we need a way to let the guest tell
the host "the report is done, don't wait for more", then the host
continues to the next step - sending the non-free pages to the
destination. The following method is a conclusion of other comments,
with some new thought. Please have a check if it is good.
Two new configuration registers in total:
- cmd_reg: the command register, combined from the previous host2guest
and guest2host. I think we can use the same register for host requesting
and guest ACKing, since the guest writing will trap to QEMU, that is,
all the writes to the register are performed in QEMU, and we can keep
things work in a correct way there.
- cmd_id_reg: the sequence id of the free page report command.
-- free page report:
- host requests the guest to start reporting by "cmd_reg |
REPORT_START";
- guest ACKs to the host about receiving the start reporting
request by "cmd_reg | REPORT_START", host will clear the flag bit once
receiving the ACK.
- host requests the guest to stop reporting by "cmd_reg | REPORT_STOP";
- guest ACKs to the host about receiving the stop reporting request
by "cmd_reg | REPORT_STOP", host will clear the flag once receiving the ACK.
- guest tells the host about the start of the reporting by writing
"cmd id" into an outbuf, which is added to the free page vq.
- guest tells the host about the end of the reporting by writing
"0" into an outbuf, which is added to the free page vq. (we reserve
"id=0" as the stop sign)
-- ballooning:
- host requests the guest to start ballooning by "cmd_reg |
BALLOONING";
- guest ACKs to the host about receiving the request by "cmd_reg |
BALLOONING", host will clear the flag once receiving the ACK.
Some more explanations:
-- Why not let the host request the guest to start the free page
reporting simply by writing a new cmd id to the cmd_id_reg?
The configuration interrupt is shared among all the features -
ballooning, free page reporting, and future feature extensions which
need host-to-guest requests. Some features may need to add other feature
specific configuration registers, like free page reporting need the
cmd_id_reg, which is not used by ballooning. The rule here is that the
feature specific registers are read only when that feature is requested
via the cmd_reg. For example, the cmd_id_reg is read only when "cmd_reg
| REPORT_START" is true. Otherwise, when the driver receives a
configuration interrupt, it has to read both cmd_reg and cmd_id
registers to know what are requested by the host - think about the case
that ballooning requests are sent frequently while free page reporting
isn't requested, the guest has to read the cmd_id register every time a
ballooning request is sent by the host, which is not necessary. If
future new features follow this style, there will be more unnecessary
VMexits to read the unused feature specific registers.
So I think it is good to have a central control of the feature request
via only one cmd register - reading that one is enough to know what is
requested by the host.
Best,
Wei
^ permalink raw reply
* Re: [PATCH v17 6/6] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_VQ
From: Michael S. Tsirkin @ 2017-11-14 21:21 UTC (permalink / raw)
To: Wei Wang
Cc: yang.zhang.wz, kvm, penguin-kernel, liliang.opensource,
qemu-devel, virtualization, linux-mm, aarcange, virtio-dev,
mawilcox, willy, quan.xu, Nitesh Narayan Lal, Rik van Riel,
cornelia.huck, mhocko, linux-kernel, amit.shah, pbonzini, akpm,
mgorman
In-Reply-To: <5A0ADB3B.4070407@intel.com>
On Tue, Nov 14, 2017 at 08:02:03PM +0800, Wei Wang wrote:
> On 11/14/2017 01:32 AM, Michael S. Tsirkin wrote:
> > > - guest2host_cmd: written by the guest to ACK to the host about the
> > > commands that have been received. The host will clear the corresponding
> > > bits on the host2guest_cmd register. The guest also uses this register
> > > to send commands to the host (e.g. when finish free page reporting).
> > I am not sure what is the role of guest2host_cmd. Reporting of
> > the correct cmd id seems sufficient indication that guest
> > received the start command. Not getting any more seems sufficient
> > to detect stop.
> >
>
> I think the issue is when the host is waiting for the guest to report pages,
> it does not know whether the guest is going to report more or the report is
> done already. That's why we need a way to let the guest tell the host "the
> report is done, don't wait for more", then the host continues to the next
> step - sending the non-free pages to the destination. The following method
> is a conclusion of other comments, with some new thought. Please have a
> check if it is good.
config won't work well for this IMHO.
Writes to config register are hard to synchronize with the VQ.
For example, guest sends free pages, host says stop, meanwhile
guest sends stop for 1st set of pages.
How about adding a buffer with "stop" in the VQ instead?
Wastes a VQ entry which you will need to reserve for this
but is it a big deal?
> Two new configuration registers in total:
> - cmd_reg: the command register, combined from the previous host2guest and
> guest2host. I think we can use the same register for host requesting and
> guest ACKing, since the guest writing will trap to QEMU, that is, all the
> writes to the register are performed in QEMU, and we can keep things work in
> a correct way there.
> - cmd_id_reg: the sequence id of the free page report command.
>
> -- free page report:
> - host requests the guest to start reporting by "cmd_reg |
> REPORT_START";
> - guest ACKs to the host about receiving the start reporting request by
> "cmd_reg | REPORT_START", host will clear the flag bit once receiving the
> ACK.
> - host requests the guest to stop reporting by "cmd_reg | REPORT_STOP";
> - guest ACKs to the host about receiving the stop reporting request by
> "cmd_reg | REPORT_STOP", host will clear the flag once receiving the ACK.
> - guest tells the host about the start of the reporting by writing "cmd
> id" into an outbuf, which is added to the free page vq.
> - guest tells the host about the end of the reporting by writing "0"
> into an outbuf, which is added to the free page vq. (we reserve "id=0" as
> the stop sign)
>
> -- ballooning:
> - host requests the guest to start ballooning by "cmd_reg | BALLOONING";
> - guest ACKs to the host about receiving the request by "cmd_reg |
> BALLOONING", host will clear the flag once receiving the ACK.
>
>
> Some more explanations:
> -- Why not let the host request the guest to start the free page reporting
> simply by writing a new cmd id to the cmd_id_reg?
> The configuration interrupt is shared among all the features - ballooning,
> free page reporting, and future feature extensions which need host-to-guest
> requests. Some features may need to add other feature specific configuration
> registers, like free page reporting need the cmd_id_reg, which is not used
> by ballooning. The rule here is that the feature specific registers are read
> only when that feature is requested via the cmd_reg. For example, the
> cmd_id_reg is read only when "cmd_reg | REPORT_START" is true. Otherwise,
> when the driver receives a configuration interrupt, it has to read both
> cmd_reg and cmd_id registers to know what are requested by the host - think
> about the case that ballooning requests are sent frequently while free page
> reporting isn't requested, the guest has to read the cmd_id register every
> time a ballooning request is sent by the host, which is not necessary. If
> future new features follow this style, there will be more unnecessary
> VMexits to read the unused feature specific registers.
> So I think it is good to have a central control of the feature request via
> only one cmd register - reading that one is enough to know what is requested
> by the host.
>
Right now you are increasing the cost of balloon request 3x though.
How about we establish a baseline with a simple interface, and
then add the command register when it's actually benefitial.
> Best,
> Wei
^ permalink raw reply
* [vhost:vhost 1/9] drivers//virtio/virtio_balloon.c:167:3: error: implicit declaration of function 'balloon_page_push'; did you mean 'balloon_page_putback'?
From: kbuild test robot @ 2017-11-14 21:48 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, kbuild-all, kvm, virtualization
[-- Attachment #1: Type: text/plain, Size: 3357 bytes --]
tree: https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git vhost
head: 74d5d28b9903cd48c22c04f887354a3d49621c13
commit: 47349198d7a879febab785a00c39c2de379f35c2 [1/9] virtio_balloon: fix deadlock on OOM
config: sh-allyesconfig (attached as .config)
compiler: sh4-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout 47349198d7a879febab785a00c39c2de379f35c2
# save the attached .config to linux build tree
make.cross ARCH=sh
All errors (new ones prefixed by >>):
drivers//virtio/virtio_balloon.c: In function 'fill_balloon':
>> drivers//virtio/virtio_balloon.c:167:3: error: implicit declaration of function 'balloon_page_push'; did you mean 'balloon_page_putback'? [-Werror=implicit-function-declaration]
balloon_page_push(&pages, page);
^~~~~~~~~~~~~~~~~
balloon_page_putback
>> drivers//virtio/virtio_balloon.c:174:17: error: implicit declaration of function 'balloon_page_pop'; did you mean 'balloon_page_alloc'? [-Werror=implicit-function-declaration]
while ((page = balloon_page_pop(&pages))) {
^~~~~~~~~~~~~~~~
balloon_page_alloc
drivers//virtio/virtio_balloon.c:174:15: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
while ((page = balloon_page_pop(&pages))) {
^
cc1: some warnings being treated as errors
vim +167 drivers//virtio/virtio_balloon.c
143
144 static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
145 {
146 unsigned num_allocated_pages;
147 unsigned num_pfns;
148 struct page *page;
149 LIST_HEAD(pages);
150
151 /* We can only do one array worth at a time. */
152 num = min(num, ARRAY_SIZE(vb->pfns));
153
154 for (num_pfns = 0; num_pfns < num;
155 num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
156 struct page *page = balloon_page_alloc();
157
158 if (!page) {
159 dev_info_ratelimited(&vb->vdev->dev,
160 "Out of puff! Can't get %u pages\n",
161 VIRTIO_BALLOON_PAGES_PER_PAGE);
162 /* Sleep for at least 1/5 of a second before retry. */
163 msleep(200);
164 break;
165 }
166
> 167 balloon_page_push(&pages, page);
168 }
169
170 mutex_lock(&vb->balloon_lock);
171
172 vb->num_pfns = 0;
173
> 174 while ((page = balloon_page_pop(&pages))) {
175 balloon_page_enqueue(&vb->vb_dev_info, page);
176
177 vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE;
178
179 set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
180 vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
181 if (!virtio_has_feature(vb->vdev,
182 VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
183 adjust_managed_page_count(page, -1);
184 }
185
186 num_allocated_pages = vb->num_pfns;
187 /* Did we get any? */
188 if (vb->num_pfns != 0)
189 tell_host(vb, vb->inflate_vq);
190 mutex_unlock(&vb->balloon_lock);
191
192 return num_allocated_pages;
193 }
194
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 46695 bytes --]
[-- Attachment #3: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v17 6/6] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_VQ
From: Wei Wang @ 2017-11-15 3:47 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: yang.zhang.wz, kvm, penguin-kernel, liliang.opensource,
qemu-devel, virtualization, linux-mm, aarcange, virtio-dev,
mawilcox, willy, quan.xu, Nitesh Narayan Lal, Rik van Riel,
cornelia.huck, mhocko, linux-kernel, amit.shah, pbonzini, akpm,
mgorman
In-Reply-To: <20171114230805-mutt-send-email-mst@kernel.org>
On 11/15/2017 05:21 AM, Michael S. Tsirkin wrote:
> On Tue, Nov 14, 2017 at 08:02:03PM +0800, Wei Wang wrote:
>> On 11/14/2017 01:32 AM, Michael S. Tsirkin wrote:
>>>> - guest2host_cmd: written by the guest to ACK to the host about the
>>>> commands that have been received. The host will clear the corresponding
>>>> bits on the host2guest_cmd register. The guest also uses this register
>>>> to send commands to the host (e.g. when finish free page reporting).
>>> I am not sure what is the role of guest2host_cmd. Reporting of
>>> the correct cmd id seems sufficient indication that guest
>>> received the start command. Not getting any more seems sufficient
>>> to detect stop.
>>>
>> I think the issue is when the host is waiting for the guest to report pages,
>> it does not know whether the guest is going to report more or the report is
>> done already. That's why we need a way to let the guest tell the host "the
>> report is done, don't wait for more", then the host continues to the next
>> step - sending the non-free pages to the destination. The following method
>> is a conclusion of other comments, with some new thought. Please have a
>> check if it is good.
> config won't work well for this IMHO.
> Writes to config register are hard to synchronize with the VQ.
> For example, guest sends free pages, host says stop, meanwhile
> guest sends stop for 1st set of pages.
I still don't see an issue with this. Please see below:
(before jumping into the discussion, just make sure I've well explained
this point: now host-to-guest commands are done via config, and
guest-to-host commands are done via the free page vq)
Case: Host starts to request the reporting with cmd_id=1. Some time
later, Host writes "stop" to config, meantime guest happens to finish
the reporting and plan to actively send a "stop" command from the
free_page_vq().
Essentially, this is like a sync between two threads - if we
view the config interrupt handler as one thread, another is the free
page reporting worker thread.
- what the config handler does is simply:
1.1: WRITE_ONCE(vb->reporting_stop, true);
- what the reporting thread will do is
2.1: WRITE_ONCE(vb->reporting_stop, true);
2.2: send_stop_to_host_via_vq();
From the guest point of view, no matter 1.1 is executed first or 2.1
first, it doesn't make a difference to the end result -
vb->reporting_stop is set.
From the host point of view, it knows that cmd_id=1 has truly stopped
the reporting when it receives a "stop" sign via the vq.
> How about adding a buffer with "stop" in the VQ instead?
> Wastes a VQ entry which you will need to reserve for this
> but is it a big deal?
The free page vq is guest-to-host direction. Using it for host-to-guest
requests will make it bidirectional, which will result in the same issue
described before: https://lkml.org/lkml/2017/10/11/1009 (the first response)
On the other hand, I think adding another new vq for host-to-guest
requesting doesn't make a difference in essence, compared to using
config (same 1.1, 2.1, 2.2 above), but will be more complicated.
>> Two new configuration registers in total:
>> - cmd_reg: the command register, combined from the previous host2guest and
>> guest2host. I think we can use the same register for host requesting and
>> guest ACKing, since the guest writing will trap to QEMU, that is, all the
>> writes to the register are performed in QEMU, and we can keep things work in
>> a correct way there.
>> - cmd_id_reg: the sequence id of the free page report command.
>>
>> -- free page report:
>> - host requests the guest to start reporting by "cmd_reg |
>> REPORT_START";
>> - guest ACKs to the host about receiving the start reporting request by
>> "cmd_reg | REPORT_START", host will clear the flag bit once receiving the
>> ACK.
>> - host requests the guest to stop reporting by "cmd_reg | REPORT_STOP";
>> - guest ACKs to the host about receiving the stop reporting request by
>> "cmd_reg | REPORT_STOP", host will clear the flag once receiving the ACK.
>> - guest tells the host about the start of the reporting by writing "cmd
>> id" into an outbuf, which is added to the free page vq.
>> - guest tells the host about the end of the reporting by writing "0"
>> into an outbuf, which is added to the free page vq. (we reserve "id=0" as
>> the stop sign)
>>
>> -- ballooning:
>> - host requests the guest to start ballooning by "cmd_reg | BALLOONING";
>> - guest ACKs to the host about receiving the request by "cmd_reg |
>> BALLOONING", host will clear the flag once receiving the ACK.
>>
>>
>> Some more explanations:
>> -- Why not let the host request the guest to start the free page reporting
>> simply by writing a new cmd id to the cmd_id_reg?
>> The configuration interrupt is shared among all the features - ballooning,
>> free page reporting, and future feature extensions which need host-to-guest
>> requests. Some features may need to add other feature specific configuration
>> registers, like free page reporting need the cmd_id_reg, which is not used
>> by ballooning. The rule here is that the feature specific registers are read
>> only when that feature is requested via the cmd_reg. For example, the
>> cmd_id_reg is read only when "cmd_reg | REPORT_START" is true. Otherwise,
>> when the driver receives a configuration interrupt, it has to read both
>> cmd_reg and cmd_id registers to know what are requested by the host - think
>> about the case that ballooning requests are sent frequently while free page
>> reporting isn't requested, the guest has to read the cmd_id register every
>> time a ballooning request is sent by the host, which is not necessary. If
>> future new features follow this style, there will be more unnecessary
>> VMexits to read the unused feature specific registers.
>> So I think it is good to have a central control of the feature request via
>> only one cmd register - reading that one is enough to know what is requested
>> by the host.
>>
> Right now you are increasing the cost of balloon request 3x though.
Not that much, I think, just a cmd register read and ACK, and this
should be neglected compared to the ballooning time.
(I don't see a difference in the performance testing either).
Best,
Wei
^ permalink raw reply
* Re: [PATCH RFC v3 3/6] sched/idle: Add a generic poll before enter real idle path
From: Peter Zijlstra @ 2017-11-15 12:11 UTC (permalink / raw)
To: Quan Xu
Cc: Yang Zhang, Len Brown, Quan Xu, kvm, linux-doc, x86, linux-kernel,
virtualization, Kyle Huey, Ingo Molnar, Borislav Petkov,
Tom Lendacky, Andy Lutomirski, H. Peter Anvin, linux-fsdevel,
xen-devel, Thomas Gleixner, Tobias Klauser
In-Reply-To: <1510567565-5118-4-git-send-email-quan.xu0@gmail.com>
On Mon, Nov 13, 2017 at 06:06:02PM +0800, Quan Xu wrote:
> From: Yang Zhang <yang.zhang.wz@gmail.com>
>
> Implement a generic idle poll which resembles the functionality
> found in arch/. Provide weak arch_cpu_idle_poll function which
> can be overridden by the architecture code if needed.
No, we want less of those magic hooks, not more.
> Interrupts arrive which may not cause a reschedule in idle loops.
> In KVM guest, this costs several VM-exit/VM-entry cycles, VM-entry
> for interrupts and VM-exit immediately. Also this becomes more
> expensive than bare metal. Add a generic idle poll before enter
> real idle path. When a reschedule event is pending, we can bypass
> the real idle path.
Why not do a HV specific idle driver?
^ permalink raw reply
* Re: [PATCH RFC v3 0/6] x86/idle: add halt poll support
From: Peter Zijlstra @ 2017-11-15 12:13 UTC (permalink / raw)
To: Quan Xu
Cc: yang.zhang.wz, Quan Xu, kvm, linux-doc, x86, linux-kernel,
virtualization, linux-fsdevel, pbonzini, tglx
In-Reply-To: <1510570901-6373-1-git-send-email-quan.xu0@gmail.com>
On Mon, Nov 13, 2017 at 07:01:40PM +0800, Quan Xu wrote:
> Documentation/sysctl/kernel.txt | 35 ++++++++++++++++
> arch/x86/include/asm/paravirt.h | 5 ++
> arch/x86/include/asm/paravirt_types.h | 6 +++
> arch/x86/kernel/kvm.c | 73 +++++++++++++++++++++++++++++++++
> arch/x86/kernel/paravirt.c | 10 +++++
> arch/x86/kernel/process.c | 7 +++
> include/linux/kernel.h | 6 +++
> include/linux/tick.h | 2 +
> kernel/sched/idle.c | 2 +
> kernel/sysctl.c | 34 +++++++++++++++
> kernel/time/tick-sched.c | 11 +++++
> kernel/time/tick-sched.h | 3 +
> 12 files changed, 194 insertions(+), 0 deletions(-)
You seem to have forgotten to CC me on the actual patches, but no. Not
going to happen.
^ permalink raw reply
* Re: [PATCH RFC v3 0/6] x86/idle: add halt poll support
From: Xu Quan @ 2017-11-15 13:04 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Yang Zhang, Quan Xu, kvm, linux-doc, x86, linux-kernel,
virtualization, linux-fsdevel, Paolo Bonzini, Thomas Gleixner
In-Reply-To: <20171115121314.s7hhcb6f36uasrku@hirez.programming.kicks-ass.net>
[-- Attachment #1.1: Type: text/plain, Size: 1083 bytes --]
2017-11-15 20:13 GMT+08:00 Peter Zijlstra <peterz@infradead.org>:
> On Mon, Nov 13, 2017 at 07:01:40PM +0800, Quan Xu wrote:
> > Documentation/sysctl/kernel.txt | 35 ++++++++++++++++
> > arch/x86/include/asm/paravirt.h | 5 ++
> > arch/x86/include/asm/paravirt_types.h | 6 +++
> > arch/x86/kernel/kvm.c | 73
> +++++++++++++++++++++++++++++++++
> > arch/x86/kernel/paravirt.c | 10 +++++
> > arch/x86/kernel/process.c | 7 +++
> > include/linux/kernel.h | 6 +++
> > include/linux/tick.h | 2 +
> > kernel/sched/idle.c | 2 +
> > kernel/sysctl.c | 34 +++++++++++++++
> > kernel/time/tick-sched.c | 11 +++++
> > kernel/time/tick-sched.h | 3 +
> > 12 files changed, 194 insertions(+), 0 deletions(-)
>
> You seem to have forgotten to CC me on the actual patches, but no. Not
> going to happen.
>
sorry, I will forward patch 5 / patch 6 to you .. I have some problems
with Gmail on unbuntu.
Quan
[-- Attachment #1.2: Type: text/html, Size: 1832 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v17 6/6] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_VQ
From: Michael S. Tsirkin @ 2017-11-15 13:26 UTC (permalink / raw)
To: Wei Wang
Cc: yang.zhang.wz, kvm, penguin-kernel, liliang.opensource,
qemu-devel, virtualization, linux-mm, aarcange, virtio-dev,
mawilcox, willy, quan.xu, Nitesh Narayan Lal, Rik van Riel,
cornelia.huck, mhocko, linux-kernel, amit.shah, pbonzini, akpm,
mgorman
In-Reply-To: <5A0BB8EE.4050402@intel.com>
On Wed, Nov 15, 2017 at 11:47:58AM +0800, Wei Wang wrote:
> On 11/15/2017 05:21 AM, Michael S. Tsirkin wrote:
> > On Tue, Nov 14, 2017 at 08:02:03PM +0800, Wei Wang wrote:
> > > On 11/14/2017 01:32 AM, Michael S. Tsirkin wrote:
> > > > > - guest2host_cmd: written by the guest to ACK to the host about the
> > > > > commands that have been received. The host will clear the corresponding
> > > > > bits on the host2guest_cmd register. The guest also uses this register
> > > > > to send commands to the host (e.g. when finish free page reporting).
> > > > I am not sure what is the role of guest2host_cmd. Reporting of
> > > > the correct cmd id seems sufficient indication that guest
> > > > received the start command. Not getting any more seems sufficient
> > > > to detect stop.
> > > >
> > > I think the issue is when the host is waiting for the guest to report pages,
> > > it does not know whether the guest is going to report more or the report is
> > > done already. That's why we need a way to let the guest tell the host "the
> > > report is done, don't wait for more", then the host continues to the next
> > > step - sending the non-free pages to the destination. The following method
> > > is a conclusion of other comments, with some new thought. Please have a
> > > check if it is good.
> > config won't work well for this IMHO.
> > Writes to config register are hard to synchronize with the VQ.
> > For example, guest sends free pages, host says stop, meanwhile
> > guest sends stop for 1st set of pages.
>
> I still don't see an issue with this. Please see below:
> (before jumping into the discussion, just make sure I've well explained this
> point: now host-to-guest commands are done via config, and guest-to-host
> commands are done via the free page vq)
This is fine by me actually. But right now you have guest to host
not going through vq, going through command register instead -
this is how sending stop to host seems to happen.
If you make it go through vq then I think all will be well.
>
> Case: Host starts to request the reporting with cmd_id=1. Some time later,
> Host writes "stop" to config, meantime guest happens to finish the reporting
> and plan to actively send a "stop" command from the free_page_vq().
> Essentially, this is like a sync between two threads - if we view
> the config interrupt handler as one thread, another is the free page
> reporting worker thread.
>
> - what the config handler does is simply:
> 1.1: WRITE_ONCE(vb->reporting_stop, true);
>
> - what the reporting thread will do is
> 2.1: WRITE_ONCE(vb->reporting_stop, true);
> 2.2: send_stop_to_host_via_vq();
>
> From the guest point of view, no matter 1.1 is executed first or 2.1 first,
> it doesn't make a difference to the end result - vb->reporting_stop is set.
>
> From the host point of view, it knows that cmd_id=1 has truly stopped the
> reporting when it receives a "stop" sign via the vq.
>
>
> > How about adding a buffer with "stop" in the VQ instead?
> > Wastes a VQ entry which you will need to reserve for this
> > but is it a big deal?
>
> The free page vq is guest-to-host direction.
Yes, for guest to host stop sign.
> Using it for host-to-guest
> requests will make it bidirectional, which will result in the same issue
> described before: https://lkml.org/lkml/2017/10/11/1009 (the first response)
>
> On the other hand, I think adding another new vq for host-to-guest
> requesting doesn't make a difference in essence, compared to using config
> (same 1.1, 2.1, 2.2 above), but will be more complicated.
I agree with this. Host to guest can just incremenent the "free command id"
register.
>
> > > Two new configuration registers in total:
> > > - cmd_reg: the command register, combined from the previous host2guest and
> > > guest2host. I think we can use the same register for host requesting and
> > > guest ACKing, since the guest writing will trap to QEMU, that is, all the
> > > writes to the register are performed in QEMU, and we can keep things work in
> > > a correct way there.
> > > - cmd_id_reg: the sequence id of the free page report command.
> > >
> > > -- free page report:
> > > - host requests the guest to start reporting by "cmd_reg |
> > > REPORT_START";
> > > - guest ACKs to the host about receiving the start reporting request by
> > > "cmd_reg | REPORT_START", host will clear the flag bit once receiving the
> > > ACK.
> > > - host requests the guest to stop reporting by "cmd_reg | REPORT_STOP";
> > > - guest ACKs to the host about receiving the stop reporting request by
> > > "cmd_reg | REPORT_STOP", host will clear the flag once receiving the ACK.
> > > - guest tells the host about the start of the reporting by writing "cmd
> > > id" into an outbuf, which is added to the free page vq.
> > > - guest tells the host about the end of the reporting by writing "0"
> > > into an outbuf, which is added to the free page vq. (we reserve "id=0" as
> > > the stop sign)
> > >
> > > -- ballooning:
> > > - host requests the guest to start ballooning by "cmd_reg | BALLOONING";
> > > - guest ACKs to the host about receiving the request by "cmd_reg |
> > > BALLOONING", host will clear the flag once receiving the ACK.
> > >
> > >
> > > Some more explanations:
> > > -- Why not let the host request the guest to start the free page reporting
> > > simply by writing a new cmd id to the cmd_id_reg?
> > > The configuration interrupt is shared among all the features - ballooning,
> > > free page reporting, and future feature extensions which need host-to-guest
> > > requests. Some features may need to add other feature specific configuration
> > > registers, like free page reporting need the cmd_id_reg, which is not used
> > > by ballooning. The rule here is that the feature specific registers are read
> > > only when that feature is requested via the cmd_reg. For example, the
> > > cmd_id_reg is read only when "cmd_reg | REPORT_START" is true. Otherwise,
> > > when the driver receives a configuration interrupt, it has to read both
> > > cmd_reg and cmd_id registers to know what are requested by the host - think
> > > about the case that ballooning requests are sent frequently while free page
> > > reporting isn't requested, the guest has to read the cmd_id register every
> > > time a ballooning request is sent by the host, which is not necessary. If
> > > future new features follow this style, there will be more unnecessary
> > > VMexits to read the unused feature specific registers.
> > > So I think it is good to have a central control of the feature request via
> > > only one cmd register - reading that one is enough to know what is requested
> > > by the host.
> > >
> > Right now you are increasing the cost of balloon request 3x though.
>
> Not that much, I think, just a cmd register read and ACK, and this should be
> neglected compared to the ballooning time.
> (I don't see a difference in the performance testing either).
>
> Best,
> Wei
^ permalink raw reply
* Re: [PATCH v17 6/6] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_VQ
From: Michael S. Tsirkin @ 2017-11-15 20:32 UTC (permalink / raw)
To: Wei Wang
Cc: aarcange, virtio-dev, kvm, mawilcox, qemu-devel, amit.shah,
penguin-kernel, linux-kernel, willy, virtualization, linux-mm,
yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm, mhocko,
mgorman, liliang.opensource
In-Reply-To: <1509696786-1597-7-git-send-email-wei.w.wang@intel.com>
On Fri, Nov 03, 2017 at 04:13:06PM +0800, Wei Wang wrote:
> Negotiation of the VIRTIO_BALLOON_F_FREE_PAGE_VQ feature indicates the
> support of reporting hints of guest free pages to the host via
> virtio-balloon. The host requests the guest to report the free pages by
> sending commands via the virtio-balloon configuration registers.
>
> When the guest starts to report, the first element added to the free page
> vq is a sequence id of the start reporting command. The id is given by
> the host, and it indicates whether the following free pages correspond
> to the command. For example, the host may stop the report and start again
> with a new command id. The obsolete pages for the previous start command
> can be detected by the id dismatching on the host. The id is added to the
> vq using an output buffer, and the free pages are added to the vq using
> input buffer.
>
> Here are some explainations about the added configuration registers:
> - host2guest_cmd: a register used by the host to send commands to the
> guest.
> - guest2host_cmd: written by the guest to ACK to the host about the
> commands that have been received. The host will clear the corresponding
> bits on the host2guest_cmd register. The guest also uses this register
> to send commands to the host (e.g. when finish free page reporting).
> - free_page_cmd_id: the sequence id of the free page report command
> given by the host.
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> Signed-off-by: Liang Li <liang.z.li@intel.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Michal Hocko <mhocko@kernel.org>
> ---
> drivers/virtio/virtio_balloon.c | 234 ++++++++++++++++++++++++++++++++----
> include/uapi/linux/virtio_balloon.h | 11 ++
> 2 files changed, 223 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index b31fc25..4087f04 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -55,7 +55,12 @@ static struct vfsmount *balloon_mnt;
>
> struct virtio_balloon {
> struct virtio_device *vdev;
> - struct virtqueue *inflate_vq, *deflate_vq, *stats_vq;
> + struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq;
> +
> + /* Balloon's own wq for cpu-intensive work items */
> + struct workqueue_struct *balloon_wq;
> + /* The free page reporting work item submitted to the balloon wq */
> + struct work_struct report_free_page_work;
>
> /* The balloon servicing is delegated to a freezable workqueue. */
> struct work_struct update_balloon_stats_work;
> @@ -65,6 +70,10 @@ struct virtio_balloon {
> spinlock_t stop_update_lock;
> bool stop_update;
>
> + /* Stop reporting free pages */
> + bool report_free_page_stop;
> + uint32_t free_page_cmd_id;
> +
> /* Waiting for host to ack the pages we released. */
> wait_queue_head_t acked;
>
> @@ -191,6 +200,30 @@ static void send_balloon_page_sg(struct virtio_balloon *vb,
> kick_and_wait(vq, vb->acked);
> }
>
> +static void send_free_page_sg(struct virtqueue *vq, void *addr, uint32_t size)
> +{
> + int err = 0;
> + unsigned int len;
> +
> + /* Detach all the used buffers from the vq */
> + while (virtqueue_get_buf(vq, &len))
> + ;
> +
> + /*
> + * Since this is an optimization feature, losing a couple of free
> + * pages to report isn't important. We simply resturn without adding
> + * the page if the vq is full.
> + */
> + if (vq->num_free) {
> + err = add_one_sg(vq, addr, size);
> + BUG_ON(err);
> + }
> +
> + /* Batch till the vq is full */
> + if (!vq->num_free)
> + virtqueue_kick(vq);
> +}
> +
> /*
> * Send balloon pages in sgs to host. The balloon pages are recorded in the
> * page xbitmap. Each bit in the bitmap corresponds to a page of PAGE_SIZE.
> @@ -495,9 +528,8 @@ static void stats_handle_request(struct virtio_balloon *vb)
> virtqueue_kick(vq);
> }
>
> -static void virtballoon_changed(struct virtio_device *vdev)
> +static void virtballoon_cmd_balloon_memory(struct virtio_balloon *vb)
> {
> - struct virtio_balloon *vb = vdev->priv;
> unsigned long flags;
>
> spin_lock_irqsave(&vb->stop_update_lock, flags);
> @@ -506,6 +538,50 @@ static void virtballoon_changed(struct virtio_device *vdev)
> spin_unlock_irqrestore(&vb->stop_update_lock, flags);
> }
>
> +static void virtballoon_cmd_report_free_page_start(struct virtio_balloon *vb)
> +{
> + unsigned long flags;
> +
> + vb->report_free_page_stop = false;
> + spin_lock_irqsave(&vb->stop_update_lock, flags);
> + if (!vb->stop_update)
> + queue_work(vb->balloon_wq, &vb->report_free_page_work);
> + spin_unlock_irqrestore(&vb->stop_update_lock, flags);
> +}
> +
> +static void virtballoon_changed(struct virtio_device *vdev)
> +{
> + struct virtio_balloon *vb = vdev->priv;
> + u32 host2guest_cmd, guest2host_cmd = 0;
> +
> + if (!virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_VQ)) {
> + virtballoon_cmd_balloon_memory(vb);
> + return;
> + }
> +
> + virtio_cread(vb->vdev, struct virtio_balloon_config, host2guest_cmd,
> + &host2guest_cmd);
> +
> + if (host2guest_cmd & VIRTIO_BALLOON_CMD_BALLOON_MEMORY) {
> + virtballoon_cmd_balloon_memory(vb);
> + guest2host_cmd |= VIRTIO_BALLOON_CMD_BALLOON_MEMORY;
> + }
> +
> + if (host2guest_cmd & VIRTIO_BALLOON_CMD_REPORT_FREE_PAGE_START) {
> + virtballoon_cmd_report_free_page_start(vb);
> + guest2host_cmd |= VIRTIO_BALLOON_CMD_REPORT_FREE_PAGE_START;
> + }
> +
> + if (host2guest_cmd & VIRTIO_BALLOON_CMD_REPORT_FREE_PAGE_STOP) {
> + vb->report_free_page_stop = true;
> + guest2host_cmd |= VIRTIO_BALLOON_CMD_REPORT_FREE_PAGE_STOP;
> + }
> +
> + /* Ack to the host about the commands that have been received */
> + virtio_cwrite(vb->vdev, struct virtio_balloon_config, guest2host_cmd,
> + &guest2host_cmd);
> +}
> +
> static inline s64 towards_target(struct virtio_balloon *vb)
> {
> s64 target;
> @@ -597,42 +673,147 @@ static void update_balloon_size_func(struct work_struct *work)
> queue_work(system_freezable_wq, work);
> }
>
> -static int init_vqs(struct virtio_balloon *vb)
> +static bool virtio_balloon_send_free_pages(void *opaque, unsigned long pfn,
> + unsigned long nr_pages)
> {
> - struct virtqueue *vqs[3];
> - vq_callback_t *callbacks[] = { balloon_ack, balloon_ack, stats_request };
> - static const char * const names[] = { "inflate", "deflate", "stats" };
> - int err, nvqs;
> + struct virtio_balloon *vb = (struct virtio_balloon *)opaque;
> + void *addr = (void *)pfn_to_kaddr(pfn);
> + uint32_t len = nr_pages << PAGE_SHIFT;
> +
> + if (vb->report_free_page_stop)
> + return false;
> +
> + send_free_page_sg(vb->free_page_vq, addr, len);
>
> + return true;
> +}
> +
> +static void report_free_page_end(struct virtio_balloon *vb)
> +{
> + u32 cmd = VIRTIO_BALLOON_CMD_REPORT_FREE_PAGE_STOP;
> /*
> - * We expect two virtqueues: inflate and deflate, and
> - * optionally stat.
> + * The host may have already requested to stop the reporting before we
> + * finish, so no need to notify the host in this case.
> */
> - nvqs = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ) ? 3 : 2;
> - err = virtio_find_vqs(vb->vdev, nvqs, vqs, callbacks, names, NULL);
> + if (vb->report_free_page_stop)
> + return;
> + vb->report_free_page_stop = true;
> +
> + virtio_cwrite(vb->vdev, struct virtio_balloon_config, guest2host_cmd,
> + &cmd);
> +}
> +
> +static void report_free_page_cmd_id(struct virtio_balloon *vb)
> +{
> + struct scatterlist sg;
> + int err;
> +
> + virtio_cread(vb->vdev, struct virtio_balloon_config, free_page_cmd_id,
> + &vb->free_page_cmd_id);
> + sg_init_one(&sg, &vb->free_page_cmd_id, sizeof(uint32_t));
> + err = virtqueue_add_outbuf(vb->free_page_vq, &sg, 1,
> + &vb->free_page_cmd_id, GFP_KERNEL);
> + BUG_ON(err);
> +}
> +
> +static void report_free_page(struct work_struct *work)
> +{
> + struct virtio_balloon *vb;
> +
> + vb = container_of(work, struct virtio_balloon, report_free_page_work);
> + report_free_page_cmd_id(vb);
> + walk_free_mem_block(vb, 0, &virtio_balloon_send_free_pages);
> + /*
> + * The last few free page blocks that were added may not reach the
> + * batch size, but need a kick to notify the device to handle them.
> + */
> + virtqueue_kick(vb->free_page_vq);
> + report_free_page_end(vb);
> +}
> +
I think there's an issue here: if pages are poisoned and hypervisor
subsequently drops them, testing them after allocation will
trigger a false positive.
The specific configuration:
PAGE_POISONING on
PAGE_POISONING_NO_SANITY off
PAGE_POISONING_ZERO off
Solutions:
1. disable the feature in that configuration
suggested as an initial step
2. pass poison value to host so it can validate page content
before it drops it
3. pass poison value to host so it can init allocated pages with that value
In fact one nice side effect would be that unmap
becomes safe even though free list is not locked anymore.
It would be interesting to see whether this last has
any value performance-wise.
> +static int init_vqs(struct virtio_balloon *vb)
> +{
> + struct virtqueue **vqs;
> + vq_callback_t **callbacks;
> + const char **names;
> + struct scatterlist sg;
> + int i, nvqs, err = -ENOMEM;
> +
> + /* Inflateq and deflateq are used unconditionally */
> + nvqs = 2;
> + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ))
> + nvqs++;
> + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_VQ))
> + nvqs++;
> +
> + /* Allocate space for find_vqs parameters */
> + vqs = kcalloc(nvqs, sizeof(*vqs), GFP_KERNEL);
> + if (!vqs)
> + goto err_vq;
> + callbacks = kmalloc_array(nvqs, sizeof(*callbacks), GFP_KERNEL);
> + if (!callbacks)
> + goto err_callback;
> + names = kmalloc_array(nvqs, sizeof(*names), GFP_KERNEL);
> + if (!names)
> + goto err_names;
> +
> + callbacks[0] = balloon_ack;
> + names[0] = "inflate";
> + callbacks[1] = balloon_ack;
> + names[1] = "deflate";
> +
> + i = 2;
> + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
> + callbacks[i] = stats_request;
> + names[i] = "stats";
> + i++;
> + }
> +
> + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_VQ)) {
> + callbacks[i] = NULL;
> + names[i] = "free_page_vq";
> + }
> +
> + err = vb->vdev->config->find_vqs(vb->vdev, nvqs, vqs, callbacks, names,
> + NULL, NULL);
> if (err)
> - return err;
> + goto err_find;
>
> vb->inflate_vq = vqs[0];
> vb->deflate_vq = vqs[1];
> + i = 2;
> if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
> - struct scatterlist sg;
> - unsigned int num_stats;
> - vb->stats_vq = vqs[2];
> -
> + vb->stats_vq = vqs[i++];
> /*
> * Prime this virtqueue with one buffer so the hypervisor can
> * use it to signal us later (it can't be broken yet!).
> */
> - num_stats = update_balloon_stats(vb);
> -
> - sg_init_one(&sg, vb->stats, sizeof(vb->stats[0]) * num_stats);
> + sg_init_one(&sg, vb->stats, sizeof(vb->stats));
> if (virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb, GFP_KERNEL)
> - < 0)
> - BUG();
> + < 0) {
> + dev_warn(&vb->vdev->dev, "%s: add stat_vq failed\n",
> + __func__);
> + goto err_find;
> + }
> virtqueue_kick(vb->stats_vq);
> }
> +
> + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_VQ))
> + vb->free_page_vq = vqs[i];
> +
> + kfree(names);
> + kfree(callbacks);
> + kfree(vqs);
> return 0;
> +
> +err_find:
> + kfree(names);
> +err_names:
> + kfree(callbacks);
> +err_callback:
> + kfree(vqs);
> +err_vq:
> + return err;
> }
>
> #ifdef CONFIG_BALLOON_COMPACTION
> @@ -761,6 +942,13 @@ static int virtballoon_probe(struct virtio_device *vdev)
> if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_SG))
> xb_init(&vb->page_xb);
>
> + if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_VQ)) {
> + vb->balloon_wq = alloc_workqueue("balloon-wq",
> + WQ_FREEZABLE | WQ_CPU_INTENSIVE, 0);
> + INIT_WORK(&vb->report_free_page_work, report_free_page);
> + vb->report_free_page_stop = true;
> + }
> +
> vb->nb.notifier_call = virtballoon_oom_notify;
> vb->nb.priority = VIRTBALLOON_OOM_NOTIFY_PRIORITY;
> err = register_oom_notifier(&vb->nb);
> @@ -825,6 +1013,7 @@ static void virtballoon_remove(struct virtio_device *vdev)
> spin_unlock_irq(&vb->stop_update_lock);
> cancel_work_sync(&vb->update_balloon_size_work);
> cancel_work_sync(&vb->update_balloon_stats_work);
> + cancel_work_sync(&vb->report_free_page_work);
>
> remove_common(vb);
> #ifdef CONFIG_BALLOON_COMPACTION
> @@ -878,6 +1067,7 @@ static unsigned int features[] = {
> VIRTIO_BALLOON_F_STATS_VQ,
> VIRTIO_BALLOON_F_DEFLATE_ON_OOM,
> VIRTIO_BALLOON_F_SG,
> + VIRTIO_BALLOON_F_FREE_PAGE_VQ,
> };
>
> static struct virtio_driver virtio_balloon_driver = {
> diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
> index 37780a7..b758484 100644
> --- a/include/uapi/linux/virtio_balloon.h
> +++ b/include/uapi/linux/virtio_balloon.h
> @@ -35,15 +35,26 @@
> #define VIRTIO_BALLOON_F_STATS_VQ 1 /* Memory Stats virtqueue */
> #define VIRTIO_BALLOON_F_DEFLATE_ON_OOM 2 /* Deflate balloon on OOM */
> #define VIRTIO_BALLOON_F_SG 3 /* Use sg instead of PFN lists */
> +#define VIRTIO_BALLOON_F_FREE_PAGE_VQ 4 /* VQ to report free pages */
>
> /* Size of a PFN in the balloon interface. */
> #define VIRTIO_BALLOON_PFN_SHIFT 12
>
> +#define VIRTIO_BALLOON_CMD_BALLOON_MEMORY (1 << 0)
> +#define VIRTIO_BALLOON_CMD_REPORT_FREE_PAGE_START (1 << 1)
> +#define VIRTIO_BALLOON_CMD_REPORT_FREE_PAGE_STOP (1 << 2)
> +
> struct virtio_balloon_config {
> /* Number of pages host wants Guest to give up. */
> __u32 num_pages;
> /* Number of pages we've actually got in balloon. */
> __u32 actual;
> + /* Host-to-guest command, readonly by guest */
> + __u32 host2guest_cmd;
> + /* Sequence id of the free_page report command, readonly by guest */
> + __u32 free_page_cmd_id;
> + /* Guest-to-host command */
> + __u32 guest2host_cmd;
> };
>
> #define VIRTIO_BALLOON_S_SWAP_IN 0 /* Amount of memory swapped in */
> --
> 2.7.4
^ permalink raw reply
* Re: [Xen-devel] [PATCH RFC v3 0/6] x86/idle: add halt poll support
From: Konrad Rzeszutek Wilk @ 2017-11-15 21:31 UTC (permalink / raw)
To: Quan Xu
Cc: Yang Zhang, kvm, linux-doc, x86, linux-kernel, virtualization,
linux-fsdevel, xen-devel
In-Reply-To: <1510567565-5118-1-git-send-email-quan.xu0@gmail.com>
On Mon, Nov 13, 2017 at 06:05:59PM +0800, Quan Xu wrote:
> From: Yang Zhang <yang.zhang.wz@gmail.com>
>
> Some latency-intensive workload have seen obviously performance
> drop when running inside VM. The main reason is that the overhead
> is amplified when running inside VM. The most cost I have seen is
> inside idle path.
Meaning an VMEXIT b/c it is an 'halt' operation ? And then going
back in guest (VMRESUME) takes time. And hence your latency gets
all whacked b/c of this?
So if I understand - you want to use your _full_ timeslice (of the guest)
without ever (or as much as possible) to go in the hypervisor?
Which means in effect you don't care about power-saving or CPUfreq
savings, you just want to eat the full CPU for snack?
>
> This patch introduces a new mechanism to poll for a while before
> entering idle state. If schedule is needed during poll, then we
> don't need to goes through the heavy overhead path.
Schedule of what? The guest or the host?
^ permalink raw reply
* Re: [PATCH RFC v3 3/6] sched/idle: Add a generic poll before enter real idle path
From: Thomas Gleixner @ 2017-11-15 22:03 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Yang Zhang, Len Brown, Quan Xu, Daniel Lezcano, kvm, linux-doc,
x86, LKML, virtualization, Kyle Huey, Ingo Molnar,
Borislav Petkov, Quan Xu, Andy Lutomirski, H. Peter Anvin,
linux-fsdevel, xen-devel, Tom Lendacky, Tobias Klauser
In-Reply-To: <20171115121152.gqug5wzerlo3eimd@hirez.programming.kicks-ass.net>
On Wed, 15 Nov 2017, Peter Zijlstra wrote:
> On Mon, Nov 13, 2017 at 06:06:02PM +0800, Quan Xu wrote:
> > From: Yang Zhang <yang.zhang.wz@gmail.com>
> >
> > Implement a generic idle poll which resembles the functionality
> > found in arch/. Provide weak arch_cpu_idle_poll function which
> > can be overridden by the architecture code if needed.
>
> No, we want less of those magic hooks, not more.
>
> > Interrupts arrive which may not cause a reschedule in idle loops.
> > In KVM guest, this costs several VM-exit/VM-entry cycles, VM-entry
> > for interrupts and VM-exit immediately. Also this becomes more
> > expensive than bare metal. Add a generic idle poll before enter
> > real idle path. When a reschedule event is pending, we can bypass
> > the real idle path.
>
> Why not do a HV specific idle driver?
If I understand the problem correctly then he wants to avoid the heavy
lifting in tick_nohz_idle_enter() in the first place, but there is already
an interesting quirk there which makes it exit early. See commit
3c5d92a0cfb5 ("nohz: Introduce arch_needs_cpu"). The reason for this commit
looks similar. But lets not proliferate that. I'd rather see that go away.
But the irq_timings stuff is heading into the same direction, with a more
complex prediction logic which should tell you pretty good how long that
idle period is going to be and in case of an interrupt heavy workload this
would skip the extra work of stopping and restarting the tick and provide a
very good input into a polling decision.
This can be handled either in a HV specific idle driver or even in the
generic core code. If the interrupt does not arrive then you can assume
within the predicted time then you can assume that the flood stopped and
invoke halt or whatever.
That avoids all of that 'tunable and tweakable' x86 specific hackery and
utilizes common functionality which is mostly there already.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH] vhost/scsi: Use safe iteration in vhost_scsi_complete_cmd_work()
From: Byungchul Park @ 2017-11-15 23:52 UTC (permalink / raw)
To: nab, mst, jasowang; +Cc: kernel-team, netdev, linux-kernel, kvm, virtualization
In-Reply-To: <1510186649-9765-1-git-send-email-byungchul.park@lge.com>
On Thu, Nov 09, 2017 at 09:17:29AM +0900, Byungchul Park wrote:
> I am sorry for having made a mistake on it.
Hello Nicholas,
Please consider this patch urgently. I'm sorry for having changed the
original behavior with the previous patch.
The safe version of llist API should be used to keep the original
behavior.
Thanks,
Byungchul
> -----8<-----
> >From ba9a0f76dffceffa4fa3aa2d9be49cdb0d9b7d4f Mon Sep 17 00:00:00 2001
> From: Byungchul Park <byungchul.park@lge.com>
> Date: Thu, 9 Nov 2017 09:00:21 +0900
> Subject: [PATCH] vhost/scsi: Use safe iteration in vhost_scsi_complete_cmd_work()
>
> The following patch changed the behavior which originally did safe
> iteration. Make it safe as it was.
>
> 12bdcbd539c6327c09da0503c674733cb2d82cb5
> vhost/scsi: Don't reinvent the wheel but use existing llist API
>
> Signed-off-by: Byungchul Park <byungchul.park@lge.com>
> ---
> drivers/vhost/scsi.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index 046f6d2..46539ca 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c
> @@ -519,7 +519,7 @@ static void vhost_scsi_complete_cmd_work(struct vhost_work *work)
> vs_completion_work);
> DECLARE_BITMAP(signal, VHOST_SCSI_MAX_VQ);
> struct virtio_scsi_cmd_resp v_rsp;
> - struct vhost_scsi_cmd *cmd;
> + struct vhost_scsi_cmd *cmd, *t;
> struct llist_node *llnode;
> struct se_cmd *se_cmd;
> struct iov_iter iov_iter;
> @@ -527,7 +527,7 @@ static void vhost_scsi_complete_cmd_work(struct vhost_work *work)
>
> bitmap_zero(signal, VHOST_SCSI_MAX_VQ);
> llnode = llist_del_all(&vs->vs_completion_list);
> - llist_for_each_entry(cmd, llnode, tvc_completion_list) {
> + llist_for_each_entry_safe(cmd, t, llnode, tvc_completion_list) {
> se_cmd = &cmd->tvc_se_cmd;
>
> pr_debug("%s tv_cmd %p resid %u status %#02x\n", __func__,
> --
> 1.9.1
^ permalink raw reply
* Re: [PATCH] vhost/scsi: Use safe iteration in vhost_scsi_complete_cmd_work()
From: Michael S. Tsirkin @ 2017-11-16 1:30 UTC (permalink / raw)
To: Byungchul Park; +Cc: kvm, netdev, linux-kernel, virtualization, kernel-team
In-Reply-To: <20171115235239.GA4394@X58A-UD3R>
On Thu, Nov 16, 2017 at 08:52:39AM +0900, Byungchul Park wrote:
> On Thu, Nov 09, 2017 at 09:17:29AM +0900, Byungchul Park wrote:
> > I am sorry for having made a mistake on it.
>
> Hello Nicholas,
>
> Please consider this patch urgently. I'm sorry for having changed the
> original behavior with the previous patch.
>
> The safe version of llist API should be used to keep the original
> behavior.
>
> Thanks,
> Byungchul
I have included this patch in my tree.
> > -----8<-----
> > >From ba9a0f76dffceffa4fa3aa2d9be49cdb0d9b7d4f Mon Sep 17 00:00:00 2001
> > From: Byungchul Park <byungchul.park@lge.com>
> > Date: Thu, 9 Nov 2017 09:00:21 +0900
> > Subject: [PATCH] vhost/scsi: Use safe iteration in vhost_scsi_complete_cmd_work()
> >
> > The following patch changed the behavior which originally did safe
> > iteration. Make it safe as it was.
> >
> > 12bdcbd539c6327c09da0503c674733cb2d82cb5
> > vhost/scsi: Don't reinvent the wheel but use existing llist API
> >
> > Signed-off-by: Byungchul Park <byungchul.park@lge.com>
> > ---
> > drivers/vhost/scsi.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> > index 046f6d2..46539ca 100644
> > --- a/drivers/vhost/scsi.c
> > +++ b/drivers/vhost/scsi.c
> > @@ -519,7 +519,7 @@ static void vhost_scsi_complete_cmd_work(struct vhost_work *work)
> > vs_completion_work);
> > DECLARE_BITMAP(signal, VHOST_SCSI_MAX_VQ);
> > struct virtio_scsi_cmd_resp v_rsp;
> > - struct vhost_scsi_cmd *cmd;
> > + struct vhost_scsi_cmd *cmd, *t;
> > struct llist_node *llnode;
> > struct se_cmd *se_cmd;
> > struct iov_iter iov_iter;
> > @@ -527,7 +527,7 @@ static void vhost_scsi_complete_cmd_work(struct vhost_work *work)
> >
> > bitmap_zero(signal, VHOST_SCSI_MAX_VQ);
> > llnode = llist_del_all(&vs->vs_completion_list);
> > - llist_for_each_entry(cmd, llnode, tvc_completion_list) {
> > + llist_for_each_entry_safe(cmd, t, llnode, tvc_completion_list) {
> > se_cmd = &cmd->tvc_se_cmd;
> >
> > pr_debug("%s tv_cmd %p resid %u status %#02x\n", __func__,
> > --
> > 1.9.1
^ permalink raw reply
* Re: [PATCH] vhost/scsi: Use safe iteration in vhost_scsi_complete_cmd_work()
From: Byungchul Park @ 2017-11-16 1:33 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, netdev, linux-kernel, virtualization, kernel-team
In-Reply-To: <20171116033036-mutt-send-email-mst@kernel.org>
On 11/16/2017 10:30 AM, Michael S. Tsirkin wrote:
> On Thu, Nov 16, 2017 at 08:52:39AM +0900, Byungchul Park wrote:
>> On Thu, Nov 09, 2017 at 09:17:29AM +0900, Byungchul Park wrote:
>>> I am sorry for having made a mistake on it.
>>
>> Hello Nicholas,
>>
>> Please consider this patch urgently. I'm sorry for having changed the
>> original behavior with the previous patch.
>>
>> The safe version of llist API should be used to keep the original
>> behavior.
>>
>> Thanks,
>> Byungchul
>
> I have included this patch in my tree.
Thank you.
--
Thanks,
Byungchul
^ permalink raw reply
* Re: [PATCH RFC v3 3/6] sched/idle: Add a generic poll before enter real idle path
From: Peter Zijlstra @ 2017-11-16 8:45 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Yang Zhang, Len Brown, Quan Xu, Daniel Lezcano, kvm, linux-doc,
x86, LKML, virtualization, Kyle Huey, Ingo Molnar,
Borislav Petkov, Quan Xu, Andy Lutomirski, H. Peter Anvin,
linux-fsdevel, xen-devel, Tom Lendacky, Tobias Klauser
In-Reply-To: <alpine.DEB.2.20.1711152240010.2146@nanos>
On Wed, Nov 15, 2017 at 11:03:08PM +0100, Thomas Gleixner wrote:
> If I understand the problem correctly then he wants to avoid the heavy
> lifting in tick_nohz_idle_enter() in the first place, but there is already
> an interesting quirk there which makes it exit early.
Sure. And there are people who want to do the same for native.
Adding more ugly and special cases just isn't the way to go about doing
that.
I'm fairly sure I've told the various groups that want to tinker with
this to work together on this. I've also in fairly significant detail
sketched how to rework the idle code and idle predictors.
At this point I'm too tired to dig any of that up, so I'll just keep
saying no to patches that don't even attempt to go in the right
direction.
^ permalink raw reply
* Re: [PATCH RFC v3 3/6] sched/idle: Add a generic poll before enter real idle path
From: Thomas Gleixner @ 2017-11-16 8:58 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Yang Zhang, Len Brown, Quan Xu, Daniel Lezcano, kvm, linux-doc,
x86, LKML, virtualization, Kyle Huey, Ingo Molnar,
Borislav Petkov, Quan Xu, Andy Lutomirski, H. Peter Anvin,
linux-fsdevel, xen-devel, Tom Lendacky, Tobias Klauser
In-Reply-To: <20171116084555.it2hqajxxd6ld5tq@hirez.programming.kicks-ass.net>
On Thu, 16 Nov 2017, Peter Zijlstra wrote:
> On Wed, Nov 15, 2017 at 11:03:08PM +0100, Thomas Gleixner wrote:
> > If I understand the problem correctly then he wants to avoid the heavy
> > lifting in tick_nohz_idle_enter() in the first place, but there is already
> > an interesting quirk there which makes it exit early.
>
> Sure. And there are people who want to do the same for native.
>
> Adding more ugly and special cases just isn't the way to go about doing
> that.
>
> I'm fairly sure I've told the various groups that want to tinker with
> this to work together on this. I've also in fairly significant detail
> sketched how to rework the idle code and idle predictors.
>
> At this point I'm too tired to dig any of that up, so I'll just keep
> saying no to patches that don't even attempt to go in the right
> direction.
That's why I said: But lets not proliferate that. I'd rather see that go
away.
And yes, the VM folks should talk to those who are trying to solve similar
problems for native (embedded/mobile).
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH RFC v3 3/6] sched/idle: Add a generic poll before enter real idle path
From: Quan Xu @ 2017-11-16 9:12 UTC (permalink / raw)
To: Thomas Gleixner, Peter Zijlstra
Cc: Yang Zhang, Len Brown, Daniel Lezcano, kvm, linux-doc, x86, LKML,
virtualization, Kyle Huey, Ingo Molnar, Borislav Petkov, Quan Xu,
Andy Lutomirski, H. Peter Anvin, linux-fsdevel, xen-devel,
Tom Lendacky, Tobias Klauser
In-Reply-To: <alpine.DEB.2.20.1711152240010.2146@nanos>
On 2017-11-16 06:03, Thomas Gleixner wrote:
> On Wed, 15 Nov 2017, Peter Zijlstra wrote:
>
>> On Mon, Nov 13, 2017 at 06:06:02PM +0800, Quan Xu wrote:
>>> From: Yang Zhang <yang.zhang.wz@gmail.com>
>>>
>>> Implement a generic idle poll which resembles the functionality
>>> found in arch/. Provide weak arch_cpu_idle_poll function which
>>> can be overridden by the architecture code if needed.
>> No, we want less of those magic hooks, not more.
>>
>>> Interrupts arrive which may not cause a reschedule in idle loops.
>>> In KVM guest, this costs several VM-exit/VM-entry cycles, VM-entry
>>> for interrupts and VM-exit immediately. Also this becomes more
>>> expensive than bare metal. Add a generic idle poll before enter
>>> real idle path. When a reschedule event is pending, we can bypass
>>> the real idle path.
>> Why not do a HV specific idle driver?
> If I understand the problem correctly then he wants to avoid the heavy
> lifting in tick_nohz_idle_enter() in the first place, but there is already
> an interesting quirk there which makes it exit early. See commit
> 3c5d92a0cfb5 ("nohz: Introduce arch_needs_cpu"). The reason for this commit
> looks similar. But lets not proliferate that. I'd rather see that go away.
agreed.
Even we can get more benifit than commit 3c5d92a0cfb5 ("nohz: Introduce
arch_needs_cpu")
in kvm guest. I won't proliferate that..
> But the irq_timings stuff is heading into the same direction, with a more
> complex prediction logic which should tell you pretty good how long that
> idle period is going to be and in case of an interrupt heavy workload this
> would skip the extra work of stopping and restarting the tick and provide a
> very good input into a polling decision.
interesting. I have tested with IRQ_TIMINGS related code, which seems
not working so far.
Also I'd like to help as much as I can.
> This can be handled either in a HV specific idle driver or even in the
> generic core code. If the interrupt does not arrive then you can assume
> within the predicted time then you can assume that the flood stopped and
> invoke halt or whatever.
>
> That avoids all of that 'tunable and tweakable' x86 specific hackery and
> utilizes common functionality which is mostly there already.
here is some sample code. Poll for a while before enter halt in
cpuidle_enter_state()
If I get a reschedule event, then don't try to enter halt. (I hope this
is the right direction as Peter mentioned in another email)
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -210,6 +210,13 @@ int cpuidle_enter_state(struct cpuidle_device *dev,
struct cpuidle_driver *drv,
target_state = &drv->states[index];
}
+#ifdef CONFIG_PARAVIRT
+ paravirt_idle_poll();
+
+ if (need_resched())
+ return -EBUSY;
+#endif
+
/* Take note of the planned idle state. */
sched_idle_set_state(target_state);
thanks,
Quan
Alibaba Cloud
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox