Re: [Xen-devel] [PATCH v9 3/5] xen/mem_sharing: VM forking

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Tue, Feb 25, 2020 at 6:28 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 24.02.2020 13:39, Roger Pau Monné wrote:
> > On Fri, Feb 21, 2020 at 10:49:21AM -0800, Tamas K Lengyel wrote:
> >> +static int mem_sharing_fork(struct domain *d, struct domain *cd)
> >> +{
> >> +    int rc = -EINVAL;
> >> +
> >> +    if ( !cd->controller_pause_count )
> >> +        return rc;
> >
> > -EBUSY might be better here.
> >
> >> +
> >> +    /*
> >> +     * We only want to get and pause the parent once, not each time this
> >> +     * operation is restarted due to preemption.
> >> +     */
> >> +    if ( !cd->parent_paused )
> >> +    {
> >> +        ASSERT(get_domain(d));
> >
> > We are trying to avoid such constructs, instead I suggest:
> >
> > if ( !get_domain(parent) )
> > {
> >     ASSERT_UNREACHABLE();
> >     return -EBUSY;
> > }
>
> But isn't the ASSERT() here wrong anyway? I.e. what is it that
> guarantees that d hasn't gone away? If it's the caller of this
> function, then wouldn't it be get_knownalive_domain() that
> wants using here?

During the fork hypercall the parent's pause count is incremented via
domain_pause. It can't go away until the reference count goes down to
zero, which only happens when it's forks are all gone.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel