[PATCH] cpupools: retry cpupool-destroy if domain in cpupool is dying

[PATCH] cpupools: retry cpupool-destroy if domain in cpupool is dying

[Juergen Gross]

When a cpupool is destroyed just after the last domain has been stopped the
domain might already be removed from the cpupool without having decremented
the domain count of the cpupool. This will result in rejection of the
cpupool-destroy operation.
It is easy to detect this situation and to return EAGAIN in this case which
is already handled in libxc by doing a retry.

Signed-off-by: Juergen Gross <juergen.gross@ts.fujitsu.com>
---
 xen/common/cpupool.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
index 4a0e569..ac833f0 100644
--- a/xen/common/cpupool.c
+++ b/xen/common/cpupool.c
@@ -348,6 +348,8 @@ static int cpupool_unassign_cpu(struct cpupool *c, unsigned int cpu)
             cpupool0->n_dom++;
         }
         rcu_read_unlock(&domlist_read_lock);
+        if ( (c->n_dom > 0) && !ret )
+            ret = -EAGAIN;
         if ( ret )
             goto out;
     }
-- 
1.7.10.4

Re: [PATCH] cpupools: retry cpupool-destroy if domain in cpupool is dying

[George Dunlap]

On Wed, May 7, 2014 at 8:52 AM, Juergen Gross
<juergen.gross@ts.fujitsu.com> wrote:
> When a cpupool is destroyed just after the last domain has been stopped the
> domain might already be removed from the cpupool without having decremented
> the domain count of the cpupool. This will result in rejection of the
> cpupool-destroy operation.

I'm a bit confused.  What's the sched_move_domain() for, then?  If
we're going to handle "dying domains" by doing a retry, could we just
get rid of it?

 -George

> It is easy to detect this situation and to return EAGAIN in this case which
> is already handled in libxc by doing a retry.
>
> Signed-off-by: Juergen Gross <juergen.gross@ts.fujitsu.com>
> ---
>  xen/common/cpupool.c |    2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
> index 4a0e569..ac833f0 100644
> --- a/xen/common/cpupool.c
> +++ b/xen/common/cpupool.c
> @@ -348,6 +348,8 @@ static int cpupool_unassign_cpu(struct cpupool *c, unsigned int cpu)
>              cpupool0->n_dom++;
>          }
>          rcu_read_unlock(&domlist_read_lock);
> +        if ( (c->n_dom > 0) && !ret )
> +            ret = -EAGAIN;
>          if ( ret )
>              goto out;
>      }
> --
> 1.7.10.4
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] cpupools: retry cpupool-destroy if domain in cpupool is dying

[Juergen Gross]

On 07.05.2014 15:10, George Dunlap wrote:
> On Wed, May 7, 2014 at 8:52 AM, Juergen Gross
> <juergen.gross@ts.fujitsu.com> wrote:
>> When a cpupool is destroyed just after the last domain has been stopped the
>> domain might already be removed from the cpupool without having decremented
>> the domain count of the cpupool. This will result in rejection of the
>> cpupool-destroy operation.
>
> I'm a bit confused.  What's the sched_move_domain() for, then?  If
> we're going to handle "dying domains" by doing a retry, could we just
> get rid of it?

The sched_move_domain() is still needed for cases where a domain stays
dying for a longer time, e.g. when a dom0 process is still referencing
some of it's memory pages. This may be a rare situation, but being unable
to use a physical cpu for another cpupool just because of this case is
worse than this little piece of code, IMO.


Juergen

>
>   -George
>
>> It is easy to detect this situation and to return EAGAIN in this case which
>> is already handled in libxc by doing a retry.
>>
>> Signed-off-by: Juergen Gross <juergen.gross@ts.fujitsu.com>
>> ---
>>   xen/common/cpupool.c |    2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
>> index 4a0e569..ac833f0 100644
>> --- a/xen/common/cpupool.c
>> +++ b/xen/common/cpupool.c
>> @@ -348,6 +348,8 @@ static int cpupool_unassign_cpu(struct cpupool *c, unsigned int cpu)
>>               cpupool0->n_dom++;
>>           }
>>           rcu_read_unlock(&domlist_read_lock);
>> +        if ( (c->n_dom > 0) && !ret )
>> +            ret = -EAGAIN;
>>           if ( ret )
>>               goto out;
>>       }
>> --
>> 1.7.10.4
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>


-- 
Juergen Gross                 Principal Developer Operating Systems
PSO PM&D ES&S SWE OS6                  Telephone: +49 (0) 89 62060 2932
Fujitsu                                   e-mail: juergen.gross@ts.fujitsu.com
Mies-van-der-Rohe-Str. 8                Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html

Re: [PATCH] cpupools: retry cpupool-destroy if domain in cpupool is dying

[George Dunlap]

On Wed, May 7, 2014 at 2:23 PM, Juergen Gross
<juergen.gross@ts.fujitsu.com> wrote:
> On 07.05.2014 15:10, George Dunlap wrote:
>>
>> On Wed, May 7, 2014 at 8:52 AM, Juergen Gross
>> <juergen.gross@ts.fujitsu.com> wrote:
>>>
>>> When a cpupool is destroyed just after the last domain has been stopped
>>> the
>>> domain might already be removed from the cpupool without having
>>> decremented
>>> the domain count of the cpupool. This will result in rejection of the
>>> cpupool-destroy operation.
>>
>>
>> I'm a bit confused.  What's the sched_move_domain() for, then?  If
>> we're going to handle "dying domains" by doing a retry, could we just
>> get rid of it?
>
>
> The sched_move_domain() is still needed for cases where a domain stays
> dying for a longer time, e.g. when a dom0 process is still referencing
> some of it's memory pages. This may be a rare situation, but being unable
> to use a physical cpu for another cpupool just because of this case is
> worse than this little piece of code, IMO.

And I take it there are times when the move fails for whatever reason?

Could you add a comment explaining this above the for() loop then, for
posterity?

 -George

Re: [PATCH] cpupools: retry cpupool-destroy if domain in cpupool is dying

[Juergen Gross]

On 08.05.2014 17:10, George Dunlap wrote:
> On Wed, May 7, 2014 at 2:23 PM, Juergen Gross
> <juergen.gross@ts.fujitsu.com> wrote:
>> On 07.05.2014 15:10, George Dunlap wrote:
>>>
>>> On Wed, May 7, 2014 at 8:52 AM, Juergen Gross
>>> <juergen.gross@ts.fujitsu.com> wrote:
>>>>
>>>> When a cpupool is destroyed just after the last domain has been stopped
>>>> the
>>>> domain might already be removed from the cpupool without having
>>>> decremented
>>>> the domain count of the cpupool. This will result in rejection of the
>>>> cpupool-destroy operation.
>>>
>>>
>>> I'm a bit confused.  What's the sched_move_domain() for, then?  If
>>> we're going to handle "dying domains" by doing a retry, could we just
>>> get rid of it?
>>
>>
>> The sched_move_domain() is still needed for cases where a domain stays
>> dying for a longer time, e.g. when a dom0 process is still referencing
>> some of it's memory pages. This may be a rare situation, but being unable
>> to use a physical cpu for another cpupool just because of this case is
>> worse than this little piece of code, IMO.
>
> And I take it there are times when the move fails for whatever reason?

ENOMEM for example.

> Could you add a comment explaining this above the for() loop then, for
> posterity?

Could you define 'this', please? The reason for the sched_move_domain()
is mentioned in the head comment of the function (zombie domains). The
possibility of a failing sched_move_domain() is obvious by the return
value checking.


Juergen

-- 
Juergen Gross                 Principal Developer Operating Systems
PSO PM&D ES&S SWE OS6                  Telephone: +49 (0) 89 62060 2932
Fujitsu                                   e-mail: juergen.gross@ts.fujitsu.com
Mies-van-der-Rohe-Str. 8                Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html

Re: [PATCH] cpupools: retry cpupool-destroy if domain in cpupool is dying

[George Dunlap]

On Fri, May 9, 2014 at 6:01 AM, Juergen Gross
<juergen.gross@ts.fujitsu.com> wrote:
> On 08.05.2014 17:10, George Dunlap wrote:
>>
>> On Wed, May 7, 2014 at 2:23 PM, Juergen Gross
>> <juergen.gross@ts.fujitsu.com> wrote:
>>>
>>> On 07.05.2014 15:10, George Dunlap wrote:
>>>>
>>>>
>>>> On Wed, May 7, 2014 at 8:52 AM, Juergen Gross
>>>> <juergen.gross@ts.fujitsu.com> wrote:
>>>>>
>>>>>
>>>>> When a cpupool is destroyed just after the last domain has been stopped
>>>>> the
>>>>> domain might already be removed from the cpupool without having
>>>>> decremented
>>>>> the domain count of the cpupool. This will result in rejection of the
>>>>> cpupool-destroy operation.
>>>>
>>>>
>>>>
>>>> I'm a bit confused.  What's the sched_move_domain() for, then?  If
>>>> we're going to handle "dying domains" by doing a retry, could we just
>>>> get rid of it?
>>>
>>>
>>>
>>> The sched_move_domain() is still needed for cases where a domain stays
>>> dying for a longer time, e.g. when a dom0 process is still referencing
>>> some of it's memory pages. This may be a rare situation, but being unable
>>> to use a physical cpu for another cpupool just because of this case is
>>> worse than this little piece of code, IMO.
>>
>>
>> And I take it there are times when the move fails for whatever reason?
>
>
> ENOMEM for example.
>
>
>> Could you add a comment explaining this above the for() loop then, for
>> posterity?
>
>
> Could you define 'this', please? The reason for the sched_move_domain()
> is mentioned in the head comment of the function (zombie domains). The
> possibility of a failing sched_move_domain() is obvious by the return
> value checking.

