[PATCH] Avoid panic when adjusting sedf parameters

[PATCH] Avoid panic when adjusting sedf parameters

[Juergen Gross]

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

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 ++++



[-- Attachment #2: xen-staging.hg.patch --]
[-- Type: text/x-patch, Size: 1214 bytes --]

# HG changeset patch
# User Juergen Gross <juergen.gross@ts.fujitsu.com>
# Date 1321533195 -3600
# Node ID 7167eaa187b3cd996637b1f995707ff9196d00d3
# Parent  fd3567cafe1c7ccd0ddba0ad7fb067d435e13529
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

diff -r fd3567cafe1c -r 7167eaa187b3 xen/common/sched_sedf.c
--- a/xen/common/sched_sedf.c	Tue Nov 15 14:50:18 2011 +0100
+++ b/xen/common/sched_sedf.c	Thu Nov 17 13:33:15 2011 +0100
@@ -1304,6 +1304,8 @@ static void sedf_dump_cpu_state(const st
     rcu_read_lock(&domlist_read_lock);
     for_each_domain ( d )
     {
+        if ( (d->cpupool ? d->cpupool->sched : &sched_sedf_def) != ops )
+            continue;
         for_each_vcpu(d, ed)
         {
             if ( !__task_on_queue(ed) && (ed->processor == i) )
@@ -1369,6 +1371,8 @@ static int sedf_adjust_weights(struct cp
     rcu_read_lock(&domlist_read_lock);
     for_each_domain( d )
     {
+        if ( c != d->cpupool )
+            continue;
         for_each_vcpu ( d, p )
         {
             if ( (cpu = p->processor) >= nr_cpus )

[-- 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] Avoid panic when adjusting sedf parameters

[George Dunlap]

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

[PATCH] Avoid panic when adjusting sedf parameters

[Juergen Gross]

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

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 ++++++



[-- Attachment #2: xen-staging.hg.patch --]
[-- Type: text/x-patch, Size: 3446 bytes --]

# HG changeset patch
# User Juergen Gross <juergen.gross@ts.fujitsu.com>
# Date 1321534918 -3600
# Node ID 8eea2b53adb207b8ab92148c8248b60831639e52
# Parent  dbdc840f8f62db58321b5009e5e0f7833066386f
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.
Introduces for_each_domain_in_cpupool macro as it is usable 4 times now.

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

diff -r dbdc840f8f62 -r 8eea2b53adb2 xen/common/cpupool.c
--- a/xen/common/cpupool.c	Wed Nov 16 18:21:14 2011 +0000
+++ b/xen/common/cpupool.c	Thu Nov 17 14:01:58 2011 +0100
@@ -309,10 +309,8 @@ int cpupool_unassign_cpu(struct cpupool 
     if ( (c->n_dom > 0) && (cpumask_weight(c->cpu_valid) == 1) &&
          (cpu != cpupool_moving_cpu) )
     {
-        for_each_domain(d)
+        for_each_domain_in_cpupool(d, c)
         {
-            if ( d->cpupool != c )
-                continue;
             if ( !d->is_dying )
             {
                 ret = -EBUSY;
diff -r dbdc840f8f62 -r 8eea2b53adb2 xen/common/sched_sedf.c
--- a/xen/common/sched_sedf.c	Wed Nov 16 18:21:14 2011 +0000
+++ b/xen/common/sched_sedf.c	Thu Nov 17 14:01:58 2011 +0100
@@ -1304,6 +1304,8 @@ static void sedf_dump_cpu_state(const st
     rcu_read_lock(&domlist_read_lock);
     for_each_domain ( d )
     {
+        if ( (d->cpupool ? d->cpupool->sched : &sched_sedf_def) != ops )
+            continue;
         for_each_vcpu(d, ed)
         {
             if ( !__task_on_queue(ed) && (ed->processor == i) )
@@ -1335,10 +1337,8 @@ static int sedf_adjust_weights(struct cp
 
     /* Sum across all weights. */
     rcu_read_lock(&domlist_read_lock);
-    for_each_domain( d )
+    for_each_domain_in_cpupool( d, c )
     {
-        if ( c != d->cpupool )
-            continue;
         for_each_vcpu( d, p )
         {
             if ( (cpu = p->processor) >= nr_cpus )
@@ -1367,7 +1367,7 @@ static int sedf_adjust_weights(struct cp
 
     /* Adjust all slices (and periods) to the new weight. */
     rcu_read_lock(&domlist_read_lock);
-    for_each_domain( d )
+    for_each_domain_in_cpupool( d, c )
     {
         for_each_vcpu ( d, p )
         {
diff -r dbdc840f8f62 -r 8eea2b53adb2 xen/common/schedule.c
--- a/xen/common/schedule.c	Wed Nov 16 18:21:14 2011 +0000
+++ b/xen/common/schedule.c	Thu Nov 17 14:01:58 2011 +0100
@@ -538,11 +538,8 @@ int cpu_disable_scheduler(unsigned int c
     if ( c == NULL )
         return ret;
 
-    for_each_domain ( d )
+    for_each_domain_in_cpupool ( d, c )
     {
-        if ( d->cpupool != c )
-            continue;
-
         affinity_broken = 0;
 
         for_each_vcpu ( d, v )
diff -r dbdc840f8f62 -r 8eea2b53adb2 xen/include/xen/sched.h
--- 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 )) \
+       if ((_d)->cpupool == (_c))
+
 #define for_each_vcpu(_d,_v)                    \
  for ( (_v) = (_d)->vcpu ? (_d)->vcpu[0] : NULL; \
        (_v) != NULL;                            \

[-- 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] Avoid panic when adjusting sedf parameters

[Juergen Gross]

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

Re: [PATCH] Avoid panic when adjusting sedf parameters

[Jan Beulich]

>>> 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;                            \

Re: [PATCH] Avoid panic when adjusting sedf parameters

[Keir Fraser]

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

Re: [PATCH] Avoid panic when adjusting sedf parameters

[Keir Fraser]

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

Re: [PATCH] Avoid panic when adjusting sedf parameters

[Juergen Gross]

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

Re: [PATCH] Avoid panic when adjusting sedf parameters

[Juergen Gross]

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

Re: [PATCH] Avoid panic when adjusting sedf parameters

[Jan Beulich]

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

Re: [PATCH] Avoid panic when adjusting sedf parameters

[Jan Beulich]

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

[PATCH] Avoid panic when adjusting sedf parameters

[Juergen Gross]

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

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 ++++++++++++++++++++



[-- Attachment #2: xen-staging.hg.patch --]
[-- Type: text/x-patch, Size: 4348 bytes --]

# HG changeset patch
# User Juergen Gross <juergen.gross@ts.fujitsu.com>
# Date 1321540598 -3600
# Node ID fabc84feab2c1a6d22777cdbc670f65508e6fc46
# Parent  dbdc840f8f62db58321b5009e5e0f7833066386f
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.
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

diff -r dbdc840f8f62 -r fabc84feab2c xen/common/cpupool.c
--- a/xen/common/cpupool.c	Wed Nov 16 18:21:14 2011 +0000
+++ b/xen/common/cpupool.c	Thu Nov 17 15:36:38 2011 +0100
@@ -309,10 +309,9 @@ int cpupool_unassign_cpu(struct cpupool 
     if ( (c->n_dom > 0) && (cpumask_weight(c->cpu_valid) == 1) &&
          (cpu != cpupool_moving_cpu) )
     {
-        for_each_domain(d)
+        rcu_read_lock(&domlist_read_lock);
+        for_each_domain_in_cpupool(d, c)
         {
-            if ( d->cpupool != c )
-                continue;
             if ( !d->is_dying )
             {
                 ret = -EBUSY;
@@ -327,6 +326,7 @@ int cpupool_unassign_cpu(struct cpupool 
             }
             cpupool0->n_dom++;
         }
+        rcu_read_unlock(&domlist_read_lock);
         if ( ret )
             goto out;
     }
diff -r dbdc840f8f62 -r fabc84feab2c xen/common/sched_sedf.c
--- a/xen/common/sched_sedf.c	Wed Nov 16 18:21:14 2011 +0000
+++ b/xen/common/sched_sedf.c	Thu Nov 17 15:36:38 2011 +0100
@@ -1304,6 +1304,8 @@ static void sedf_dump_cpu_state(const st
     rcu_read_lock(&domlist_read_lock);
     for_each_domain ( d )
     {
+        if ( (d->cpupool ? d->cpupool->sched : &sched_sedf_def) != ops )
+            continue;
         for_each_vcpu(d, ed)
         {
             if ( !__task_on_queue(ed) && (ed->processor == i) )
@@ -1335,10 +1337,8 @@ static int sedf_adjust_weights(struct cp
 
     /* Sum across all weights. */
     rcu_read_lock(&domlist_read_lock);
-    for_each_domain( d )
+    for_each_domain_in_cpupool( d, c )
     {
-        if ( c != d->cpupool )
-            continue;
         for_each_vcpu( d, p )
         {
             if ( (cpu = p->processor) >= nr_cpus )
@@ -1367,7 +1367,7 @@ static int sedf_adjust_weights(struct cp
 
     /* Adjust all slices (and periods) to the new weight. */
     rcu_read_lock(&domlist_read_lock);
-    for_each_domain( d )
+    for_each_domain_in_cpupool( d, c )
     {
         for_each_vcpu ( d, p )
         {
diff -r dbdc840f8f62 -r fabc84feab2c xen/common/schedule.c
--- a/xen/common/schedule.c	Wed Nov 16 18:21:14 2011 +0000
+++ b/xen/common/schedule.c	Thu Nov 17 15:36:38 2011 +0100
@@ -538,11 +538,8 @@ int cpu_disable_scheduler(unsigned int c
     if ( c == NULL )
         return ret;
 
-    for_each_domain ( d )
+    for_each_domain_in_cpupool ( d, c )
     {
-        if ( d->cpupool != c )
-            continue;
-
         affinity_broken = 0;
 
         for_each_vcpu ( d, v )
diff -r dbdc840f8f62 -r fabc84feab2c xen/include/xen/sched.h
--- a/xen/include/xen/sched.h	Wed Nov 16 18:21:14 2011 +0000
+++ b/xen/include/xen/sched.h	Thu Nov 17 15:36:38 2011 +0100
@@ -564,10 +564,30 @@ extern struct domain *domain_list;
 extern struct domain *domain_list;
 
 /* Caller must hold the domlist_read_lock or domlist_update_lock. */
+static inline struct domain *first_domain_in_cpupool( struct cpupool *c)
+{
+    struct domain *d;
+    for (d = rcu_dereference(domain_list); d && d->cpupool != c;
+         d = rcu_dereference(d->next_in_list));
+    return d;
+}
+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(_d)                     \
  for ( (_d) = rcu_dereference(domain_list);     \
        (_d) != NULL;                            \
        (_d) = rcu_dereference((_d)->next_in_list )) \
+
+#define for_each_domain_in_cpupool(_d,_c)       \
+ for ( (_d) = first_domain_in_cpupool(_c);      \
+       (_d) != NULL;                            \
+       (_d) = next_domain_in_cpupool((_d), (_c)))
 
 #define for_each_vcpu(_d,_v)                    \
  for ( (_v) = (_d)->vcpu ? (_d)->vcpu[0] : NULL; \

[-- 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] Avoid panic when adjusting sedf parameters

[Juergen Gross]

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