Re: [Xen-devel] [PATCH v2 2/3] xen: add hypercall option to temporarily pin a vcpu

[Juergen Gross]

On 02/03/16 18:21, Anshul Makkar wrote:
> Hi,
> 
> 
> -Original Message-
> From: Xen-devel [mailto:xen-devel-boun...@lists.xen.org] On Behalf Of George 
> Dunlap
> Sent: 01 March 2016 15:53
> To: Juergen Gross <jgr...@suse.com>; xen-devel@lists.xen.org
> Cc: Wei Liu <wei.l...@citrix.com>; Stefano Stabellini 
> <stefano.stabell...@citrix.com>; George Dunlap <george.dun...@citrix.com>; 
> Andrew Cooper <andrew.coop...@citrix.com>; Dario Faggioli 
> <dario.faggi...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; David 
> Vrabel <david.vra...@citrix.com>; jbeul...@suse.com
> Subject: Re: [Xen-devel] [PATCH v2 2/3] xen: add hypercall option to 
> temporarily pin a vcpu
> 
> On 01/03/16 09:02, Juergen Gross wrote:
>> Some hardware (e.g. Dell studio 1555 laptops) require SMIs to be 
>> called on physical cpu 0 only. Linux drivers like dcdbas or i8k try to 
>> achieve this by pinning the running thread to cpu 0, but in Dom0 this 
>> is not enough: the vcpu must be pinned to physical cpu 0 via Xen, too.
>>
>> Add a stable hypercall option SCHEDOP_pin_temp to the sched_op 
>> hypercall to achieve this. It is taking a physical cpu number as 
>> parameter. If pinning is possible (the calling domain has the 
>> privilege to make the call and the cpu is available in the domain's
>> cpupool) the calling vcpu is pinned to the specified cpu. The old cpu 
>> affinity is saved. To undo the temporary pinning a cpu -1 is 
>> specified. This will restore the original cpu affinity for the vcpu.
>>
>> Signed-off-by: Juergen Gross <jgr...@suse.com>
>> ---
>> V2: - limit operation to hardware domain as suggested by Jan Beulich
>> - some style issues corrected as requested by Jan Beulich
>> - use fixed width types in interface as requested by Jan Beulich
>> - add compat layer checking as requested by Jan Beulich
>> ---
>>  xen/common/compat/schedule.c |  4 ++
>>  xen/common/schedule.c| 92 
>> +---
>>  xen/include/public/sched.h   | 17 
>>  xen/include/xlat.lst |  1 +
>>  4 files changed, 109 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/common/compat/schedule.c 
>> b/xen/common/compat/schedule.c index 812c550..73b0f01 100644
>> --- a/xen/common/compat/schedule.c
>> +++ b/xen/common/compat/schedule.c
>> @@ -10,6 +10,10 @@
>>  
>>  #define do_sched_op compat_sched_op
>>  
>> +#define xen_sched_pin_temp sched_pin_temp CHECK_sched_pin_temp; 
>> +#undef xen_sched_pin_temp
>> +
>>  #define xen_sched_shutdown sched_shutdown  CHECK_sched_shutdown;  
>> #undef xen_sched_shutdown diff --git a/xen/common/schedule.c 
>> b/xen/common/schedule.c index b0d4b18..653f852 100644
>> --- a/xen/common/schedule.c
>> +++ b/xen/common/schedule.c
>> @@ -271,6 +271,12 @@ int sched_move_domain(struct domain *d, struct cpupool 
>> *c)
>>  struct scheduler *old_ops;
>>  void *old_domdata;
>>  
>> +for_each_vcpu ( d, v )
>> +{
>> +if ( v->affinity_broken )
>> +return -EBUSY;
>> +}
>> +
>>  domdata = SCHED_OP(c->sched, alloc_domdata, d);
>>  if ( domdata == NULL )
>>  return -ENOMEM;
>> @@ -669,6 +675,14 @@ int cpu_disable_scheduler(unsigned int cpu)
>>  if ( cpumask_empty(_affinity) &&
>>   cpumask_test_cpu(cpu, v->cpu_hard_affinity) )
>>  {
>> +if ( v->affinity_broken )
>> +{
>> +/* The vcpu is temporarily pinned, can't move it. */
>> +vcpu_schedule_unlock_irqrestore(lock, flags, v);
>> +ret = -EBUSY;
>> +break;
>> +}
> 
> Does this mean that if the user closes the laptop lid while one of these 
> drivers has vcpu0 pinned, that Xen will crash (see 
> xen/arch/x86/smpboot.c:__cpu_disable())?  Or is it the OS's job to make sure 
> that all temporary pins are removed before suspending?
> 
> Also -- have you actually tested the "cpupool move while pinned"
> functionality to make sure it actually works?  There's a weird bit in
> cpupool_unassign_cpu_helper() where after calling cpu_disable_scheduler(cpu), 
> it unconditionally sets the cpu bit in the cpupool_free_cpus mask, even if it 
> returns an error.  That can't be right, even for the existing -EAGAIN case, 
> can it?
> 
> I see that you have a loop to retry this call several times in the next 
> patch; but what if 

Re: [Xen-devel] [PATCH v2 2/3] xen: add hypercall option to temporarily pin a vcpu

[Anshul Makkar]

Hi,


-Original Message-
From: Xen-devel [mailto:xen-devel-boun...@lists.xen.org] On Behalf Of George 
Dunlap
Sent: 01 March 2016 15:53
To: Juergen Gross <jgr...@suse.com>; xen-devel@lists.xen.org
Cc: Wei Liu <wei.l...@citrix.com>; Stefano Stabellini 
<stefano.stabell...@citrix.com>; George Dunlap <george.dun...@citrix.com>; 
Andrew Cooper <andrew.coop...@citrix.com>; Dario Faggioli 
<dario.faggi...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; David Vrabel 
<david.vra...@citrix.com>; jbeul...@suse.com
Subject: Re: [Xen-devel] [PATCH v2 2/3] xen: add hypercall option to 
temporarily pin a vcpu

On 01/03/16 09:02, Juergen Gross wrote:
> Some hardware (e.g. Dell studio 1555 laptops) require SMIs to be 
> called on physical cpu 0 only. Linux drivers like dcdbas or i8k try to 
> achieve this by pinning the running thread to cpu 0, but in Dom0 this 
> is not enough: the vcpu must be pinned to physical cpu 0 via Xen, too.
> 
> Add a stable hypercall option SCHEDOP_pin_temp to the sched_op 
> hypercall to achieve this. It is taking a physical cpu number as 
> parameter. If pinning is possible (the calling domain has the 
> privilege to make the call and the cpu is available in the domain's
> cpupool) the calling vcpu is pinned to the specified cpu. The old cpu 
> affinity is saved. To undo the temporary pinning a cpu -1 is 
> specified. This will restore the original cpu affinity for the vcpu.
> 
> Signed-off-by: Juergen Gross <jgr...@suse.com>
> ---
> V2: - limit operation to hardware domain as suggested by Jan Beulich
> - some style issues corrected as requested by Jan Beulich
> - use fixed width types in interface as requested by Jan Beulich
> - add compat layer checking as requested by Jan Beulich
> ---
>  xen/common/compat/schedule.c |  4 ++
>  xen/common/schedule.c| 92 
> +---
>  xen/include/public/sched.h   | 17 
>  xen/include/xlat.lst |  1 +
>  4 files changed, 109 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/common/compat/schedule.c 
> b/xen/common/compat/schedule.c index 812c550..73b0f01 100644
> --- a/xen/common/compat/schedule.c
> +++ b/xen/common/compat/schedule.c
> @@ -10,6 +10,10 @@
>  
>  #define do_sched_op compat_sched_op
>  
> +#define xen_sched_pin_temp sched_pin_temp CHECK_sched_pin_temp; 
> +#undef xen_sched_pin_temp
> +
>  #define xen_sched_shutdown sched_shutdown  CHECK_sched_shutdown;  
> #undef xen_sched_shutdown diff --git a/xen/common/schedule.c 
> b/xen/common/schedule.c index b0d4b18..653f852 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -271,6 +271,12 @@ int sched_move_domain(struct domain *d, struct cpupool 
> *c)
>  struct scheduler *old_ops;
>  void *old_domdata;
>  
> +for_each_vcpu ( d, v )
> +{
> +if ( v->affinity_broken )
> +return -EBUSY;
> +}
> +
>  domdata = SCHED_OP(c->sched, alloc_domdata, d);
>  if ( domdata == NULL )
>  return -ENOMEM;
> @@ -669,6 +675,14 @@ int cpu_disable_scheduler(unsigned int cpu)
>  if ( cpumask_empty(_affinity) &&
>   cpumask_test_cpu(cpu, v->cpu_hard_affinity) )
>  {
> +if ( v->affinity_broken )
> +{
> +/* The vcpu is temporarily pinned, can't move it. */
> +vcpu_schedule_unlock_irqrestore(lock, flags, v);
> +ret = -EBUSY;
> +break;
> +}

Does this mean that if the user closes the laptop lid while one of these 
drivers has vcpu0 pinned, that Xen will crash (see 
xen/arch/x86/smpboot.c:__cpu_disable())?  Or is it the OS's job to make sure 
that all temporary pins are removed before suspending?

Also -- have you actually tested the "cpupool move while pinned"
functionality to make sure it actually works?  There's a weird bit in
cpupool_unassign_cpu_helper() where after calling cpu_disable_scheduler(cpu), 
it unconditionally sets the cpu bit in the cpupool_free_cpus mask, even if it 
returns an error.  That can't be right, even for the existing -EAGAIN case, can 
it?

I see that you have a loop to retry this call several times in the next patch; 
but what if it fails every time -- what state is the system in?

And, in general, what happens if the device driver gets mixed up and forgets to 
unpin the vcpu?  Is the only recourse to reboot your host (or deal with the 
fact that you can't reconfigure your cpupools)?

 -George

Sorry, lost the original thread so replying at the top of mail chain.

+static XSM_INLINE int xsm_schedop_pin_temp(XSM_DEFAULT_VOID) 
+{ 
+ XSM_ASSERT_ACTI

Re: [Xen-devel] [PATCH v2 2/3] xen: add hypercall option to temporarily pin a vcpu

[Juergen Gross]

On 02/03/16 17:03, Dario Faggioli wrote:
> On Wed, 2016-03-02 at 16:34 +0100, Juergen Gross wrote:
>> On 02/03/16 10:27, Dario Faggioli wrote:
>>>  
>>> However, an xl flag is easier to add, easier to document and easier
>>> and
>>> more natural to find, from the point of view of an user that really
>>> needs it. And perhaps it could turn out useful for other situations
>>> in
>>> future. So, I guess I'd say:
>>>  - yes, let's add that
>>>  - let's do it as a "force flag" of `xl vcpu-pin'.
>> Which raises the question: how to do that on the libxl level?
>>
> Ah, right.
> 
>> a) expand libxl_set_vcpuaffinity() with another parameter (is this
>> even
>>possible? I could do some ifdeffery, but the API would change...)
>>
>> b) add a libxl_set_vcpuaffinity_force() variant
>>
>> c) imply the force flag by specifying both hard and soft maps as NULL
>>(it _is_ basically just that: keep both affinity sets), implying
>> that
>>it makes no sense to specify any affinities with the -f flag
>> (which
>>renders the "force" meaning rather strange, would be more a
>> "restore"
>>now).
>>
> Eheh, tools' maintainers' call. My preference would be b).
> 
> I don't like a), mostly because that would mean everyone will need to
> specify a parameter that it is really only necessary in special cases.
> 
> I could live with c), but it indeed makes the semantic too convoluted
> for my taste.
> 
> I guess, however, that even if going for b), we need to decide whether
> to require a cpumask or not, and what to do if one passes NULL. Maybe
> we can have a cpumask parameter and,
>  - if it is not NULL, force affinity to that,
>  - if it is NULL, just 'restore';
> what do you think?

