Re: [PATCH v14 3/3] xen/tools: VM forking toolstack side

[Wei Liu <wl@xxxxxxx>]

On Fri, Apr 10, 2020 at 10:20:49AM -0600, Tamas K Lengyel wrote:
> On Fri, Apr 10, 2020 at 10:19 AM Wei Liu <wl@xxxxxxx> wrote:
> >
> > On Fri, Apr 10, 2020 at 10:05:50AM -0600, Tamas K Lengyel wrote:
> > > On Thu, Apr 9, 2020 at 11:30 AM Tamas K Lengyel
> > > <tamas.k.lengyel@xxxxxxxxx> wrote:
> > > >
> > > > On Thu, Apr 9, 2020 at 11:11 AM Wei Liu <wl@xxxxxxx> wrote:
> > > > >
> > > > > On Thu, Apr 09, 2020 at 10:59:55AM -0600, Tamas K Lengyel wrote:
> > > > > [...]
> > > > > > >
> > > > > > > >
> > > > > > > > > >
> > > > > > > > > > +/*
> > > > > > > > > > + * The parent domain is expected to be created with 
> > > > > > > > > > default settings for
> > > > > > > > > > + * - max_evtch_port
> > > > > > > > > > + * - max_grant_frames
> > > > > > > > > > + * - max_maptrack_frames
> > > > > > > > > > + */
> > > > > > > > > > +int libxl_domain_fork_vm(libxl_ctx *ctx, uint32_t pdomid, 
> > > > > > > > > > uint32_t max_vcpus, uint32_t *domid)
> > > > > > > > > > +{
> > > > > > > > > > +    int rc;
> > > > > > > > > > +    struct xen_domctl_createdomain create = {0};
> > > > > > > > > > +    create.flags |= XEN_DOMCTL_CDF_hvm;
> > > > > > > > > > +    create.flags |= XEN_DOMCTL_CDF_hap;
> > > > > > > > > > +    create.flags |= XEN_DOMCTL_CDF_oos_off;
> > > > > > > > > > +    create.arch.emulation_flags = (XEN_X86_EMU_ALL & 
> > > > > > > > > > ~XEN_X86_EMU_VPCI);
> > > > > > > > > > +    create.ssidref = SECINITSID_DOMU;
> > > > > > > > > > +    create.max_vcpus = max_vcpus;
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Some questions:
> > > > > > > > >
> > > > > > > > > Why does the caller need to specify the number of vcpus?
> > > > > > > > >
> > > > > > > > > Since the parent (source) domain is around, can you retrieve 
> > > > > > > > > its domain
> > > > > > > > > configuration such that you know its max_vcpus and other 
> > > > > > > > > information
> > > > > > > > > including max_evtchn_port and maptrack frames?
> > > > > > > >
> > > > > > > > Because we want to avoid having to issue an extra hypercall for 
> > > > > > > > these.
> > > > > > > > Normally these pieces of information will be available for the 
> > > > > > > > user
> > > > > > > > and won't change, so we save time by not querying for it every 
> > > > > > > > time a
> > > > > > > > fork is created. Remember, we might be creating thousands of 
> > > > > > > > forks in
> > > > > > > > a very short time, so those extra hypercalls add up.
> > > > > > >
> > > > > > > I see. Speed is a big concern to you.
> > > > > > >
> > > > > > > What I was referring to doesn't require issuing hypercalls but 
> > > > > > > requires
> > > > > > > calling libxl_retrieve_domain_configuration. That's likely to be 
> > > > > > > even
> > > > > > > slower than making a hypercall.
> > > > > >
> > > > > > Right. We only want to parse the domain config if the device model 
> > > > > > is
> > > > > > being launched.
> > > > > >
> > > > > > >
> > > > > > > I'm afraid the current incarnation of libxl_domain_fork_vm cannot 
> > > > > > > become
> > > > > > > supported (as in Xen's support statement) going forward, because 
> > > > > > > it is
> > > > > > > leaky.
> > > > > >
> > > > > > What do you mean by leaky?
> > > > >
> > > > > It requires the caller to specify the number of max_vcpus. The reason
> > > > > for doing that is because you want to avoid extra hypercall(s). There 
> > > > > is
> > > > > nothing that stops someone from coming along in the future claiming 
> > > > > some
> > > > > other parameters are required because of the same concern you have
> > > > > today. It is an optimisation, not a must-have in terms of 
> > > > > functionality.
> > > > >
> > > > > To me the number shouldn't be specified by the caller in the first
> > > > > place. It is like forking a process somehow requires you to specify 
> > > > > how
> > > > > many threads it will have.
> > > >
> > > > I agree. It's not how I wanted to have the interface work but
> > > > unfortunately this was the least "ugly" of the possible solutions
> > > > given the circumstances.
> > >
> > > By the way, it would be trivial to query the parent in case max_vcpus
> > > is not provided by the user. But I would still like to keep the option
> > > available to skip that step if the number is known already. Let me
> > > know if you would like me to add that.
> >
> > I'm still waiting for Ian and Anthony to chime in to see whether they
> > agree to keep this interface unstable.
> >
> > At the end of the day, if it is unstable, I don't really care that much.
> > I'm happy to let you (the developer and user) have more say in that
> > case.  I will instead divert my (limited) time to code that your patch
> > touches which is also used by other stable functions.
> 
> SGTM

Ian and Anthony, your opinions?

Wei.