[PATCH 6/10] Allow vcpu to pause self

[PATCH 6/10] Allow vcpu to pause self

[Tian, Kevin]

[-- Attachment #1: Type: text/plain, Size: 2367 bytes --]

Add self pause ability, which is required by vcpu0/dom0 when
running on a AP. This can't be satisfied by existing interface,
since the new flag also serves as a sync point.

Signed-off-by Kevin Tian <kevin.tian@intel.com>

diff -r d5315422dbc8 xen/common/domain.c
--- a/xen/common/domain.c	Mon May 14 18:35:31 2007 -0400
+++ b/xen/common/domain.c	Mon May 14 20:21:04 2007 -0400
@@ -530,6 +530,17 @@ void vcpu_pause_nosync(struct vcpu *v)
     vcpu_sleep_nosync(v);
 }
 
+/* _VPF_need_sync serves not only as flag for sync pause, but also
+ * as hint for other cpu waiting for this pause.
+ */
+void vcpu_pause_self(void)
+{
+    struct vcpu *v = current;
+
+    set_bit(_VPF_need_sync, &v->pause_flags);
+    vcpu_pause_nosync(v);
+}
+
 void vcpu_unpause(struct vcpu *v)
 {
     if ( atomic_dec_and_test(&v->pause_count) )
diff -r d5315422dbc8 xen/common/schedule.c
--- a/xen/common/schedule.c	Mon May 14 18:35:31 2007 -0400
+++ b/xen/common/schedule.c	Mon May 14 18:54:28 2007 -0400
@@ -691,6 +691,13 @@ void context_saved(struct vcpu *prev)
 
     if ( unlikely(test_bit(_VPF_migrating, &prev->pause_flags)) )
         vcpu_migrate(prev);
+
+    if ( unlikely(test_bit(_VPF_need_sync, &prev->pause_flags)) )
+    {
+        sync_vcpu_execstate(prev);
+        /* test and clear can be split, since here is the only clear
point */
+        clear_bit(_VPF_need_sync, &prev->pause_flags);
+    }
 }
 
 /* The scheduler timer: force a run through the scheduler */
diff -r d5315422dbc8 xen/include/xen/sched.h
--- a/xen/include/xen/sched.h	Mon May 14 18:35:31 2007 -0400
+++ b/xen/include/xen/sched.h	Mon May 14 20:21:30 2007 -0400
@@ -457,6 +457,9 @@ extern struct domain *domain_list;
  /* VCPU affinity has changed: migrating to a new CPU. */
 #define _VPF_migrating       3
 #define VPF_migrating        (1UL<<_VPF_migrating)
