paravirt: cleanup lazy mode handling

RFC PATCH In review Jeremy Fitzhardinge 3 participants 7 messages Oct 1, 2007 – Oct 2, 2007

Subsystems (2)

virtio core PARAVIRT_OPS interface

lore

Share & tools

lore

Jeremy Fitzhardinge

author

2007-10-01 23:46 UTC #0 lore raw

RFC PATCH 1/1 paravirt: cleanup 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 <[email protected]>

Cc Andi Kleen <[email protected]>

Cc Zach Amsden <[email protected]>

Cc Rusty Russell <[email protected]>

Cc Avi Kivity <[email protected]>

Cc Anthony Liguory <[email protected]>

Cc "Glauber de Oliveira Costa" <[email protected]>

Cc "Nakajima, Jun" <[email protected]>

---
 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(-)

===================================================================

arch/i386/kernel/paravirt.c +73 -5

@@ -265,6 +265,69 @@ int paravirt_disable_iospace(void)
 	}
 
 	return ret;
+}
+
+static DEFINE_PER_CPU(enum paravirt_lazy_mode, paravirt_lazy_mode) = PARAVIRT_LAZY_NONE;
+
+static void enter_lazy(enum paravirt_lazy_mode mode, struct pv_lazy_ops *ops)
+{
+	BUG_ON(x86_read_percpu(paravirt_lazy_mode) != PARAVIRT_LAZY_NONE);
+	BUG_ON(preemptible());
+
+	x86_write_percpu(paravirt_lazy_mode, mode);
+	(*ops->enter)();
+}
+
+static void leave_lazy(enum paravirt_lazy_mode mode, struct pv_lazy_ops *ops)
+{
+	BUG_ON(x86_read_percpu(paravirt_lazy_mode) != mode);
+	BUG_ON(preemptible());
+
+	x86_write_percpu(paravirt_lazy_mode, PARAVIRT_LAZY_NONE);
+	(*ops->leave)();
+}
+
+static void flush_lazy(enum paravirt_lazy_mode mode, struct pv_lazy_ops *ops)
+{
+	if (x86_read_percpu(paravirt_lazy_mode) == mode) {
+		(*ops->leave)();
+		(*ops->enter)();
+	}
+}
+
+void arch_enter_lazy_mmu_mode(void)
+{
+	enter_lazy(PARAVIRT_LAZY_MMU, &pv_mmu_ops.lazy_mode);
+}
+
+void arch_leave_lazy_mmu_mode(void)
+{
+	leave_lazy(PARAVIRT_LAZY_MMU, &pv_mmu_ops.lazy_mode);
+}
+
+void arch_flush_lazy_mmu_mode(void)
+{
+	flush_lazy(PARAVIRT_LAZY_MMU, &pv_mmu_ops.lazy_mode);
+}
+
+void arch_enter_lazy_cpu_mode(void)
+{
+	enter_lazy(PARAVIRT_LAZY_CPU, &pv_cpu_ops.lazy_mode);
+}
+
+void arch_leave_lazy_cpu_mode(void)
+{
+	leave_lazy(PARAVIRT_LAZY_CPU, &pv_cpu_ops.lazy_mode);
+}
+
+void arch_flush_lazy_cpu_mode(void)
+{
+	flush_lazy(PARAVIRT_LAZY_CPU, &pv_cpu_ops.lazy_mode);
+}
+
+enum paravirt_lazy_mode paravirt_get_lazy_mode(void)
+{
+	return x86_read_percpu(paravirt_lazy_mode);
 }
 
 struct paravirt_ops paravirt_ops = {

@@ -333,6 +396,11 @@ struct paravirt_ops paravirt_ops = {
 
 		 .set_iopl_mask = native_set_iopl_mask,
 		 .io_delay = native_io_delay,
+
+		 .lazy_mode = {
+			 .enter = paravirt_nop,
+			 .leave = paravirt_nop,
+		 },
 	 },
 
 	.pv_apic_ops = {

@@ -346,10 +414,6 @@ struct paravirt_ops paravirt_ops = {
 #endif
 	 },
 
-	.pv_misc_ops = {
-		 .set_lazy_mode = paravirt_nop,
-	 },
-
 	.pv_mmu_ops = {
 		 .pagetable_setup_start = native_pagetable_setup_start,
 		 .pagetable_setup_done = native_pagetable_setup_done,

@@ -400,6 +464,11 @@ struct paravirt_ops paravirt_ops = {
 		 .dup_mmap = paravirt_nop,
 		 .exit_mmap = paravirt_nop,
 		 .activate_mm = paravirt_nop,
+
+		 .lazy_mode = {
+			 .enter = paravirt_nop,
+			 .leave = paravirt_nop,
+		 },
 	 },
 };

@@ -417,7 +486,6 @@ static void __init __used pv_aliases(voi
 
 	substructure(pv_info);
 	substructure(pv_init_ops);
-	substructure(pv_misc_ops);
 	substructure(pv_time_ops);
 	substructure(pv_cpu_ops);
 	substructure(pv_irq_ops);

===================================================================

arch/i386/kernel/vmi.c +23 ~~-19~~

54 lines not on this page · raw message

===================================================================

arch/i386/xen/enlighten.c +13 ~~-30~~

80 lines not on this page · raw message

===================================================================

arch/i386/xen/mmu.c +1 -1

@@ -154,7 +154,7 @@ void xen_set_pte_at(struct mm_struct *mm
 		    pte_t *ptep, pte_t pteval)
 {
 	if (mm == current->mm || mm == &init_mm) {
-		if (xen_get_lazy_mode() == PARAVIRT_LAZY_MMU) {
+		if (paravirt_get_lazy_mode() == PARAVIRT_LAZY_MMU) {
 			struct multicall_space mcs;
 			mcs = xen_mc_entry(0);

===================================================================

arch/i386/xen/multicalls.h +1 -1

@@ -35,7 +35,7 @@ void xen_mc_flush(void);
 /* Issue a multicall if we're not in a lazy mode */
 static inline void xen_mc_issue(unsigned mode)
 {
-	if ((xen_get_lazy_mode() & mode) == 0)
+	if ((paravirt_get_lazy_mode() & mode) == 0)
 		xen_mc_flush();
 
 	/* restore flags saved in xen_mc_batch */

===================================================================

arch/i386/xen/xen-ops.h +0 -7

@@ -31,13 +31,6 @@ bool xen_vcpu_stolen(int vcpu);
 
 void xen_mark_init_mm_pinned(void);
 
-DECLARE_PER_CPU(enum paravirt_lazy_mode, xen_lazy_mode);
-
-static inline unsigned xen_get_lazy_mode(void)
-{
-	return x86_read_percpu(xen_lazy_mode);
-}
-
 void __init xen_fill_possible_map(void);
 
 void __init xen_setup_vcpu_info_placement(void);

===================================================================

include/asm-i386/paravirt.h +26 ~~-41~~

113 lines not on this page · raw message

Rusty Russell

2007-10-02 01:34 UTC #1 lore raw

Quoted 7 lines · Jeremy Fitzhardinge

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.

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.

Jeremy Fitzhardinge

author

2007-10-02 06:29 UTC #2 lore raw

Quoted 5 lines · Rusty Russell

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 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 it's probably worth a bit more attention.

Quoted 10 lines

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);
}

Quoted 8 lines

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);
}

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

Quoted 3 lines

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

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

Rusty Russell

2007-10-02 07:53 UTC #3 lore raw

Quoted 19 lines · Jeremy Fitzhardinge

Quoted 18 lines · Rusty Russell
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?
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());

Quoted 6 lines

Quoted 3 lines
	x86_write_percpu(paravirt_lazy_mode, PARAVIRT_LAZY_NONE);
}
Er, they should probably call something to make the switch actually happen, no?

No, they're helpers. eg:

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

Quoted 6 lines

Quoted 2 lines
The only trick would be that the flushes are so rarely required it's probably worth putting the unlikely() in the top level:
Sure, I guess. Would it make any difference? (I've never personally noticed likely/unlikely change the generated code in any seriously positive way.)

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.

Jeremy Fitzhardinge

author

2007-10-02 22:43 UTC #4 lore raw

Quoted 8 lines · Rusty Russell

No, they're helpers. eg:
static void lguest_exit_lazy(enum paravirt_lazy_mode mode) {
paravirt_exit_lazy(mode);
lguest_flush_hcalls();
}

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

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

Signed-off-by Jeremy Fitzhardinge <[email protected]>

Cc Andi Kleen <[email protected]>

Cc Zach Amsden <[email protected]>

Cc Rusty Russell <[email protected]>

Cc Avi Kivity <[email protected]>

Cc Anthony Liguory <[email protected]>

Cc "Glauber de Oliveira Costa" <[email protected]>

Cc "Nakajima, Jun" <[email protected]>

---
 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(-)

===================================================================

arch/i386/kernel/paravirt.c +53 -5

@@ -265,6 +265,49 @@ int paravirt_disable_iospace(void)
 	}
 
 	return ret;
+}
+
+static DEFINE_PER_CPU(enum paravirt_lazy_mode, paravirt_lazy_mode) = PARAVIRT_LAZY_NONE;
+
+static inline void 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);
+}
+
+void paravirt_leave_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);
+}
+
+void paravirt_enter_lazy_mmu(void)
+{
+	enter_lazy(PARAVIRT_LAZY_MMU);
+}
+
+void paravirt_leave_lazy_mmu(void)
+{
+	paravirt_leave_lazy(PARAVIRT_LAZY_MMU);
+}
+
+void paravirt_enter_lazy_cpu(void)
+{
+	enter_lazy(PARAVIRT_LAZY_CPU);
+}
+
+void paravirt_leave_lazy_cpu(void)
+{
+	paravirt_leave_lazy(PARAVIRT_LAZY_CPU);
+}
+
+enum paravirt_lazy_mode paravirt_get_lazy_mode(void)
+{
+	return x86_read_percpu(paravirt_lazy_mode);
 }
 
 struct paravirt_ops paravirt_ops = {

@@ -333,6 +376,11 @@ struct paravirt_ops paravirt_ops = {
 
 		 .set_iopl_mask = native_set_iopl_mask,
 		 .io_delay = native_io_delay,
+
+		 .lazy_mode = {
+			 .enter = paravirt_nop,
+			 .leave = paravirt_nop,
+		 },
 	 },
 
 	.pv_apic_ops = {

@@ -346,10 +394,6 @@ struct paravirt_ops paravirt_ops = {
 #endif
 	 },
 
-	.pv_misc_ops = {
-		 .set_lazy_mode = paravirt_nop,
-	 },
-
 	.pv_mmu_ops = {
 		 .pagetable_setup_start = native_pagetable_setup_start,
 		 .pagetable_setup_done = native_pagetable_setup_done,

@@ -400,6 +444,11 @@ struct paravirt_ops paravirt_ops = {
 		 .dup_mmap = paravirt_nop,
 		 .exit_mmap = paravirt_nop,
 		 .activate_mm = paravirt_nop,
+
+		 .lazy_mode = {
+			 .enter = paravirt_nop,
+			 .leave = paravirt_nop,
+		 },
 	 },
 };

@@ -417,7 +466,6 @@ static void __init __used pv_aliases(voi
 
 	substructure(pv_info);
 	substructure(pv_init_ops);
-	substructure(pv_misc_ops);
 	substructure(pv_time_ops);
 	substructure(pv_cpu_ops);
 	substructure(pv_irq_ops);

===================================================================

arch/i386/kernel/vmi.c +26 ~~-19~~

57 lines not on this page · raw message

===================================================================

arch/i386/xen/enlighten.c +14 ~~-30~~

81 lines not on this page · raw message

===================================================================

arch/i386/xen/mmu.c +1 -1

@@ -154,7 +154,7 @@ void xen_set_pte_at(struct mm_struct *mm
 		    pte_t *ptep, pte_t pteval)
 {
 	if (mm == current->mm || mm == &init_mm) {
-		if (xen_get_lazy_mode() == PARAVIRT_LAZY_MMU) {
+		if (paravirt_get_lazy_mode() == PARAVIRT_LAZY_MMU) {
 			struct multicall_space mcs;
 			mcs = xen_mc_entry(0);

===================================================================

arch/i386/xen/multicalls.h +1 -1

@@ -35,7 +35,7 @@ void xen_mc_flush(void);
 /* Issue a multicall if we're not in a lazy mode */
 static inline void xen_mc_issue(unsigned mode)
 {
-	if ((xen_get_lazy_mode() & mode) == 0)
+	if ((paravirt_get_lazy_mode() & mode) == 0)
 		xen_mc_flush();
 
 	/* restore flags saved in xen_mc_batch */

===================================================================

arch/i386/xen/xen-ops.h +0 -7

@@ -31,13 +31,6 @@ bool xen_vcpu_stolen(int vcpu);
 
 void xen_mark_init_mm_pinned(void);
 
-DECLARE_PER_CPU(enum paravirt_lazy_mode, xen_lazy_mode);
-
-static inline unsigned xen_get_lazy_mode(void)
-{
-	return x86_read_percpu(xen_lazy_mode);
-}
-
 void __init xen_fill_possible_map(void);
 
 void __init xen_setup_vcpu_info_placement(void);

===================================================================

include/asm-i386/paravirt.h +33 ~~-19~~

120 lines not on this page · raw message

Avi Kivity

2007-10-02 05:48 UTC #5 lore raw

In reply to Jeremy Fitzhardinge

Quoted 20 lines · Jeremy Fitzhardinge

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.

Quoted 4 lines

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

The code doesn't support having both lazy modes active at once. Maybe that's not an issue, but aren't the two modes orthogonal?

arch/i386/xen/multicalls.h void xen_mc_flush(void); · line 39

Quoted 9 lines

arch/i386/xen/multicalls.h +1 -1

@@ -35,7 +35,7 @@ void xen_mc_flush(void);
 /* Issue a multicall if we're not in a lazy mode */
 static inline void xen_mc_issue(unsigned mode)
 {
-	if ((xen_get_lazy_mode() & mode) == 0)
+	if ((paravirt_get_lazy_mode() & mode) == 0)
 		xen_mc_flush();

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

Signature · 1 line

Any sufficiently difficult bug is indistinguishable from a feature.

Jeremy Fitzhardinge

author

2007-10-02 06:24 UTC #6 lore raw

Quoted 2 lines · Avi Kivity

The code doesn't support having both lazy modes active at once. Maybe that's not an issue, but aren't the two modes orthogonal?

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 is only used for context switches).

Quoted 13 lines

arch/i386/xen/multicalls.h void xen_mc_flush(void); · line 39
Quoted 10 lines
arch/i386/xen/multicalls.h +1 -1
@@ -35,7 +35,7 @@ void xen_mc_flush(void);
 /* Issue a multicall if we're not in a lazy mode */
 static inline void xen_mc_issue(unsigned mode)
 {
-    if ((xen_get_lazy_mode() & mode) == 0)
+    if ((paravirt_get_lazy_mode() & mode) == 0)
         xen_mc_flush();
This snippet looks like it wants to support concurrently active lazy modes.

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