Oh, sorry -- I misunderstood the patch.  I thought you were adding
code to handle the case when sched_move_domain() failed -- but
actually, you're handling the case where you go through the
for_each_domain_in_cpupool() loop, successfully call
sched_move_domain() on each such domain (and decrement n_dom each
time), but still somehow at the end have a positive n_dom.

Do I have it right now, or am I still confused?

So there are times when a domain might not come up in the
for_each_domain_in_cpupool() loop, but for some reason still be in he
n_dom reference count?

That doesn't seem like it should be allowed to happen; and at the
moment I'm failing to see how that should happen, unless you have a
race somewhere.

Sorry if I'm just being really dense here, but from what I can tell:
* for_each_domain_in_cpupool() iterates through the domain list,
looking for domains such that d->cpupool == c
* d->cpupool is only modified when the cpupool_lock is held
* Whenever d->cpupool is modified, n_dom for the appropriate cpupools
are also modified
* In this function, you hold cpupool_lock

So how is it that you have a situation where d->cpupool != c, but
c->n_dom is counting that domain?

 -George

Re: [PATCH] cpupools: retry cpupool-destroy if domain in cpupool is dying

[George Dunlap]

On Mon, May 12, 2014 at 11:50 AM, George Dunlap
<George.Dunlap@eu.citrix.com> wrote:
> moment I'm failing to see how that should happen, unless you have a

s/should/could/;

Re: [PATCH] cpupools: retry cpupool-destroy if domain in cpupool is dying

[Juergen Gross]

On 12.05.2014 12:50, George Dunlap wrote:
> On Fri, May 9, 2014 at 6:01 AM, Juergen Gross
> <juergen.gross@ts.fujitsu.com> wrote:
>> On 08.05.2014 17:10, George Dunlap wrote:
>>>
>>> On Wed, May 7, 2014 at 2:23 PM, Juergen Gross
>>> <juergen.gross@ts.fujitsu.com> wrote:
>>>>
>>>> On 07.05.2014 15:10, George Dunlap wrote:
>>>>>
>>>>>
>>>>> On Wed, May 7, 2014 at 8:52 AM, Juergen Gross
>>>>> <juergen.gross@ts.fujitsu.com> wrote:
>>>>>>
>>>>>>
>>>>>> When a cpupool is destroyed just after the last domain has been stopped
>>>>>> the
>>>>>> domain might already be removed from the cpupool without having
>>>>>> decremented
>>>>>> the domain count of the cpupool. This will result in rejection of the
>>>>>> cpupool-destroy operation.
>>>>>
>>>>>
>>>>>
>>>>> I'm a bit confused.  What's the sched_move_domain() for, then?  If
>>>>> we're going to handle "dying domains" by doing a retry, could we just
>>>>> get rid of it?
>>>>
>>>>
>>>>
>>>> The sched_move_domain() is still needed for cases where a domain stays
>>>> dying for a longer time, e.g. when a dom0 process is still referencing
>>>> some of it's memory pages. This may be a rare situation, but being unable
>>>> to use a physical cpu for another cpupool just because of this case is
>>>> worse than this little piece of code, IMO.
>>>
>>>
>>> And I take it there are times when the move fails for whatever reason?
>>
>>
>> ENOMEM for example.
>>
>>
>>> Could you add a comment explaining this above the for() loop then, for
>>> posterity?
>>
>>
>> Could you define 'this', please? The reason for the sched_move_domain()
>> is mentioned in the head comment of the function (zombie domains). The
>> possibility of a failing sched_move_domain() is obvious by the return
>> value checking.
>
> Oh, sorry -- I misunderstood the patch.  I thought you were adding
> code to handle the case when sched_move_domain() failed -- but
> actually, you're handling the case where you go through the
> for_each_domain_in_cpupool() loop, successfully call
> sched_move_domain() on each such domain (and decrement n_dom each
> time), but still somehow at the end have a positive n_dom.
>
> Do I have it right now, or am I still confused?
>
> So there are times when a domain might not come up in the
> for_each_domain_in_cpupool() loop, but for some reason still be in he
> n_dom reference count?
>
> That doesn't seem like it should be allowed to happen; and at the
> moment I'm failing to see how that should happen, unless you have a
> race somewhere.
>
> Sorry if I'm just being really dense here, but from what I can tell:
> * for_each_domain_in_cpupool() iterates through the domain list,
> looking for domains such that d->cpupool == c
> * d->cpupool is only modified when the cpupool_lock is held
> * Whenever d->cpupool is modified, n_dom for the appropriate cpupools
> are also modified
> * In this function, you hold cpupool_lock
>
> So how is it that you have a situation where d->cpupool != c, but
> c->n_dom is counting that domain?

Sorry, my explanation above seems to be wrong, the patch is correct. I should
have written the complete patch when I discovered the problem, not only the
source modification (it took some time to verify the solution works).

This is the correct problem description:

