Re: [Xen-devel] [PATCH v12 2/3] x86/mem_sharing: reset a fork

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Thu, Mar 26, 2020 at 8:53 AM Tamas K Lengyel <tamas@xxxxxxxxxxxxx> wrote:
>
> On Thu, Mar 26, 2020 at 8:52 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >
> > On 26.03.2020 15:48, Tamas K Lengyel wrote:
> > > On Thu, Mar 26, 2020 at 4:17 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> > >>
> > >> On 23.03.2020 18:04, Tamas K Lengyel wrote:
> > >>> +static int mem_sharing_fork_reset(struct domain *d, struct domain *pd)
> > >>> +{
> > >>> +    int rc;
> > >>> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > >>> +    struct page_info *page, *tmp;
> > >>> +
> > >>> +    spin_lock(&d->page_alloc_lock);
> > >>> +    domain_pause(d);
> > >>
> > >> Why do you take the lock first?
> > >
> > > No particular reason - does the order matter?
> >
> > I think you'd better avoid holding a lock for extended periods
> > of time. And what's perhaps worse, what if a vCPU of the domain
> > sits in Xen trying to acquire this lock - you'd deadlock trying
> > to pause the domain then.
>
> OK, I'll invert them order then.

It turns out we also need to take the recursive lock here since we'll
free the pages. Fixed now and everything works as expected.

Tamas