[PATCH v9 0/3] xenstored: use new libxenmanage functionality

[PATCH v9 0/3] xenstored: use new libxenmanage functionality

[Juergen Gross]

The new libxenmanage interfaces make it possible to:

- detect that a domid has been recycled
- avoid having to query all known domains when one of them has changed
  state

Use those interfaces in xenstored in order to make it more efficient and
failure proof.

Changes in V2:
- new patch 1
- former patch 5 mover earlier, now patch 2 (can go in without the rest
  of the series)
- addressed comments

Changes in V3:
- addressed comments

Changes in V4:
- patches 1 and 3 of V3 dropped, as already committed
- addressed comments

Changes in V5:
- addressed comments

Changes in V6:
- patch 1 of V5 has been committed
- new patches 1-3 for fixing a race and avoiding new races with the
  added functionality (result of a comment by Jan Beulich)
- rework of locking in patch 4 (Jan Beulich)

Changes in V7:
- addressed comments
- rebase

Changes in V8:
- patch 1 of v7 has gone in
- addressed comments
- new patches 7-9 using the new unique_id and xenmanage_poll_changed_domain()

Changes in V9:
- all but the last three patches of v8 have gone in
- addressed comments

Juergen Gross (3):
  docs: update xenstore migration stream definition
  tools/xenstored: use unique_id to identify new domain with same domid
  tools/xenstored: use xenmanage_poll_changed_domain()

 docs/designs/xenstore-migration.md |  18 ++++-
 tools/xenstored/domain.c           | 113 +++++++++++++++++++++++------
 tools/xenstored/xenstore_state.h   |   3 +-
 3 files changed, 110 insertions(+), 24 deletions(-)

-- 
2.43.0

[PATCH v9 1/3] docs: update xenstore migration stream definition

[Juergen Gross]

In order to close a race window for Xenstore live update when using
the new unique_id of domains, the migration stream needs to contain
this unique_id for each domain known by Xenstore.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V8:
- new patch
V9:
- instead of using an offset, use a flag for unique_id presence
  (Julien Grall)
---
 docs/designs/xenstore-migration.md | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/docs/designs/xenstore-migration.md b/docs/designs/xenstore-migration.md
index 082314bf72..a0acd76ce1 100644
--- a/docs/designs/xenstore-migration.md
+++ b/docs/designs/xenstore-migration.md
@@ -156,7 +156,7 @@ the domain being migrated.
 ```
     0       1       2       3       4       5       6       7    octet
 +-------+-------+-------+-------+-------+-------+-------+-------+
-| conn-id                       | conn-type     |               |
+| conn-id                       | conn-type     | fields        |
 +-------------------------------+---------------+---------------+
 | conn-spec
 ...
@@ -165,6 +165,9 @@ the domain being migrated.
 +---------------+---------------+-------------------------------+
 | data
 ...
++---------------------------------------------------------------+
+| unique-id                                                     |
++---------------------------------------------------------------+
 ```
 
 
@@ -178,6 +181,16 @@ the domain being migrated.
 |                | 0x0001: socket                               |
 |                | 0x0002 - 0xFFFF: reserved for future use     |
 |                |                                              |
+| `fields`       | A collection of flags indicating presence    |
+|                | of additional fields after the variable      |
+|                | length `data` part. The additional fields    |
+|                | will start after a possible padding for      |
+|                | aligning to a 8 octet boundary.              |
+|                | Defined flag values (to be or-ed):           |
+|                | 0x0001: `unique_id` present (only needed for |
+|                |         `shared ring` connection in live     |
+|                |         update streams).                     |
+|                |                                              |
 | `conn-spec`    | See below                                    |
 |                |                                              |
 | `in-data-len`  | The length (in octets) of any data read      |
@@ -193,6 +206,9 @@ the domain being migrated.
 | `data`         | Pending data: first in-data-len octets of    |
 |                | read data, then out-data-len octets of       |
 |                | written data (any of both may be empty)      |