When a domain is destroyed, it is removed from the domain_list first,
then it is removed from the cpupool. So for_each_domain_in_cpupool() can
miss the domain while n_dom isn't yet decremented. This scenario will
happen as long as there are references to the domain.

I'll update the patch accordingly.

Thanks for trying to understand :-)


Juergen

-- 
Juergen Gross                 Principal Developer Operating Systems
PSO PM&D ES&S SWE OS6                  Telephone: +49 (0) 89 62060 2932
Fujitsu                                   e-mail: juergen.gross@ts.fujitsu.com
Mies-van-der-Rohe-Str. 8                Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html

Re: [PATCH] cpupools: retry cpupool-destroy if domain in cpupool is dying

[Juergen Gross]

On 14.05.2014 15:13, Jan Beulich wrote:
>>>> On 14.05.14 at 15:05, <juergen.gross@ts.fujitsu.com> wrote:
>> On 14.05.2014 14:28, Jan Beulich wrote:
>>>>>> On 14.05.14 at 12:35, <juergen.gross@ts.fujitsu.com> wrote:
>>>> sched_destroy_vcpu() and sched_destroy_domain() have to happen before
>>>> cpupool_rm_domain(). This could be avoided if the domain would be moved to
>>>> cpupool0 in domain_destroy().
>>>>
>>>> Hmm, doesn't sound too bad. This would be just symmetrical to domain
>>>> creation. What do you think?
>>>
>>> I'm always in favor of symmetry, where possible and suitable. So
>>> unless George objects or sees problems with this, why don't you
>>> give this a try?
>>
>> One problem arises: sched_move_domain() can fail. Is there a preferred way to
>> handle this situation in domain_destroy() ? I could try to defer destroying
>> the domain until sched_move_domain() succeeds, but using a busy loop doing this
>> seems contra-productive and a timer based solution requires a timer
>> structure.
>>
>> I could reuse the domain watchdog_timer entries if I move
>> watchdog_domain_destroy() to domain_destroy() (which seems to be not critical).
>>
>> OTOH this seems a little bit hacky...
>
> Indeed it does. Yet - does that move have to happen in
> domain_destroy()? I.e. can't it be pulled even further ahead into
> domain_kill()? That one already is preemptable/resumable (via
> guarantees we expect from the tools side iirc), since
> domain_relinquish_resources() may take quite long a time. Of course
> that'll work only if no failure condition in sched_move_domain() is
> permanent.

Aah, excellent!

Now the patch seems to be very simple! I'll do some tests to verify it isn't
breaking something.


Juergen

-- 
Juergen Gross                 Principal Developer Operating Systems
PSO PM&D ES&S SWE OS6                  Telephone: +49 (0) 89 62060 2932
Fujitsu                                   e-mail: juergen.gross@ts.fujitsu.com
Mies-van-der-Rohe-Str. 8                Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html

Re: [PATCH] cpupools: retry cpupool-destroy if domain in cpupool is dying

[Jan Beulich]

>>> On 14.05.14 at 15:05, <juergen.gross@ts.fujitsu.com> wrote:
> On 14.05.2014 14:28, Jan Beulich wrote:
>>>>> On 14.05.14 at 12:35, <juergen.gross@ts.fujitsu.com> wrote:
>>> sched_destroy_vcpu() and sched_destroy_domain() have to happen before
>>> cpupool_rm_domain(). This could be avoided if the domain would be moved to
>>> cpupool0 in domain_destroy().
>>>
>>> Hmm, doesn't sound too bad. This would be just symmetrical to domain
>>> creation. What do you think?
>>
>> I'm always in favor of symmetry, where possible and suitable. So
>> unless George objects or sees problems with this, why don't you
>> give this a try?
> 
> One problem arises: sched_move_domain() can fail. Is there a preferred way to
> handle this situation in domain_destroy() ? I could try to defer destroying
> the domain until sched_move_domain() succeeds, but using a busy loop doing this
> seems contra-productive and a timer based solution requires a timer 
> structure.
> 
> I could reuse the domain watchdog_timer entries if I move
> watchdog_domain_destroy() to domain_destroy() (which seems to be not critical).
> 
> OTOH this seems a little bit hacky...

Indeed it does. Yet - does that move have to happen in
domain_destroy()? I.e. can't it be pulled even further ahead into
domain_kill()? That one already is preemptable/resumable (via
guarantees we expect from the tools side iirc), since
domain_relinquish_resources() may take quite long a time. Of course
that'll work only if no failure condition in sched_move_domain() is
permanent.

Jan

Re: [PATCH] cpupools: retry cpupool-destroy if domain in cpupool is dying

[Juergen Gross]