I would just let the force flag restore the old setting (thus clearing
the affinity_broken flag) and then apply the normal affinity settings.

> Actually, at Xen level, the override only acts on hard affinity...
> should libxl take only one cpumask (for hard affinity only), or both
> hard and soft?

Just as the user is specifying: 0, 1 or 2.

> I'd say just one for hard is enough, unless we want to make space for a
> potential future situation where we will want to break and restore soft
> affinity as well...

The force flag would be just an add-on. That's rather easy in the
hypervisor and in the tools.


Juergen


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 2/3] xen: add hypercall option to temporarily pin a vcpu

[Dario Faggioli]

On Wed, 2016-03-02 at 16:34 +0100, Juergen Gross wrote:
> On 02/03/16 10:27, Dario Faggioli wrote:
> > 
> > However, an xl flag is easier to add, easier to document and easier
> > and
> > more natural to find, from the point of view of an user that really
> > needs it. And perhaps it could turn out useful for other situations
> > in
> > future. So, I guess I'd say:
> >  - yes, let's add that
> >  - let's do it as a "force flag" of `xl vcpu-pin'.
> Which raises the question: how to do that on the libxl level?
> 
Ah, right.

> a) expand libxl_set_vcpuaffinity() with another parameter (is this
> even
>    possible? I could do some ifdeffery, but the API would change...)
> 
> b) add a libxl_set_vcpuaffinity_force() variant
> 
> c) imply the force flag by specifying both hard and soft maps as NULL
>    (it _is_ basically just that: keep both affinity sets), implying
> that
>    it makes no sense to specify any affinities with the -f flag
> (which
>    renders the "force" meaning rather strange, would be more a
> "restore"
>    now).
> 
Eheh, tools' maintainers' call. My preference would be b).

