Xen devel - Nov 2011 - [PATCH] Avoid panic when adjusting sedf parameters

When using sedf scheduler in a cpupool the system might panic when setting
sedf scheduling parameters for a domain.

Signed-off-by: juergen.gross@ts.fujitsu.com


1 file changed, 4 insertions(+)
xen/common/sched_sedf.c |    4 ++++




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

On Thu, Nov 17, 2011 at 12:33 PM, Juergen Gross
<juergen.gross@ts.fujitsu.com> wrote:
> When using sedf scheduler in a cpupool the system might panic when setting
> sedf scheduling parameters for a domain.
The cpupool structures keep track of which domain is in which pool,
right?  I wonder if a more elegant solution might be to make a
for_each_domain_cpupool() macro.

Just an idea, not a NACK; if you don''t think my idea is good /
don''t
have time / inclination to do it now, I''m fine with this patch..

 -George

>
> Signed-off-by: juergen.gross@ts.fujitsu.com
>
>
> 1 file changed, 4 insertions(+)
> xen/common/sched_sedf.c |    4 ++++
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

When using sedf scheduler in a cpupool the system might panic when setting
sedf scheduling parameters for a domain.
Introduces for_each_domain_in_cpupool macro as it is usable 4 times now.

Signed-off-by: juergen.gross@ts.fujitsu.com


4 files changed, 12 insertions(+), 11 deletions(-)
xen/common/cpupool.c    |    4 +---
xen/common/sched_sedf.c |    8 ++++----
xen/common/schedule.c   |    5 +----
xen/include/xen/sched.h |    6 ++++++




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

On 11/17/2011 01:43 PM, George Dunlap wrote:
> On Thu, Nov 17, 2011 at 12:33 PM, Juergen Gross
> <juergen.gross@ts.fujitsu.com>  wrote:
>> When using sedf scheduler in a cpupool the system might panic when
setting
>> sedf scheduling parameters for a domain.
> The cpupool structures keep track of which domain is in which pool,
> right?  I wonder if a more elegant solution might be to make a
> for_each_domain_cpupool() macro.
>
> Just an idea, not a NACK; if you don''t think my idea is good /
don''t
> have time / inclination to do it now, I''m fine with this patch..
Good idea. New patch coming soon.


Juergen

-- 
Juergen Gross                 Principal Developer Operating Systems
PDG ES&S SWE OS6                       Telephone: +49 (0) 89 3222 2967
Fujitsu Technology Solutions              e-mail: juergen.gross@ts.fujitsu.com
Domagkstr. 28                           Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html


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

>>> On 17.11.11 at 14:03, Juergen Gross
<juergen.gross@ts.fujitsu.com> wrote:
>--- a/xen/include/xen/sched.h	Wed Nov 16 18:21:14 2011 +0000
>+++ b/xen/include/xen/sched.h	Thu Nov 17 14:01:58 2011 +0100
>@@ -569,6 +569,12 @@ extern struct domain *domain_list;
>        (_d) != NULL;                            \
>        (_d) = rcu_dereference((_d)->next_in_list )) \
> 
>+#define for_each_domain_in_cpupool(_d,_c)       \
>+ for ( (_d) = rcu_dereference(domain_list);     \
>+       (_d) != NULL;                            \
>+       (_d) = rcu_dereference((_d)->next_in_list )) \
Wouldn''t this, up to here, simply be for_each_domain()?
>+       if ((_d)->cpupool == (_c))
This is dangerous - consider code like

    if ( x )
        for_each_domain_in_cpupool ()
            function();
    else
        other_stuff;

which would now associate the else with the wrong (inner) if. One
possible solution that comes to mind would be

#define for_each_domain_in_cpupool(_d,_c) \
    for_each_domain_in_cpupool (_d) \
        if ((_d)->cpupool != (_c)) \
            continue; \
        else

but I think I had seen a more clever solution to this problem, but cannot
remember/locate it right now.