On 14.05.2014 14:28, Jan Beulich wrote:
>>>> On 14.05.14 at 12:35, <juergen.gross@ts.fujitsu.com> wrote:
>> On 14.05.2014 12:15, Jan Beulich wrote:
>>> What prevents cpupool_rm_domain() getting moved from
>>> complete_domain_destroy() to domain_destroy(), before the
>>> domain gets taken off the list? I actually assume that there are
>>> more things here that may not really need deferring until the
>>> last possible moment...
>>
>> sched_destroy_vcpu() and sched_destroy_domain() have to happen before
>> cpupool_rm_domain(). This could be avoided if the domain would be moved to
>> cpupool0 in domain_destroy().
>>
>> Hmm, doesn't sound too bad. This would be just symmetrical to domain
>> creation. What do you think?
>
> I'm always in favor of symmetry, where possible and suitable. So
> unless George objects or sees problems with this, why don't you
> give this a try?

One problem arises: sched_move_domain() can fail. Is there a preferred way to
handle this situation in domain_destroy() ? I could try to defer destroying
the domain until sched_move_domain() succeeds, but using a busy loop doing this
seems contra-productive and a timer based solution requires a timer structure.

I could reuse the domain watchdog_timer entries if I move
watchdog_domain_destroy() to domain_destroy() (which seems to be not critical).

OTOH this seems a little bit hacky...


Juergen

-- 
Juergen Gross                 Principal Developer Operating Systems
PSO PM&D ES&S SWE OS6                  Telephone: +49 (0) 89 62060 2932
Fujitsu                                   e-mail: juergen.gross@ts.fujitsu.com
Mies-van-der-Rohe-Str. 8                Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html

Re: [PATCH] cpupools: retry cpupool-destroy if domain in cpupool is dying

[Tim Deegan]

At 10:50 +0100 on 14 May (1400061034), George Dunlap wrote:
> On Wed, May 14, 2014 at 10:48 AM, George Dunlap
> <George.Dunlap@eu.citrix.com> wrote:
> > On Wed, May 14, 2014 at 10:16 AM, George Dunlap
> > <George.Dunlap@eu.citrix.com> wrote:
> >> On Mon, May 12, 2014 at 12:49 PM, Juergen Gross
> >> <juergen.gross@ts.fujitsu.com> wrote:
> >>> When a cpupool is destroyed just after the last domain has been stopped the
> >>> domain might already be removed from the domain list without being removed
> >>> from the cpupool.
> >>> It is easy to detect this situation and to return EAGAIN in this case which
> >>> is already handled in libxc by doing a retry.
> >>
> >> OK, I hate to be picky over two lines, but it still seems to me like
> >> this is papering over issues instead of dealing with them properly.
> >> The real problem here is that "for_each_domain_in_cpupool()" doesn't
> >> actually go over every domain in the cpupool.  Instead of making it so
> >> that it actually does, you're compensating for that fact in an ad-hoc
> >> fashion.
> >>
> >> Now as it happens, it looks like all the other current uses of
> >> for_each_domain_in_cpupool() work just fine if there are domains in
> >> the pool it doesn't see, as long as they're about to disappear.  But
> >> we've already seen a bug caused because of a situation where "don't
> >> see domains that are about to disappear" *does* actually cause a
> >> problem; working around it is just setting a trap for future
> >> developers to fall into.  (And who knows, there may already be a bug
> >> we haven't discovered in the other invocations of
> >> for_each_domain_in_cpupool()).
> >
> > Really this seems like a race in our rcu implementation wrt the domain
> > list.  It seems like ideally, if you grab the domlist_read_lock, you
> > should either get the domain on the list, or the domain off the list
> > *and* complete_domain_destroy() completed...

I don't think that's somethng that can be done with RCU.  The
guarantee you get as a reader is that if you _do_ see the domain on
the list, complete_domain_destroy() _hasn't_ been called (and in
particular the domain struct hasn't been freed).

To guarantee that any domain you _don't_ see _has_ been destroyed would
need a full mutex that the caller of complete_domain_destroy() could
hold to exclude you.

Tim.

Re: [PATCH] cpupools: retry cpupool-destroy if domain in cpupool is dying

[Jan Beulich]

>>> On 14.05.14 at 12:35, <juergen.gross@ts.fujitsu.com> wrote:
> On 14.05.2014 12:15, Jan Beulich wrote:
>> What prevents cpupool_rm_domain() getting moved from
>> complete_domain_destroy() to domain_destroy(), before the
>> domain gets taken off the list? I actually assume that there are
>> more things here that may not really need deferring until the
>> last possible moment...
> 
> sched_destroy_vcpu() and sched_destroy_domain() have to happen before
> cpupool_rm_domain(). This could be avoided if the domain would be moved to
> cpupool0 in domain_destroy().
> 
> Hmm, doesn't sound too bad. This would be just symmetrical to domain
> creation. What do you think?

I'm always in favor of symmetry, where possible and suitable. So
unless George objects or sees problems with this, why don't you
give this a try?

Jan

Re: [PATCH] cpupools: retry cpupool-destroy if domain in cpupool is dying

[Juergen Gross]

On 14.05.2014 12:15, Jan Beulich wrote:
>>>> On 14.05.14 at 11:56, <juergen.gross@ts.fujitsu.com> wrote:
>> On 14.05.2014 11:16, George Dunlap wrote:
>>> On Mon, May 12, 2014 at 12:49 PM, Juergen Gross
>>> <juergen.gross@ts.fujitsu.com> wrote:
>>>> When a cpupool is destroyed just after the last domain has been stopped the
>>>> domain might already be removed from the domain list without being removed
>>>> from the cpupool.
>>>> It is easy to detect this situation and to return EAGAIN in this case which
>>>> is already handled in libxc by doing a retry.
>>>
>>> OK, I hate to be picky over two lines, but it still seems to me like
>>> this is papering over issues instead of dealing with them properly.
>>> The real problem here is that "for_each_domain_in_cpupool()" doesn't
>>> actually go over every domain in the cpupool.  Instead of making it so
>>> that it actually does, you're compensating for that fact in an ad-hoc
>>> fashion.
>>>
>>> Now as it happens, it looks like all the other current uses of
>>> for_each_domain_in_cpupool() work just fine if there are domains in
>>> the pool it doesn't see, as long as they're about to disappear.  But
>>> we've already seen a bug caused because of a situation where "don't
>>> see domains that are about to disappear" *does* actually cause a
>>> problem; working around it is just setting a trap for future
>>> developers to fall into.  (And who knows, there may already be a bug
>>> we haven't discovered in the other invocations of
>>> for_each_domain_in_cpupool()).
>>
>> This isn't unique to for_each_domain_in_cpupool(). It is a problem for all
>> uses of for_each_domain() which are related to resources freed only in
>> complete_domain_destroy().
>
> Of which there shouldn't be that many, if any at all.
>
> What prevents cpupool_rm_domain() getting moved from
> complete_domain_destroy() to domain_destroy(), before the
> domain gets taken off the list? I actually assume that there are
> more things here that may not really need deferring until the
> last possible moment...

sched_destroy_vcpu() and sched_destroy_domain() have to happen before
cpupool_rm_domain(). This could be avoided if the domain would be moved to
cpupool0 in domain_destroy().

Hmm, doesn't sound too bad. This would be just symmetrical to domain
creation. What do you think?


Juergen

-- 
Juergen Gross                 Principal Developer Operating Systems
PSO PM&D ES&S SWE OS6                  Telephone: +49 (0) 89 62060 2932
Fujitsu                                   e-mail: juergen.gross@ts.fujitsu.com
Mies-van-der-Rohe-Str. 8                Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html

Re: [PATCH] cpupools: retry cpupool-destroy if domain in cpupool is dying

[George Dunlap]

On 05/14/2014 11:15 AM, Jan Beulich wrote:
>>>> On 14.05.14 at 11:56, <juergen.gross@ts.fujitsu.com> wrote:
>> On 14.05.2014 11:16, George Dunlap wrote:
>>> On Mon, May 12, 2014 at 12:49 PM, Juergen Gross
>>> <juergen.gross@ts.fujitsu.com> wrote:
>>>> When a cpupool is destroyed just after the last domain has been stopped the
>>>> domain might already be removed from the domain list without being removed
>>>> from the cpupool.
>>>> It is easy to detect this situation and to return EAGAIN in this case which
>>>> is already handled in libxc by doing a retry.
>>> OK, I hate to be picky over two lines, but it still seems to me like
>>> this is papering over issues instead of dealing with them properly.
>>> The real problem here is that "for_each_domain_in_cpupool()" doesn't
>>> actually go over every domain in the cpupool.  Instead of making it so
>>> that it actually does, you're compensating for that fact in an ad-hoc
>>> fashion.
>>>
>>> Now as it happens, it looks like all the other current uses of
>>> for_each_domain_in_cpupool() work just fine if there are domains in
>>> the pool it doesn't see, as long as they're about to disappear.  But
>>> we've already seen a bug caused because of a situation where "don't
>>> see domains that are about to disappear" *does* actually cause a
>>> problem; working around it is just setting a trap for future
>>> developers to fall into.  (And who knows, there may already be a bug
>>> we haven't discovered in the other invocations of
>>> for_each_domain_in_cpupool()).
>> This isn't unique to for_each_domain_in_cpupool(). It is a problem for all
>> uses of for_each_domain() which are related to resources freed only in
>> complete_domain_destroy().
> Of which there shouldn't be that many, if any at all.
>
> What prevents cpupool_rm_domain() getting moved from
> complete_domain_destroy() to domain_destroy(), before the
> domain gets taken off the list? I actually assume that there are
> more things here that may not really need deferring until the
> last possible moment...

Well the domain needs to be taken out of the scheduler for one, which is 
done by sched_destroy_domain().  That's where I stopped the "dependency 
tracking", as it seemed somewhat likely that somewhere there would be 
things that depended on having a valid scheduler. (That may not be 
correct, but it would take a careful reading to convince yourself it was 
true.)

  -George