I don't like a), mostly because that would mean everyone will need to
specify a parameter that it is really only necessary in special cases.

I could live with c), but it indeed makes the semantic too convoluted
for my taste.

I guess, however, that even if going for b), we need to decide whether
to require a cpumask or not, and what to do if one passes NULL. Maybe
we can have a cpumask parameter and,
 - if it is not NULL, force affinity to that,
 - if it is NULL, just 'restore';
what do you think?

Actually, at Xen level, the override only acts on hard affinity...
should libxl take only one cpumask (for hard affinity only), or both
hard and soft?
I'd say just one for hard is enough, unless we want to make space for a
potential future situation where we will want to break and restore soft
affinity as well...

Dario
-- 
<> (Raistlin Majere)
-
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R Ltd., Cambridge (UK)



signature.asc
Description: This is a digitally signed message part
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 2/3] xen: add hypercall option to temporarily pin a vcpu

[Juergen Gross]

On 02/03/16 10:27, Dario Faggioli wrote:
> On Wed, 2016-03-02 at 08:14 +0100, Juergen Gross wrote:
>> On 01/03/16 16:52, George Dunlap wrote:
>>>  
>>>
>>> Also -- have you actually tested the "cpupool move while pinned"
>>> functionality to make sure it actually works?  There's a weird bit
>>> in
>>> cpupool_unassign_cpu_helper() where after calling
>>> cpu_disable_scheduler(cpu), it unconditionally sets the cpu bit in
>>> the
>>> cpupool_free_cpus mask, even if it returns an error.  That can't be
>>> right, even for the existing -EAGAIN case, can it?
>> That should be no problem. Such a failure can be repaired easily by
>> adding the cpu to the cpupool again. 
>>
> And there's not much else one can do, I would say. When we are in
> cpu_disable_scheduler(), coming from
> cpupool_unassign_cpu()-->cpupool_unassign_cpu() we're already halfway
> through removing the cpu from the pool (e.g., we already cleared the
> relevant bit from the cpupool's cpu_valid mask).
> 
> And we don't actually want to revert that, as doing so would allow the
> scheduler to start again moving vcpus to that cpu (and the following
> attempts will risk failing with EAGAIN again :-D).
> 
> FWIW, I've also found that part rather weird for quite some time... But
> it does indeed makes sense, IMO.
> 
>> Adding a comment seems to be a
>> good idea. :-)
>>
> Yep. Should we also add an error message for the user to be able to see
> it, even if she can't read the comment in the source code? (Not
> necessarily right there, if that would make it trigger too much... just
> in a place where it can be seen in the case the user actually need to
> do something).
> 
>> What is wrong and even worse, schedule_cpu_switch() returning an
>> error
>> will leak domlist_read_lock. 
>>
> Indeed, good catch. :-)
> 
>>> And, in general, what happens if the device driver gets mixed up
>>> and
>>> forgets to unpin the vcpu?  Is the only recourse to reboot your
>>> host (or
>>> deal with the fact that you can't reconfigure your cpupools)?
>> Unless we add a "forced" option to "xl vcpu-pin", yes.
>>
> Which would be fine to have, IMO. I'm not sure if it would better be an
> `xl vcpu-pin' flag, or a separate utility (as Jan is also saying).
> 
> A separate utility would fit better the "emergency nature" of the
> thing, avoiding having to clobber xl for that (as this will be the
> only, pretty uncommon, case where such flag would be needed).
> 
> However, an xl flag is easier to add, easier to document and easier and
> more natural to find, from the point of view of an user that really
> needs it. And perhaps it could turn out useful for other situations in
> future. So, I guess I'd say:
>  - yes, let's add that
>  - let's do it as a "force flag" of `xl vcpu-pin'.

Which raises the question: how to do that on the libxl level?

a) expand libxl_set_vcpuaffinity() with another parameter (is this even
   possible? I could do some ifdeffery, but the API would change...)

b) add a libxl_set_vcpuaffinity_force() variant

c) imply the force flag by specifying both hard and soft maps as NULL
   (it _is_ basically just that: keep both affinity sets), implying that
   it makes no sense to specify any affinities with the -f flag (which
   renders the "force" meaning rather strange, would be more a "restore"
   now).


Juergen

> 
> Regards,
> Dario
> 


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 2/3] xen: add hypercall option to temporarily pin a vcpu

[Juergen Gross]

On 02/03/16 12:49, Dario Faggioli wrote:
> On Wed, 2016-03-02 at 12:19 +0100, Juergen Gross wrote:
>> On 02/03/16 10:27, Dario Faggioli wrote:
>>>  
>>> Yep. Should we also add an error message for the user to be able to
>>> see
>>> it, even if she can't read the comment in the source code? (Not
>>> necessarily right there, if that would make it trigger too much...
>>> just
>>> in a place where it can be seen in the case the user actually need
>>> to
>>> do something).
>> I'd rather add the error message to xl. That's where the user will
>> see
>> it and where he can react at once. The message can even tell the user
>> the correct command, which would be a very strange thing to do in the
>> hypervisor.
>>
> Sure, wherever it's most useful.
> 
>> Another patch, I guess. :-)
>>
> Yeah, sorry. :-)

Sarcio ergo sum! :-)


Juergen

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 2/3] xen: add hypercall option to temporarily pin a vcpu

[Dario Faggioli]

On Wed, 2016-03-02 at 12:19 +0100, Juergen Gross wrote:
> On 02/03/16 10:27, Dario Faggioli wrote:
> > 
> > Yep. Should we also add an error message for the user to be able to
> > see
> > it, even if she can't read the comment in the source code? (Not
> > necessarily right there, if that would make it trigger too much...
> > just
> > in a place where it can be seen in the case the user actually need
> > to
> > do something).
> I'd rather add the error message to xl. That's where the user will
> see
> it and where he can react at once. The message can even tell the user
> the correct command, which would be a very strange thing to do in the
> hypervisor.
> 
Sure, wherever it's most useful.

> Another patch, I guess. :-)
> 
Yeah, sorry. :-)

