Re: [Xen-devel] [PATCH] libxl: Documentation about the domain configuration on disk
On Wed, Dec 19, 2018 at 03:02:36PM +, Ian Jackson wrote:
> Anthony PERARD writes ("Re: [PATCH] libxl: Documentation about the domain
> configuration on disk"):
> > I think there is already a race, and `xl destroy` can leak QEMU. I've
> > called `xl create` with a sleep before spawn_local_dm, and during the
> > sleep, I call `xl destroy` with a sleep after it had an oportunity to
> > kill QEMU. So we have:
> >
> > 1 domain creation xc_domain_create
> > 2 domain destruction doesn't kill qemu, it's not there yet.
> > 3 domain creation spawn qemu
> > 4 domain creation creates libxl-json
>
> I think the ordering of 3 vs 4 is a violation of my `thing' rules.
> This has gone unnoticed because we haven't been treating qemu itself
> as a `thing'.
>
> If we did treat qemu itself as a Thing, it would be necessary to hold
> the libxl-json lock while forking it. But fork is slow.
>
> > > Maybe qemu's existence is `primary non-qmp state' and in fact domain
> > > destruction is not allowed to destroy it without holding the
> > > libxl-json lock. But I bet that rule is not honoured right now.
> >
> > I think it's fine for domain destruction to kill QEMU without any lock.
> > Any threads communicating via QMP should receive an error.
>
> If qemu's existence is `primary qmp state' then the rules in my other
> mail imply holding the qmp lock for it while spawning it. I think
> that would be doable ?
Yes, I think that can be done. And we can probably hold the lock from
just before spawning QEMU, until we have created libxl-json.
> > > Comments, anyone ?
> >
> > That slow lock idea looks fine otherwise, we could call it
> > "libxl-qmp-lock" for now and have it mandatory when adding/removing
> > things via QMP. If a slow lock is needed for other thing than QMP, we
> > can change the meaning.
>
> I don't mind calling it the `libxl qmp lock' in code or documentation,
> but the actual filename needs to not to be `libxl-qmp-lock' because we
> cannot easily change it later. And there would be obvious advantages
> to making the name the same everywhere.
>
> Anyway, I hope the above observations make sense to you. Let me know
> what you think.
All those observations sounds good, I'll work on this new lock, try to
find a good name, and write some documentation on when it should be
hold.
Thanks,
--
Anthony PERARD
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] libxl: Documentation about the domain configuration on disk
Anthony PERARD writes ("Re: [PATCH] libxl: Documentation about the domain
configuration on disk"):
> I think there is already a race, and `xl destroy` can leak QEMU. I've
> called `xl create` with a sleep before spawn_local_dm, and during the
> sleep, I call `xl destroy` with a sleep after it had an oportunity to
> kill QEMU. So we have:
>
> 1 domain creation xc_domain_create
> 2 domain destruction doesn't kill qemu, it's not there yet.
> 3 domain creation spawn qemu
> 4 domain creation creates libxl-json
I think the ordering of 3 vs 4 is a violation of my `thing' rules.
This has gone unnoticed because we haven't been treating qemu itself
as a `thing'.
If we did treat qemu itself as a Thing, it would be necessary to hold
the libxl-json lock while forking it. But fork is slow.
> > Maybe qemu's existence is `primary non-qmp state' and in fact domain
> > destruction is not allowed to destroy it without holding the
> > libxl-json lock. But I bet that rule is not honoured right now.
>
> I think it's fine for domain destruction to kill QEMU without any lock.
> Any threads communicating via QMP should receive an error.
If qemu's existence is `primary qmp state' then the rules in my other
mail imply holding the qmp lock for it while spawning it. I think
that would be doable ?
> > Comments, anyone ?
>
> That slow lock idea looks fine otherwise, we could call it
> "libxl-qmp-lock" for now and have it mandatory when adding/removing
> things via QMP. If a slow lock is needed for other thing than QMP, we
> can change the meaning.
I don't mind calling it the `libxl qmp lock' in code or documentation,
but the actual filename needs to not to be `libxl-qmp-lock' because we
cannot easily change it later. And there would be obvious advantages
to making the name the same everywhere.
Anyway, I hope the above observations make sense to you. Let me know
what you think.
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] libxl: Documentation about the domain configuration on disk
On Fri, Dec 07, 2018 at 07:05:38PM +, Ian Jackson wrote:
> Wei Liu writes ("Re: [PATCH] libxl: Documentation about the domain
> configuration on disk"):
> > On Thu, Dec 06, 2018 at 02:57:33PM +, Anthony PERARD wrote:
> > > Anyway, that comment block isn't very helpful because it basically says
> > > that we can't depriv QEMU, I mean do hotplug with a deprived QEMU. It
> > > assumes that we can keep a lock on the userdata while updating the
> > > guest, but we can't keep the lock while talking with QEMU (or more
> > > generaly: we can't keep the lock while doing any async operation).
> > >
> > > But there is one useful piece of information:
> > > Here we maintain one invariant: every device in xenstore must have
> > > an entry in JSON file.
> > > (xenstore is describe as "primary reference" just before that sentence).
> >
> > Yes. That.
> ...
> > When removing a CD, you only need to update the primary source -- QEMU
> > in this case, you can leave libxl-json untouched. It is allowed to have
> > stale entries in libxl-json. This is implied in "We may not even need
> > this ..." further above.
>
> If you leave a state entry in libxl-json then you would reject
> attempts to insert a new cd, because the libxl-json would tell you one
> is already present.
>
> I think the lock difficulty Anthony identifies is real. We need to
> either develop a new set of locking rules, or make all acquisitions of
> the libxl-json lock slow.
>
>
> The invariant that we want to maintain is:
>
> * Nothing may exist in the primary config without
> a corresponding entry in libxl-json.
>
> The rules that implement that are:
>
> * No-one may edit the libxl-json without holding the lock.
>
> * You may not cause a thing to be added to the primary config
> unless you have held the libxl-json lock continuously
> since ensuring that the libxl-json config describes it.
>
> * Conversely you may not cause a thing to be removed from
> the libxl-json unless you have held the libxl-json lock
> continueously since ensuring the thing is absent
> from the primary config.
>
> And unfortunately much code acquiring the libxl-json lock expects it
> to be fast.
>
>
> How about the following scheme. We split the libxl-json lock into
> two. I'm going to call them the fast lock and the slow lock.
>
> * The fast lock is the existing libxl-json lock.
>
> * The slow lock is outside the libxl-json lock in the lock
> hierarchy. It is also outside the libxl_ctx lock. It is
> to be acquired by an ao event callback.
>
> * No-one may read or edit the libxl-json without holding the fast
> lock across their read operation, or their read/modify/write
> cycle.
>
> * However, there are special rules for thing removal/addition, for
> things added/removed via qmp. Call these `qmp things'. It is
> permissible to add or remove a qmp thing across two separate
> acquisitions of the fast lock, one to read the old state of the
> thing, and one to read/modify/write to update (only) the new state
> of the thing. This is subject to the thing add/removal rule, from
> before, which becomes:
>
> * You may not cause a thing to be added to the primary config
> unless you have held the relevant thing lock continuously
> since ensuring that the libxl-json config describes it.
>
> * Conversely you may not cause a thing to be removed from the
> libxl-json unless you have held the relevant thing lock
> continuously since ensuring the thing is absent from the primary
> config.
>
> * The `relevant thing lock' is the slow lock for qmp things, and the
> fast lock for other things.
>
> * Acquiring the fast lock fails for a destroyed domain, as at
> present.
I found out that this isn't exactly true. There is a short window where
a thread can aquire the fast lock for a domain that is about to be
destroyed. But this is fine as long as the thread tries to read the
userdata before doing anything else. (The window is between
userdata_destroyall and xc_domain_destroy.)
>
> I think this maintains the invariant.
>
> I haven't figured out domain destruction. Ideally domain destruction
> could happen without taking the slow lock.
>
> I think this is probably possible if we make sure that qemu is always
> killed before the libxl-json is removed. The result is that if a qmp
> thing operation races with domain destruction, and in its 1st read
> gets an existing libxl-json from before destruction, the qmp thing
> will, when it acquires the fast lock again, necessarily the libxl-json
> will not exist, and the qmp operation will bomb out.
I've check the domain destruction functions in libxl. And that appear to
be true from my understanding. Relevent actions taken in libxl, in that
order:
- kill QEMU
- userdata_destroyall
- xc_domain_destroy
> But I don't exactly know how this relates to domain creation. In
> general I haven't
Re: [Xen-devel] [PATCH] libxl: Documentation about the domain configuration on disk
Wei Liu writes ("Re: [PATCH] libxl: Documentation about the domain
configuration on disk"):
> On Thu, Dec 06, 2018 at 02:57:33PM +, Anthony PERARD wrote:
> > Anyway, that comment block isn't very helpful because it basically says
> > that we can't depriv QEMU, I mean do hotplug with a deprived QEMU. It
> > assumes that we can keep a lock on the userdata while updating the
> > guest, but we can't keep the lock while talking with QEMU (or more
> > generaly: we can't keep the lock while doing any async operation).
> >
> > But there is one useful piece of information:
> > Here we maintain one invariant: every device in xenstore must have
> > an entry in JSON file.
> > (xenstore is describe as "primary reference" just before that sentence).
>
> Yes. That.
...
> When removing a CD, you only need to update the primary source -- QEMU
> in this case, you can leave libxl-json untouched. It is allowed to have
> stale entries in libxl-json. This is implied in "We may not even need
> this ..." further above.
If you leave a state entry in libxl-json then you would reject
attempts to insert a new cd, because the libxl-json would tell you one
is already present.
I think the lock difficulty Anthony identifies is real. We need to
either develop a new set of locking rules, or make all acquisitions of
the libxl-json lock slow.
The invariant that we want to maintain is:
* Nothing may exist in the primary config without
a corresponding entry in libxl-json.
The rules that implement that are:
* No-one may edit the libxl-json without holding the lock.
* You may not cause a thing to be added to the primary config
unless you have held the libxl-json lock continuously
since ensuring that the libxl-json config describes it.
* Conversely you may not cause a thing to be removed from
the libxl-json unless you have held the libxl-json lock
continueously since ensuring the thing is absent
from the primary config.
And unfortunately much code acquiring the libxl-json lock expects it
to be fast.
How about the following scheme. We split the libxl-json lock into
two. I'm going to call them the fast lock and the slow lock.
* The fast lock is the existing libxl-json lock.
* The slow lock is outside the libxl-json lock in the lock
hierarchy. It is also outside the libxl_ctx lock. It is
to be acquired by an ao event callback.
* No-one may read or edit the libxl-json without holding the fast
lock across their read operation, or their read/modify/write
cycle.
* However, there are special rules for thing removal/addition, for
things added/removed via qmp. Call these `qmp things'. It is
permissible to add or remove a qmp thing across two separate
acquisitions of the fast lock, one to read the old state of the
thing, and one to read/modify/write to update (only) the new state
of the thing. This is subject to the thing add/removal rule, from
before, which becomes:
* You may not cause a thing to be added to the primary config
unless you have held the relevant thing lock continuously
since ensuring that the libxl-json config describes it.
* Conversely you may not cause a thing to be removed from the
libxl-json unless you have held the relevant thing lock
continuously since ensuring the thing is absent from the primary
config.
* The `relevant thing lock' is the slow lock for qmp things, and the
fast lock for other things.
* Acquiring the fast lock fails for a destroyed domain, as at
present.
I think this maintains the invariant.
I haven't figured out domain destruction. Ideally domain destruction
could happen without taking the slow lock.
I think this is probably possible if we make sure that qemu is always
killed before the libxl-json is removed. The result is that if a qmp
thing operation races with domain destruction, and in its 1st read
gets an existing libxl-json from before destruction, the qmp thing
will, when it acquires the fast lock again, necessarily the libxl-json
will not exist, and the qmp operation will bomb out.
But I don't exactly know how this relates to domain creation. In
general I haven't thought about races between domain creation and
domain destruction. We don't want a situation like this:
1 domain destruction tries to kill qemu (but it doesn't exist yet)
2 domain creation creates libxl-json and qemu
3 domain creation complets
4 qmp add reads libxl-json, sees thing absent
5 qmp add sends qmp command
6 qmp add gets qmp response and updates libxl-json
7 domain destruction deletes the libxl-json
8 domain destruction crashes before it destroys domain in Xen
Maybe qemu's existence is `primary non-qmp state' and in fact domain
destruction is not allowed to destroy it without holding the
libxl-json lock. But I bet that rule is not honoured right now.
Comments, anyone ?
> Anyway, I'm not overly opposed to adding some easy to grep pointers, but
>
Re: [Xen-devel] [PATCH] libxl: Documentation about the domain configuration on disk
On Thu, Dec 06, 2018 at 04:09:14PM +, Anthony PERARD wrote: > On Thu, Dec 06, 2018 at 03:46:22PM +, Wei Liu wrote: > > Anyway, I'm not overly opposed to adding some easy to grep pointers, but > > CODING_STYLE looks wrong to me. Maybe README.dev? > > To me, CODING_STYLE in libxl looks like a combination of both > CODING_STYLE and HACKING that exist in qemu.git. > > Maybe adding README.dev or HACKING might be ok. Or maybe adding pointers > in some places in libxl_internal.h might work too. Either README.dev or HACKING works for me. Wei. > > -- > Anthony PERARD ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] libxl: Documentation about the domain configuration on disk
On Thu, Dec 06, 2018 at 03:46:22PM +, Wei Liu wrote: > Anyway, I'm not overly opposed to adding some easy to grep pointers, but > CODING_STYLE looks wrong to me. Maybe README.dev? To me, CODING_STYLE in libxl looks like a combination of both CODING_STYLE and HACKING that exist in qemu.git. Maybe adding README.dev or HACKING might be ok. Or maybe adding pointers in some places in libxl_internal.h might work too. -- Anthony PERARD ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] libxl: Documentation about the domain configuration on disk
On Thu, Dec 06, 2018 at 02:57:33PM +, Anthony PERARD wrote: > On Thu, Dec 06, 2018 at 12:16:40PM +, Wei Liu wrote: > > On Thu, Dec 06, 2018 at 10:43:32AM +, Anthony PERARD wrote: > > > +UPDATE OF DOMAIN CONFIGURATION > > > +-- > > > + > > > +Also known as "libxl-json" userdata or `libxl_domain_config'. > > > + > > > +Whenever a running domain have its configuration updated, like changing > > > +media in a cdrom drive, the domain configuration in libxl private data > > > +store needs to be updated as well. The domain configuration should > > > +contain *more* information about the domain rather than less, stale data > > > +are easier to spot that missing data. > > > + > > > +Here is an example of how to update the domain configuration: > > > + * Remove current media from cdrom drive > > > + * Update domain configuration with media removed > > > > We may not even need this because the primary source in this case is > > QEMU. See below. > > > > > + ( we could stop here) > > > + * Update domain configuration to add media we are about to insert > > > + * Insert media into cdrom drive > > > > In essence we need a primary reference while using libxl-json file as a > > secondary source. > > > > When doing device hotplug, the primary source is xenstore. It may become > > QEMU in the future if we move to a model where everything is > > communicated via QMP. > > > > When doing CDROM insertion and rejection, the primary source is QEMU > > state. > > I'm not trying to figure out what primary source should be here, I'm > trying to find out how the secondary source, namely "libxl-json", should > behave, what it should contain, when to update it compare the primary > source (what a guest ultimately see). > > > All in all I think your description is not wrong but it failed to > > capture the high-level intent -- always update libxl-json before > > updating the primary source. > > That isn't what Ian said IRL, I don't think. From what I understand, > when removing a media/disk, first remove the media, then update > libxl-json; when adding a media/disk, first update libxl-json, then add > the media. OK I should have been clearer on this. When removing a CD, you only need to update the primary source -- QEMU in this case, you can leave libxl-json untouched. It is allowed to have stale entries in libxl-json. This is implied in "We may not even need this ..." further above. When inserting a CD, always update libxl-json first, then add the media to QEMU. My previous reply was for this part. Yet I think CDROM has its own quirks. IIRC it is more an exception than the norm. The existing code doesn't match what you wrote either. > > > > + > > > +Retrieve / store domain configuration from / to libxl private data store > > > +are done with `libxl__get_domain_configuration' and > > > +`libxl__set_domain_configuration'. Consult libxl_internal.h for more > > > +information. > > > + > > > > What do you think about the text around libxl_internal.h:L2598? > > If only I knew this comment existed :-(. It is burried, don't mention > "libxl-json" or "userdata" or "domain config" but only the not very > helpful term "json config"... Hmm, ... it actualy have "domain > configuration" once. > > Anyway, that comment block isn't very helpful because it basically says > that we can't depriv QEMU, I mean do hotplug with a deprived QEMU. It > assumes that we can keep a lock on the userdata while updating the > guest, but we can't keep the lock while talking with QEMU (or more > generaly: we can't keep the lock while doing any async operation). > > But there is one useful piece of information: > Here we maintain one invariant: every device in xenstore must have > an entry in JSON file. > (xenstore is describe as "primary reference" just before that sentence). > Yes. That. > This is what I would like my past self to be able to find out more > easly, and having the information in CODING_STYLE would make sense I > think. > > > Maybe we should extend that comment block? > > I still think it would be helpful to have pointers in CODING_STYLE, as > there isn't a single place in libxl_internal.h where the information I > was looking for could be added. > Anyway, I'm not overly opposed to adding some easy to grep pointers, but CODING_STYLE looks wrong to me. Maybe README.dev? Wei. > Thanks, > > -- > Anthony PERARD ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] libxl: Documentation about the domain configuration on disk
On Thu, Dec 06, 2018 at 12:16:40PM +, Wei Liu wrote: > On Thu, Dec 06, 2018 at 10:43:32AM +, Anthony PERARD wrote: > > +UPDATE OF DOMAIN CONFIGURATION > > +-- > > + > > +Also known as "libxl-json" userdata or `libxl_domain_config'. > > + > > +Whenever a running domain have its configuration updated, like changing > > +media in a cdrom drive, the domain configuration in libxl private data > > +store needs to be updated as well. The domain configuration should > > +contain *more* information about the domain rather than less, stale data > > +are easier to spot that missing data. > > + > > +Here is an example of how to update the domain configuration: > > + * Remove current media from cdrom drive > > + * Update domain configuration with media removed > > We may not even need this because the primary source in this case is > QEMU. See below. > > > + ( we could stop here) > > + * Update domain configuration to add media we are about to insert > > + * Insert media into cdrom drive > > In essence we need a primary reference while using libxl-json file as a > secondary source. > > When doing device hotplug, the primary source is xenstore. It may become > QEMU in the future if we move to a model where everything is > communicated via QMP. > > When doing CDROM insertion and rejection, the primary source is QEMU > state. I'm not trying to figure out what primary source should be here, I'm trying to find out how the secondary source, namely "libxl-json", should behave, what it should contain, when to update it compare the primary source (what a guest ultimately see). > All in all I think your description is not wrong but it failed to > capture the high-level intent -- always update libxl-json before > updating the primary source. That isn't what Ian said IRL, I don't think. From what I understand, when removing a media/disk, first remove the media, then update libxl-json; when adding a media/disk, first update libxl-json, then add the media. > > + > > +Retrieve / store domain configuration from / to libxl private data store > > +are done with `libxl__get_domain_configuration' and > > +`libxl__set_domain_configuration'. Consult libxl_internal.h for more > > +information. > > + > > What do you think about the text around libxl_internal.h:L2598? If only I knew this comment existed :-(. It is burried, don't mention "libxl-json" or "userdata" or "domain config" but only the not very helpful term "json config"... Hmm, ... it actualy have "domain configuration" once. Anyway, that comment block isn't very helpful because it basically says that we can't depriv QEMU, I mean do hotplug with a deprived QEMU. It assumes that we can keep a lock on the userdata while updating the guest, but we can't keep the lock while talking with QEMU (or more generaly: we can't keep the lock while doing any async operation). But there is one useful piece of information: Here we maintain one invariant: every device in xenstore must have an entry in JSON file. (xenstore is describe as "primary reference" just before that sentence). This is what I would like my past self to be able to find out more easly, and having the information in CODING_STYLE would make sense I think. > Maybe we should extend that comment block? I still think it would be helpful to have pointers in CODING_STYLE, as there isn't a single place in libxl_internal.h where the information I was looking for could be added. Thanks, -- Anthony PERARD ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] libxl: Documentation about the domain configuration on disk
On Thu, Dec 06, 2018 at 10:43:32AM +, Anthony PERARD wrote: > It isn't currently written how one is supposed to update the domain > configuration in libxl's private data store. This patch attempt to > document the unwritten rules. > > Signed-off-by: Anthony PERARD > --- > tools/libxl/CODING_STYLE | 24 > 1 file changed, 24 insertions(+) > > diff --git a/tools/libxl/CODING_STYLE b/tools/libxl/CODING_STYLE > index 3d572f6925..2ce8529c9b 100644 > --- a/tools/libxl/CODING_STYLE > +++ b/tools/libxl/CODING_STYLE > @@ -186,6 +186,30 @@ When using an ao sub-operation, you should normally: > cancel or destroy method. > > > +UPDATE OF DOMAIN CONFIGURATION > +-- > + > +Also known as "libxl-json" userdata or `libxl_domain_config'. > + > +Whenever a running domain have its configuration updated, like changing > +media in a cdrom drive, the domain configuration in libxl private data > +store needs to be updated as well. The domain configuration should > +contain *more* information about the domain rather than less, stale data > +are easier to spot that missing data. > + > +Here is an example of how to update the domain configuration: > + * Remove current media from cdrom drive > + * Update domain configuration with media removed We may not even need this because the primary source in this case is QEMU. See below. > + ( we could stop here) > + * Update domain configuration to add media we are about to insert > + * Insert media into cdrom drive In essence we need a primary reference while using libxl-json file as a secondary source. When doing device hotplug, the primary source is xenstore. It may become QEMU in the future if we move to a model where everything is communicated via QMP. When doing CDROM insertion and rejection, the primary source is QEMU state. All in all I think your description is not wrong but it failed to capture the high-level intent -- always update libxl-json before updating the primary source. > + > +Retrieve / store domain configuration from / to libxl private data store > +are done with `libxl__get_domain_configuration' and > +`libxl__set_domain_configuration'. Consult libxl_internal.h for more > +information. > + What do you think about the text around libxl_internal.h:L2598? Maybe we should extend that comment block? Wei. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