Re: [PATCH] cpupools: retry cpupool-destroy if domain in cpupool is dying

[Jan Beulich]

>>> On 14.05.14 at 11:56, <juergen.gross@ts.fujitsu.com> wrote:
> On 14.05.2014 11:16, George Dunlap wrote:
>> On Mon, May 12, 2014 at 12:49 PM, Juergen Gross
>> <juergen.gross@ts.fujitsu.com> wrote:
>>> When a cpupool is destroyed just after the last domain has been stopped the
>>> domain might already be removed from the domain list without being removed
>>> from the cpupool.
>>> It is easy to detect this situation and to return EAGAIN in this case which
>>> is already handled in libxc by doing a retry.
>>
>> OK, I hate to be picky over two lines, but it still seems to me like
>> this is papering over issues instead of dealing with them properly.
>> The real problem here is that "for_each_domain_in_cpupool()" doesn't
>> actually go over every domain in the cpupool.  Instead of making it so
>> that it actually does, you're compensating for that fact in an ad-hoc
>> fashion.
>>
>> Now as it happens, it looks like all the other current uses of
>> for_each_domain_in_cpupool() work just fine if there are domains in
>> the pool it doesn't see, as long as they're about to disappear.  But
>> we've already seen a bug caused because of a situation where "don't
>> see domains that are about to disappear" *does* actually cause a
>> problem; working around it is just setting a trap for future
>> developers to fall into.  (And who knows, there may already be a bug
>> we haven't discovered in the other invocations of
>> for_each_domain_in_cpupool()).
> 
> This isn't unique to for_each_domain_in_cpupool(). It is a problem for all
> uses of for_each_domain() which are related to resources freed only in
> complete_domain_destroy().

Of which there shouldn't be that many, if any at all.

What prevents cpupool_rm_domain() getting moved from
complete_domain_destroy() to domain_destroy(), before the
domain gets taken off the list? I actually assume that there are
more things here that may not really need deferring until the
last possible moment...

Jan

Re: [PATCH] cpupools: retry cpupool-destroy if domain in cpupool is dying

[Juergen Gross]

On 14.05.2014 11:16, George Dunlap wrote:
> On Mon, May 12, 2014 at 12:49 PM, Juergen Gross
> <juergen.gross@ts.fujitsu.com> wrote:
>> When a cpupool is destroyed just after the last domain has been stopped the
>> domain might already be removed from the domain list without being removed
>> from the cpupool.
>> It is easy to detect this situation and to return EAGAIN in this case which
>> is already handled in libxc by doing a retry.
>
> OK, I hate to be picky over two lines, but it still seems to me like
> this is papering over issues instead of dealing with them properly.
> The real problem here is that "for_each_domain_in_cpupool()" doesn't
> actually go over every domain in the cpupool.  Instead of making it so
> that it actually does, you're compensating for that fact in an ad-hoc
> fashion.
>
> Now as it happens, it looks like all the other current uses of
> for_each_domain_in_cpupool() work just fine if there are domains in
> the pool it doesn't see, as long as they're about to disappear.  But
> we've already seen a bug caused because of a situation where "don't
> see domains that are about to disappear" *does* actually cause a
> problem; working around it is just setting a trap for future
> developers to fall into.  (And who knows, there may already be a bug
> we haven't discovered in the other invocations of
> for_each_domain_in_cpupool()).

This isn't unique to for_each_domain_in_cpupool(). It is a problem for all
uses of for_each_domain() which are related to resources freed only in
complete_domain_destroy().

In theory I could built a domain list for each cpupool which I could use for
for_each_domain_in_cpupool(). In this case there would be situations when
for_each_domain_in_cpupool() sees a domain which isn't seen by
for_each_domain(). Do you think this would be better? I don't.


Juergen

-- 
Juergen Gross                 Principal Developer Operating Systems
PSO PM&D ES&S SWE OS6                  Telephone: +49 (0) 89 62060 2932
Fujitsu                                   e-mail: juergen.gross@ts.fujitsu.com
Mies-van-der-Rohe-Str. 8                Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html

Re: [PATCH] cpupools: retry cpupool-destroy if domain in cpupool is dying

[George Dunlap]

