On Thu, Mar 26, 2020 at 8:53 AM Tamas K Lengyel <ta...@tklengyel.com> wrote:
>
> On Thu, Mar 26, 2020 at 8:52 AM Jan Beulich <jbeul...@suse.com> wrote:
> >
> > On 26.03.2020 15:48, Tamas K Lengyel wrote:
> > > On Thu, Mar 26, 2020 at 4:17 AM Jan Beulich <jbeul...@suse.com> 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
