Re: [PATCH RFC] paravirt: cleanup lazy mode handling

Previous thread: RT scheduling: wakeup bug? by Mike Kravetz on Monday, October 1, 2007 - 6:15 pm. (6 messages)

Next thread: libata-add-human-readable-error-value-decoding-v3.patch (was: -mm merge plans for 2.6.24) by Robert Hancock on Monday, October 1, 2007 - 7:46 pm. (1 message)
To: Virtualization Mailing List <virtualization@...>
Cc: Linux Kernel Mailing List <linux-kernel@...>, Andi Kleen <ak@...>, Zachary Amsden <zach@...>, Rusty Russell <rusty@...>, Avi Kivity <avi@...>, Anthony Liguori <anthony@...>, Glauber de Oliveira Costa <glommer@...>, Nakajima, Jun <jun.nakajima@...>
Date: Monday, October 1, 2007 - 7:46 pm

Currently, the set_lazy_mode pv_op is overloaded with 5 functions:
1. enter lazy cpu mode
2. leave lazy cpu mode
3. enter lazy mmu mode
4. leave lazy mmu mode
5. flush pending batched operations

This complicates each paravirt backend, since it needs to deal with
all the possible state transitions, handling flushing, etc. In
particular, flushing is quite distinct from the other 4 functions, and
seems to just cause complication.

This patch removes the set_lazy_mode operation, and adds "enter" and
"leave" lazy mode operations on mmu_ops and cpu_ops. All the logic
associated with enter and leaving lazy states is now in common code
(basically BUG_ONs to make sure that no mode is current when entering
a lazy mode, and make sure that the mode is current when leaving).
Also, flush is handled in a common way, by simply leaving and
re-entering the lazy mode.

The result is that the Xen and VMI lazy mode implementations are much
simpler; as would lguest's be.

Signed-off-by: Jeremy Fitzhardinge <jeremy@xensource.com>
Cc: Andi Kleen <ak@suse.de>
Cc: Zach Amsden <zach@vmware.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Avi Kivity <avi@qumranet.com>
Cc: Anthony Liguory <aliguori@us.ibm.com>
Cc: "Glauber de Oliveira Costa" <glommer@gmail.com>
Cc: "Nakajima, Jun" <jun.nakajima@intel.com>

---
arch/i386/kernel/paravirt.c | 78 ++++++++++++++++++++++++++++++++++++++++---
arch/i386/kernel/vmi.c | 42 ++++++++++++-----------
arch/i386/xen/enlighten.c | 43 +++++++----------------
arch/i386/xen/mmu.c | 2 -
arch/i386/xen/multicalls.h | 2 -
arch/i386/xen/xen-ops.h | 7 ---
include/asm-i386/paravirt.h | 67 ++++++++++++++----------------------
7 files changed, 137 insertions(+), 104 deletions(-)

===================================================================
--- a/arch/i386/kernel/paravirt.c
+++ b/arch/i386/kernel/paravirt.c
@@ -265,6 +265,69 @@ int paravirt_disable_iospace(void)
}
...

To: Jeremy Fitzhardinge <jeremy@...>
Cc: Virtualization Mailing List <virtualization@...>, Linux Kernel Mailing List <linux-kernel@...>, Andi Kleen <ak@...>, Zachary Amsden <zach@...>, Rusty Russell <rusty@...>, Anthony Liguori <anthony@...>, Glauber de Oliveira Costa <glommer@...>, Nakajima, Jun <jun.nakajima@...>
Date: Tuesday, October 2, 2007 - 1:48 am

The code doesn't support having both lazy modes active at once. Maybe

This snippet looks like it wants to support concurrently active lazy modes.

--
Any sufficiently difficult bug is indistinguishable from a feature.

-