Regards,
Dario
-- 
<> (Raistlin Majere)
-
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R Ltd., Cambridge (UK)



signature.asc
Description: This is a digitally signed message part
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 2/3] xen: add hypercall option to temporarily pin a vcpu

[Juergen Gross]

On 02/03/16 10:27, Dario Faggioli wrote:
> On Wed, 2016-03-02 at 08:14 +0100, Juergen Gross wrote:
>> On 01/03/16 16:52, George Dunlap wrote:
>>>  
>>>
>>> Also -- have you actually tested the "cpupool move while pinned"
>>> functionality to make sure it actually works?  There's a weird bit
>>> in
>>> cpupool_unassign_cpu_helper() where after calling
>>> cpu_disable_scheduler(cpu), it unconditionally sets the cpu bit in
>>> the
>>> cpupool_free_cpus mask, even if it returns an error.  That can't be
>>> right, even for the existing -EAGAIN case, can it?
>> That should be no problem. Such a failure can be repaired easily by
>> adding the cpu to the cpupool again. 
>>
> And there's not much else one can do, I would say. When we are in
> cpu_disable_scheduler(), coming from
> cpupool_unassign_cpu()-->cpupool_unassign_cpu() we're already halfway
> through removing the cpu from the pool (e.g., we already cleared the
> relevant bit from the cpupool's cpu_valid mask).
> 
> And we don't actually want to revert that, as doing so would allow the
> scheduler to start again moving vcpus to that cpu (and the following
> attempts will risk failing with EAGAIN again :-D).

Yep.

> 
> FWIW, I've also found that part rather weird for quite some time... But
> it does indeed makes sense, IMO.
> 
>> Adding a comment seems to be a
>> good idea. :-)
>>
> Yep. Should we also add an error message for the user to be able to see
> it, even if she can't read the comment in the source code? (Not
> necessarily right there, if that would make it trigger too much... just
> in a place where it can be seen in the case the user actually need to
> do something).

I'd rather add the error message to xl. That's where the user will see
it and where he can react at once. The message can even tell the user
the correct command, which would be a very strange thing to do in the
hypervisor.

Another patch, I guess. :-)

> 
>> What is wrong and even worse, schedule_cpu_switch() returning an
>> error
>> will leak domlist_read_lock. 
>>
> Indeed, good catch. :-)
> 
>>> And, in general, what happens if the device driver gets mixed up
>>> and
>>> forgets to unpin the vcpu?  Is the only recourse to reboot your
>>> host (or
>>> deal with the fact that you can't reconfigure your cpupools)?
>> Unless we add a "forced" option to "xl vcpu-pin", yes.
>>
> Which would be fine to have, IMO. I'm not sure if it would better be an
> `xl vcpu-pin' flag, or a separate utility (as Jan is also saying).
> 
> A separate utility would fit better the "emergency nature" of the
> thing, avoiding having to clobber xl for that (as this will be the
> only, pretty uncommon, case where such flag would be needed).
> 
> However, an xl flag is easier to add, easier to document and easier and
> more natural to find, from the point of view of an user that really
> needs it. And perhaps it could turn out useful for other situations in
> future. So, I guess I'd say:
>  - yes, let's add that
>  - let's do it as a "force flag" of `xl vcpu-pin'.

Okay, patch will follow...


Juergen

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 2/3] xen: add hypercall option to temporarily pin a vcpu

[Dario Faggioli]

On Wed, 2016-03-02 at 08:14 +0100, Juergen Gross wrote:
> On 01/03/16 16:52, George Dunlap wrote:
> > 
> > 
> > Also -- have you actually tested the "cpupool move while pinned"
> > functionality to make sure it actually works?  There's a weird bit
> > in
> > cpupool_unassign_cpu_helper() where after calling
> > cpu_disable_scheduler(cpu), it unconditionally sets the cpu bit in
> > the
> > cpupool_free_cpus mask, even if it returns an error.  That can't be
> > right, even for the existing -EAGAIN case, can it?
> That should be no problem. Such a failure can be repaired easily by
> adding the cpu to the cpupool again. 
>
And there's not much else one can do, I would say. When we are in
cpu_disable_scheduler(), coming from
cpupool_unassign_cpu()-->cpupool_unassign_cpu() we're already halfway
through removing the cpu from the pool (e.g., we already cleared the
relevant bit from the cpupool's cpu_valid mask).

And we don't actually want to revert that, as doing so would allow the
scheduler to start again moving vcpus to that cpu (and the following
attempts will risk failing with EAGAIN again :-D).

FWIW, I've also found that part rather weird for quite some time... But
it does indeed makes sense, IMO.

> Adding a comment seems to be a
> good idea. :-)
> 
Yep. Should we also add an error message for the user to be able to see
it, even if she can't read the comment in the source code? (Not
necessarily right there, if that would make it trigger too much... just
in a place where it can be seen in the case the user actually need to
do something).

> What is wrong and even worse, schedule_cpu_switch() returning an
> error
> will leak domlist_read_lock. 
>
Indeed, good catch. :-)

> > And, in general, what happens if the device driver gets mixed up
> > and
> > forgets to unpin the vcpu?  Is the only recourse to reboot your
> > host (or
> > deal with the fact that you can't reconfigure your cpupools)?
> Unless we add a "forced" option to "xl vcpu-pin", yes.
> 
Which would be fine to have, IMO. I'm not sure if it would better be an
`xl vcpu-pin' flag, or a separate utility (as Jan is also saying).

A separate utility would fit better the "emergency nature" of the
thing, avoiding having to clobber xl for that (as this will be the
only, pretty uncommon, case where such flag would be needed).

However, an xl flag is easier to add, easier to document and easier and
more natural to find, from the point of view of an user that really
needs it. And perhaps it could turn out useful for other situations in
future. So, I guess I'd say:
 - yes, let's add that
 - let's do it as a "force flag" of `xl vcpu-pin'.

Regards,
Dario
-- 
<> (Raistlin Majere)
-
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R Ltd., Cambridge (UK)



signature.asc
Description: This is a digitally signed message part
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 2/3] xen: add hypercall option to temporarily pin a vcpu

[Juergen Gross]

On 01/03/16 16:52, George Dunlap wrote:
> On 01/03/16 09:02, Juergen Gross wrote:
>> Some hardware (e.g. Dell studio 1555 laptops) require SMIs to be
>> called on physical cpu 0 only. Linux drivers like dcdbas or i8k try
>> to achieve this by pinning the running thread to cpu 0, but in Dom0
>> this is not enough: the vcpu must be pinned to physical cpu 0 via
>> Xen, too.
>>
>> Add a stable hypercall option SCHEDOP_pin_temp to the sched_op
>> hypercall to achieve this. It is taking a physical cpu number as
>> parameter. If pinning is possible (the calling domain has the
>> privilege to make the call and the cpu is available in the domain's
>> cpupool) the calling vcpu is pinned to the specified cpu. The old
>> cpu affinity is saved. To undo the temporary pinning a cpu -1 is
>> specified. This will restore the original cpu affinity for the vcpu.
>>
>> Signed-off-by: Juergen Gross 
>> ---
>> V2: - limit operation to hardware domain as suggested by Jan Beulich
>> - some style issues corrected as requested by Jan Beulich
>> - use fixed width types in interface as requested by Jan Beulich
>> - add compat layer checking as requested by Jan Beulich
>> ---
>>  xen/common/compat/schedule.c |  4 ++
>>  xen/common/schedule.c| 92 
>> +---
>>  xen/include/public/sched.h   | 17 
>>  xen/include/xlat.lst |  1 +
>>  4 files changed, 109 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/common/compat/schedule.c b/xen/common/compat/schedule.c
>> index 812c550..73b0f01 100644
>> --- a/xen/common/compat/schedule.c
>> +++ b/xen/common/compat/schedule.c
>> @@ -10,6 +10,10 @@
>>  
>>  #define do_sched_op compat_sched_op
>>  
>> +#define xen_sched_pin_temp sched_pin_temp
>> +CHECK_sched_pin_temp;
>> +#undef xen_sched_pin_temp
>> +
>>  #define xen_sched_shutdown sched_shutdown
>>  CHECK_sched_shutdown;
>>  #undef xen_sched_shutdown
>> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
>> index b0d4b18..653f852 100644
>> --- a/xen/common/schedule.c
>> +++ b/xen/common/schedule.c
>> @@ -271,6 +271,12 @@ int sched_move_domain(struct domain *d, struct cpupool 
>> *c)
>>  struct scheduler *old_ops;
>>  void *old_domdata;
>>  
>> +for_each_vcpu ( d, v )
>> +{
>> +if ( v->affinity_broken )
>> +return -EBUSY;
>> +}
>> +
>>  domdata = SCHED_OP(c->sched, alloc_domdata, d);
>>  if ( domdata == NULL )
>>  return -ENOMEM;
>> @@ -669,6 +675,14 @@ int cpu_disable_scheduler(unsigned int cpu)
>>  if ( cpumask_empty(_affinity) &&
>>   cpumask_test_cpu(cpu, v->cpu_hard_affinity) )
>>  {
>> +if ( v->affinity_broken )
>> +{
>> +/* The vcpu is temporarily pinned, can't move it. */
>> +vcpu_schedule_unlock_irqrestore(lock, flags, v);
>> +ret = -EBUSY;
>> +break;
>> +}
> 
> Does this mean that if the user closes the laptop lid while one of these
> drivers has vcpu0 pinned, that Xen will crash (see
> xen/arch/x86/smpboot.c:__cpu_disable())?  Or is it the OS's job to make
> sure that all temporary pins are removed before suspending?

Yes, this must be ensured by the OS.

> Also -- have you actually tested the "cpupool move while pinned"
> functionality to make sure it actually works?  There's a weird bit in
> cpupool_unassign_cpu_helper() where after calling
> cpu_disable_scheduler(cpu), it unconditionally sets the cpu bit in the
> cpupool_free_cpus mask, even if it returns an error.  That can't be
> right, even for the existing -EAGAIN case, can it?

That should be no problem. Such a failure can be repaired easily by
adding the cpu to the cpupool again. Adding a comment seems to be a
good idea. :-)

What is wrong and even worse, schedule_cpu_switch() returning an error
will leak domlist_read_lock. I'll write another patch to correct this
issue.

> I see that you have a loop to retry this call several times in the next
> patch; but what if it fails every time -- what state is the system in?

The cpu can be added to the original cpupool via "xl cpupool-add" again.

> And, in general, what happens if the device driver gets mixed up and
> forgets to unpin the vcpu?  Is the only recourse to reboot your host (or
> deal with the fact that you can't reconfigure your cpupools)?

Unless we add a "forced" option to "xl vcpu-pin", yes.

Thanks for the thorough review,

Juergen

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 2/3] xen: add hypercall option to temporarily pin a vcpu

[Jan Beulich]

