Re: [Xen-devel] [PATCH v4 15/18] xen/mem_sharing: VM forking

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Thu, Jan 9, 2020 at 9:03 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 09.01.2020 16:57, Tamas K Lengyel wrote:
> > On Thu, Jan 9, 2020 at 8:34 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >>
> >> On 09.01.2020 16:10, Roger Pau Monné wrote:
> >>> On Thu, Jan 09, 2020 at 06:41:12AM -0700, Tamas K Lengyel wrote:
> >>>> On Thu, Jan 9, 2020 at 3:29 AM Julien Grall <julien@xxxxxxx> wrote:
> >>>>>
> >>>>> Hi Tamas,
> >>>>>
> >>>>> On 08/01/2020 17:14, Tamas K Lengyel wrote:
> >>>>>> +static int mem_sharing_fork(struct domain *d, struct domain *cd)
> >>>>>> +{
> >>>>>> +    int rc;
> >>>>>> +
> >>>>>> +    if ( !d->controller_pause_count &&
> >>>>>> +         (rc = domain_pause_by_systemcontroller(d)) )
> >>>>>
> >>>>> AFAIU, the parent domain will be paused if it wasn't paused before and
> >>>>> this will not be unpaused by the same hypercall. Right?
> >>>>
> >>>> Yes, it needs to remain paused as long as there are forks active from
> >>>> it. Afterwards it can be unpaused.
> >>>
> >>> If you want the parent domain to remain paused for as long as the
> >>> forks are active, shouldn't each fork increment the pause count on
> >>> creation and decrement it when the fork is destroyed?
> >>>
> >>> How can you assure no other operation or entity has incremented
> >>> controller_pause_count temporary and is likely to decrement it at some
> >>> point while forks are still active?
> >>
> >> The _by_systemcontroller variants look wrong to be used here anyway.
> >> Why is this not simply domain_{,un}pause()?
> >>
> >
> > My reasoning was that by default the user should pause the parent VM
> > before forking. This sanity checks just mimicks that step in case the
> > user didn't do that already. But I guess either would work, I don't
> > really see much difference between the two.
>
> The main difference is that the one you currently use updates
> d->controller_pause_count, which can be updated by other domctls, but
> which shouldn't be updated behind the back of a component in Xen which
> needs the entity paused.
>

Alright, I'll switch it.

Thanks,
Tamas

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