Jan
>+
> #define for_each_vcpu(_d,_v)                    \
>  for ( (_v) = (_d)->vcpu ? (_d)->vcpu[0] : NULL; \
>        (_v) != NULL;                            \

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

On 17/11/2011 13:30, "Jan Beulich" <JBeulich@suse.com> wrote:
>> +#define for_each_domain_in_cpupool(_d,_c)       \
>> + for ( (_d) = rcu_dereference(domain_list);     \
>> +       (_d) != NULL;                            \
>> +       (_d) = rcu_dereference((_d)->next_in_list )) \
> 
> Wouldn''t this, up to here, simply be for_each_domain()?
> 
>> +       if ((_d)->cpupool == (_c))
> 
> This is dangerous - consider code like
I also wonder (and this is true for the existing open-coded versions too)
whether we have sufficient locking around use of d->cpupool? Do these loops
hold enough locks to ensure that d->cpupool doesn''t change under
their feet?

 -- Keir



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

On 17/11/2011 13:30, "Jan Beulich" <JBeulich@suse.com> wrote:
> which would now associate the else with the wrong (inner) if. One
> possible solution that comes to mind would be
> 
> #define for_each_domain_in_cpupool(_d,_c) \
>     for_each_domain_in_cpupool (_d) \
>         if ((_d)->cpupool != (_c)) \
>             continue; \
>         else
> 
> but I think I had seen a more clever solution to this problem, but cannot
> remember/locate it right now.
Given the gcc ({}) construction, you could do a double-loop:
 for ( (_d) = rcu_dereference(domain_list);     \
       (_d) != NULL;                            \
       ({ while ((_d) = rcu_dereference((_d)->next_in_list != NULL)
             if ((_d)->cpupool == (_c)) break;
          (_d); }) )

A bit ugly. ;-) And I still worry about cpupool locking...

 -- Keir



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

On 11/17/2011 02:48 PM, Keir Fraser wrote:
> On 17/11/2011 13:30, "Jan Beulich"<JBeulich@suse.com> 
wrote:
>
>>> +#define for_each_domain_in_cpupool(_d,_c)       \
>>> + for ( (_d) = rcu_dereference(domain_list);     \
>>> +       (_d) != NULL;                            \
>>> +       (_d) = rcu_dereference((_d)->next_in_list )) \
>> Wouldn''t this, up to here, simply be for_each_domain()?
>>
>>> +       if ((_d)->cpupool == (_c))
>> This is dangerous - consider code like
> I also wonder (and this is true for the existing open-coded versions too)
> whether we have sufficient locking around use of d->cpupool? Do these
loops
> hold enough locks to ensure that d->cpupool doesn''t change
under their feet?
d->cpupool is changed in three functions:

I was just preparing a patch for cpupool_unassign_cpu() which is lacking
rcu_lock_domain().
cpupool_add_domain() is called only for domains not yet in the domain list.
sched_move_domain() is only called with domain lock held.


Juergen

-- 
Juergen Gross                 Principal Developer Operating Systems
PDG ES&S SWE OS6                       Telephone: +49 (0) 89 3222 2967
Fujitsu Technology Solutions              e-mail: juergen.gross@ts.fujitsu.com
Domagkstr. 28                           Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html


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

On 11/17/2011 02:52 PM, Keir Fraser wrote:
> On 17/11/2011 13:30, "Jan Beulich"<JBeulich@suse.com> 
wrote:
>
>> which would now associate the else with the wrong (inner) if. One
>> possible solution that comes to mind would be
>>
>> #define for_each_domain_in_cpupool(_d,_c) \
>>      for_each_domain_in_cpupool (_d) \
>>          if ((_d)->cpupool != (_c)) \
>>              continue; \
>>          else
>>
>> but I think I had seen a more clever solution to this problem, but
cannot
>> remember/locate it right now.
> Given the gcc ({}) construction, you could do a double-loop:
>   for ( (_d) = rcu_dereference(domain_list);     \
>         (_d) != NULL;                            \
>         ({ while ((_d) = rcu_dereference((_d)->next_in_list != NULL)
>               if ((_d)->cpupool == (_c)) break;
>            (_d); }) )
>
> A bit ugly. ;-) And I still worry about cpupool locking...
What about:

static inline struct domain *next_domain_in_cpupool(
     struct domain *d, struct cpupool *c)
{
     for (d = rcu_dereference(d->next_in_list); d && d->cpupool !=
c;
          d = rcu_dereference(d->next_in_list));
     return d;
}

#define for_each_domain_in_cpupool(_d,_c)       \
  for ( (_d) = rcu_dereference(domain_list);     \
        (_d) != NULL;                            \
        (_d) = next_domain_in_cpupool((_d), (_c)))


Juergen

-- 
Juergen Gross                 Principal Developer Operating Systems
PDG ES&S SWE OS6                       Telephone: +49 (0) 89 3222 2967
Fujitsu Technology Solutions              e-mail: juergen.gross@ts.fujitsu.com
Domagkstr. 28                           Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html


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

>>> On 17.11.11 at 14:52, Keir Fraser <keir.xen@gmail.com> wrote:
> On 17/11/2011 13:30, "Jan Beulich" <JBeulich@suse.com>
wrote:
> 
>> which would now associate the else with the wrong (inner) if. One
>> possible solution that comes to mind would be
>> 
>> #define for_each_domain_in_cpupool(_d,_c) \
>>     for_each_domain_in_cpupool (_d) \
>>         if ((_d)->cpupool != (_c)) \
>>             continue; \
>>         else
>> 
>> but I think I had seen a more clever solution to this problem, but
cannot
>> remember/locate it right now.
> 
> Given the gcc ({}) construction, you could do a double-loop:
>  for ( (_d) = rcu_dereference(domain_list);     \
>        (_d) != NULL;                            \
>        ({ while ((_d) = rcu_dereference((_d)->next_in_list != NULL)
>              if ((_d)->cpupool == (_c)) break;
>           (_d); }) )
> 
> A bit ugly. ;-) And I still worry about cpupool locking...
No - the very first domain would make into the body of the loop
without its pool being checked. The (adjusted) construct would
need to go into the checking (middle) portion of the for.

Jan


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

>>> On 17.11.11 at 15:04, Juergen Gross
<juergen.gross@ts.fujitsu.com> wrote:
> On 11/17/2011 02:52 PM, Keir Fraser wrote:
>> On 17/11/2011 13:30, "Jan Beulich"<JBeulich@suse.com> 
wrote:
>>
>>> which would now associate the else with the wrong (inner) if. One
>>> possible solution that comes to mind would be
>>>
>>> #define for_each_domain_in_cpupool(_d,_c) \
>>>      for_each_domain_in_cpupool (_d) \
>>>          if ((_d)->cpupool != (_c)) \
>>>              continue; \
>>>          else
>>>
>>> but I think I had seen a more clever solution to this problem, but
cannot
>>> remember/locate it right now.
>> Given the gcc ({}) construction, you could do a double-loop:
>>   for ( (_d) = rcu_dereference(domain_list);     \
>>         (_d) != NULL;                            \
>>         ({ while ((_d) = rcu_dereference((_d)->next_in_list != NULL)
>>               if ((_d)->cpupool == (_c)) break;
>>            (_d); }) )
>>
>> A bit ugly. ;-) And I still worry about cpupool locking...
> 
> What about:
> 
> static inline struct domain *next_domain_in_cpupool(
>      struct domain *d, struct cpupool *c)
> {
>      for (d = rcu_dereference(d->next_in_list); d &&
d->cpupool != c;
>           d = rcu_dereference(d->next_in_list));
>      return d;
> }
> 
> #define for_each_domain_in_cpupool(_d,_c)       \
>   for ( (_d) = rcu_dereference(domain_list);     \
>         (_d) != NULL;                            \
>         (_d) = next_domain_in_cpupool((_d), (_c)))
Same problem as with Keir''s variant - you''d enter the loop
body for
the first domain on the list regardless of its cpupool. But with a
first_domain_in_cpupool() counterpart this might be usable. Or, as
said in the other reply, putting a more complex construct in the
middle clause.

Jan


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

When using sedf scheduler in a cpupool the system might panic when setting
sedf scheduling parameters for a domain.
Introduces for_each_domain_in_cpupool macro as it is usable 4 times now.
Add appropriate locking in cpupool_unassign_cpu().

Signed-off-by: juergen.gross@ts.fujitsu.com


4 files changed, 28 insertions(+), 11 deletions(-)
xen/common/cpupool.c    |    6 +++---
xen/common/sched_sedf.c |    8 ++++----
xen/common/schedule.c   |    5 +----
xen/include/xen/sched.h |   20 ++++++++++++++++++++




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

On 11/17/2011 03:32 PM, Jan Beulich wrote:
>>>> On 17.11.11 at 15:04, Juergen
Gross<juergen.gross@ts.fujitsu.com>  wrote:
>> On 11/17/2011 02:52 PM, Keir Fraser wrote:
>>> On 17/11/2011 13:30, "Jan
Beulich"<JBeulich@suse.com>   wrote:
>>>
>>>> which would now associate the else with the wrong (inner) if.
One
>>>> possible solution that comes to mind would be
>>>>
>>>> #define for_each_domain_in_cpupool(_d,_c) \
>>>>       for_each_domain_in_cpupool (_d) \
>>>>           if ((_d)->cpupool != (_c)) \
>>>>               continue; \
>>>>           else
>>>>
>>>> but I think I had seen a more clever solution to this problem,
but cannot
>>>> remember/locate it right now.
>>> Given the gcc ({}) construction, you could do a double-loop:
>>>    for ( (_d) = rcu_dereference(domain_list);     \
>>>          (_d) != NULL;                            \
>>>          ({ while ((_d) = rcu_dereference((_d)->next_in_list !=
NULL)
>>>                if ((_d)->cpupool == (_c)) break;
>>>             (_d); }) )
>>>
>>> A bit ugly. ;-) And I still worry about cpupool locking...
>> What about:
>>
>> static inline struct domain *next_domain_in_cpupool(
>>       struct domain *d, struct cpupool *c)
>> {
>>       for (d = rcu_dereference(d->next_in_list); d&& 
d->cpupool != c;
>>            d = rcu_dereference(d->next_in_list));
>>       return d;
>> }
>>
>> #define for_each_domain_in_cpupool(_d,_c)       \
>>    for ( (_d) = rcu_dereference(domain_list);     \
>>          (_d) != NULL;                            \
>>          (_d) = next_domain_in_cpupool((_d), (_c)))
> Same problem as with Keir''s variant - you''d enter the
loop body for
> the first domain on the list regardless of its cpupool. But with a
> first_domain_in_cpupool() counterpart this might be usable. Or, as
> said in the other reply, putting a more complex construct in the
> middle clause.
I think adding first_domain_in_cpupool() is a good way to go.
Sending out adjusted patch with appropriate locking added in
cpupool_unassign_cpu() soon.


Juergen

-- 
Juergen Gross                 Principal Developer Operating Systems
PDG ES&S SWE OS6                       Telephone: +49 (0) 89 3222 2967
Fujitsu Technology Solutions              e-mail: juergen.gross@ts.fujitsu.com
Domagkstr. 28                           Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html


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