Skip to contentSkip to conversation

libxl: Documentation about the domain configuration on disk

PATCH In review Anthony PERARD 3 participants 10 messages Dec 6, 2018 – Dec 19, 2018

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 libxl: Documentation about the domain configuration on disk

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.

---
 tools/libxl/CODING_STYLE | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)
tools/libxl/CODING_STYLE +24 -0
index 3d572f6925..2ce8529c9b 100644
@@ -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 could stop here)
+ * Update domain configuration to add media we are about to insert
+ * Insert media into cdrom drive
+
+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.
+
+
 FORMATTING AND NAMING
 ---------------------
 
Signature · 7 lines
Anthony PERARD


_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
#1 lore raw
Quoted 21 lines · Anthony PERARD

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.

---
 tools/libxl/CODING_STYLE | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)
tools/libxl/CODING_STYLE +4 -0
index 3d572f6925..2ce8529c9b 100644
@@ -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'.

tools/libxl/CODING_STYLE When using an ao sub-operation, you should normally: · line 202

Quoted 10 lines
tools/libxl/CODING_STYLE +10 -0
+
+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.

tools/libxl/CODING_STYLE When using an ao sub-operation, you should normally: · line 205

Quoted 3 lines
tools/libxl/CODING_STYLE +3 -0
+ ( 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.

tools/libxl/CODING_STYLE When using an ao sub-operation, you should normally: · line 211

Quoted 6 lines
tools/libxl/CODING_STYLE +6 -0
+
+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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

#2 lore raw
Quoted 26 lines · Wei Liu
Quoted 4 lines · Anthony PERARD
tools/libxl/CODING_STYLE +4 -0
+UPDATE OF DOMAIN CONFIGURATION
+------------------------------
+
+Also known as "libxl-json" userdata or `libxl_domain_config'.

tools/libxl/CODING_STYLE When using an ao sub-operation, you should normally: · line 202

Quoted 10 lines
tools/libxl/CODING_STYLE +10 -0
+
+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.

Quoted 3 lines

+ ( 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.

Quoted 6 lines

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).

Quoted 3 lines

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.

Quoted 8 lines

tools/libxl/CODING_STYLE When using an ao sub-operation, you should normally: · line 211

Quoted 6 lines
tools/libxl/CODING_STYLE +6 -0
+
+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.

Quoted 1 line

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,

Signature · 6 lines
Anthony PERARD

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
#3 lore raw
Quoted 39 lines · Anthony PERARD
Quoted 26 lines · Wei Liu
Quoted 4 lines · Anthony PERARD
tools/libxl/CODING_STYLE +4 -0
+UPDATE OF DOMAIN CONFIGURATION
+------------------------------
+
+Also known as "libxl-json" userdata or `libxl_domain_config'.

tools/libxl/CODING_STYLE When using an ao sub-operation, you should normally: · line 202

Quoted 10 lines
tools/libxl/CODING_STYLE +10 -0
+
+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.

Quoted 3 lines

+ ( 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.

Quoted 6 lines

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).

Quoted 8 lines
Quoted 3 lines

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.

Quoted 21 lines
Quoted 9 lines

tools/libxl/CODING_STYLE When using an ao sub-operation, you should normally: · line 211

Quoted 7 lines
tools/libxl/CODING_STYLE +6 -0
+
+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).

Quoted 5 lines

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.

Quoted 10 lines

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.

Quoted 1 line

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.

Quoted 4 lines

Thanks,

Signature · 1 line
Anthony PERARD

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

#4 lore raw
Quoted 2 lines · Wei Liu

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.

Signature · 6 lines
Anthony PERARD

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
#5 lore raw
Quoted 9 lines · Anthony PERARD
Quoted 2 lines · Wei Liu

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.

Quoted 3 lines
Signature · 2 lines
-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

#6 lore raw
In reply to Wei Liu

Wei Liu writes ("Re: [PATCH] libxl: Documentation about the domain configuration on disk"):

Quoted 13 lines · Anthony PERARD
Quoted 10 lines

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.

...

Quoted 4 lines

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 ?

Quoted 2 lines

Anyway, I'm not overly opposed to adding some easy to grep pointers, but CODING_STYLE looks wrong to me. Maybe README.dev?

I'm sorry I didn't make that comment easy to find. I think maybe adding cross-reference comments (perhaps several, in the places you looked) would be a good idea.

The problem with a file like HACKING or CODING_STYLE is that it will become a large list of things unconnected to what they are about, and therefore once again hard to find.

Ian.

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

#7 lore raw
Quoted 81 lines · Ian Jackson

Wei Liu writes ("Re: [PATCH] libxl: Documentation about the domain configuration on disk"):

Quoted 13 lines · Anthony PERARD
Quoted 10 lines

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.

...

Quoted 4 lines

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.

Quoted 5 lines

* 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.)

Quoted 6 lines

I think this maintains the invariant.

I haven't figured out domain destruction. Ideally domain destruction could happen without taking the slow lock.

Quoted 6 lines

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

Quoted 4 lines

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:

Quoted 8 lines

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

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
5 domain creation complets
6 domain destruction deletes libxl-json
7 domain destruction xc_domain_destroy and complets

And QEMU leaks.

I don't know where domain destruction could crash.
And I don't think qmp add is relevant in the race between creation and destruction.

Quoted 3 lines

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.

Quoted 1 line

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.

Signature · 6 lines
Anthony PERARD

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
#8 lore raw

Anthony PERARD writes ("Re: [PATCH] libxl: Documentation about the domain configuration on disk"):

Quoted 9 lines

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.

Quoted 6 lines
Quoted 3 lines

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 ?

Quoted 6 lines
Quoted 1 line

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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

#9 lore raw
Quoted 22 lines · Ian Jackson

Anthony PERARD writes ("Re: [PATCH] libxl: Documentation about the domain configuration on disk"):

Quoted 9 lines

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.

Quoted 4 lines
Quoted 3 lines

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.

Quoted 6 lines
Quoted 2 lines

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.

Quoted 7 lines
Quoted 6 lines
Quoted 1 line

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.

Quoted 7 lines

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,

Signature · 6 lines
Anthony PERARD

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel