[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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.

Wei.

> 
> Thanks,
> Tamas



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.