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


[Xen-devel] Re: [PATCH] mem_sharing: fix race condition of nominate and

To: Tim Deegan <Tim.Deegan@xxxxxxxxxx>
Subject: [Xen-devel] Re: [PATCH] mem_sharing: fix race condition of nominate and unshare
From: Jui-Hao Chiang <juihaochiang@xxxxxxxxx>
Date: Fri, 7 Jan 2011 14:02:00 +0800
Cc: tinnycloud <tinnycloud@xxxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Thu, 06 Jan 2011 22:02:50 -0800
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:received:in-reply-to :references:date:message-id:subject:from:to:cc:content-type; bh=MeGzw8hFa+tmFrmFRiFg97RXZ8RGFGOrMBLp0zVIi+4=; b=YP4uegpzEQkyes2URigpxI1EbskrUUKQiq9718b1mzVFStvKghriqJAiCEWJTMNleq Fvogmb9Bq+ffILOzYrN8W1xLB1qAE98MjLQ7PuHbcfSBb+Kghj+7t80/CrQP6VGrObXd J1GPqdY7JujJOyAmddVJjas8VopUH9R4fj7xs=
Domainkey-signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; b=IwZjRwiU8254yjZ0YS/MnNtXTxII74i4MtIZYHevBkoYNv7H19p5BvimFkqI4oFtp5 C9sQ29ZDAviiHd6ufo+cJfQ9iBU65esLKF+sghvgOtTXO1S1SBKNU4Twri6zT9IpVkW3 E8gLOEEQOsEhr3+wHpBvrQlg76nMbEhrNFbVs=
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <AANLkTinmpiusLqegGZA+bZWpDXPM+7Wq2nt8MZa0Ocet@xxxxxxxxxxxxxx>
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>
References: <AANLkTinMp1v1zex2BfcUuszotPuxJFWZQNUp40gu_gxL@xxxxxxxxxxxxxx> <20110106165450.GO21948@xxxxxxxxxxxxxxxxxxxxxxx> <AANLkTinmpiusLqegGZA+bZWpDXPM+7Wq2nt8MZa0Ocet@xxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx

Oops, I forgot again.
After this change, unshare() has a potential problem of deadlock for shr_lock and p2m_lock with different locking order.
Assume two CPUs do the following
CPU1: hvm_hap_nested_page_fault() => unshare() => p2m_change_type() (locking order: shr_lock, p2m_lock)
CPU2: p2m_teardown() => unshare() (locking order: p2m_lock, shr_lock)
When CPU1 grabs shr_lock and CPU2 grabs p2m_lock, they deadlock later.

So it seems better to fix the following rules
(1) Fix locking order: p2m_lock ---> shr_lock
(2) Any function in mem_sharing, if modifying/checking p2m entry is necessary, it must hold p2m_lock and then shr_lock. Later on, when changing p2m entries, don't call any p2m function which locks p2m again

So for p2m functions, it seems better to provide some functions which don't call p2m_lock again.
What do you think? If that's ok, I will do it in this way.

Hmm,  after looking it deeper, I summarize as the following
(1) It seems all the users of shr_lock, nominate/share/unshare, will check/modify p2m type.
- nominate: p2m_change_type()
- share: set_shared_p2m_entry()
- unshare: set_shared_p2m_entry() and p2m_change_type()
(2) The functions which call unshare()
- hvm_hap_nested_page_fault(): I don't see any p2m_lock holded
- p2m_tear_down(): hold p2m_lock
- gfn_to_mfn_unshare(): I don't see any p2m_lock holded

One of the solution is to
(a) Simply replace shr_lock with p2m_lock.
(b) In unshare(), apply the following: if (!p2m_locked_by_me(p2m)) call p2m_lock, otherwise, don't lock it.
(c) p2m_change_type() and set_shared_p2m_entry() are pretty similar, we can merge the functionality into one function, which does NOT take p2m_lock, and keep the original p2m_change_type() unchanged.

Correct me if wrong.

Xen-devel mailing list