+ /* VCPU needs full context sync once switched out */
+#define _VPF_need_sync       4
+#define VPF_need_sync        (1UL<<_VPF_need_sync)
 
 static inline int vcpu_runnable(struct vcpu *v)
 {
@@ -467,6 +470,7 @@ static inline int vcpu_runnable(struct v
 
 void vcpu_pause(struct vcpu *v);
 void vcpu_pause_nosync(struct vcpu *v);
+void vcpu_pause_self(void);
 void domain_pause(struct domain *d);
 void vcpu_unpause(struct vcpu *v);
 void domain_unpause(struct domain *d);

[-- Attachment #2: vcpu_pause_self.patch --]
[-- Type: application/octet-stream, Size: 2366 bytes --]

Add self pause ability, which is required by vcpu0/dom0 when
running on a AP. This can't be satisfied by existing interface,
since the new flag also serves as a sync point.

Signed-off-by Kevin Tian <kevin.tian@intel.com>

diff -r d5315422dbc8 xen/common/domain.c
--- a/xen/common/domain.c	Mon May 14 18:35:31 2007 -0400
+++ b/xen/common/domain.c	Mon May 14 20:21:04 2007 -0400
@@ -530,6 +530,17 @@ void vcpu_pause_nosync(struct vcpu *v)
     vcpu_sleep_nosync(v);
 }
 
+/* _VPF_need_sync serves not only as flag for sync pause, but also
+ * as hint for other cpu waiting for this pause.
+ */
+void vcpu_pause_self(void)
+{
+    struct vcpu *v = current;
+
+    set_bit(_VPF_need_sync, &v->pause_flags);
+    vcpu_pause_nosync(v);
+}
+
 void vcpu_unpause(struct vcpu *v)
 {
     if ( atomic_dec_and_test(&v->pause_count) )
diff -r d5315422dbc8 xen/common/schedule.c
--- a/xen/common/schedule.c	Mon May 14 18:35:31 2007 -0400
+++ b/xen/common/schedule.c	Mon May 14 18:54:28 2007 -0400
@@ -691,6 +691,13 @@ void context_saved(struct vcpu *prev)
 
     if ( unlikely(test_bit(_VPF_migrating, &prev->pause_flags)) )
         vcpu_migrate(prev);
+
+    if ( unlikely(test_bit(_VPF_need_sync, &prev->pause_flags)) )
+    {
+        sync_vcpu_execstate(prev);
+        /* test and clear can be split, since here is the only clear point */
+        clear_bit(_VPF_need_sync, &prev->pause_flags);
+    }
 }
 
 /* The scheduler timer: force a run through the scheduler */
diff -r d5315422dbc8 xen/include/xen/sched.h
--- a/xen/include/xen/sched.h	Mon May 14 18:35:31 2007 -0400
+++ b/xen/include/xen/sched.h	Mon May 14 20:21:30 2007 -0400
@@ -457,6 +457,9 @@ extern struct domain *domain_list;
  /* VCPU affinity has changed: migrating to a new CPU. */
 #define _VPF_migrating       3
 #define VPF_migrating        (1UL<<_VPF_migrating)
+ /* VCPU needs full context sync once switched out */
+#define _VPF_need_sync       4
+#define VPF_need_sync        (1UL<<_VPF_need_sync)
 
 static inline int vcpu_runnable(struct vcpu *v)
 {
@@ -467,6 +470,7 @@ static inline int vcpu_runnable(struct v
 
 void vcpu_pause(struct vcpu *v);
 void vcpu_pause_nosync(struct vcpu *v);
+void vcpu_pause_self(void);
 void domain_pause(struct domain *d);
 void vcpu_unpause(struct vcpu *v);
 void domain_unpause(struct domain *d);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH 6/10] Allow vcpu to pause self

[Keir Fraser]

On 27/6/07 14:37, "Tian, Kevin" <kevin.tian@intel.com> wrote:

> Add self pause ability, which is required by vcpu0/dom0 when
> running on a AP. This can't be satisfied by existing interface,
> since the new flag also serves as a sync point.
> 
> Signed-off-by Kevin Tian <kevin.tian@intel.com>

I think this should not be needed. Why is dom0/vcpu0 special at all? If you
are doing the final work from a softirq context, can't dom0/vcpu0 simply be
paused like all others at that point? If not then we'll need to make some
arrangement using vcpu_set_affinity() - I won't add another flag on the
context-switch path.

So currently patches 6,7,9,10 are not applied. Patches 6 & 7 because they
need more iteration, as commented above. Patches 9 & 10 will likely change
when the platform_op hypercall interface is slimmed down, so I'm leaving
them out for now.

All other patches are in (although the platform_op interface part of patch 2
is disabled).

 -- Keir

RE: [PATCH 6/10] Allow vcpu to pause self

[Tian, Kevin]

>From: Keir Fraser [mailto:keir@xensource.com]
>Sent: 2007年7月12日 1:02
>
>On 27/6/07 14:37, "Tian, Kevin" <kevin.tian@intel.com> wrote:
>
>> Add self pause ability, which is required by vcpu0/dom0 when
>> running on a AP. This can't be satisfied by existing interface,
>> since the new flag also serves as a sync point.
>>
>> Signed-off-by Kevin Tian <kevin.tian@intel.com>
>
>I think this should not be needed. Why is dom0/vcpu0 special at all? If
>you
>are doing the final work from a softirq context, can't dom0/vcpu0 simply
>be
>paused like all others at that point? If not then we'll need to make some
>arrangement using vcpu_set_affinity() - I won't add another flag on the
>context-switch path.

I tried to recall the reason for adding this flag. The major reason is that 
sleep hypercall happens on dom0/vcpu0's context, while actual 
enter_state may happen in softirq on idle vcpu context. As a result, we 
need to update rax as return value to dom0/vcpu0 which means lazy 
state required flush into per-vcpu guest context before updating. 
However existing vcpu_pause doesn't work on self context and 
vcpu_pause_nosync leaves lazy state there. That's why a new flag is 
added to allow lazy context sync-ed after switching out.

But after a further thinking, based on the fact that enter_state will force
a lazy context flush on all CPUs now, this interface can be abandoned 
then.

>
>So currently patches 6,7,9,10 are not applied. Patches 6 & 7 because
>they
>need more iteration, as commented above. Patches 9 & 10 will likely
>change
>when the platform_op hypercall interface is slimmed down, so I'm
>leaving
>them out for now.

I'll resend later.

>
>All other patches are in (although the platform_op interface part of patch
>2
>is disabled).
>

Thanks so much,
Kevin

RE: [PATCH 6/10] Allow vcpu to pause self

[Tian, Kevin]

>From: Tian, Kevin
>Sent: 2007年7月12日 10:37
>>
>>I think this should not be needed. Why is dom0/vcpu0 special at all? If
>>you
>>are doing the final work from a softirq context, can't dom0/vcpu0 simply
>>be
>>paused like all others at that point? If not then we'll need to make some
>>arrangement using vcpu_set_affinity() - I won't add another flag on the
>>context-switch path.
>
>I tried to recall the reason for adding this flag. The major reason is that
>sleep hypercall happens on dom0/vcpu0's context, while actual
>enter_state may happen in softirq on idle vcpu context. As a result, we
>need to update rax as return value to dom0/vcpu0 which means lazy
>state required flush into per-vcpu guest context before updating.
>However existing vcpu_pause doesn't work on self context and
>vcpu_pause_nosync leaves lazy state there. That's why a new flag is
>added to allow lazy context sync-ed after switching out.
>
>But after a further thinking, based on the fact that enter_state will force
>a lazy context flush on all CPUs now, this interface can be abandoned
>then.
>

Seems issue still existing. It's possible that force lazy context flush 
in enter_state is done before dom0/vcpu0 enters context switch, 
since softirq is sent out before pause. How to find a safe point where 
we know that dom0/vcpu0 is definitely switched out?

Vcpu_set_affinity doesn't solve the problem, since migrated vcpu 
won't continue with previous flow. Or do you mean forcing user to set 
such affinity explicitly before requesting suspend?

Thanks,
Kevin

RE: [PATCH 6/10] Allow vcpu to pause self

[Tian, Kevin]

Oh, I can check is_running flag bit of dom0/vcpu0 as a sync point 
before requesting lazy context flush on all CPUs. :-)

Thanks,
Kevin

>From: Tian, Kevin
>Sent: 2007年7月12日 13:06
>
>>From: Tian, Kevin
>>Sent: 2007年7月12日 10:37
>>>
>>>I think this should not be needed. Why is dom0/vcpu0 special at all? If
>>>you
>>>are doing the final work from a softirq context, can't dom0/vcpu0
>simply
>>>be
>>>paused like all others at that point? If not then we'll need to make
>some
>>>arrangement using vcpu_set_affinity() - I won't add another flag on the
>>>context-switch path.
>>
>>I tried to recall the reason for adding this flag. The major reason is that
>>sleep hypercall happens on dom0/vcpu0's context, while actual
>>enter_state may happen in softirq on idle vcpu context. As a result, we
>>need to update rax as return value to dom0/vcpu0 which means lazy
>>state required flush into per-vcpu guest context before updating.
>>However existing vcpu_pause doesn't work on self context and
>>vcpu_pause_nosync leaves lazy state there. That's why a new flag is
>>added to allow lazy context sync-ed after switching out.
>>
>>But after a further thinking, based on the fact that enter_state will force
>>a lazy context flush on all CPUs now, this interface can be abandoned
>>then.
>>
>
>Seems issue still existing. It's possible that force lazy context flush
>in enter_state is done before dom0/vcpu0 enters context switch,
>since softirq is sent out before pause. How to find a safe point where
>we know that dom0/vcpu0 is definitely switched out?
>
>Vcpu_set_affinity doesn't solve the problem, since migrated vcpu
>won't continue with previous flow. Or do you mean forcing user to set
>such affinity explicitly before requesting suspend?
>
>Thanks,
>Kevin

Re: [PATCH 6/10] Allow vcpu to pause self

[Keir Fraser]

Oh, by the way, shouldn't the lazy context flush be driven via cpu
hot-unplug? That would seem the more general place to do the flush.

 -- Keir

On 12/7/07 07:02, "Tian, Kevin" <kevin.tian@intel.com> wrote:

> Oh, I can check is_running flag bit of dom0/vcpu0 as a sync point
> before requesting lazy context flush on all CPUs. :-)
> 
> Thanks,
> Kevin
> 
>> From: Tian, Kevin
>> Sent: 2007年7月12日 13:06
>> 
>>> From: Tian, Kevin
>>> Sent: 2007年7月12日 10:37
>>>> 
>>>> I think this should not be needed. Why is dom0/vcpu0 special at all? If
>>>> you
>>>> are doing the final work from a softirq context, can't dom0/vcpu0
>> simply
>>>> be
>>>> paused like all others at that point? If not then we'll need to make
>> some
>>>> arrangement using vcpu_set_affinity() - I won't add another flag on the
>>>> context-switch path.
>>> 
>>> I tried to recall the reason for adding this flag. The major reason is that
>>> sleep hypercall happens on dom0/vcpu0's context, while actual
>>> enter_state may happen in softirq on idle vcpu context. As a result, we
>>> need to update rax as return value to dom0/vcpu0 which means lazy
>>> state required flush into per-vcpu guest context before updating.
>>> However existing vcpu_pause doesn't work on self context and
>>> vcpu_pause_nosync leaves lazy state there. That's why a new flag is
>>> added to allow lazy context sync-ed after switching out.
>>> 
>>> But after a further thinking, based on the fact that enter_state will force
>>> a lazy context flush on all CPUs now, this interface can be abandoned
>>> then.
>>> 
>> 
>> Seems issue still existing. It's possible that force lazy context flush
>> in enter_state is done before dom0/vcpu0 enters context switch,
>> since softirq is sent out before pause. How to find a safe point where
>> we know that dom0/vcpu0 is definitely switched out?
>> 
>> Vcpu_set_affinity doesn't solve the problem, since migrated vcpu
>> won't continue with previous flow. Or do you mean forcing user to set
>> such affinity explicitly before requesting suspend?
>> 
>> Thanks,
>> Kevin
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH 6/10] Allow vcpu to pause self