On Wed, May 14, 2014 at 10:48 AM, George Dunlap
<George.Dunlap@eu.citrix.com> wrote:
> On Wed, May 14, 2014 at 10:16 AM, George Dunlap
> <George.Dunlap@eu.citrix.com> wrote:
>> On Mon, May 12, 2014 at 12:49 PM, Juergen Gross
>> <juergen.gross@ts.fujitsu.com> wrote:
>>> When a cpupool is destroyed just after the last domain has been stopped the
>>> domain might already be removed from the domain list without being removed
>>> from the cpupool.
>>> It is easy to detect this situation and to return EAGAIN in this case which
>>> is already handled in libxc by doing a retry.
>>
>> OK, I hate to be picky over two lines, but it still seems to me like
>> this is papering over issues instead of dealing with them properly.
>> The real problem here is that "for_each_domain_in_cpupool()" doesn't
>> actually go over every domain in the cpupool.  Instead of making it so
>> that it actually does, you're compensating for that fact in an ad-hoc
>> fashion.
>>
>> Now as it happens, it looks like all the other current uses of
>> for_each_domain_in_cpupool() work just fine if there are domains in
>> the pool it doesn't see, as long as they're about to disappear.  But
>> we've already seen a bug caused because of a situation where "don't
>> see domains that are about to disappear" *does* actually cause a
>> problem; working around it is just setting a trap for future
>> developers to fall into.  (And who knows, there may already be a bug
>> we haven't discovered in the other invocations of
>> for_each_domain_in_cpupool()).
>
> Really this seems like a race in our rcu implementation wrt the domain
> list.  It seems like ideally, if you grab the domlist_read_lock, you
> should either get the domain on the list, or the domain off the list
> *and* complete_domain_destroy() completed...

Sorry, should cc' the relevant maintainers for this question...

 -George

Re: [PATCH] cpupools: retry cpupool-destroy if domain in cpupool is dying

[George Dunlap]

On Wed, May 14, 2014 at 10:16 AM, George Dunlap
<George.Dunlap@eu.citrix.com> wrote:
> On Mon, May 12, 2014 at 12:49 PM, Juergen Gross
> <juergen.gross@ts.fujitsu.com> wrote:
>> When a cpupool is destroyed just after the last domain has been stopped the
>> domain might already be removed from the domain list without being removed
>> from the cpupool.
>> It is easy to detect this situation and to return EAGAIN in this case which
>> is already handled in libxc by doing a retry.
>
> OK, I hate to be picky over two lines, but it still seems to me like
> this is papering over issues instead of dealing with them properly.
> The real problem here is that "for_each_domain_in_cpupool()" doesn't
> actually go over every domain in the cpupool.  Instead of making it so
> that it actually does, you're compensating for that fact in an ad-hoc
> fashion.
>
> Now as it happens, it looks like all the other current uses of
> for_each_domain_in_cpupool() work just fine if there are domains in
> the pool it doesn't see, as long as they're about to disappear.  But
> we've already seen a bug caused because of a situation where "don't
> see domains that are about to disappear" *does* actually cause a
> problem; working around it is just setting a trap for future
> developers to fall into.  (And who knows, there may already be a bug
> we haven't discovered in the other invocations of
> for_each_domain_in_cpupool()).

Really this seems like a race in our rcu implementation wrt the domain
list.  It seems like ideally, if you grab the domlist_read_lock, you
should either get the domain on the list, or the domain off the list
*and* complete_domain_destroy() completed...

 -George

Re: [PATCH] cpupools: retry cpupool-destroy if domain in cpupool is dying

[George Dunlap]

On Mon, May 12, 2014 at 12:49 PM, Juergen Gross
<juergen.gross@ts.fujitsu.com> wrote:
> When a cpupool is destroyed just after the last domain has been stopped the
> domain might already be removed from the domain list without being removed
> from the cpupool.
> It is easy to detect this situation and to return EAGAIN in this case which
> is already handled in libxc by doing a retry.

OK, I hate to be picky over two lines, but it still seems to me like
this is papering over issues instead of dealing with them properly.
The real problem here is that "for_each_domain_in_cpupool()" doesn't
actually go over every domain in the cpupool.  Instead of making it so
that it actually does, you're compensating for that fact in an ad-hoc
fashion.

Now as it happens, it looks like all the other current uses of
for_each_domain_in_cpupool() work just fine if there are domains in
the pool it doesn't see, as long as they're about to disappear.  But
we've already seen a bug caused because of a situation where "don't
see domains that are about to disappear" *does* actually cause a
problem; working around it is just setting a trap for future
developers to fall into.  (And who knows, there may already be a bug
we haven't discovered in the other invocations of
for_each_domain_in_cpupool()).

 -George

[PATCH] cpupools: retry cpupool-destroy if domain in cpupool is dying

[Juergen Gross]

When a cpupool is destroyed just after the last domain has been stopped the
domain might already be removed from the domain list without being removed
from the cpupool.
It is easy to detect this situation and to return EAGAIN in this case which
is already handled in libxc by doing a retry.

Signed-off-by: Juergen Gross <juergen.gross@ts.fujitsu.com>
---
 xen/common/cpupool.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
index 4a0e569..ac833f0 100644
--- a/xen/common/cpupool.c
+++ b/xen/common/cpupool.c
@@ -348,6 +348,8 @@ static int cpupool_unassign_cpu(struct cpupool *c, unsigned int cpu)
             cpupool0->n_dom++;
         }
         rcu_read_unlock(&domlist_read_lock);
+        if ( (c->n_dom > 0) && !ret )
+            ret = -EAGAIN;
         if ( ret )
             goto out;
     }
-- 
1.7.10.4