* 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
* Re: [PATCH RFC v3 3/6] sched/idle: Add a generic poll before enter real idle path
From: Quan Xu @ 2017-11-16 9:29 UTC (permalink / raw)
To: Peter Zijlstra, Thomas Gleixner
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: <20171116084555.it2hqajxxd6ld5tq@hirez.programming.kicks-ass.net>
On 2017-11-16 16:45, 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.
Peter, take care.
I really have considered this factor, and try my best not to interfere
with scheduler/idle code.
if irq_timings code is ready, I can use it directly. I think irq_timings
is not an easy task, I'd
like to help as much as I can. Also don't try to touch tick_nohz* code
again.
as tglx suggested, this can be handled either in a HV specific idle driver or even in the generic core code.
I hope this is in the right direction.
Quan
Alibaba Cloud
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH RFC v3 3/6] sched/idle: Add a generic poll before enter real idle path
From: Daniel Lezcano @ 2017-11-16 9:45 UTC (permalink / raw)
To: Quan Xu, Thomas Gleixner, Peter Zijlstra
Cc: Yang Zhang, Len Brown, 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: <46086489-5a01-16e1-9314-70ae53c01952@gmail.com>
On 16/11/2017 10:12, Quan Xu wrote:
>
>
> 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.
I don't know how you tested it, can you elaborate what you meant by
"seems not working so far" ?
There are still some work to do to be more efficient. The prediction
based on the irq timings is all right if the interrupts have a simple
periodicity. But as soon as there is a pattern, the current code can't
handle it properly and does bad predictions.
I'm working on a self-learning pattern detection which is too heavy for
the kernel, and with it we should be able to detect properly the
patterns and re-ajust the period if it changes. I'm in the process of
making it suitable for kernel code (both math and perf).
One improvement which can be done right now and which can help you is
the interrupts rate on the CPU. It is possible to compute it and that
will give an accurate information for the polling decision.
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ 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 9:47 UTC (permalink / raw)
To: Quan Xu
Cc: Yang Zhang, Len Brown, Daniel Lezcano, kvm, linux-doc,
Peter Zijlstra, 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: <13a3a5c5-ec14-131f-fe6a-01700783de81@gmail.com>
On Thu, 16 Nov 2017, Quan Xu wrote:
> On 2017-11-16 16:45, Peter Zijlstra wrote:
>
> I really have considered this factor, and try my best not to interfere with
> scheduler/idle code.
>
> if irq_timings code is ready, I can use it directly. I think irq_timings
> is not an easy task, I'd like to help as much as I can.
It's not a question whether irq_timings code is ready or not.
The infrastructure is there. I said that before and I'm happy to repeat:
All parties who need this kind of prediction should:
1) Talk to each other
2) Work together to make it usable for _ALL_ use cases
AFAICT, that never happened. Otherwise there would be either progress on
that or at least a reasonable explanation WHY it cannot be done.
Peter and myself made it entirely clear several times in the past that this
must be solved at the generic level without any magic hackery in random
places. But the only thing we've seen so far is the magic hackery coming
around in a different flavour some time after we rejected the last one.
We can play that game forever. The outcome is extremly predictable.
Thanks,
tglx
^ 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 9:53 UTC (permalink / raw)
To: Quan Xu
Cc: Yang Zhang, Len Brown, Daniel Lezcano, kvm, linux-doc,
Peter Zijlstra, 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: <46086489-5a01-16e1-9314-70ae53c01952@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1296 bytes --]
On Thu, 16 Nov 2017, Quan Xu wrote:
> On 2017-11-16 06:03, Thomas Gleixner wrote:
> --- 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
That's just plain wrong. We don't want to see any of this PARAVIRT crap in
anything outside the architecture/hypervisor interfacing code which really
needs it.
The problem can and must be solved at the generic level in the first place
to gather the data which can be used to make such decisions.
How that information is used might be either completely generic or requires
system specific variants. But as long as we don't have any information at
all we cannot discuss that.
Please sit down and write up which data needs to be considered to make
decisions about probabilistic polling. Then we need to compare and contrast
that with the data which is necessary to make power/idle state decisions.
I would be very surprised if this data would not overlap by at least 90%.
Thanks,
tglx
[-- 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: Wei Wang @ 2017-11-16 11:59 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: <20171115152307-mutt-send-email-mst@kernel.org>
On 11/15/2017 09:26 PM, Michael S. Tsirkin wrote:
> 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.
OK, thanks for the suggestions. I think one more issue left here:
Previously, when the guest receives a config interrupt, it blindly adds
the balloon work item to the workqueue in virtballoon_changed(), because
only ballooning uses the config.
Now, free page reporting is requested via config, too.
We have the following two options:
Option 1: add "diff = towards_target()" to virtballoon_changed(), and if
diff = 0, it will not add the balloon work item to the wq.
Option 2: add "cmd" for the host-to-guest request, and add the item when
"cmd | CMD_BALLOON" is true.
I'm inclined to take option 1 now. Which one would you prefer?
Best,
Wei
^ permalink raw reply
* Re: [virtio-dev] Re: [PATCH v17 6/6] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_VQ
From: Wei Wang @ 2017-11-16 13:27 UTC (permalink / raw)
To: Michael S. Tsirkin
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: <20171115220743-mutt-send-email-mst@kernel.org>
On 11/16/2017 04:32 AM, Michael S. Tsirkin wrote:
> 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>
>> ---
>>
>> +
>> +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
Thanks for the finding.
Similar to this option: I'm thinking could we make walk_free_mem_block()
simply return if that option is on?
That is, at the beginning of the function:
if (!page_poisoning_enabled())
return;
I think in most usages, people would not choose to use the poisoning
option due to the added overhead.
Probably we could make it a separate fix patch of this report following
patch 5 to explain the above reasons in the commit.
> 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.
I haven't got this point yet, how would it bring performance benefit?
> It would be interesting to see whether this last has
> any value performance-wise.
>
Best,
Wei
^ permalink raw reply
* [PULL] vhost/virtio/qemu: cleanups and fixes
From: Michael S. Tsirkin @ 2017-11-16 18:51 UTC (permalink / raw)
To: Linus Torvalds
Cc: mhocko, kvm, mst, penguin-kernel, netdev, somlo, linux-kernel,
den, virtualization, byungchul.park, stefanha, marcandre.lureau
DMA support in FW CFG had to be pushed out as it caused ltp failures -
likely a compatibility issue, and could be a hypervisor bug, but we need
to figure it out first. There's still a small chance it'll happen
shortly, then I might do another pull request just for that.
The following changes since commit bebc6082da0a9f5d47a1ea2edc099bf671058bd4:
Linux 4.14 (2017-11-12 10:46:13 -0800)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git tags/for_linus
for you to fetch changes up to c1d0c3f623ada808904dec676da0126f5b800630:
fw_cfg: fix the command line module name (2017-11-14 23:57:40 +0200)
----------------------------------------------------------------
virtio, vhost, qemu: bugfixes, cleanups
Fixes in qemu, vhost and virtio.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
----------------------------------------------------------------
Byungchul Park (1):
vhost/scsi: Use safe iteration in vhost_scsi_complete_cmd_work()
Marc-André Lureau (1):
fw_cfg: fix the command line module name
Michael S. Tsirkin (2):
virtio_balloon: fix deadlock on OOM
vhost: fix end of range for access_ok
Stefan Hajnoczi (1):
vhost/vsock: fix uninitialized vhost_vsock->guest_cid
drivers/firmware/qemu_fw_cfg.c | 8 ++++----
drivers/vhost/scsi.c | 4 ++--
drivers/vhost/vhost.c | 4 ++--
drivers/vhost/vsock.c | 2 ++
drivers/virtio/virtio_balloon.c | 24 +++++++++++++++++++-----
include/linux/balloon_compaction.h | 35 ++++++++++++++++++++++++++++++++++-
mm/balloon_compaction.c | 28 +++++++++++++++++++++-------
7 files changed, 84 insertions(+), 21 deletions(-)
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 02/13] x86/paravirt: Fix output constraint macro names
From: Josh Poimboeuf @ 2017-11-16 20:50 UTC (permalink / raw)
To: Juergen Gross
Cc: Boris Ostrovsky, Rusty Russell, Mike Galbraith, xen-devel,
Peter Zijlstra, Jiri Slaby, x86, linux-kernel, Sasha Levin,
Chris Wright, Thomas Gleixner, Andy Lutomirski, H. Peter Anvin,
Borislav Petkov, live-patching, Alok Kataria, virtualization,
Linus Torvalds, Ingo Molnar
In-Reply-To: <5d2799f9-fb5d-381e-a576-15098626201f@suse.com>
On Wed, Oct 25, 2017 at 11:33:43AM +0200, Juergen Gross wrote:
> On 04/10/17 17:58, Josh Poimboeuf wrote:
> > Some of the paravirt '*_CLOBBERS' macros refer to output constraints
> > instead of clobbers, which makes the code extra confusing. Rename the
> > output constraint related macros to '*_OUTPUTS'.
> >
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
>
> I'm fine with the changes, but you might want to rename the "call_clbr"
> parameter of ____PVOP_[V]CALL, too, e.g. to "outputs".
Yeah, good catch.
> You could then drop the "CALL_" from the macros, too.
Hm, which macros are you referring to, and why?
--
Josh
^ permalink raw reply
* Re: [PATCH 03/13] x86/paravirt: Convert native patch assembly code strings to macros
From: Josh Poimboeuf @ 2017-11-16 21:04 UTC (permalink / raw)
To: Juergen Gross
Cc: Boris Ostrovsky, Rusty Russell, Mike Galbraith, xen-devel,
Peter Zijlstra, Jiri Slaby, x86, linux-kernel, Sasha Levin,
Chris Wright, Thomas Gleixner, Andy Lutomirski, H. Peter Anvin,
Borislav Petkov, live-patching, Alok Kataria, virtualization,
Linus Torvalds, Ingo Molnar
In-Reply-To: <a1c4f192-f141-7488-ae6e-ad07101ba8da@suse.com>
On Wed, Oct 25, 2017 at 11:46:18AM +0200, Juergen Gross wrote:
> On 04/10/17 17:58, Josh Poimboeuf wrote:
> > Convert the hard-coded native patch assembly code strings to macros to
> > facilitate sharing common code between 32-bit and 64-bit.
> >
> > These macros will also be used by a future patch which requires the GCC
> > extended asm syntax of two '%' characters instead of one when specifying
> > a register name.
> >
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
>
> Reviewed-by: Juergen Gross <jgross@suse.com>
>
> Mind adding another patch to merge the now nearly identical
> paravirt_patch_32.c and paravirt_patch_64.c either into paravirt.c
> or paravirt_patch.c? This would require only very few #ifdef.
Good idea, will do.
--
Josh
^ permalink raw reply
* Re: [PATCH 10/13] x86/alternative: Support indirect call replacement
From: Josh Poimboeuf @ 2017-11-16 21:19 UTC (permalink / raw)
To: Juergen Gross
Cc: Boris Ostrovsky, Rusty Russell, Mike Galbraith, xen-devel,
Peter Zijlstra, Jiri Slaby, x86, linux-kernel, Sasha Levin,
Chris Wright, Thomas Gleixner, Andy Lutomirski, H. Peter Anvin,
Borislav Petkov, live-patching, Alok Kataria, virtualization,
Linus Torvalds, Ingo Molnar
In-Reply-To: <c57fb310-141b-2262-7e17-bf37d3afdf82@suse.com>
On Wed, Oct 25, 2017 at 01:25:02PM +0200, Juergen Gross wrote:
> On 04/10/17 17:58, Josh Poimboeuf wrote:
> > Add alternative patching support for replacing an instruction with an
> > indirect call. This will be needed for the paravirt alternatives.
> >
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > ---
> > arch/x86/kernel/alternative.c | 22 +++++++++++++++-------
> > 1 file changed, 15 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> > index 3344d3382e91..81c577c7deba 100644
> > --- a/arch/x86/kernel/alternative.c
> > +++ b/arch/x86/kernel/alternative.c
> > @@ -410,20 +410,28 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
> > insnbuf_sz = a->replacementlen;
> >
> > /*
> > - * 0xe8 is a relative jump; fix the offset.
> > - *
> > - * Instruction length is checked before the opcode to avoid
> > - * accessing uninitialized bytes for zero-length replacements.
> > + * Fix the address offsets for call and jump instructions which
> > + * use PC-relative addressing.
> > */
> > if (a->replacementlen == 5 && *insnbuf == 0xe8) {
> > + /* direct call */
> > *(s32 *)(insnbuf + 1) += replacement - instr;
> > - DPRINTK("Fix CALL offset: 0x%x, CALL 0x%lx",
> > + DPRINTK("Fix direct CALL offset: 0x%x, CALL 0x%lx",
> > *(s32 *)(insnbuf + 1),
> > (unsigned long)instr + *(s32 *)(insnbuf + 1) + 5);
> > - }
> >
> > - if (a->replacementlen && is_jmp(replacement[0]))
> > + } else if (a->replacementlen == 6 && *insnbuf == 0xff &&
> > + *(insnbuf+1) == 0x15) {
> > + /* indirect call */
> > + *(s32 *)(insnbuf + 2) += replacement - instr;
> > + DPRINTK("Fix indirect CALL offset: 0x%x, CALL *0x%lx",
> > + *(s32 *)(insnbuf + 2),
> > + (unsigned long)instr + *(s32 *)(insnbuf + 2) + 6);
> > +
> > + } else if (a->replacementlen && is_jmp(replacement[0])) {
>
> Is this correct? Without your patch this was:
>
> if (*insnbuf == 0xe8) ...
> if (is_jmp(replacement[0])) ...
>
> Now you have:
>
> if (*insnbuf == 0xe8) ...
> else if (*insnbuf == 0xff15) ...
> else if (is_jmp(replacement[0])) ...
>
> So only one or none of the three variants will be executed. In the past
> it could be none, one or both.
It can't be a call *and* a jump. It's either one or the other.
Maybe it's a little confusing that the jump check uses replacement[0]
while the other checks use *insnbuf? They have the same value, so the
same variable should probably be used everywhere for consistency. I can
make them more consistent.
--
Josh
^ permalink raw reply
* [PATCH] virto_net: remove empty file 'virtio_net.'
From: Joel Stanley @ 2017-11-17 2:46 UTC (permalink / raw)
To: Jason Wang, David S . Miller
Cc: netdev, virtualization, linux-kernel, Michael S . Tsirkin
Looks like this was mistakenly added to the tree as part of
commit 186b3c998c50 ("virtio-net: support XDP_REDIRECT").
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
drivers/net/virtio_net. | 0
1 file changed, 0 insertions(+), 0 deletions(-)
delete mode 100644 drivers/net/virtio_net.
diff --git a/drivers/net/virtio_net. b/drivers/net/virtio_net.
deleted file mode 100644
index e69de29bb2d1..000000000000
--
2.14.1
^ permalink raw reply
* Re: [PATCH] virto_net: remove empty file 'virtio_net.'
From: Jason Wang @ 2017-11-17 2:55 UTC (permalink / raw)
To: Joel Stanley, David S . Miller
Cc: netdev, virtualization, linux-kernel, Michael S . Tsirkin
In-Reply-To: <20171117024636.15515-1-joel@jms.id.au>
On 2017年11月17日 10:46, Joel Stanley wrote:
> Looks like this was mistakenly added to the tree as part of
> commit 186b3c998c50 ("virtio-net: support XDP_REDIRECT").
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> drivers/net/virtio_net. | 0
> 1 file changed, 0 insertions(+), 0 deletions(-)
> delete mode 100644 drivers/net/virtio_net.
>
> diff --git a/drivers/net/virtio_net. b/drivers/net/virtio_net.
> deleted file mode 100644
> index e69de29bb2d1..000000000000
My bad, don't know what happens at that time.
This is for -net.
Acked-by: Jason Wang <jasowang@redhat.com>
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH] virto_net: remove empty file 'virtio_net.'
From: Michael S. Tsirkin @ 2017-11-17 4:15 UTC (permalink / raw)
To: Joel Stanley; +Cc: netdev, David S . Miller, linux-kernel, virtualization
In-Reply-To: <20171117024636.15515-1-joel@jms.id.au>
On Fri, Nov 17, 2017 at 01:16:36PM +1030, Joel Stanley wrote:
> Looks like this was mistakenly added to the tree as part of
> commit 186b3c998c50 ("virtio-net: support XDP_REDIRECT").
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/net/virtio_net. | 0
> 1 file changed, 0 insertions(+), 0 deletions(-)
> delete mode 100644 drivers/net/virtio_net.
>
> diff --git a/drivers/net/virtio_net. b/drivers/net/virtio_net.
> deleted file mode 100644
> index e69de29bb2d1..000000000000
> --
> 2.14.1
^ permalink raw reply
* Re: [PATCH] virto_net: remove empty file 'virtio_net.'
From: David Miller @ 2017-11-17 5:06 UTC (permalink / raw)
To: jasowang; +Cc: netdev, virtualization, joel, linux-kernel, mst
In-Reply-To: <05f54295-af35-6105-69fd-065e6cd6e19c@redhat.com>
From: Jason Wang <jasowang@redhat.com>
Date: Fri, 17 Nov 2017 10:55:44 +0800
>
>
> On 2017年11月17日 10:46, Joel Stanley wrote:
>> Looks like this was mistakenly added to the tree as part of
>> commit 186b3c998c50 ("virtio-net: support XDP_REDIRECT").
>>
>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>> ---
>> drivers/net/virtio_net. | 0
>> 1 file changed, 0 insertions(+), 0 deletions(-)
>> delete mode 100644 drivers/net/virtio_net.
>>
>> diff --git a/drivers/net/virtio_net. b/drivers/net/virtio_net.
>> deleted file mode 100644
>> index e69de29bb2d1..000000000000
>
> My bad, don't know what happens at that time.
>
> This is for -net.
>
> Acked-by: Jason Wang <jasowang@redhat.com>
Applied, thanks everyone.
^ permalink raw reply
* Re: [PATCH 10/13] x86/alternative: Support indirect call replacement
From: Juergen Gross @ 2017-11-17 5:46 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Boris Ostrovsky, Rusty Russell, Mike Galbraith, xen-devel,
Peter Zijlstra, Jiri Slaby, x86, linux-kernel, Sasha Levin,
Chris Wright, Thomas Gleixner, Andy Lutomirski, H. Peter Anvin,
Borislav Petkov, live-patching, Alok Kataria, virtualization,
Linus Torvalds, Ingo Molnar
In-Reply-To: <20171116211929.g4q5wa52sq64nhe5@treble>
On 16/11/17 22:19, Josh Poimboeuf wrote:
> On Wed, Oct 25, 2017 at 01:25:02PM +0200, Juergen Gross wrote:
>> On 04/10/17 17:58, Josh Poimboeuf wrote:
>>> Add alternative patching support for replacing an instruction with an
>>> indirect call. This will be needed for the paravirt alternatives.
>>>
>>> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
>>> ---
>>> arch/x86/kernel/alternative.c | 22 +++++++++++++++-------
>>> 1 file changed, 15 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
>>> index 3344d3382e91..81c577c7deba 100644
>>> --- a/arch/x86/kernel/alternative.c
>>> +++ b/arch/x86/kernel/alternative.c
>>> @@ -410,20 +410,28 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start,
>>> insnbuf_sz = a->replacementlen;
>>>
>>> /*
>>> - * 0xe8 is a relative jump; fix the offset.
>>> - *
>>> - * Instruction length is checked before the opcode to avoid
>>> - * accessing uninitialized bytes for zero-length replacements.
>>> + * Fix the address offsets for call and jump instructions which
>>> + * use PC-relative addressing.
>>> */
>>> if (a->replacementlen == 5 && *insnbuf == 0xe8) {
>>> + /* direct call */
>>> *(s32 *)(insnbuf + 1) += replacement - instr;
>>> - DPRINTK("Fix CALL offset: 0x%x, CALL 0x%lx",
>>> + DPRINTK("Fix direct CALL offset: 0x%x, CALL 0x%lx",
>>> *(s32 *)(insnbuf + 1),
>>> (unsigned long)instr + *(s32 *)(insnbuf + 1) + 5);
>>> - }
>>>
>>> - if (a->replacementlen && is_jmp(replacement[0]))
>>> + } else if (a->replacementlen == 6 && *insnbuf == 0xff &&
>>> + *(insnbuf+1) == 0x15) {
>>> + /* indirect call */
>>> + *(s32 *)(insnbuf + 2) += replacement - instr;
>>> + DPRINTK("Fix indirect CALL offset: 0x%x, CALL *0x%lx",
>>> + *(s32 *)(insnbuf + 2),
>>> + (unsigned long)instr + *(s32 *)(insnbuf + 2) + 6);
>>> +
>>> + } else if (a->replacementlen && is_jmp(replacement[0])) {
>>
>> Is this correct? Without your patch this was:
>>
>> if (*insnbuf == 0xe8) ...
>> if (is_jmp(replacement[0])) ...
>>
>> Now you have:
>>
>> if (*insnbuf == 0xe8) ...
>> else if (*insnbuf == 0xff15) ...
>> else if (is_jmp(replacement[0])) ...
>>
>> So only one or none of the three variants will be executed. In the past
>> it could be none, one or both.
>
> It can't be a call *and* a jump. It's either one or the other.
>
> Maybe it's a little confusing that the jump check uses replacement[0]
> while the other checks use *insnbuf? They have the same value, so the
Right, I was fooled by that.
> same variable should probably be used everywhere for consistency. I can
> make them more consistent.
>
I'd appreciate that. :-)
Juergen
^ permalink raw reply
* Re: [PATCH 02/13] x86/paravirt: Fix output constraint macro names
From: Juergen Gross @ 2017-11-17 6:55 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Boris Ostrovsky, Rusty Russell, Mike Galbraith, xen-devel,
Peter Zijlstra, Jiri Slaby, x86, linux-kernel, Sasha Levin,
Chris Wright, Thomas Gleixner, Andy Lutomirski, H. Peter Anvin,
Borislav Petkov, live-patching, Alok Kataria, virtualization,
Linus Torvalds, Ingo Molnar
In-Reply-To: <20171116205042.wyl6x2q5tjwmoyeg@treble>
On 16/11/17 21:50, Josh Poimboeuf wrote:
> On Wed, Oct 25, 2017 at 11:33:43AM +0200, Juergen Gross wrote:
>> On 04/10/17 17:58, Josh Poimboeuf wrote:
>>> Some of the paravirt '*_CLOBBERS' macros refer to output constraints
>>> instead of clobbers, which makes the code extra confusing. Rename the
>>> output constraint related macros to '*_OUTPUTS'.
>>>
>>> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
>>
>> I'm fine with the changes, but you might want to rename the "call_clbr"
>> parameter of ____PVOP_[V]CALL, too, e.g. to "outputs".
>
> Yeah, good catch.
>
>> You could then drop the "CALL_" from the macros, too.
>
> Hm, which macros are you referring to, and why?
Good question. I think I didn't take the *CALLEE* macros into account.
So please ignore this remark.
Juergen
^ 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-17 11:23 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.1711161048000.2191@nanos>
On 2017-11-16 17:53, Thomas Gleixner wrote:
> On Thu, 16 Nov 2017, Quan Xu wrote:
>> On 2017-11-16 06:03, Thomas Gleixner wrote:
>> --- 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
> That's just plain wrong. We don't want to see any of this PARAVIRT crap in
> anything outside the architecture/hypervisor interfacing code which really
> needs it.
>
> The problem can and must be solved at the generic level in the first place
> to gather the data which can be used to make such decisions.
>
> How that information is used might be either completely generic or requires
> system specific variants. But as long as we don't have any information at
> all we cannot discuss that.
>
> Please sit down and write up which data needs to be considered to make
> decisions about probabilistic polling. Then we need to compare and contrast
> that with the data which is necessary to make power/idle state decisions.
>
> I would be very surprised if this data would not overlap by at least 90%.
>
Peter, tglx
Thanks for your comments..
rethink of this patch set,
1. which data needs to considerd to make decisions about probabilistic
polling
I really need to write up which data needs to considerd to make
decisions about probabilistic polling. At last several months,
I always focused on the data _from idle to reschedule_, then to bypass
the idle loops. unfortunately, this makes me touch scheduler/idle/nohz
code inevitably.
with tglx's suggestion, the data which is necessary to make power/idle
state decisions, is the last idle state's residency time. IIUC this data
is duration from idle to wakeup, which maybe by reschedule irq or other irq.
I also test that the reschedule irq overlap by more than 90% (trace the
need_resched status after cpuidle_idle_call), when I run ctxsw/netperf for
one minute.
as the overlap, I think I can input the last idle state's residency time
to make decisions about probabilistic polling, as @dev->last_residency does.
it is much easier to get data.
2. do a HV specific idle driver (function)
so far, power management is not exposed to guest.. idle is simple for
KVM guest,
calling "sti" / "hlt"(cpuidle_idle_call() --> default_idle_call())..
thanks Xen guys, who has implemented the paravirt framework. I can
implement it
as easy as following:
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -465,6 +465,12 @@ static void __init
kvm_apf_trap_init(void)
update_intr_gate(X86_TRAP_PF, async_page_fault);
}
+static __cpuidle void kvm_safe_halt(void)
+{
+ /* 1. POLL, if need_resched() --> return */
+
+ asm volatile("sti; hlt": : :"memory"); /* 2. halt */
+
+ /* 3. get the last idle state's residency time */
+
+ /* 4. update poll duration based on last idle state's
residency time */
+}
+
void __init kvm_guest_init(void)
{
int i;
@@ -490,6 +496,8 @@ void __init kvm_guest_init(void)
if (kvmclock_vsyscall)
kvm_setup_vsyscall_timeinfo();
+ pv_irq_ops.safe_halt = kvm_safe_halt;
+
#ifdef CONFIG_SMP
then, I am no need to introduce a new pvops, and never modify
schedule/idle/nohz code again.
also I can narrow all of the code down in arch/x86/kernel/kvm.c.
If this is in the right direction, I will send a new patch set next week..
thanks,
Quan
Alibaba Cloud
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [virtio-dev] Re: [PATCH v17 6/6] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_VQ
From: Wei Wang @ 2017-11-17 11:35 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: aarcange, virtio-dev, cornelia.huck, kvm, mawilcox,
penguin-kernel, liliang.opensource, qemu-devel, willy,
linux-kernel, mhocko, linux-mm, pbonzini, amit.shah, quan.xu,
yang.zhang.wz, virtualization, mgorman, akpm
In-Reply-To: <5A0D923C.4020807@intel.com>
On 11/16/2017 09:27 PM, Wei Wang wrote:
> On 11/16/2017 04:32 AM, Michael S. Tsirkin wrote:
>> 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>
>>> ---
>>>
>>> +
>>> +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
>
> Thanks for the finding.
> Similar to this option: I'm thinking could we make
> walk_free_mem_block() simply return if that option is on?
> That is, at the beginning of the function:
> if (!page_poisoning_enabled())
> return;
>
Thought about it more, I think it would be better to put this logic to
virtio_balloon:
send_free_page_cmd_id(vb, &vb->start_cmd_id);
if (page_poisoning_enabled() &&
!IS_ENABLED(CONFIG_PAGE_POISONING_NO_SANITY))
walk_free_mem_block(vb, 0,
&virtio_balloon_send_free_pages);
send_free_page_cmd_id(vb, &vb->stop_cmd_id);
walk_free_mem_block() should be a more generic API, and this potential
page poisoning issue is specific to live migration which is only one use
case of this function, so I think it is better to handle it in the
special use case itself.
Best,
Wei
^ 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-17 11:36 UTC (permalink / raw)
To: Quan Xu
Cc: Yang Zhang, Len Brown, Daniel Lezcano, kvm, linux-doc,
Peter Zijlstra, 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: <564b8a6e-8ddd-4e3d-c670-10f1697e6c06@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3978 bytes --]
On Fri, 17 Nov 2017, Quan Xu wrote:
> On 2017-11-16 17:53, Thomas Gleixner wrote:
> > That's just plain wrong. We don't want to see any of this PARAVIRT crap in
> > anything outside the architecture/hypervisor interfacing code which really
> > needs it.
> >
> > The problem can and must be solved at the generic level in the first place
> > to gather the data which can be used to make such decisions.
> >
> > How that information is used might be either completely generic or requires
> > system specific variants. But as long as we don't have any information at
> > all we cannot discuss that.
> >
> > Please sit down and write up which data needs to be considered to make
> > decisions about probabilistic polling. Then we need to compare and contrast
> > that with the data which is necessary to make power/idle state decisions.
> >
> > I would be very surprised if this data would not overlap by at least 90%.
> >
> 1. which data needs to considerd to make decisions about probabilistic polling
>
> I really need to write up which data needs to considerd to make
> decisions about probabilistic polling. At last several months,
> I always focused on the data _from idle to reschedule_, then to bypass
> the idle loops. unfortunately, this makes me touch scheduler/idle/nohz
> code inevitably.
>
> with tglx's suggestion, the data which is necessary to make power/idle
> state decisions, is the last idle state's residency time. IIUC this data
> is duration from idle to wakeup, which maybe by reschedule irq or other irq.
That's part of the picture, but not complete.
> I also test that the reschedule irq overlap by more than 90% (trace the
> need_resched status after cpuidle_idle_call), when I run ctxsw/netperf for
> one minute.
>
> as the overlap, I think I can input the last idle state's residency time
> to make decisions about probabilistic polling, as @dev->last_residency does.
> it is much easier to get data.
That's only true for your particular use case.
>
> 2. do a HV specific idle driver (function)
>
> so far, power management is not exposed to guest.. idle is simple for KVM
> guest,
> calling "sti" / "hlt"(cpuidle_idle_call() --> default_idle_call())..
> thanks Xen guys, who has implemented the paravirt framework. I can implement
> it
> as easy as following:
>
> --- a/arch/x86/kernel/kvm.c
Your email client is using a very strange formatting.
> +++ b/arch/x86/kernel/kvm.c
> @@ -465,6 +465,12 @@ static void __init kvm_apf_trap_init(void)
> update_intr_gate(X86_TRAP_PF, async_page_fault);
> }
>
> +static __cpuidle void kvm_safe_halt(void)
> +{
> + /* 1. POLL, if need_resched() --> return */
> +
> + asm volatile("sti; hlt": : :"memory"); /* 2. halt */
> +
> + /* 3. get the last idle state's residency time */
> +
> + /* 4. update poll duration based on last idle state's
> residency time */
> +}
> +
> void __init kvm_guest_init(void)
> {
> int i;
> @@ -490,6 +496,8 @@ void __init kvm_guest_init(void)
> if (kvmclock_vsyscall)
> kvm_setup_vsyscall_timeinfo();
>
> + pv_irq_ops.safe_halt = kvm_safe_halt;
> +
> #ifdef CONFIG_SMP
>
>
> then, I am no need to introduce a new pvops, and never modify
> schedule/idle/nohz code again.
> also I can narrow all of the code down in arch/x86/kernel/kvm.c.
>
> If this is in the right direction, I will send a new patch set next week..
This is definitely better than what you proposed so far and implementing it
as a prove of concept seems to be worthwhile.
But I doubt that this is the final solution. It's not generic and not
necessarily suitable for all use case scenarios.
Thanks,
tglx
[-- 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: [virtio-dev] Re: [PATCH v17 6/6] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_VQ
From: Wei Wang @ 2017-11-17 11:48 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: aarcange, virtio-dev, cornelia.huck, kvm, mawilcox,
penguin-kernel, liliang.opensource, qemu-devel, willy,
linux-kernel, mhocko, linux-mm, pbonzini, amit.shah, quan.xu,
yang.zhang.wz, virtualization, mgorman, akpm
In-Reply-To: <5A0EC967.5090407@intel.com>
On 11/17/2017 07:35 PM, Wei Wang wrote:
> On 11/16/2017 09:27 PM, Wei Wang wrote:
>> On 11/16/2017 04:32 AM, Michael S. Tsirkin wrote:
>>> 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>
>>>> ---
>>>>
>>>> +
>>>> +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
>>
>> Thanks for the finding.
>> Similar to this option: I'm thinking could we make
>> walk_free_mem_block() simply return if that option is on?
>> That is, at the beginning of the function:
>> if (!page_poisoning_enabled())
>> return;
>>
>
>
> Thought about it more, I think it would be better to put this logic to
> virtio_balloon:
>
> send_free_page_cmd_id(vb, &vb->start_cmd_id);
> if (page_poisoning_enabled() &&
> !IS_ENABLED(CONFIG_PAGE_POISONING_NO_SANITY))
> walk_free_mem_block(vb, 0,
> &virtio_balloon_send_free_pages);
logic should be inverse:
if (!(page_poisoning_enabled() &&
!IS_ENABLED(CONFIG_PAGE_POISONING_NO_SANITY)))
Best,
Wei
^ 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-17 12:21 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Yang Zhang, Len Brown, Daniel Lezcano, kvm, linux-doc,
Peter Zijlstra, 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.1711171229100.7700@nanos>
On 2017-11-17 19:36, Thomas Gleixner wrote:
> On Fri, 17 Nov 2017, Quan Xu wrote:
>> On 2017-11-16 17:53, Thomas Gleixner wrote:
>>> That's just plain wrong. We don't want to see any of this PARAVIRT crap in
>>> anything outside the architecture/hypervisor interfacing code which really
>>> needs it.
>>>
>>> The problem can and must be solved at the generic level in the first place
>>> to gather the data which can be used to make such decisions.
>>>
>>> How that information is used might be either completely generic or requires
>>> system specific variants. But as long as we don't have any information at
>>> all we cannot discuss that.
>>>
>>> Please sit down and write up which data needs to be considered to make
>>> decisions about probabilistic polling. Then we need to compare and contrast
>>> that with the data which is necessary to make power/idle state decisions.
>>>
>>> I would be very surprised if this data would not overlap by at least 90%.
>>>
>> 1. which data needs to considerd to make decisions about probabilistic polling
>>
>> I really need to write up which data needs to considerd to make
>> decisions about probabilistic polling. At last several months,
>> I always focused on the data _from idle to reschedule_, then to bypass
>> the idle loops. unfortunately, this makes me touch scheduler/idle/nohz
>> code inevitably.
>>
>> with tglx's suggestion, the data which is necessary to make power/idle
>> state decisions, is the last idle state's residency time. IIUC this data
>> is duration from idle to wakeup, which maybe by reschedule irq or other irq.
> That's part of the picture, but not complete.
tglx, could you share more? I am very curious about it..
>> I also test that the reschedule irq overlap by more than 90% (trace the
>> need_resched status after cpuidle_idle_call), when I run ctxsw/netperf for
>> one minute.
>>
>> as the overlap, I think I can input the last idle state's residency time
>> to make decisions about probabilistic polling, as @dev->last_residency does.
>> it is much easier to get data.
> That's only true for your particular use case.
>
>> 2. do a HV specific idle driver (function)
>>
>> so far, power management is not exposed to guest.. idle is simple for KVM
>> guest,
>> calling "sti" / "hlt"(cpuidle_idle_call() --> default_idle_call())..
>> thanks Xen guys, who has implemented the paravirt framework. I can implement
>> it
>> as easy as following:
>>
>> --- a/arch/x86/kernel/kvm.c
> Your email client is using a very strange formatting.
my bad, I insert space to highlight these code.
> This is definitely better than what you proposed so far and implementing it
> as a prove of concept seems to be worthwhile.
>
> But I doubt that this is the final solution. It's not generic and not
> necessarily suitable for all use case scenarios.
>
>
yes, I am exhausted :):)
could you tell me the gap to be generic and necessarily suitable for
all use case scenarios? as lack of irq/idle predictors?
I really want to upstream it for all of public cloud users/providers..
as kvm host has a similar one, is it possible to upstream with following
conditions? :
1). add a QEMU configuration, whether enable or not, by default
disable.
2). add some "TODO" comments near the code.
3). ...
anyway, thanks for your help..
Quan
Alibaba Cloud
^ permalink raw reply
* Re: [virtio-dev] Re: [PATCH v17 6/6] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_VQ
From: Michael S. Tsirkin @ 2017-11-17 12:44 UTC (permalink / raw)
To: Wei Wang
Cc: aarcange, virtio-dev, cornelia.huck, kvm, mawilcox,
penguin-kernel, liliang.opensource, qemu-devel, willy,
linux-kernel, mhocko, linux-mm, pbonzini, amit.shah, quan.xu,
yang.zhang.wz, virtualization, mgorman, akpm
In-Reply-To: <5A0EC967.5090407@intel.com>
On Fri, Nov 17, 2017 at 07:35:03PM +0800, Wei Wang wrote:
> On 11/16/2017 09:27 PM, Wei Wang wrote:
> > On 11/16/2017 04:32 AM, Michael S. Tsirkin wrote:
> > > 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>
> > > > ---
> > > >
> > > > +
> > > > +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
> >
> > Thanks for the finding.
> > Similar to this option: I'm thinking could we make walk_free_mem_block()
> > simply return if that option is on?
> > That is, at the beginning of the function:
> > if (!page_poisoning_enabled())
> > return;
> >
>
>
> Thought about it more, I think it would be better to put this logic to
> virtio_balloon:
>
> send_free_page_cmd_id(vb, &vb->start_cmd_id);
> if (page_poisoning_enabled() &&
> !IS_ENABLED(CONFIG_PAGE_POISONING_NO_SANITY))
> walk_free_mem_block(vb, 0, &virtio_balloon_send_free_pages);
> send_free_page_cmd_id(vb, &vb->stop_cmd_id);
>
>
> walk_free_mem_block() should be a more generic API, and this potential page
> poisoning issue is specific to live migration which is only one use case of
> this function, so I think it is better to handle it in the special use case
> itself.
>
> Best,
> Wei
>
It's a quick work-around but it doesn't make me very happy.
AFAIK e.g. RHEL has a debug kernel with poisoning enabled.
If this never uses free page hinting at all, it will
be much less useful for debugging guests.
--
MST
^ permalink raw reply
* Re: [virtio-dev] Re: [PATCH v17 6/6] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_VQ
From: Michael S. Tsirkin @ 2017-11-17 13:18 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: <5A0D923C.4020807@intel.com>
On Thu, Nov 16, 2017 at 09:27:24PM +0800, Wei Wang wrote:
> On 11/16/2017 04:32 AM, Michael S. Tsirkin wrote:
> > 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>
> > > ---
> > >
> > > +
> > > +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
>
> Thanks for the finding.
> Similar to this option: I'm thinking could we make walk_free_mem_block()
> simply return if that option is on?
> That is, at the beginning of the function:
> if (!page_poisoning_enabled())
> return;
>
> I think in most usages, people would not choose to use the poisoning option
> due to the added overhead.
>
>
> Probably we could make it a separate fix patch of this report following
> patch 5 to explain the above reasons in the commit.
>
> > 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.
>
> I haven't got this point yet, how would it bring performance benefit?
Upon getting a free page, host could check that its content
matches the poison value. If it doesn't page has been used.
But let's ignore this for now.
> > It would be interesting to see whether this last has
> > any value performance-wise.
> >
>
> Best,
> Wei
^ permalink raw reply
* Re: [PATCH 01/13] x86/paravirt: remove wbinvd() paravirt interface
From: Borislav Petkov @ 2017-11-17 14:39 UTC (permalink / raw)
To: Josh Poimboeuf
Cc: Juergen Gross, Rusty Russell, Mike Galbraith, xen-devel,
Peter Zijlstra, Jiri Slaby, Boris Ostrovsky, x86, linux-kernel,
Sasha Levin, Chris Wright, Thomas Gleixner, Andy Lutomirski,
H. Peter Anvin, live-patching, Alok Kataria, virtualization,
Linus Torvalds, Ingo Molnar
In-Reply-To: <3f9bf17f6372ee92d984f6c7fd13e0cb14bc0e0a.1507128293.git.jpoimboe@redhat.com>
On Wed, Oct 04, 2017 at 10:58:22AM -0500, Josh Poimboeuf wrote:
> Since lguest was removed, only the native version of wbinvd() is used.
> The paravirt interface is no longer needed.
>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
> arch/x86/include/asm/paravirt.h | 5 -----
> arch/x86/include/asm/paravirt_types.h | 1 -
> arch/x86/include/asm/special_insns.h | 7 +------
> arch/x86/kernel/paravirt.c | 1 -
> arch/x86/kernel/paravirt_patch_64.c | 2 --
> arch/x86/xen/enlighten_pv.c | 2 --
> 6 files changed, 1 insertion(+), 17 deletions(-)
Reviewed-by: Borislav Petkov <bp@suse.de>
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ 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