[Keir Fraser]

On 12/7/07 06:05, "Tian, Kevin" <kevin.tian@intel.com> wrote:

>> But after a further thinking, based on the fact that enter_state will force
>> a lazy context flush on all CPUs now, this interface can be abandoned
>> then.
>> 
> 
> Seems issue still existing. It's possible that force lazy context flush
> in enter_state is done before dom0/vcpu0 enters context switch,
> since softirq is sent out before pause. How to find a safe point where
> we know that dom0/vcpu0 is definitely switched out?

How about doing the whole suspend/resume in dom0/vcpu0 context? Why switch
to a softirq at all? You can force dom0/vcpu0 onto cpu0 temporarily by
wrapping the suspend/resume in a pair of calls to vcpu_set_affinity().

If your register save/restore across the low-level S3 entry/exit is
comprehensive, then it should be fine to do it in dom0/vcpu0 context.

 -- Keir

Re: [PATCH 6/10] Allow vcpu to pause self

[Keir Fraser]

On 12/7/07 08:44, "Keir Fraser" <keir@xensource.com> wrote:

> How about doing the whole suspend/resume in dom0/vcpu0 context? Why switch
> to a softirq at all? You can force dom0/vcpu0 onto cpu0 temporarily by
> wrapping the suspend/resume in a pair of calls to vcpu_set_affinity().