+|                |                                              |
+| `unique-id`    | Unique identifier of a domain                |
+|                |                                              |
 
 In case of live update the connection record for the connection via which
 the live update command was issued will contain the response for the live
-- 
2.43.0

Re: [PATCH v9 1/3] docs: update xenstore migration stream definition

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 3454 bytes --]

On 14.03.25 13:18, Juergen Gross wrote:
> In order to close a race window for Xenstore live update when using
> the new unique_id of domains, the migration stream needs to contain
> this unique_id for each domain known by Xenstore.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

This patch is still missing an Ack.

Julien?


Juergen

> ---
> V8:
> - new patch
> V9:
> - instead of using an offset, use a flag for unique_id presence
>    (Julien Grall)
> ---
>   docs/designs/xenstore-migration.md | 18 +++++++++++++++++-
>   1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/designs/xenstore-migration.md b/docs/designs/xenstore-migration.md
> index 082314bf72..a0acd76ce1 100644
> --- a/docs/designs/xenstore-migration.md
> +++ b/docs/designs/xenstore-migration.md
> @@ -156,7 +156,7 @@ the domain being migrated.
>   ```
>       0       1       2       3       4       5       6       7    octet
>   +-------+-------+-------+-------+-------+-------+-------+-------+
> -| conn-id                       | conn-type     |               |
> +| conn-id                       | conn-type     | fields        |
>   +-------------------------------+---------------+---------------+
>   | conn-spec
>   ...
> @@ -165,6 +165,9 @@ the domain being migrated.
>   +---------------+---------------+-------------------------------+
>   | data
>   ...
> ++---------------------------------------------------------------+
> +| unique-id                                                     |
> ++---------------------------------------------------------------+
>   ```
>   
>   
> @@ -178,6 +181,16 @@ the domain being migrated.
>   |                | 0x0001: socket                               |
>   |                | 0x0002 - 0xFFFF: reserved for future use     |
>   |                |                                              |
> +| `fields`       | A collection of flags indicating presence    |
> +|                | of additional fields after the variable      |
> +|                | length `data` part. The additional fields    |
> +|                | will start after a possible padding for      |
> +|                | aligning to a 8 octet boundary.              |
> +|                | Defined flag values (to be or-ed):           |
> +|                | 0x0001: `unique_id` present (only needed for |
> +|                |         `shared ring` connection in live     |
> +|                |         update streams).                     |
> +|                |                                              |
>   | `conn-spec`    | See below                                    |
>   |                |                                              |
>   | `in-data-len`  | The length (in octets) of any data read      |
> @@ -193,6 +206,9 @@ the domain being migrated.
>   | `data`         | Pending data: first in-data-len octets of    |
>   |                | read data, then out-data-len octets of       |
>   |                | written data (any of both may be empty)      |
> +|                |                                              |
> +| `unique-id`    | Unique identifier of a domain                |
> +|                |                                              |
>   
>   In case of live update the connection record for the connection via which
>   the live update command was issued will contain the response for the live


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v9 1/3] docs: update xenstore migration stream definition

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 4458 bytes --]

On 25.03.25 10:17, Juergen Gross wrote:
> On 14.03.25 13:18, Juergen Gross wrote:
>> In order to close a race window for Xenstore live update when using
>> the new unique_id of domains, the migration stream needs to contain
>> this unique_id for each domain known by Xenstore.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> This patch is still missing an Ack.

PING?