>>> On 01.03.16 at 16:55,  wrote:
> On 01/03/16 15:52, George Dunlap wrote:
>> On 01/03/16 09:02, Juergen Gross wrote:
>>> --- a/xen/common/schedule.c
>>> +++ b/xen/common/schedule.c
>>> @@ -271,6 +271,12 @@ int sched_move_domain(struct domain *d, struct cpupool 
>>> *c)
>>>  struct scheduler *old_ops;
>>>  void *old_domdata;
>>>  
>>> +for_each_vcpu ( d, v )
>>> +{
>>> +if ( v->affinity_broken )
>>> +return -EBUSY;
>>> +}
>>> +
>>>  domdata = SCHED_OP(c->sched, alloc_domdata, d);
>>>  if ( domdata == NULL )
>>>  return -ENOMEM;
>>> @@ -669,6 +675,14 @@ int cpu_disable_scheduler(unsigned int cpu)
>>>  if ( cpumask_empty(_affinity) &&
>>>   cpumask_test_cpu(cpu, v->cpu_hard_affinity) )
>>>  {
>>> +if ( v->affinity_broken )
>>> +{
>>> +/* The vcpu is temporarily pinned, can't move it. */
>>> +vcpu_schedule_unlock_irqrestore(lock, flags, v);
>>> +ret = -EBUSY;
>>> +break;
>>> +}
>> 
>> Does this mean that if the user closes the laptop lid while one of these
>> drivers has vcpu0 pinned, that Xen will crash (see
>> xen/arch/x86/smpboot.c:__cpu_disable())?  Or is it the OS's job to make
>> sure that all temporary pins are removed before suspending?
>> 
>> Also -- have you actually tested the "cpupool move while pinned"
>> functionality to make sure it actually works?  There's a weird bit in
>> cpupool_unassign_cpu_helper() where after calling
>> cpu_disable_scheduler(cpu), it unconditionally sets the cpu bit in the
>> cpupool_free_cpus mask, even if it returns an error.  That can't be
>> right, even for the existing -EAGAIN case, can it?
>> 
>> I see that you have a loop to retry this call several times in the next
>> patch; but what if it fails every time -- what state is the system in?
>> 
>> And, in general, what happens if the device driver gets mixed up and
>> forgets to unpin the vcpu?  Is the only recourse to reboot your host (or
>> deal with the fact that you can't reconfigure your cpupools)?
> 
> (I should say, I think this probably is the best solution to this
> problem; I just want to make sure we think about the error cases carefully.)

I guess in the worst case there could be a utility or xl command
doing the missing unpin in such an emergency?

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 2/3] xen: add hypercall option to temporarily pin a vcpu

[George Dunlap]

On 01/03/16 15:52, George Dunlap wrote:
> On 01/03/16 09:02, Juergen Gross wrote:
>> Some hardware (e.g. Dell studio 1555 laptops) require SMIs to be
>> called on physical cpu 0 only. Linux drivers like dcdbas or i8k try
>> to achieve this by pinning the running thread to cpu 0, but in Dom0
>> this is not enough: the vcpu must be pinned to physical cpu 0 via
>> Xen, too.
>>
>> Add a stable hypercall option SCHEDOP_pin_temp to the sched_op
>> hypercall to achieve this. It is taking a physical cpu number as
>> parameter. If pinning is possible (the calling domain has the
>> privilege to make the call and the cpu is available in the domain's
>> cpupool) the calling vcpu is pinned to the specified cpu. The old
>> cpu affinity is saved. To undo the temporary pinning a cpu -1 is
>> specified. This will restore the original cpu affinity for the vcpu.
>>
>> Signed-off-by: Juergen Gross 
>> ---
>> V2: - limit operation to hardware domain as suggested by Jan Beulich
>> - some style issues corrected as requested by Jan Beulich
>> - use fixed width types in interface as requested by Jan Beulich
>> - add compat layer checking as requested by Jan Beulich
>> ---
>>  xen/common/compat/schedule.c |  4 ++
>>  xen/common/schedule.c| 92 
>> +---
>>  xen/include/public/sched.h   | 17 
>>  xen/include/xlat.lst |  1 +
>>  4 files changed, 109 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/common/compat/schedule.c b/xen/common/compat/schedule.c
>> index 812c550..73b0f01 100644
>> --- a/xen/common/compat/schedule.c
>> +++ b/xen/common/compat/schedule.c
>> @@ -10,6 +10,10 @@
>>  
>>  #define do_sched_op compat_sched_op
>>  
>> +#define xen_sched_pin_temp sched_pin_temp
>> +CHECK_sched_pin_temp;
>> +#undef xen_sched_pin_temp
>> +
>>  #define xen_sched_shutdown sched_shutdown
>>  CHECK_sched_shutdown;
>>  #undef xen_sched_shutdown
>> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
>> index b0d4b18..653f852 100644
>> --- a/xen/common/schedule.c
>> +++ b/xen/common/schedule.c
>> @@ -271,6 +271,12 @@ int sched_move_domain(struct domain *d, struct cpupool 
>> *c)
>>  struct scheduler *old_ops;
>>  void *old_domdata;
>>  
>> +for_each_vcpu ( d, v )
>> +{
>> +if ( v->affinity_broken )
>> +return -EBUSY;
>> +}
>> +
>>  domdata = SCHED_OP(c->sched, alloc_domdata, d);
>>  if ( domdata == NULL )
>>  return -ENOMEM;
>> @@ -669,6 +675,14 @@ int cpu_disable_scheduler(unsigned int cpu)
>>  if ( cpumask_empty(_affinity) &&
>>   cpumask_test_cpu(cpu, v->cpu_hard_affinity) )
>>  {
>> +if ( v->affinity_broken )
>> +{
>> +/* The vcpu is temporarily pinned, can't move it. */
>> +vcpu_schedule_unlock_irqrestore(lock, flags, v);
>> +ret = -EBUSY;
>> +break;
>> +}
> 
> Does this mean that if the user closes the laptop lid while one of these
> drivers has vcpu0 pinned, that Xen will crash (see
> xen/arch/x86/smpboot.c:__cpu_disable())?  Or is it the OS's job to make
> sure that all temporary pins are removed before suspending?
> 
> Also -- have you actually tested the "cpupool move while pinned"
> functionality to make sure it actually works?  There's a weird bit in
> cpupool_unassign_cpu_helper() where after calling
> cpu_disable_scheduler(cpu), it unconditionally sets the cpu bit in the
> cpupool_free_cpus mask, even if it returns an error.  That can't be
> right, even for the existing -EAGAIN case, can it?
> 
> I see that you have a loop to retry this call several times in the next
> patch; but what if it fails every time -- what state is the system in?
> 
> And, in general, what happens if the device driver gets mixed up and
> forgets to unpin the vcpu?  Is the only recourse to reboot your host (or
> deal with the fact that you can't reconfigure your cpupools)?

(I should say, I think this probably is the best solution to this
problem; I just want to make sure we think about the error cases carefully.)

 -George

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 2/3] xen: add hypercall option to temporarily pin a vcpu

[George Dunlap]

On 01/03/16 09:02, Juergen Gross wrote:
> Some hardware (e.g. Dell studio 1555 laptops) require SMIs to be
> called on physical cpu 0 only. Linux drivers like dcdbas or i8k try
> to achieve this by pinning the running thread to cpu 0, but in Dom0
> this is not enough: the vcpu must be pinned to physical cpu 0 via
> Xen, too.
> 
> Add a stable hypercall option SCHEDOP_pin_temp to the sched_op
> hypercall to achieve this. It is taking a physical cpu number as
> parameter. If pinning is possible (the calling domain has the
> privilege to make the call and the cpu is available in the domain's
> cpupool) the calling vcpu is pinned to the specified cpu. The old
> cpu affinity is saved. To undo the temporary pinning a cpu -1 is
> specified. This will restore the original cpu affinity for the vcpu.
> 
> Signed-off-by: Juergen Gross 
> ---
> V2: - limit operation to hardware domain as suggested by Jan Beulich
> - some style issues corrected as requested by Jan Beulich
> - use fixed width types in interface as requested by Jan Beulich
> - add compat layer checking as requested by Jan Beulich
> ---
>  xen/common/compat/schedule.c |  4 ++
>  xen/common/schedule.c| 92 
> +---
>  xen/include/public/sched.h   | 17 
>  xen/include/xlat.lst |  1 +
>  4 files changed, 109 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/common/compat/schedule.c b/xen/common/compat/schedule.c
> index 812c550..73b0f01 100644
> --- a/xen/common/compat/schedule.c
> +++ b/xen/common/compat/schedule.c
> @@ -10,6 +10,10 @@
>  
>  #define do_sched_op compat_sched_op
>  
> +#define xen_sched_pin_temp sched_pin_temp
> +CHECK_sched_pin_temp;
> +#undef xen_sched_pin_temp
> +
>  #define xen_sched_shutdown sched_shutdown
>  CHECK_sched_shutdown;
>  #undef xen_sched_shutdown
> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index b0d4b18..653f852 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -271,6 +271,12 @@ int sched_move_domain(struct domain *d, struct cpupool 
> *c)
>  struct scheduler *old_ops;
>  void *old_domdata;
>  
> +for_each_vcpu ( d, v )
> +{
> +if ( v->affinity_broken )
> +return -EBUSY;
> +}
> +
>  domdata = SCHED_OP(c->sched, alloc_domdata, d);
>  if ( domdata == NULL )
>  return -ENOMEM;
> @@ -669,6 +675,14 @@ int cpu_disable_scheduler(unsigned int cpu)
>  if ( cpumask_empty(_affinity) &&
>   cpumask_test_cpu(cpu, v->cpu_hard_affinity) )
>  {
> +if ( v->affinity_broken )
> +{
> +/* The vcpu is temporarily pinned, can't move it. */
> +vcpu_schedule_unlock_irqrestore(lock, flags, v);
> +ret = -EBUSY;
> +break;
> +}

Does this mean that if the user closes the laptop lid while one of these
drivers has vcpu0 pinned, that Xen will crash (see
xen/arch/x86/smpboot.c:__cpu_disable())?  Or is it the OS's job to make
sure that all temporary pins are removed before suspending?

Also -- have you actually tested the "cpupool move while pinned"
functionality to make sure it actually works?  There's a weird bit in
cpupool_unassign_cpu_helper() where after calling
cpu_disable_scheduler(cpu), it unconditionally sets the cpu bit in the
cpupool_free_cpus mask, even if it returns an error.  That can't be
right, even for the existing -EAGAIN case, can it?

I see that you have a loop to retry this call several times in the next
patch; but what if it fails every time -- what state is the system in?

And, in general, what happens if the device driver gets mixed up and
forgets to unpin the vcpu?  Is the only recourse to reboot your host (or
deal with the fact that you can't reconfigure your cpupools)?

 -George


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 2/3] xen: add hypercall option to temporarily pin a vcpu

[George Dunlap]

On 01/03/16 12:15, Dario Faggioli wrote:
> On Tue, 2016-03-01 at 12:58 +0100, Juergen Gross wrote:
>> On 01/03/16 12:55, David Vrabel wrote:
>>>
>>> On 01/03/16 09:02, Juergen Gross wrote:

 Some hardware (e.g. Dell studio 1555 laptops) require SMIs to be
 called on physical cpu 0 only. Linux drivers like dcdbas or i8k
 try
 to achieve this by pinning the running thread to cpu 0, but in
 Dom0
 this is not enough: the vcpu must be pinned to physical cpu 0 via
 Xen, too.

 Add a stable hypercall option SCHEDOP_pin_temp to the sched_op
 hypercall to achieve this. It is taking a physical cpu number as
 parameter. If pinning is possible (the calling domain has the
 privilege to make the call and the cpu is available in the
 domain's
 cpupool) the calling vcpu is pinned to the specified cpu. The old
 cpu affinity is saved. To undo the temporary pinning a cpu -1 is
 specified. This will restore the original cpu affinity for the
 vcpu.
>>> I suggest SCHEDOP_pin_override as a name.
>>
>> I'm fine with that. Any objections?
>>
> Not at all. I actually like it a lot.

+1 to the name.

 -George

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 2/3] xen: add hypercall option to temporarily pin a vcpu

[Dario Faggioli]

On Tue, 2016-03-01 at 12:58 +0100, Juergen Gross wrote:
> On 01/03/16 12:55, David Vrabel wrote:
> > 
> > On 01/03/16 09:02, Juergen Gross wrote:
> > > 
> > > Some hardware (e.g. Dell studio 1555 laptops) require SMIs to be
> > > called on physical cpu 0 only. Linux drivers like dcdbas or i8k
> > > try
> > > to achieve this by pinning the running thread to cpu 0, but in
> > > Dom0
> > > this is not enough: the vcpu must be pinned to physical cpu 0 via
> > > Xen, too.
> > > 
> > > Add a stable hypercall option SCHEDOP_pin_temp to the sched_op
> > > hypercall to achieve this. It is taking a physical cpu number as
> > > parameter. If pinning is possible (the calling domain has the
> > > privilege to make the call and the cpu is available in the
> > > domain's
> > > cpupool) the calling vcpu is pinned to the specified cpu. The old
> > > cpu affinity is saved. To undo the temporary pinning a cpu -1 is
> > > specified. This will restore the original cpu affinity for the
> > > vcpu.
> > I suggest SCHEDOP_pin_override as a name.
>
> I'm fine with that. Any objections?
> 
Not at all. I actually like it a lot.

Thanks and Regards,
Dario
-- 
<> (Raistlin Majere)
-
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R Ltd., Cambridge (UK)



signature.asc
Description: This is a digitally signed message part
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 2/3] xen: add hypercall option to temporarily pin a vcpu

[Juergen Gross]

On 01/03/16 12:55, David Vrabel wrote:
> On 01/03/16 09:02, Juergen Gross wrote:
>> Some hardware (e.g. Dell studio 1555 laptops) require SMIs to be
>> called on physical cpu 0 only. Linux drivers like dcdbas or i8k try
>> to achieve this by pinning the running thread to cpu 0, but in Dom0
>> this is not enough: the vcpu must be pinned to physical cpu 0 via
>> Xen, too.
>>
>> Add a stable hypercall option SCHEDOP_pin_temp to the sched_op
>> hypercall to achieve this. It is taking a physical cpu number as
>> parameter. If pinning is possible (the calling domain has the
>> privilege to make the call and the cpu is available in the domain's
>> cpupool) the calling vcpu is pinned to the specified cpu. The old
>> cpu affinity is saved. To undo the temporary pinning a cpu -1 is
>> specified. This will restore the original cpu affinity for the vcpu.
> 
> I suggest SCHEDOP_pin_override as a name.

I'm fine with that. Any objections?


Juergen


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 2/3] xen: add hypercall option to temporarily pin a vcpu

[Juergen Gross]

On 01/03/16 12:27, Jan Beulich wrote:
 On 01.03.16 at 10:02,  wrote:
>> @@ -752,14 +766,20 @@ static int vcpu_set_affinity(
>>  struct vcpu *v, const cpumask_t *affinity, cpumask_t *which)
>>  {
>>  spinlock_t *lock;
>> +int ret = 0;
>>  
>>  lock = vcpu_schedule_lock_irq(v);
>>  
>> -cpumask_copy(which, affinity);
>> +if ( v->affinity_broken )
>> +ret = -EBUSY;
>> +else
>> +{
>> +cpumask_copy(which, affinity);
>>  
>> -/* Always ask the scheduler to re-evaluate placement
>> - * when changing the affinity */
>> -set_bit(_VPF_migrating, >pause_flags);
>> +/* Always ask the scheduler to re-evaluate placement
>> + * when changing the affinity */
>> +set_bit(_VPF_migrating, >pause_flags);
> 
> When you touch code like this, would it be possible to at once fix
> the coding style issues it (the comment in this case) has?

Sure, NP.

> 
>> @@ -978,6 +998,51 @@ void watchdog_domain_destroy(struct domain *d)
>>  kill_timer(>watchdog_timer[i]);
>>  }
>>  
>> +static long do_pin_temp(int cpu)
> 
> As expressed before, throughout this patch I dislike the "temp"
> naming, when the temporary nature of this operation isn't being
> enforced by anything.
> 
> Apart from that I (vaguely) recall there having been previous
> suggestions in the direction of (temporary), which have got
> rejected.
> 
> On both points I think we need to have input from the scheduler
> maintainers.

Okay. I don't mind changing the name. We should just agree on one.

> 
>> +{
>> +struct vcpu *v = current;
>> +spinlock_t *lock;
>> +long ret = -EINVAL;
> 
> "int" seems completely sufficient for both the variable and the
> function return type.

Hmm, yes.

> 
>> @@ -1087,6 +1152,23 @@ ret_t do_sched_op(int cmd, 
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>  break;
>>  }
>>  
>> +case SCHEDOP_pin_temp:
>> +{
>> +struct sched_pin_temp sched_pin_temp;
>> +
>> +ret = -EFAULT;
>> +if ( copy_from_guest(_pin_temp, arg, 1) )
>> +break;
>> +
>> +ret = -EPERM;
>> +if ( !is_hardware_domain(current->domain) )
>> +break;
> 
> I'd generally suggest swapping these two.

Will do.

> 
>> --- a/xen/include/public/sched.h
>> +++ b/xen/include/public/sched.h
>> @@ -118,6 +118,17 @@
>>   * With id != 0 and timeout != 0, poke watchdog timer and set new timeout.
>>   */
>>  #define SCHEDOP_watchdog6
>> +
>> +/*
>> + * Temporarily pin the current vcpu to one physical cpu or undo that 
>> pinning.
>> + * @arg == pointer to sched_pin_temp_t structure.
>> + *
>> + * Setting pcpu to -1 will undo a previous temporary pinning and restore the
>> + * previous cpu affinity. The temporary aspect of the pinning isn't enforced
>> + * by the hypervisor.
> 
> This comment is now out of sync with the code, since you now
> accept any negative CPU number as "undo" request.

Will change it.


Juergen

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 2/3] xen: add hypercall option to temporarily pin a vcpu

[David Vrabel]

On 01/03/16 09:02, Juergen Gross wrote:
> Some hardware (e.g. Dell studio 1555 laptops) require SMIs to be
> called on physical cpu 0 only. Linux drivers like dcdbas or i8k try
> to achieve this by pinning the running thread to cpu 0, but in Dom0
> this is not enough: the vcpu must be pinned to physical cpu 0 via
> Xen, too.
> 
> Add a stable hypercall option SCHEDOP_pin_temp to the sched_op
> hypercall to achieve this. It is taking a physical cpu number as
> parameter. If pinning is possible (the calling domain has the
> privilege to make the call and the cpu is available in the domain's
> cpupool) the calling vcpu is pinned to the specified cpu. The old
> cpu affinity is saved. To undo the temporary pinning a cpu -1 is
> specified. This will restore the original cpu affinity for the vcpu.

I suggest SCHEDOP_pin_override as a name.

David

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 2/3] xen: add hypercall option to temporarily pin a vcpu

[Jan Beulich]

>>> On 01.03.16 at 10:02,  wrote:
> @@ -752,14 +766,20 @@ static int vcpu_set_affinity(
>  struct vcpu *v, const cpumask_t *affinity, cpumask_t *which)
>  {
>  spinlock_t *lock;
> +int ret = 0;
>  
>  lock = vcpu_schedule_lock_irq(v);
>  
> -cpumask_copy(which, affinity);
> +if ( v->affinity_broken )
> +ret = -EBUSY;
> +else
> +{
> +cpumask_copy(which, affinity);
>  
> -/* Always ask the scheduler to re-evaluate placement
> - * when changing the affinity */
> -set_bit(_VPF_migrating, >pause_flags);
> +/* Always ask the scheduler to re-evaluate placement
> + * when changing the affinity */
> +set_bit(_VPF_migrating, >pause_flags);

When you touch code like this, would it be possible to at once fix
the coding style issues it (the comment in this case) has?

> @@ -978,6 +998,51 @@ void watchdog_domain_destroy(struct domain *d)
>  kill_timer(>watchdog_timer[i]);
>  }
>  
> +static long do_pin_temp(int cpu)

As expressed before, throughout this patch I dislike the "temp"
naming, when the temporary nature of this operation isn't being
enforced by anything.

Apart from that I (vaguely) recall there having been previous
suggestions in the direction of (temporary), which have got
rejected.

On both points I think we need to have input from the scheduler
maintainers.

> +{
> +struct vcpu *v = current;
> +spinlock_t *lock;
> +long ret = -EINVAL;

"int" seems completely sufficient for both the variable and the
function return type.

> @@ -1087,6 +1152,23 @@ ret_t do_sched_op(int cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>  break;
>  }
>  
> +case SCHEDOP_pin_temp:
> +{
> +struct sched_pin_temp sched_pin_temp;
> +
> +ret = -EFAULT;
> +if ( copy_from_guest(_pin_temp, arg, 1) )
> +break;
> +
> +ret = -EPERM;
> +if ( !is_hardware_domain(current->domain) )
> +break;

I'd generally suggest swapping these two.

> --- a/xen/include/public/sched.h
> +++ b/xen/include/public/sched.h
> @@ -118,6 +118,17 @@
>   * With id != 0 and timeout != 0, poke watchdog timer and set new timeout.
>   */
>  #define SCHEDOP_watchdog6
> +
> +/*
> + * Temporarily pin the current vcpu to one physical cpu or undo that pinning.
> + * @arg == pointer to sched_pin_temp_t structure.
> + *
> + * Setting pcpu to -1 will undo a previous temporary pinning and restore the
> + * previous cpu affinity. The temporary aspect of the pinning isn't enforced
> + * by the hypervisor.

This comment is now out of sync with the code, since you now
accept any negative CPU number as "undo" request.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel