This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
Home Products Support Community News


Re: [Xen-devel] Cpu pools discussion

To: Keir Fraser <keir.fraser@xxxxxxxxxxxxx>
Subject: Re: [Xen-devel] Cpu pools discussion
From: Juergen Gross <juergen.gross@xxxxxxxxxxxxxx>
Date: Wed, 29 Jul 2009 13:06:59 +0200
Cc: Tim Deegan <Tim.Deegan@xxxxxxxxxxxxx>, George Dunlap <dunlapg@xxxxxxxxx>, Zhigang Wang <zhigang.x.wang@xxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Wed, 29 Jul 2009 04:07:36 -0700
Dkim-signature: v=1; a=rsa-sha256; c=simple/simple; d=ts.fujitsu.com; i=juergen.gross@xxxxxxxxxxxxxx; q=dns/txt; s=s1536b; t=1248865711; x=1280401711; h=from:sender:reply-to:subject:date:message-id:to:cc: mime-version:content-transfer-encoding:content-id: content-description:resent-date:resent-from:resent-sender: resent-to:resent-cc:resent-message-id:in-reply-to: references:list-id:list-help:list-unsubscribe: list-subscribe:list-post:list-owner:list-archive; z=From:=20Juergen=20Gross=20<juergen.gross@xxxxxxxxxxxxxx> |Subject:=20Re:=20[Xen-devel]=20Cpu=20pools=20discussion |Date:=20Wed,=2029=20Jul=202009=2013:06:59=20+0200 |Message-ID:=20<4A702D53.60707@xxxxxxxxxxxxxx>|To:=20Keir =20Fraser=20<keir.fraser@xxxxxxxxxxxxx>|CC:=20Tim=20Deega n=20<Tim.Deegan@xxxxxxxxxxxxx>,=20=0D=0A=20George=20Dunla p=20<dunlapg@xxxxxxxxx>,=0D=0A=20Zhigang=20Wang=20<zhigan g.x.wang@xxxxxxxxxx>,=20=0D=0A=20"xen-devel@xxxxxxxxxxxxx ce.com"=20<xen-devel@xxxxxxxxxxxxxxxxxxx>|MIME-Version: =201.0|Content-Transfer-Encoding:=207bit|In-Reply-To:=20< C695D67F.10DE9%keir.fraser@xxxxxxxxxxxxx>|References:=20< C695D67F.10DE9%keir.fraser@xxxxxxxxxxxxx>; bh=vdKbt1xu4fyf7S5PoZaTL1N1Kp6RPSTHwtUf6S9B2vQ=; b=GoK26U5HP/xNvTFlICCICmjM1/HU+eKGh2P/8v9wk0lMFRynU+nMTdaw 6+B+lFUTjwMlLH4gbZUBI9RbUIfaLAZMnVavhRKQd6rgnb1+6u+xmCjR5 25uPpokDyVbPv7sRtBDmaExJ8BWVHW/4uEUgWsINuXgpAODgCwvpXf/Hu iZTcuC9JSwYsNvMpU/Fj5Go1zp3X5lrMc/Cca+3mWaEVyg8B6ZGm6vHA0 uneAEwl8iYzB1USBr266odtEEPCrh;
Domainkey-signature: s=s1536a; d=ts.fujitsu.com; c=nofws; q=dns; h=X-SBRSScore:X-IronPort-AV:Received:X-IronPort-AV: Received:Received:Message-ID:Date:From:Organization: User-Agent:MIME-Version:To:CC:Subject:References: In-Reply-To:X-Enigmail-Version:Content-Type: Content-Transfer-Encoding; b=XEU18KeeJCGNWJyeQsH6ROTEI0upg2JIpOz1rBPtUZ1F5de+ZitToqdA CD4GCCmJMmlwF6Yxh+UsrCCkAFQxjIABpkydzdbKfsak8oUZOAzRjK3hu RjUZrWaVu+EogV8RdwAi7egEbfO2yaVymBtSXe0ML8CjHEPxCWAnGZ/Us rr/VinB9KeH8KGE65rs/tjImqPUGXuVv3qLWn2CYQl8uAFCL9MYkXg0o/ zh1GjDgkRl3x5gC2XEHXeW4oo/7XL;
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <C695D67F.10DE9%keir.fraser@xxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Organization: Fujitsu Technology Solutions
References: <C695D67F.10DE9%keir.fraser@xxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mozilla-Thunderbird (X11/20090707)
Keir Fraser wrote:
> On 29/07/2009 09:52, "Juergen Gross" <juergen.gross@xxxxxxxxxxxxxx> wrote:
>>>> I can add an explicit check not to unassign borrowed cpus, if you like.
>>> Your new interface ought to be responsible for its own synchronisation
>>> needs. And if it's not you should implement the appropriate assertions
>>> regarding e.g., spin_is_locked(), plus a code comment. It's simple
>>> negligence to do neither.
>> You are right.
>> I will add a check to ensure borrowed cpus are not allowed to change the 
>> pool.
> A couple more comments.
> It is not safe to domain_pause() while you hold locks. It can deadlock, as
> domain_pause() waits for the domain to be descheduled, but it could be
> spinning on a lock you hold. Also it looks like a domain can be moved away
> from a pool while the pool is paused, and then you would leak a pause
> refcount.
> Secondly, I think that the cpupool_borrow/return calls should be embedded
> within vcpu_{lock,unlock,locked_change}_affinity(); also I see no need to
> have cpupool_return_cpu() return anything as you should be able to make a
> decision to move onto another CPU on the next scheduling round anyway (which
> can always be forced by setting SCHEDULE_SOFTIRQ).
> Really I dislike this patch greatly, as you can tell. ;-) The patchset as a
> whole is *ginormous*, the Xen patch by itself is pretty big and complicated
> and I believe full of races and deadlocks. I've just picked up on a few
> obvious ones from a very brief read.

The main problems you mention here are related to the cpupool_borrow stuff,
which is the main objection of George, too (its not my favourite part of the
patch, too).

Would you feel better if I'd try to eliminate the reason for cpupool_borrow?
This function is needed only for continue_hypercall_on_cpu outside of the
current pool. I think it should be possible to replace those by
on_selected_cpus with less impact on the whole system.

I tried not to change any interfaces which are not directly related to the
pools in the first run. If the result of this approach forces you to reject
the patch, I would be happy to change it.
I agree with you it would be better not to need that borrow stuff, but I don't
know whether you would like the continue_hypercall_on_cpu elimination more
(or which solution would cause less pain).

The next step after that would be to split up the xen patch into logical
pieces. I would suggest to change the scheduler internals in a separate patch
(mainly the elimination of the local variables) to make the functional
changes required for the pools more obvious. This should reduce the pure
pool related patch by a factor of 2.

Regarding races: I tested the "normal" pool interfaces (cpu add/remove, domain
create/destroy/move) rather intensive (multiple concurrent scripts ran for
several hours). The cpu borrow stuff was NOT tested very much. There are 3
use cases for this interface:
- cpu microcode loading is running at system boot (this was my favourite test
- enter deep sleep only continues on cpu 0, which I removed only occasionally
  from pool 0
- I don't think I could test cpu hotplug...


Juergen Gross                 Principal Developer Operating Systems
TSP ES&S SWE OS6                       Telephone: +49 (0) 89 636 47950
Fujitsu Technolgy Solutions               e-mail: juergen.gross@xxxxxxxxxxxxxx
Otto-Hahn-Ring 6                        Internet: ts.fujitsu.com
D-81739 Muenchen                 Company details: ts.fujitsu.com/imprint.html

Xen-devel mailing list