From mboxrd@z Thu Jan 1 00:00:00 1970 From: Juergen Gross Subject: Re: [PATCH] cpupools: retry cpupool-destroy if domain in cpupool is dying Date: Mon, 12 May 2014 13:31:27 +0200 Message-ID: <5370B10F.2080308@ts.fujitsu.com> References: <1399449149-5531-1-git-send-email-juergen.gross@ts.fujitsu.com> <536A33B5.7000908@ts.fujitsu.com> <536C6142.30904@ts.fujitsu.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: George Dunlap Cc: "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 12.05.2014 12:50, George Dunlap wrote: > On Fri, May 9, 2014 at 6:01 AM, Juergen Gross > wrote: >> On 08.05.2014 17:10, George Dunlap wrote: >>> >>> On Wed, May 7, 2014 at 2:23 PM, Juergen Gross >>> wrote: >>>> >>>> On 07.05.2014 15:10, George Dunlap wrote: >>>>> >>>>> >>>>> On Wed, May 7, 2014 at 8:52 AM, Juergen Gross >>>>> 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