From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Jan Beulich" <JBeulich@novell.com>
Subject: Re: [RFC][PATCH] walking the page lists needs the
	page_alloc lock
Date: Thu, 12 Aug 2010 16:09:24 +0100
Message-ID: <4C642AC4020000780000F8D8@vpn.id2.novell.com>
References: <20100723134913.GQ13291@whitby.uk.xensource.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: quoted-printable
Return-path: <xen-devel-bounces@lists.xensource.com>
In-Reply-To: <20100723134913.GQ13291@whitby.uk.xensource.com>
Content-Disposition: inline
List-Unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xensource.com>
List-Help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-Subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
Sender: xen-devel-bounces@lists.xensource.com
Errors-To: xen-devel-bounces@lists.xensource.com
To: Tim Deegan <Tim.Deegan@citrix.com>
Cc: xen-devel@lists.xensource.com
List-Id: xen-devel@lists.xenproject.org

>>> On 23.07.10 at 15:49, Tim Deegan <Tim.Deegan@citrix.com> wrote:
> There are a few places in Xen where we walk a domain's page lists
> without holding the page_alloc lock.  They race with updates to the page
> lists, which are normally rare but can be quite common under PoD when
> the domain is close to its memory limit and the PoD reclaimer is busy.
> This patch protects those places by taking the page_alloc lock.
>=20
> I think this is OK for the two debug-key printouts - they don't run from
> irq context and look deadlock-free.  The tboot change seems safe too

While the comment says the patch would leave debug key printouts
alone, ...

> unless tboot shutdown functions are called from irq context or with the
> page_alloc lock held.  The p2m one is the scariest but there are already
> code paths in PoD that take the page_alloc lock with the p2m lock held
> so it's no worse than existing code.=20
>=20
> Signed-off-by: Tim Deegan <Tim.Deegan@citrix.com>
>=20
> diff -r e8dbc1262f52 xen/arch/x86/domain.c
> --- a/xen/arch/x86/domain.c	Wed Jul 21 09:02:10 2010 +0100
> +++ b/xen/arch/x86/domain.c	Fri Jul 23 14:33:22 2010 +0100
> @@ -139,12 +139,14 @@ void dump_pageframe_info(struct domain *

... the actual patch still touches a respective function. It would seem
to me that this part ought to be reverted.

>      }
>      else
>      {
> +        spin_lock(&d->page_alloc_lock);
>          page_list_for_each ( page, &d->page_list )
>          {
>              printk("    DomPage %p: caf=3D%08lx, taf=3D%" PRtype_info =
"\n",
>                     _p(page_to_mfn(page)),
>                     page->count_info, page->u.inuse.type_info);
>          }
> +        spin_unlock(&d->page_alloc_lock);
>      }
> =20
>      if ( is_hvm_domain(d) )
> @@ -152,12 +154,14 @@ void dump_pageframe_info(struct domain *
>          p2m_pod_dump_data(d);
>      }
> =20
> +    spin_lock(&d->page_alloc_lock);
>      page_list_for_each ( page, &d->xenpage_list )
>      {
>          printk("    XenPage %p: caf=3D%08lx, taf=3D%" PRtype_info "\n",
>                 _p(page_to_mfn(page)),
>                 page->count_info, page->u.inuse.type_info);
>      }
> +    spin_unlock(&d->page_alloc_lock);
>  }
> =20
>  struct domain *alloc_domain_struct(void)

Sorry for not noticing this earlier.

Jan