> 
> Julien?
> 
> 
> Juergen
> 
>> ---
>> V8:
>> - new patch
>> V9:
>> - instead of using an offset, use a flag for unique_id presence
>>    (Julien Grall)
>> ---
>>   docs/designs/xenstore-migration.md | 18 +++++++++++++++++-
>>   1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/docs/designs/xenstore-migration.md b/docs/designs/xenstore- 
>> migration.md
>> index 082314bf72..a0acd76ce1 100644
>> --- a/docs/designs/xenstore-migration.md
>> +++ b/docs/designs/xenstore-migration.md
>> @@ -156,7 +156,7 @@ the domain being migrated.
>>   ```
>>       0       1       2       3       4       5       6       7    octet
>>   +-------+-------+-------+-------+-------+-------+-------+-------+
>> -| conn-id                       | conn-type     |               |
>> +| conn-id                       | conn-type     | fields        |
>>   +-------------------------------+---------------+---------------+
>>   | conn-spec
>>   ...
>> @@ -165,6 +165,9 @@ the domain being migrated.
>>   +---------------+---------------+-------------------------------+
>>   | data
>>   ...
>> ++---------------------------------------------------------------+
>> +| unique-id                                                     |
>> ++---------------------------------------------------------------+
>>   ```
>> @@ -178,6 +181,16 @@ the domain being migrated.
>>   |                | 0x0001: socket                               |
>>   |                | 0x0002 - 0xFFFF: reserved for future use     |
>>   |                |                                              |
>> +| `fields`       | A collection of flags indicating presence    |
>> +|                | of additional fields after the variable      |
>> +|                | length `data` part. The additional fields    |
>> +|                | will start after a possible padding for      |
>> +|                | aligning to a 8 octet boundary.              |
>> +|                | Defined flag values (to be or-ed):           |
>> +|                | 0x0001: `unique_id` present (only needed for |
>> +|                |         `shared ring` connection in live     |
>> +|                |         update streams).                     |
>> +|                |                                              |
>>   | `conn-spec`    | See below                                    |
>>   |                |                                              |
>>   | `in-data-len`  | The length (in octets) of any data read      |
>> @@ -193,6 +206,9 @@ the domain being migrated.
>>   | `data`         | Pending data: first in-data-len octets of    |
>>   |                | read data, then out-data-len octets of       |
>>   |                | written data (any of both may be empty)      |
>> +|                |                                              |
>> +| `unique-id`    | Unique identifier of a domain                |
>> +|                |                                              |
>>   In case of live update the connection record for the connection via which
>>   the live update command was issued will contain the response for the live
> 


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v9 1/3] docs: update xenstore migration stream definition

[Anthony PERARD]

On Thu, Apr 03, 2025 at 01:45:32PM +0200, Juergen Gross wrote:
> On 25.03.25 10:17, Juergen Gross wrote:
> > On 14.03.25 13:18, Juergen Gross wrote:
> > > In order to close a race window for Xenstore live update when using
> > > the new unique_id of domains, the migration stream needs to contain
> > > this unique_id for each domain known by Xenstore.
> > > 
> > > Signed-off-by: Juergen Gross <jgross@suse.com>
> > 
> > This patch is still missing an Ack.
> 
> PING?

Julien's concern looked to be addressed. And the patch looks fine to me,
so:

Acked-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks,

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

[PATCH v9 2/3] tools/xenstored: use unique_id to identify new domain with same domid

[Juergen Gross]

Use the new unique_id of a domain in order to detect that a domain
has been replaced with another one reusing the doamin-id of the old
domain.

While changing the related code, switch from "dom_invalid" to
"dom_valid" in order to avoid double negation and use "bool" as type
for it.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V8:
- new patch
V9:
- adapt to different LU-record layout
- "dom_invalid" -> "dom_valid" (Jason Andryuk)
---
 tools/xenstored/domain.c         | 65 ++++++++++++++++++++++++++------
 tools/xenstored/xenstore_state.h |  3 +-
 2 files changed, 55 insertions(+), 13 deletions(-)

diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c
index a6506a5bb2..fc0992d3a5 100644
--- a/tools/xenstored/domain.c
+++ b/tools/xenstored/domain.c
@@ -110,6 +110,7 @@ struct domain
 {
 	/* The id of this domain */
 	unsigned int domid;
+	uint64_t unique_id;
 
 	/* Event channel port */
 	evtchn_port_t port;
@@ -624,18 +625,26 @@ static int check_domain(const void *k, void *v, void *arg)
 {
 	unsigned int state;
 	struct connection *conn;
-	int dom_invalid;
+	bool dom_valid;
 	struct domain *domain = v;
 	bool *notify = arg;
+	uint64_t unique_id;
+
+	dom_valid = !xenmanage_get_domain_info(xm_handle, domain->domid,
+					       &state, &unique_id);
+	if (dom_valid) {
+		if (!domain->unique_id)
+			domain->unique_id = unique_id;
+		else if (domain->unique_id != unique_id)
+			dom_valid = false;
+	}
 
-	dom_invalid = xenmanage_get_domain_info(xm_handle, domain->domid,
-						&state, NULL);
 	if (!domain->introduced) {
-		if (dom_invalid)
+		if (!dom_valid)
 			talloc_free(domain);
 		return 0;
 	}
-	if (!dom_invalid) {
+	if (dom_valid) {
 		if ((state & XENMANAGE_GETDOMSTATE_STATE_SHUTDOWN)
 		    && !domain->shutdown) {
 			domain->shutdown = true;
@@ -747,7 +756,8 @@ int domain_max_global_acc(const void *ctx, struct connection *conn)
 	return 0;
 }
 
-static struct domain *alloc_domain(const void *context, unsigned int domid)
+static struct domain *alloc_domain(const void *context, unsigned int domid,
+				   uint64_t unique_id)
 {
 	struct domain *domain;
 
@@ -758,6 +768,7 @@ static struct domain *alloc_domain(const void *context, unsigned int domid)
 	}
 
 	domain->domid = domid;
+	domain->unique_id = unique_id;
 	domain->generation = generation;
 	domain->introduced = false;
 
@@ -777,16 +788,27 @@ static struct domain *find_or_alloc_domain(const void *ctx, unsigned int domid)
 	struct domain *domain;
 
 	domain = find_domain_struct(domid);
-	return domain ? : alloc_domain(ctx, domid);
+	/* If domain not already known, use unique_id = 0 meaning "unknown". */
+	return domain ? : alloc_domain(ctx, domid, 0);
 }
 
 static struct domain *find_or_alloc_existing_domain(unsigned int domid)
 {
 	struct domain *domain;
+	uint64_t unique_id = 0;
+	bool dom_valid = true;
 
 	domain = find_domain_struct(domid);
-	if (!domain && !xenmanage_get_domain_info(xm_handle, domid, NULL, NULL))
-		domain = alloc_domain(NULL, domid);
+	if (!domain || !domain->unique_id)
+		dom_valid = !xenmanage_get_domain_info(xm_handle, domid,
+						       NULL, &unique_id);
+
+	if (dom_valid) {
+		if (!domain)
+			domain = alloc_domain(NULL, domid, unique_id);
+		else if (unique_id)
+			domain->unique_id = unique_id;
+	}
 
 	return domain;
 }
@@ -1321,15 +1343,16 @@ int domain_alloc_permrefs(struct node_perms *perms)
 {
 	unsigned int i, domid;
 	struct domain *d;
+	uint64_t unique_id;
 
 	for (i = 0; i < perms->num; i++) {
 		domid = perms->p[i].id;
 		d = find_domain_struct(domid);
 		if (!d) {
 			if (xenmanage_get_domain_info(xm_handle, domid,
-						      NULL, NULL))
+						      NULL, &unique_id))
 				perms->p[i].perms |= XS_PERM_IGNORE;
-			else if (!alloc_domain(NULL, domid))
+			else if (!alloc_domain(NULL, domid, unique_id))
 				return ENOMEM;
 		}
 	}
@@ -1697,12 +1720,14 @@ const char *dump_state_connections(FILE *fp)
 	struct xs_state_record_header head;
 	struct connection *c;
 
+	BUILD_BUG_ON(sizeof(c->domain->unique_id) != sizeof(uint64_t));
+
 	list_for_each_entry(c, &connections, list) {
 		head.type = XS_STATE_TYPE_CONN;
 		head.length = sizeof(sc);
 
 		sc.conn_id = conn_id++;
-		sc.pad = 0;
+		sc.fields = 0;
 		memset(&sc.spec, 0, sizeof(sc.spec));
 		if (c->domain) {
 			sc.conn_type = XS_STATE_CONN_TYPE_RING;
@@ -1720,6 +1745,10 @@ const char *dump_state_connections(FILE *fp)
 			return ret;
 		head.length += sc.data_in_len + sc.data_out_len;
 		head.length = ROUNDUP(head.length, 3);
+		if (c->domain) {
+			sc.fields |= XS_STATE_CONN_FIELDS_UNIQ_ID;
+			head.length += sizeof(uint64_t);
+		}
 		if (fwrite(&head, sizeof(head), 1, fp) != 1)
 			return "Dump connection state error";
 		if (fwrite(&sc, offsetof(struct xs_state_connection, data),
@@ -1731,6 +1760,9 @@ const char *dump_state_connections(FILE *fp)
 		ret = dump_state_align(fp);
 		if (ret)
 			return ret;
+		if (c->domain &&
+		    fwrite(&c->domain->unique_id, sizeof(uint64_t), 1, fp) != 1)
+			return "Dump connection state error";
 
 		ret = dump_state_watches(fp, c, sc.conn_id);
 		if (ret)
@@ -1748,6 +1780,7 @@ void read_state_connection(const void *ctx, const void *state)
 
 	if (sc->conn_type == XS_STATE_CONN_TYPE_SOCKET) {
 		conn = add_socket_connection(sc->spec.socket_fd);
+		domain = NULL;
 	} else {
 		domain = introduce_domain(ctx, sc->spec.ring.domid,
 					  sc->spec.ring.evtchn, true);
@@ -1778,6 +1811,14 @@ void read_state_connection(const void *ctx, const void *state)
 	conn->conn_id = sc->conn_id;
 
 	read_state_buffered_data(ctx, conn, sc);
+
+	/* Validity of unique_id will be tested by check_domains() later. */
+	if ((sc->fields & XS_STATE_CONN_FIELDS_UNIQ_ID) && domain) {
+		unsigned long off;
+
+		off = sizeof(*sc) + sc->data_in_len + sc->data_out_len;
+		domain->unique_id = *(uint64_t *)(state + ROUNDUP(off, 3));
+	}
 }
 
 struct domain_acc {
diff --git a/tools/xenstored/xenstore_state.h b/tools/xenstored/xenstore_state.h
index ae0d053c8f..bad966caf4 100644
--- a/tools/xenstored/xenstore_state.h
+++ b/tools/xenstored/xenstore_state.h
@@ -74,7 +74,8 @@ struct xs_state_connection {
     uint16_t conn_type;
 #define XS_STATE_CONN_TYPE_RING   0
 #define XS_STATE_CONN_TYPE_SOCKET 1
-    uint16_t pad;
+    uint16_t fields;
+#define XS_STATE_CONN_FIELDS_UNIQ_ID 0x0001
     union {
         struct {
             uint16_t domid;  /* Domain-Id. */
-- 
2.43.0

Re: [PATCH v9 2/3] tools/xenstored: use unique_id to identify new domain with same domid

[Jason Andryuk]

On 2025-03-14 08:18, Juergen Gross wrote:
> Use the new unique_id of a domain in order to detect that a domain
> has been replaced with another one reusing the doamin-id of the old
> domain.
> 
> While changing the related code, switch from "dom_invalid" to
> "dom_valid" in order to avoid double negation and use "bool" as type
> for it.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V8:
> - new patch
> V9:
> - adapt to different LU-record layout
> - "dom_invalid" -> "dom_valid" (Jason Andryuk)

Thanks.

Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

One thought below.

> ---
>   tools/xenstored/domain.c         | 65 ++++++++++++++++++++++++++------
>   tools/xenstored/xenstore_state.h |  3 +-
>   2 files changed, 55 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c
> index a6506a5bb2..fc0992d3a5 100644
> --- a/tools/xenstored/domain.c
> +++ b/tools/xenstored/domain.c

> @@ -1778,6 +1811,14 @@ void read_state_connection(const void *ctx, const void *state)
>   	conn->conn_id = sc->conn_id;
>   
>   	read_state_buffered_data(ctx, conn, sc);
> +
> +	/* Validity of unique_id will be tested by check_domains() later. */
> +	if ((sc->fields & XS_STATE_CONN_FIELDS_UNIQ_ID) && domain) {

Is it worth adding a sanity check for the other bits in sc->fields == 0? 
  And a check domain != NULL when  XS_STATE_CONN_FIELDS_UNIQ_ID is set?

Regards,
Jason

> +		unsigned long off;
> +
> +		off = sizeof(*sc) + sc->data_in_len + sc->data_out_len;
> +		domain->unique_id = *(uint64_t *)(state + ROUNDUP(off, 3));
> +	}
>   }
>   

Re: [PATCH v9 2/3] tools/xenstored: use unique_id to identify new domain with same domid

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1872 bytes --]

On 14.03.25 13:58, Jason Andryuk wrote:
> On 2025-03-14 08:18, Juergen Gross wrote:
>> Use the new unique_id of a domain in order to detect that a domain
>> has been replaced with another one reusing the doamin-id of the old
>> domain.
>>
>> While changing the related code, switch from "dom_invalid" to
>> "dom_valid" in order to avoid double negation and use "bool" as type
>> for it.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V8:
>> - new patch
>> V9:
>> - adapt to different LU-record layout
>> - "dom_invalid" -> "dom_valid" (Jason Andryuk)
> 
> Thanks.
> 
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
> 
> One thought below.
> 
>> ---
>>   tools/xenstored/domain.c         | 65 ++++++++++++++++++++++++++------
>>   tools/xenstored/xenstore_state.h |  3 +-
>>   2 files changed, 55 insertions(+), 13 deletions(-)
>>
>> diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c
>> index a6506a5bb2..fc0992d3a5 100644
>> --- a/tools/xenstored/domain.c
>> +++ b/tools/xenstored/domain.c
> 
>> @@ -1778,6 +1811,14 @@ void read_state_connection(const void *ctx, const void 
>> *state)
>>       conn->conn_id = sc->conn_id;
>>       read_state_buffered_data(ctx, conn, sc);
>> +
>> +    /* Validity of unique_id will be tested by check_domains() later. */
>> +    if ((sc->fields & XS_STATE_CONN_FIELDS_UNIQ_ID) && domain) {
> 
> Is it worth adding a sanity check for the other bits in sc->fields == 0?

Definitely not. Unknown flags can be ignored, they should never result
in an error. Otherwise LU to an older Xenstore might not be possible
any longer.

> And a check domain != NULL when  XS_STATE_CONN_FIELDS_UNIQ_ID is set?

No, I don't think so. Failing a LU due to such a case would inhibit the
possibility to fix such a bug using LU.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v9 2/3] tools/xenstored: use unique_id to identify new domain with same domid

[Andrew Cooper]

On 14/03/2025 12:18 pm, Juergen Gross wrote:
> Use the new unique_id of a domain in order to detect that a domain
> has been replaced with another one reusing the doamin-id of the old

domain.

> domain.
>
> While changing the related code, switch from "dom_invalid" to
> "dom_valid" in order to avoid double negation and use "bool" as type
> for it.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>

[PATCH v9 3/3] tools/xenstored: use xenmanage_poll_changed_domain()

[Juergen Gross]

Instead of checking each known domain after having received a
VIRQ_DOM_EXC event, use the new xenmanage_poll_changed_domain()
function for directly getting the domid of a domain having changed
its state.

A test doing "xl shutdown" of 1000 guests has shown to reduce the
consumed cpu time of xenstored by 6% with this change applied.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
---
V8:
- new patch
---
 tools/xenstored/domain.c | 64 +++++++++++++++++++++++++++++-----------
 1 file changed, 46 insertions(+), 18 deletions(-)

diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c
index fc0992d3a5..e1d5e8d614 100644
--- a/tools/xenstored/domain.c
+++ b/tools/xenstored/domain.c
@@ -621,30 +621,24 @@ static int destroy_domain(void *_domain)
 	return 0;
 }
 
-static int check_domain(const void *k, void *v, void *arg)
+static int do_check_domain(struct domain *domain, bool *notify,
+			   unsigned int state, uint64_t unique_id)
 {
-	unsigned int state;
 	struct connection *conn;
-	bool dom_valid;
-	struct domain *domain = v;
-	bool *notify = arg;
-	uint64_t unique_id;
 
-	dom_valid = !xenmanage_get_domain_info(xm_handle, domain->domid,
-					       &state, &unique_id);
-	if (dom_valid) {
+	if (unique_id) {
 		if (!domain->unique_id)
 			domain->unique_id = unique_id;
 		else if (domain->unique_id != unique_id)
-			dom_valid = false;
+			unique_id = 0;
 	}
 
 	if (!domain->introduced) {
-		if (!dom_valid)
+		if (!unique_id)
 			talloc_free(domain);
 		return 0;
 	}
-	if (dom_valid) {
+	if (unique_id) {
 		if ((state & XENMANAGE_GETDOMSTATE_STATE_SHUTDOWN)
 		    && !domain->shutdown) {
 			domain->shutdown = true;
@@ -667,6 +661,21 @@ static int check_domain(const void *k, void *v, void *arg)
 	return 0;
 }
 
+static int check_domain(const void *k, void *v, void *arg)
+{
+	struct domain *domain = v;
+	unsigned int state;
+	uint64_t unique_id;
+
+	if (xenmanage_get_domain_info(xm_handle, domain->domid, &state,
+				      &unique_id)) {
+		unique_id = 0;
+		state = 0;
+	}
+
+	return do_check_domain(domain, arg, state, unique_id);
+}
+
 void check_domains(void)
 {
 	bool notify = false;
@@ -678,6 +687,30 @@ void check_domains(void)
 		fire_special_watches("@releaseDomain");
 }
 
+static struct domain *find_domain_struct(unsigned int domid)
+{
+	return hashtable_search(domhash, &domid);
+}
+
+static void do_check_domains(void)
+{
+	unsigned int domid;
+	unsigned int state;
+	uint64_t unique_id;
+	struct domain *domain;
+	bool notify = false;
+
+	while (!xenmanage_poll_changed_domain(xm_handle, &domid, &state,
+					      &unique_id)) {
+		domain = find_domain_struct(domid);
+		if (domain)
+			do_check_domain(domain, &notify, state, unique_id);
+	}
+
+	if (notify)
+		fire_special_watches("@releaseDomain");
+}
+
 /* We scan all domains rather than use the information given here. */
 void handle_event(void)
 {
@@ -687,7 +720,7 @@ void handle_event(void)
 		barf_perror("Failed to read from event fd");
 
 	if (port == virq_port)
-		check_domains();
+		do_check_domains();
 
 	if (xenevtchn_unmask(xce_handle, port) == -1)
 		barf_perror("Failed to write to event fd");
@@ -698,11 +731,6 @@ static char *talloc_domain_path(const void *context, unsigned int domid)
 	return talloc_asprintf(context, "/local/domain/%u", domid);
 }
 
-static struct domain *find_domain_struct(unsigned int domid)
-{
-	return hashtable_search(domhash, &domid);
-}
-
 int domain_get_quota(const void *ctx, struct connection *conn,
 		     unsigned int domid)
 {
-- 
2.43.0