On Fri, Jan 7, 2011 at 12:54 AM, Tim Deegan <Tim.Deegan@xxxxxxxxxx>
At 16:11 +0000 on 06 Jan (1294330319), Jui-Hao Chiang wrote:I'm going to apply this, since it looks like an improvement, but I'm not
> Hi, this patch does the following
> (1) When updating/checking p2m type for mem_sharing, we must hold shr_lock
> (2) For nominate operation, if the page is already nominated, return the handle from page_info->shr_handle
> (3) For unshare operation, it is possible that multiple users unshare a page via hvm_hap_nested_page_fault() at the same time. If the page is already un-shared by someone else, simply return success.
convinced it properly solves the problem.
It seems tinnycloud's case is when dom0 try to RW maps a shared page, which should unshare it properly, and change the type count.
But there is still a bug hidden in page_make_sharable() which fails to recover type count when the call fails.
Now I trace it again, and found something different than what we have discussed before.
I will find it and submit patch again.
> NOTE: we assume that nobody holds page_alloc_lock/p2m_lock before calling nominate/share/unshare.As I told you earlier, that's not the case. p2m_teardown() can call
mem_sharing_unshare_page() with the p2m lock held.
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.