To: Avi Kivity <avi@...>
Cc: Virtualization Mailing List <virtualization@...>, Linux Kernel Mailing List <linux-kernel@...>, Andi Kleen <ak@...>, Zachary Amsden <zach@...>, Rusty Russell <rusty@...>, Anthony Liguori <anthony@...>, Glauber de Oliveira Costa <glommer@...>, Nakajima, Jun <jun.nakajima@...>
Date: Tuesday, October 2, 2007 - 2:24 am

Hm, well, that's a good question. The initial semantics of the lazy
mode calls were "what VMI wants", and they're still not really nailed
down. VMI doesn't support having both active at once, and its a little
unclear what it would mean anyway. For now, we don't support multiple
lazy modes active at once, and the kernel never tries to do it (in fact,
it would be a bug, since lazy mmu must be preempt disabled, and lazy cpu

Yeah, its a little more general than it needs to be.

J

-

To: Jeremy Fitzhardinge <jeremy@...>
Cc: Virtualization Mailing List <virtualization@...>, Linux Kernel Mailing List <linux-kernel@...>, Andi Kleen <ak@...>, Zachary Amsden <zach@...>, Avi Kivity <avi@...>, Anthony Liguori <anthony@...>, Glauber de Oliveira Costa <glommer@...>, Nakajima, Jun <jun.nakajima@...>
Date: Monday, October 1, 2007 - 9:34 pm

That's good, but this code does lose on native because we no longer
simply replace the entire thing with noops.

Perhaps inverting this and having (inline) helpers is the way to go?
I'm thinking something like:

static inline void paravirt_enter_lazy(enum paravirt_lazy_mode mode)
{
BUG_ON(x86_read_percpu(paravirt_lazy_mode) != PARAVIRT_LAZY_NONE);
BUG_ON(preemptible());

x86_write_percpu(paravirt_lazy_mode, mode);
}

static inline void paravirt_exit_lazy(enum paravirt_lazy_mode mode)
{
BUG_ON(x86_read_percpu(paravirt_lazy_mode) != mode);
BUG_ON(preemptible());

x86_write_percpu(paravirt_lazy_mode, PARAVIRT_LAZY_NONE);
}

The only trick would be that the flushes are so rarely required it's
probably worth putting the unlikely() in the top level:

static void arch_flush_lazy_cpu_mode(void)
{
if (unlikely(x86_read_percpu(paravirt_lazy_mode)) {
PVOP_VCALL0(cpu_ops.enter_lazy);
PVOP_VCALL0(cpu_ops.exit_lazy);
}
}

static void arch_flush_lazy_mmy_mode(void)
{
if (unlikely(x86_read_percpu(paravirt_lazy_mode)) {
PVOP_VCALL0(mmu_ops.enter_lazy);
PVOP_VCALL0(mmu_ops.exit_lazy);
}
}

Thoughts?
Rusty.

-

To: Rusty Russell <rusty@...>
Cc: Virtualization Mailing List <virtualization@...>, Linux Kernel Mailing List <linux-kernel@...>, Andi Kleen <ak@...>, Zachary Amsden <zach@...>, Avi Kivity <avi@...>, Anthony Liguori <anthony@...>, Glauber de Oliveira Costa <glommer@...>, Nakajima, Jun <jun.nakajima@...>
Date: Tuesday, October 2, 2007 - 2:29 am

I'm thinking that the overhead will be unmeasurably small, and its not
really worth any more complexity. That's almost certainly true for lazy
mmu mode, but lazy cpu is used in the middle of a context switch, so

Er, they should probably call something to make the switch actually

Sure, I guess. Would it make any difference? (I've never personally
noticed likely/unlikely change the generated code in any seriously
positive way.)

J
-

To: Jeremy Fitzhardinge <jeremy@...>
Cc: Virtualization Mailing List <virtualization@...>, Linux Kernel Mailing List <linux-kernel@...>, Andi Kleen <ak@...>, Zachary Amsden <zach@...>, Avi Kivity <avi@...>, Anthony Liguori <anthony@...>, Glauber de Oliveira Costa <glommer@...>, Nakajima, Jun <jun.nakajima@...>
Date: Tuesday, October 2, 2007 - 3:53 am

No, they're helpers. eg:

static void lguest_exit_lazy(enum paravirt_lazy_mode mode)
{
paravirt_exit_lazy(mode);
lguest_flush_hcalls();

Probably overkill (I was trying to avoid the branch for the case where
we don't need to flush, as that's always what happens).

So just expose a flush hook:

static inline void arch_flush_lazy_cpu_mode(void)
{
PVOP_VCALL1(flush_lazy_mode, PARAVIRT_LAZY_CPU);
}

....

static void lguest_flush_lazy_mode(enum paravirt_lazy_mode mode)
{
if (unlikely(x86_read_percpu(paravirt_lazy_mode) == mode)) {
lguest_lazy_cpu_leave();
lguest_lazy_cpu_enter();
}
}

Cheers,
Rusty.

-

To: Rusty Russell <rusty@...>
Cc: Virtualization Mailing List <virtualization@...>, Linux Kernel Mailing List <linux-kernel@...>, Andi Kleen <ak@...>, Zachary Amsden <zach@...>, Avi Kivity <avi@...>, Anthony Liguori <anthony@...>, Glauber de Oliveira Costa <glommer@...>, Nakajima, Jun <jun.nakajima@...>
Date: Tuesday, October 2, 2007 - 6:43 pm

OK, how does this sit with you?

Subject: paravirt: clean up lazy mode handling

Currently, the set_lazy_mode pv_op is overloaded with 5 functions:
1. enter lazy cpu mode
2. leave lazy cpu mode
3. enter lazy mmu mode
4. leave lazy mmu mode
5. flush pending batched operations

This complicates each paravirt backend, since it needs to deal with
all the possible state transitions, handling flushing, etc. In
particular, flushing is quite distinct from the other 4 functions, and
seems to just cause complication.

This patch removes the set_lazy_mode operation, and adds "enter" and
"leave" lazy mode operations on mmu_ops and cpu_ops. All the logic
associated with enter and leaving lazy states is now in common code
(basically BUG_ONs to make sure that no mode is current when entering
a lazy mode, and make sure that the mode is current when leaving).
Also, flush is handled in a common way, by simply leaving and
re-entering the lazy mode.

The result is that the Xen and VMI lazy mode implementations are much
simpler; as would lguest's be.

Signed-off-by: Jeremy Fitzhardinge <jeremy@xensource.com>
Cc: Andi Kleen <ak@suse.de>
Cc: Zach Amsden <zach@vmware.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Avi Kivity <avi@qumranet.com>
Cc: Anthony Liguory <aliguori@us.ibm.com>
Cc: "Glauber de Oliveira Costa" <glommer@gmail.com>
Cc: "Nakajima, Jun" <jun.nakajima@intel.com>

---
arch/i386/kernel/paravirt.c | 58 +++++++++++++++++++++++++++++++++++++++----
arch/i386/kernel/vmi.c | 45 +++++++++++++++++++--------------
arch/i386/xen/enlighten.c | 44 ++++++++++----------------------
arch/i386/xen/mmu.c | 2 -
arch/i386/xen/multicalls.h | 2 -
arch/i386/xen/xen-ops.h | 7 -----
include/asm-i386/paravirt.h | 52 ++++++++++++++++++++++++--------------
7 files changed, 128 insertions(+), 82 deletions(-)

===================================================================
--- a/arch/i386/kernel/p...

Previous thread: RT scheduling: wakeup bug? by Mike Kravetz on Monday, October 1, 2007 - 6:15 pm. (6 messages)

Next thread: libata-add-human-readable-error-value-decoding-v3.patch (was: -mm merge plans for 2.6.24) by Robert Hancock on Monday, October 1, 2007 - 7:46 pm. (1 message)