You may need to temporarily rewrite ->schedule_tail as well, to regain
control after the scheduling softirq.

 -- Keir

RE: [PATCH 6/10] Allow vcpu to pause self

[Tian, Kevin]

>From: Keir Fraser [mailto:keir@xensource.com]
>Sent: 2007年7月12日 15:41
>
>Oh, by the way, shouldn't the lazy context flush be driven via cpu
>hot-unplug? That would seem the more general place to do the flush.
>
> -- Keir

Agree.

Thanks,
Kevin

RE: [PATCH 6/10] Allow vcpu to pause self

[Tian, Kevin]

>From: Keir Fraser [mailto:keir@xensource.com]
>Sent: 2007年7月12日 16:01
>
>On 12/7/07 08:44, "Keir Fraser" <keir@xensource.com> wrote:
>
>> How about doing the whole suspend/resume in dom0/vcpu0 context?
>Why switch
>> to a softirq at all? You can force dom0/vcpu0 onto cpu0 temporarily by
>> wrapping the suspend/resume in a pair of calls to vcpu_set_affinity().
>
>You may need to temporarily rewrite ->schedule_tail as well, to regain
>control after the scheduling softirq.
>
> -- Keir

Yes, schedule_tail has to be overridden with a special stub to continue 
with previous suspend flow. Maybe we can make it generic, for example, 
to create a new vcpu_migrate_and_continue, which basically:
	- save continue point to vcpu structure
	- override schedule_tail with a helper function
	- switch out
Then that helper function jumps to continue point on new processor. At
finish, reset stack and recover schedule_tail.

Is it sounds like the way?

Thanks,
Kevin

Re: [PATCH 6/10] Allow vcpu to pause self

[Keir Fraser]

On 12/7/07 09:23, "Tian, Kevin" <kevin.tian@intel.com> wrote:

> Yes, schedule_tail has to be overridden with a special stub to continue
> with previous suspend flow. Maybe we can make it generic, for example,
> to create a new vcpu_migrate_and_continue, which basically:
> - save continue point to vcpu structure
> - override schedule_tail with a helper function
> - switch out
> Then that helper function jumps to continue point on new processor. At
> finish, reset stack and recover schedule_tail.
> 
> Is it sounds like the way?

I don't mind whether you opn code this in acpi/power.c, or add a new
interface function. Whichever works out best.

 -- Keir