[PATCH v5 00/14] tools/xenstore: rework internal accounting

[PATCH v5 00/14] tools/xenstore: rework internal accounting

[Juergen Gross]

This series reworks the Xenstore internal accounting to use a uniform
generic framework. It is adding some additional useful diagnostic
information, like accounting trace and max. per-domain and global quota
values seen.

Changes in V2:
- added patch 1 (leftover from previous series)
- rebase

Changes in V3:
- addressed comments

Changes in V4:
- fixed patch 3

Changes in V5:
- addressed comments

Juergen Gross (14):
  tools/xenstore: take transaction internal nodes into account for quota
  tools/xenstore: manage per-transaction domain accounting data in an
    array
  tools/xenstore: introduce accounting data array for per-domain values
  tools/xenstore: add framework to commit accounting data on success
    only
  tools/xenstore: use accounting buffering for node accounting
  tools/xenstore: add current connection to domain_memory_add()
    parameters
  tools/xenstore: use accounting data array for per-domain values
  tools/xenstore: add accounting trace support
  tools/xenstore: add TDB access trace support
  tools/xenstore: switch transaction accounting to generic accounting
  tools/xenstore: remember global and per domain max accounting values
  tools/xenstore: use generic accounting for remaining quotas
  tools/xenstore: switch get_optval_int() to get_optval_uint()
  tools/xenstore: switch quota management to be table based

 docs/misc/xenstore.txt                 |   5 +-
 tools/xenstore/xenstored_control.c     |  65 ++--
 tools/xenstore/xenstored_core.c        | 176 +++++-----
 tools/xenstore/xenstored_core.h        |  24 +-
 tools/xenstore/xenstored_domain.c      | 432 ++++++++++++++++++-------
 tools/xenstore/xenstored_domain.h      |  59 +++-
 tools/xenstore/xenstored_transaction.c |  24 +-
 tools/xenstore/xenstored_watch.c       |  15 +-
 8 files changed, 521 insertions(+), 279 deletions(-)

-- 
2.35.3

[PATCH v5 01/14] tools/xenstore: take transaction internal nodes into account for quota

[Juergen Gross]

The accounting for the number of nodes of a domain in an active
transaction is not working correctly, as it is checking the node quota
only against the number of nodes outside the transaction.

This can result in the transaction finally failing, as node quota is
checked at the end of the transaction again.

On the other hand even in a transaction deleting many nodes, new nodes
might not be creatable, in case the node quota was already reached at
the start of the transaction.

Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---
V3:
- rewrite of commit message (Julien Grall)
---
 tools/xenstore/xenstored_domain.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index f62be2245c..dbbf97accc 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -1116,9 +1116,8 @@ int domain_nbentry_fix(unsigned int domid, int num, bool update)
 
 int domain_nbentry(struct connection *conn)
 {
-	return (domain_is_unprivileged(conn))
-		? conn->domain->nbentry
-		: 0;
+	return domain_is_unprivileged(conn)
+	       ? domain_nbentry_add(conn, conn->id, 0, true) : 0;
 }
 
 static bool domain_chk_quota(struct domain *domain, int mem)
-- 
2.35.3

[PATCH v5 02/14] tools/xenstore: manage per-transaction domain accounting data in an array

[Juergen Gross]

In order to prepare keeping accounting data in an array instead of
using independent fields, switch the struct changed_domain accounting
data to that scheme, for now only using an array with one element.

In order to be able to extend this scheme add the needed indexing enum
to xenstored_domain.h.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
V2:
- make "what" parameter of acc_add_changed_dom() an enum type, and
  assert() that it won't exceed the accounting array (Julien Grall)
---
 tools/xenstore/xenstored_domain.c | 19 +++++++++++--------
 tools/xenstore/xenstored_domain.h | 10 ++++++++++
 2 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index dbbf97accc..609a9a13ab 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -99,8 +99,8 @@ struct changed_domain
 	/* Identifier of the changed domain. */
 	unsigned int domid;
 
-	/* Amount by which this domain's nbentry field has changed. */
-	int nbentry;
+	/* Accounting data. */
+	int acc[ACC_TR_N];
 };
 
 static struct hashtable *domhash;
@@ -550,7 +550,7 @@ int acc_fix_domains(struct list_head *head, bool chk_quota, bool update)
 	int cnt;
 
 	list_for_each_entry(cd, head, list) {
-		cnt = domain_nbentry_fix(cd->domid, cd->nbentry, update);
+		cnt = domain_nbentry_fix(cd->domid, cd->acc[ACC_NODES], update);
 		if (!update) {
 			if (chk_quota && cnt >= quota_nb_entry_per_domain)
 				return ENOSPC;
@@ -595,19 +595,21 @@ static struct changed_domain *acc_get_changed_domain(const void *ctx,
 	return cd;
 }
 
-static int acc_add_dom_nbentry(const void *ctx, struct list_head *head, int val,
-			       unsigned int domid)
+static int acc_add_changed_dom(const void *ctx, struct list_head *head,
+			       enum accitem what, int val, unsigned int domid)
 {
 	struct changed_domain *cd;
 
+	assert(what < ARRAY_SIZE(cd->acc));
+
 	cd = acc_get_changed_domain(ctx, head, domid);
 	if (!cd)
 		return 0;
 
 	errno = 0;
-	cd->nbentry += val;
+	cd->acc[what] += val;
 
-	return cd->nbentry;
+	return cd->acc[what];
 }
 
 static void domain_conn_reset(struct domain *domain)
@@ -1071,7 +1073,8 @@ static int domain_nbentry_add(struct connection *conn, unsigned int domid,
 
 	if (conn && conn->transaction) {
 		head = transaction_get_changed_domains(conn->transaction);
-		ret = acc_add_dom_nbentry(conn->transaction, head, add, domid);
+		ret = acc_add_changed_dom(conn->transaction, head, ACC_NODES,
+					  add, domid);
 		if (errno) {
 			fail_transaction(conn->transaction);
 			return -1;
diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h
index 279cccb3ad..40803574f6 100644
--- a/tools/xenstore/xenstored_domain.h
+++ b/tools/xenstore/xenstored_domain.h
@@ -19,6 +19,16 @@
 #ifndef _XENSTORED_DOMAIN_H
 #define _XENSTORED_DOMAIN_H
 
+/*
+ * All accounting data is stored in a per-domain array.
+ * Depending on the account item there might be other scopes as well, like e.g.
+ * a per transaction array.
+ */
+enum accitem {
+	ACC_NODES,
+	ACC_TR_N,		/* Number of elements per transaction. */
+};
+
 void handle_event(void);
 
 void check_domains(void);
-- 
2.35.3

[PATCH v5 03/14] tools/xenstore: introduce accounting data array for per-domain values

[Juergen Gross]

Introduce the scheme of an accounting data array for per-domain
accounting data and use it initially for the number of nodes owned by
a domain.

Make the accounting data type to be unsigned int, as no data is allowed
to be negative at any time.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
V3:
- remove domid parameter from domain_acc_add_chk() (Julien Grall)
- rename domain_acc_add_chk() (Julien Grall)
- modify overflow check (Julien Grall)
V4:
- fix overflow check
---
 tools/xenstore/xenstored_domain.c | 70 ++++++++++++++++++-------------
 tools/xenstore/xenstored_domain.h |  3 +-
 2 files changed, 43 insertions(+), 30 deletions(-)

diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index 609a9a13ab..30fb9acec6 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -69,8 +69,8 @@ struct domain
 	/* Has domain been officially introduced? */
 	bool introduced;
 
-	/* number of entry from this domain in the store */
-	int nbentry;
+	/* Accounting data for this domain. */
+	unsigned int acc[ACC_N];
 
 	/* Amount of memory allocated for this domain. */
 	int memory;
@@ -246,7 +246,7 @@ static int domain_tree_remove_sub(const void *ctx, struct connection *conn,
 
 	if (keep_orphans) {
 		set_tdb_key(node->name, &key);
-		domain->nbentry--;
+		domain_nbentry_dec(NULL, domain->domid);
 		node->perms.p[0].id = priv_domid;
 		node->acc.memory = 0;
 		domain_nbentry_inc(NULL, priv_domid);
@@ -270,7 +270,7 @@ static int domain_tree_remove_sub(const void *ctx, struct connection *conn,
 		ret = WALK_TREE_SKIP_CHILDREN;
 	}
 
-	return domain->nbentry > 0 ? ret : WALK_TREE_SUCCESS_STOP;
+	return domain->acc[ACC_NODES] ? ret : WALK_TREE_SUCCESS_STOP;
 }
 
 static void domain_tree_remove(struct domain *domain)
@@ -278,7 +278,7 @@ static void domain_tree_remove(struct domain *domain)
 	int ret;
 	struct walk_funcs walkfuncs = { .enter = domain_tree_remove_sub };
 
-	if (domain->nbentry > 0) {
+	if (domain->acc[ACC_NODES]) {
 		ret = walk_node_tree(domain, NULL, "/", &walkfuncs, domain);
 		if (ret == WALK_TREE_ERROR_STOP)
 			syslog(LOG_ERR,
@@ -437,7 +437,7 @@ int domain_get_quota(const void *ctx, struct connection *conn,
 	resp = talloc_asprintf_append(resp, "%-16s: %8d\n", #t, e); \
 	if (!resp) return ENOMEM
 
-	ent(nodes, d->nbentry);
+	ent(nodes, d->acc[ACC_NODES]);
 	ent(watches, d->nbwatch);
 	ent(transactions, ta);
 	ent(outstanding, d->nboutstanding);
@@ -1047,8 +1047,27 @@ int domain_adjust_node_perms(struct node *node)
 	return 0;
 }
 
-static int domain_nbentry_add(struct connection *conn, unsigned int domid,
-			      int add, bool no_dom_alloc)
+static int domain_acc_add_valid(struct domain *d, enum accitem what, int add)
+{
+	assert(what < ARRAY_SIZE(d->acc));
+
+	if ((add < 0 && -add > d->acc[what]) ||
+	    (add > 0 && (INT_MAX - d->acc[what]) < add)) {
+		/*
+		 * In a transaction when a node is being added/removed AND the
+		 * same node has been added/removed outside the transaction in
+		 * parallel, the resulting value will be wrong. This is no
+		 * problem, as the transaction will fail due to the resulting
+		 * conflict.
+		 */
+		return (add < 0) ? 0 : INT_MAX;
+	}
+
+	return d->acc[what] + add;
+}
+
+static int domain_acc_add(struct connection *conn, unsigned int domid,
+			  enum accitem what, int add, bool no_dom_alloc)
 {
 	struct domain *d;
 	struct list_head *head;
@@ -1071,56 +1090,49 @@ static int domain_nbentry_add(struct connection *conn, unsigned int domid,
 		}
 	}
 
-	if (conn && conn->transaction) {
+	if (conn && conn->transaction && what < ACC_TR_N) {
 		head = transaction_get_changed_domains(conn->transaction);
-		ret = acc_add_changed_dom(conn->transaction, head, ACC_NODES,
+		ret = acc_add_changed_dom(conn->transaction, head, what,
 					  add, domid);
 		if (errno) {
 			fail_transaction(conn->transaction);
 			return -1;
 		}
-		/*
-		 * In a transaction when a node is being added/removed AND the
-		 * same node has been added/removed outside the transaction in
-		 * parallel, the resulting number of nodes will be wrong. This
-		 * is no problem, as the transaction will fail due to the
-		 * resulting conflict.
-		 * In the node remove case the resulting number can be even
-		 * negative, which should be avoided.
-		 */
-		return max(d->nbentry + ret, 0);
+		return domain_acc_add_valid(d, what, ret);
 	}
 
-	d->nbentry += add;
+	d->acc[what] = domain_acc_add_valid(d, what, add);
 
-	return d->nbentry;
+	return d->acc[what];
 }
 
 int domain_nbentry_inc(struct connection *conn, unsigned int domid)
 {
-	return (domain_nbentry_add(conn, domid, 1, false) < 0) ? errno : 0;
+	return (domain_acc_add(conn, domid, ACC_NODES, 1, false) < 0)
+	       ? errno : 0;
 }
 
 int domain_nbentry_dec(struct connection *conn, unsigned int domid)
 {
-	return (domain_nbentry_add(conn, domid, -1, true) < 0) ? errno : 0;
+	return (domain_acc_add(conn, domid, ACC_NODES, -1, true) < 0)
+	       ? errno : 0;
 }
 
 int domain_nbentry_fix(unsigned int domid, int num, bool update)
 {
 	int ret;
 
-	ret = domain_nbentry_add(NULL, domid, update ? num : 0, update);
+	ret = domain_acc_add(NULL, domid, ACC_NODES, update ? num : 0, update);
 	if (ret < 0 || update)
 		return ret;
 
 	return domid_is_unprivileged(domid) ? ret + num : 0;
 }
 
-int domain_nbentry(struct connection *conn)
+unsigned int domain_nbentry(struct connection *conn)
 {
 	return domain_is_unprivileged(conn)
-	       ? domain_nbentry_add(conn, conn->id, 0, true) : 0;
+	       ? domain_acc_add(conn, conn->id, ACC_NODES, 0, true) : 0;
 }
 
 static bool domain_chk_quota(struct domain *domain, int mem)
@@ -1597,7 +1609,7 @@ static int domain_check_acc_init_sub(const void *k, void *v, void *arg)
 	 * If everything is correct incrementing the value for each node will
 	 * result in dom->nodes being 0 at the end.
 	 */
-	dom->nodes = -d->nbentry;
+	dom->nodes = -d->acc[ACC_NODES];
 
 	if (!hashtable_insert(domains, &dom->domid, dom)) {
 		talloc_free(dom);
@@ -1652,7 +1664,7 @@ static int domain_check_acc_cb(const void *k, void *v, void *arg)
 	if (!d)
 		return 0;
 
-	d->nbentry += dom->nodes;
+	d->acc[ACC_NODES] += dom->nodes;
 
 	return 0;
 }
diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h
index 40803574f6..9d05eb01da 100644
--- a/tools/xenstore/xenstored_domain.h
+++ b/tools/xenstore/xenstored_domain.h
@@ -27,6 +27,7 @@
 enum accitem {
 	ACC_NODES,
 	ACC_TR_N,		/* Number of elements per transaction. */
+	ACC_N = ACC_TR_N,	/* Number of elements per domain. */
 };
 
 void handle_event(void);
@@ -77,7 +78,7 @@ int domain_alloc_permrefs(struct node_perms *perms);
 int domain_nbentry_inc(struct connection *conn, unsigned int domid);
 int domain_nbentry_dec(struct connection *conn, unsigned int domid);
 int domain_nbentry_fix(unsigned int domid, int num, bool update);
-int domain_nbentry(struct connection *conn);
+unsigned int domain_nbentry(struct connection *conn);
 int domain_memory_add(unsigned int domid, int mem, bool no_quota_check);
 
 /*
-- 
2.35.3

[PATCH v5 04/14] tools/xenstore: add framework to commit accounting data on success only

[Juergen Gross]

Instead of modifying accounting data and undo those modifications in
case of an error during further processing, add a framework for
collecting the needed changes and commit them only when the whole
operation has succeeded.

This scheme can reuse large parts of the per transaction accounting.
The changed_domain handling can be reused, but the array size of the
accounting data should be possible to be different for both use cases.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V3:
- call acc_commit() earlier (Julien Grall)
- add assert() to acc_commit()
- use fixed sized acc array in struct changed_domain (Julien Grall)
V5:
- set conn->in to NULL only locally in acc_commit() (Julien Grall)
- define ACC_CHD_N in enum (Julien Grall)
---
 tools/xenstore/xenstored_core.c   |  7 ++++
 tools/xenstore/xenstored_core.h   |  3 ++
 tools/xenstore/xenstored_domain.c | 54 ++++++++++++++++++++++++++++++-
 tools/xenstore/xenstored_domain.h |  6 +++-
 4 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 3ca68681e3..8392bdec9b 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -1023,6 +1023,9 @@ static void send_error(struct connection *conn, int error)
 			break;
 		}
 	}
+
+	acc_drop(conn);
+
 	send_reply(conn, XS_ERROR, xsd_errors[i].errstring,
 			  strlen(xsd_errors[i].errstring) + 1);
 }
@@ -1034,6 +1037,9 @@ void send_reply(struct connection *conn, enum xsd_sockmsg_type type,
 
 	assert(type != XS_WATCH_EVENT);
 
+	/* Commit accounting now, as later errors won't undo any changes. */
+	acc_commit(conn);
+
 	if ( len > XENSTORE_PAYLOAD_MAX ) {
 		send_error(conn, E2BIG);
 		return;
@@ -2195,6 +2201,7 @@ struct connection *new_connection(const struct interface_funcs *funcs)
 	new->is_stalled = false;
 	new->transaction_started = 0;
 	INIT_LIST_HEAD(&new->out_list);
+	INIT_LIST_HEAD(&new->acc_list);
 	INIT_LIST_HEAD(&new->ref_list);
 	INIT_LIST_HEAD(&new->watches);
 	INIT_LIST_HEAD(&new->transaction_list);
diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
index c59b06551f..1f811f38cb 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -139,6 +139,9 @@ struct connection
 	struct list_head out_list;
 	uint64_t timeout_msec;
 
+	/* Not yet committed accounting data (valid if in != NULL). */
+	struct list_head acc_list;
+
 	/* Referenced requests no longer pending. */
 	struct list_head ref_list;
 
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index 30fb9acec6..e59e40088e 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -100,7 +100,7 @@ struct changed_domain
 	unsigned int domid;
 
 	/* Accounting data. */
-	int acc[ACC_TR_N];
+	int acc[ACC_CHD_N];
 };
 
 static struct hashtable *domhash;
@@ -1070,6 +1070,7 @@ static int domain_acc_add(struct connection *conn, unsigned int domid,
 			  enum accitem what, int add, bool no_dom_alloc)
 {
 	struct domain *d;
+	struct changed_domain *cd;
 	struct list_head *head;
 	int ret;
 
@@ -1090,6 +1091,22 @@ static int domain_acc_add(struct connection *conn, unsigned int domid,
 		}
 	}
 
+	/* Temporary accounting data until final commit? */
+	if (conn && conn->in && what < ACC_REQ_N) {
+		/* Consider transaction local data. */
+		ret = 0;
+		if (conn->transaction && what < ACC_TR_N) {
+			head = transaction_get_changed_domains(
+				conn->transaction);
+			cd = acc_find_changed_domain(head, domid);
+			if (cd)
+				ret = cd->acc[what];
+		}
+		ret += acc_add_changed_dom(conn->in, &conn->acc_list, what,
+					   add, domid);
+		return errno ? -1 : domain_acc_add_valid(d, what, ret);
+	}
+
 	if (conn && conn->transaction && what < ACC_TR_N) {
 		head = transaction_get_changed_domains(conn->transaction);
 		ret = acc_add_changed_dom(conn->transaction, head, what,
@@ -1106,6 +1123,41 @@ static int domain_acc_add(struct connection *conn, unsigned int domid,
 	return d->acc[what];
 }
 
+void acc_drop(struct connection *conn)
+{
+	struct changed_domain *cd;
+
+	while ((cd = list_top(&conn->acc_list, struct changed_domain, list))) {
+		list_del(&cd->list);
+		talloc_free(cd);
+	}
+}
+
+void acc_commit(struct connection *conn)
+{
+	struct changed_domain *cd;
+	enum accitem what;
+	struct buffered_data *in = conn->in;
+
+	/*
+	 * Make sure domain_acc_add() below can't add additional data to
+	 * to be committed accounting records.
+	 */
+	conn->in = NULL;
+
+	while ((cd = list_top(&conn->acc_list, struct changed_domain, list))) {
+		list_del(&cd->list);
+		for (what = 0; what < ACC_REQ_N; what++)
+			if (cd->acc[what])
+				domain_acc_add(conn, cd->domid, what,
+					       cd->acc[what], true);
+
+		talloc_free(cd);
+	}
+
+	conn->in = in;
+}
+
 int domain_nbentry_inc(struct connection *conn, unsigned int domid)
 {
 	return (domain_acc_add(conn, domid, ACC_NODES, 1, false) < 0)
diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h
index 9d05eb01da..e40657216b 100644
--- a/tools/xenstore/xenstored_domain.h
+++ b/tools/xenstore/xenstored_domain.h
@@ -25,8 +25,10 @@
  * a per transaction array.
  */
 enum accitem {
-	ACC_NODES,
+	ACC_REQ_N,		/* Number of elements per request. */
+	ACC_NODES = ACC_REQ_N,
 	ACC_TR_N,		/* Number of elements per transaction. */
+	ACC_CHD_N = ACC_TR_N,	/* max(ACC_REQ_N, ACC_TR_N), for changed dom. */
 	ACC_N = ACC_TR_N,	/* Number of elements per domain. */
 };
 
@@ -113,6 +115,8 @@ int domain_get_quota(const void *ctx, struct connection *conn,
  * If "update" is true, "chk_quota" is ignored.
  */
 int acc_fix_domains(struct list_head *head, bool chk_quota, bool update);
+void acc_drop(struct connection *conn);
+void acc_commit(struct connection *conn);
 
 /* Write rate limiting */
 
-- 
2.35.3

Re: [PATCH v5 04/14] tools/xenstore: add framework to commit accounting data on success only

[Julien Grall]

Hi Juergen,

On 08/05/2023 12:47, Juergen Gross wrote:
> Instead of modifying accounting data and undo those modifications in
> case of an error during further processing, add a framework for
> collecting the needed changes and commit them only when the whole
> operation has succeeded.
> 
> This scheme can reuse large parts of the per transaction accounting.
> The changed_domain handling can be reused, but the array size of the
> accounting data should be possible to be different for both use cases.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

[PATCH v5 05/14] tools/xenstore: use accounting buffering for node accounting

[Juergen Gross]

Add the node accounting to the accounting information buffering in
order to avoid having to undo it in case of failure.

This requires to call domain_nbentry_dec() before any changes to the
data base, as it can return an error now.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V5:
- add error handling after domain_nbentry_dec() calls (Julien Grall)
---
 tools/xenstore/xenstored_core.c   | 29 +++++++----------------------
 tools/xenstore/xenstored_domain.h |  4 ++--
 2 files changed, 9 insertions(+), 24 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 8392bdec9b..22da434e2a 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -1454,7 +1454,6 @@ static void destroy_node_rm(struct connection *conn, struct node *node)
 static int destroy_node(struct connection *conn, struct node *node)
 {
 	destroy_node_rm(conn, node);
-	domain_nbentry_dec(conn, get_node_owner(node));
 
 	/*
 	 * It is not possible to easily revert the changes in a transaction.
@@ -1645,6 +1644,9 @@ static int delnode_sub(const void *ctx, struct connection *conn,
 	if (ret > 0)
 		return WALK_TREE_SUCCESS_STOP;
 
+	if (domain_nbentry_dec(conn, get_node_owner(node)))
+		return WALK_TREE_ERROR_STOP;
+
 	/* In case of error stop the walk. */
 	if (!ret && do_tdb_delete(conn, &key, &node->acc))
 		return WALK_TREE_SUCCESS_STOP;
@@ -1657,8 +1659,6 @@ static int delnode_sub(const void *ctx, struct connection *conn,
 	watch_exact = strcmp(root, node->name);
 	fire_watches(conn, ctx, node->name, node, watch_exact, NULL);
 
-	domain_nbentry_dec(conn, get_node_owner(node));
-
 	return WALK_TREE_RM_CHILDENTRY;
 }
 
@@ -1797,29 +1797,14 @@ static int do_set_perms(const void *ctx, struct connection *conn,
 		return EPERM;
 
 	old_perms = node->perms;
-	domain_nbentry_dec(conn, get_node_owner(node));
+	if (domain_nbentry_dec(conn, get_node_owner(node)))
+		return ENOMEM;
 	node->perms = perms;
-	if (domain_nbentry_inc(conn, get_node_owner(node))) {
-		node->perms = old_perms;
-		/*
-		 * This should never fail because we had a reference on the
-		 * domain before and Xenstored is single-threaded.
-		 */
-		domain_nbentry_inc(conn, get_node_owner(node));
+	if (domain_nbentry_inc(conn, get_node_owner(node)))
 		return ENOMEM;
-	}
-
-	if (write_node(conn, node, false)) {
-		int saved_errno = errno;
 
-		domain_nbentry_dec(conn, get_node_owner(node));
-		node->perms = old_perms;
-		/* No failure possible as above. */
-		domain_nbentry_inc(conn, get_node_owner(node));
-
-		errno = saved_errno;
+	if (write_node(conn, node, false))
 		return errno;
-	}
 
 	fire_watches(conn, ctx, name, node, false, &old_perms);
 	send_ack(conn, XS_SET_PERMS);
diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h
index e40657216b..466549709f 100644
--- a/tools/xenstore/xenstored_domain.h
+++ b/tools/xenstore/xenstored_domain.h
@@ -25,9 +25,9 @@
  * a per transaction array.
  */
 enum accitem {
+	ACC_NODES,
 	ACC_REQ_N,		/* Number of elements per request. */
-	ACC_NODES = ACC_REQ_N,
-	ACC_TR_N,		/* Number of elements per transaction. */
+	ACC_TR_N = ACC_REQ_N,	/* Number of elements per transaction. */
 	ACC_CHD_N = ACC_TR_N,	/* max(ACC_REQ_N, ACC_TR_N), for changed dom. */
 	ACC_N = ACC_TR_N,	/* Number of elements per domain. */
 };
-- 
2.35.3

Re: [PATCH v5 05/14] tools/xenstore: use accounting buffering for node accounting

[Julien Grall]

Hi Juergen,

On 08/05/2023 12:47, Juergen Gross wrote:
> Add the node accounting to the accounting information buffering in
> order to avoid having to undo it in case of failure.
> 
> This requires to call domain_nbentry_dec() before any changes to the
> data base, as it can return an error now.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V5:
> - add error handling after domain_nbentry_dec() calls (Julien Grall)
> ---
>   tools/xenstore/xenstored_core.c   | 29 +++++++----------------------
>   tools/xenstore/xenstored_domain.h |  4 ++--
>   2 files changed, 9 insertions(+), 24 deletions(-)
> 
> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> index 8392bdec9b..22da434e2a 100644
> --- a/tools/xenstore/xenstored_core.c
> +++ b/tools/xenstore/xenstored_core.c
> @@ -1454,7 +1454,6 @@ static void destroy_node_rm(struct connection *conn, struct node *node)
>   static int destroy_node(struct connection *conn, struct node *node)
>   {
>   	destroy_node_rm(conn, node);
> -	domain_nbentry_dec(conn, get_node_owner(node));
>   
>   	/*
>   	 * It is not possible to easily revert the changes in a transaction.
> @@ -1645,6 +1644,9 @@ static int delnode_sub(const void *ctx, struct connection *conn,
>   	if (ret > 0)
>   		return WALK_TREE_SUCCESS_STOP;
>   
> +	if (domain_nbentry_dec(conn, get_node_owner(node)))
> +		return WALK_TREE_ERROR_STOP;

I think there is a potential issue with the buffering here. In case of 
failure, the node could have been removed, but the quota would not be 
properly accounted.

Also, I think a comment would be warrant to explain why we are returning 
WALK_TREE_ERROR_STOP here when...

> +
>   	/* In case of error stop the walk. */
>   	if (!ret && do_tdb_delete(conn, &key, &node->acc))
>   		return WALK_TREE_SUCCESS_STOP;

... this is not the case when do_tdb_delete() fails for some reasons.

> @@ -1657,8 +1659,6 @@ static int delnode_sub(const void *ctx, struct connection *conn,
>   	watch_exact = strcmp(root, node->name);
>   	fire_watches(conn, ctx, node->name, node, watch_exact, NULL);
>   
> -	domain_nbentry_dec(conn, get_node_owner(node));
> -
>   	return WALK_TREE_RM_CHILDENTRY;
>   }
>   
> @@ -1797,29 +1797,14 @@ static int do_set_perms(const void *ctx, struct connection *conn,
>   		return EPERM;
>   
>   	old_perms = node->perms;
> -	domain_nbentry_dec(conn, get_node_owner(node));
> +	if (domain_nbentry_dec(conn, get_node_owner(node)))
> +		return ENOMEM;
>   	node->perms = perms;
> -	if (domain_nbentry_inc(conn, get_node_owner(node))) {
> -		node->perms = old_perms;
> -		/*
> -		 * This should never fail because we had a reference on the
> -		 * domain before and Xenstored is single-threaded.
> -		 */
> -		domain_nbentry_inc(conn, get_node_owner(node));
> +	if (domain_nbentry_inc(conn, get_node_owner(node)))
>   		return ENOMEM;
> -	}
> -
> -	if (write_node(conn, node, false)) {
> -		int saved_errno = errno;
>   
> -		domain_nbentry_dec(conn, get_node_owner(node));
> -		node->perms = old_perms;
> -		/* No failure possible as above. */
> -		domain_nbentry_inc(conn, get_node_owner(node));
> -
> -		errno = saved_errno;
> +	if (write_node(conn, node, false))
>   		return errno;
> -	}
>   
>   	fire_watches(conn, ctx, name, node, false, &old_perms);
>   	send_ack(conn, XS_SET_PERMS);
> diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h
> index e40657216b..466549709f 100644
> --- a/tools/xenstore/xenstored_domain.h
> +++ b/tools/xenstore/xenstored_domain.h
> @@ -25,9 +25,9 @@
>    * a per transaction array.
>    */
>   enum accitem {
> +	ACC_NODES,
>   	ACC_REQ_N,		/* Number of elements per request. */
> -	ACC_NODES = ACC_REQ_N,
> -	ACC_TR_N,		/* Number of elements per transaction. */
> +	ACC_TR_N = ACC_REQ_N,	/* Number of elements per transaction. */
>   	ACC_CHD_N = ACC_TR_N,	/* max(ACC_REQ_N, ACC_TR_N), for changed dom. */
>   	ACC_N = ACC_TR_N,	/* Number of elements per domain. */
>   };


Cheers,

-- 
Julien Grall

Re: [PATCH v5 05/14] tools/xenstore: use accounting buffering for node accounting

[Juergen Gross]

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

On 09.05.23 20:46, Julien Grall wrote:
> Hi Juergen,
> 
> On 08/05/2023 12:47, Juergen Gross wrote:
>> Add the node accounting to the accounting information buffering in
>> order to avoid having to undo it in case of failure.
>>
>> This requires to call domain_nbentry_dec() before any changes to the
>> data base, as it can return an error now.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V5:
>> - add error handling after domain_nbentry_dec() calls (Julien Grall)
>> ---
>>   tools/xenstore/xenstored_core.c   | 29 +++++++----------------------
>>   tools/xenstore/xenstored_domain.h |  4 ++--
>>   2 files changed, 9 insertions(+), 24 deletions(-)
>>
>> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
>> index 8392bdec9b..22da434e2a 100644
>> --- a/tools/xenstore/xenstored_core.c
>> +++ b/tools/xenstore/xenstored_core.c
>> @@ -1454,7 +1454,6 @@ static void destroy_node_rm(struct connection *conn, 
>> struct node *node)
>>   static int destroy_node(struct connection *conn, struct node *node)
>>   {
>>       destroy_node_rm(conn, node);
>> -    domain_nbentry_dec(conn, get_node_owner(node));
>>       /*
>>        * It is not possible to easily revert the changes in a transaction.
>> @@ -1645,6 +1644,9 @@ static int delnode_sub(const void *ctx, struct 
>> connection *conn,
>>       if (ret > 0)
>>           return WALK_TREE_SUCCESS_STOP;
>> +    if (domain_nbentry_dec(conn, get_node_owner(node)))
>> +        return WALK_TREE_ERROR_STOP;
> 
> I think there is a potential issue with the buffering here. In case of failure, 
> the node could have been removed, but the quota would not be properly accounted.

You mean the case where another node has been deleted and due to accounting
buffering the corrected accounting data wouldn't be committed?

This is no problem, as an error returned by delnode_sub() will result in
corrupt() being called, which in turn will correct the accounting data.

> Also, I think a comment would be warrant to explain why we are returning 
> WALK_TREE_ERROR_STOP here when...
> 
>> +
>>       /* In case of error stop the walk. */
>>       if (!ret && do_tdb_delete(conn, &key, &node->acc))
>>           return WALK_TREE_SUCCESS_STOP;
> 
> ... this is not the case when do_tdb_delete() fails for some reasons.

The main idea was that the remove is working from the leafs towards the root.
In case one entry can't be removed, we should just stop.

OTOH returning WALK_TREE_ERROR_STOP might be cleaner, as this would make sure
that accounting data is repaired afterwards. Even if do_tdb_delete() can't
really fail in normal configurations, as the only failure reasons are:

- the node isn't found (quite unlikely, as we just read it before entering
   delnode_sub()), or
- writing the updated data base failed (in normal configurations it is in
   already allocated memory, so no way to fail that)

I think I'll switch to return WALK_TREE_ERROR_STOP here.


Juergen

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

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

Re: [PATCH v5 05/14] tools/xenstore: use accounting buffering for node accounting

[Julien Grall]

Hi,

On 10/05/2023 13:54, Juergen Gross wrote:
> On 09.05.23 20:46, Julien Grall wrote:
>> Hi Juergen,
>>
>> On 08/05/2023 12:47, Juergen Gross wrote:
>>> Add the node accounting to the accounting information buffering in
>>> order to avoid having to undo it in case of failure.
>>>
>>> This requires to call domain_nbentry_dec() before any changes to the
>>> data base, as it can return an error now.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>> V5:
>>> - add error handling after domain_nbentry_dec() calls (Julien Grall)
>>> ---
>>>   tools/xenstore/xenstored_core.c   | 29 +++++++----------------------
>>>   tools/xenstore/xenstored_domain.h |  4 ++--
>>>   2 files changed, 9 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/tools/xenstore/xenstored_core.c 
>>> b/tools/xenstore/xenstored_core.c
>>> index 8392bdec9b..22da434e2a 100644
>>> --- a/tools/xenstore/xenstored_core.c
>>> +++ b/tools/xenstore/xenstored_core.c
>>> @@ -1454,7 +1454,6 @@ static void destroy_node_rm(struct connection 
>>> *conn, struct node *node)
>>>   static int destroy_node(struct connection *conn, struct node *node)
>>>   {
>>>       destroy_node_rm(conn, node);
>>> -    domain_nbentry_dec(conn, get_node_owner(node));
>>>       /*
>>>        * It is not possible to easily revert the changes in a 
>>> transaction.
>>> @@ -1645,6 +1644,9 @@ static int delnode_sub(const void *ctx, struct 
>>> connection *conn,
>>>       if (ret > 0)
>>>           return WALK_TREE_SUCCESS_STOP;
>>> +    if (domain_nbentry_dec(conn, get_node_owner(node)))
>>> +        return WALK_TREE_ERROR_STOP;
>>
>> I think there is a potential issue with the buffering here. In case of 
>> failure, the node could have been removed, but the quota would not be 
>> properly accounted.
> 
> You mean the case where another node has been deleted and due to accounting
> buffering the corrected accounting data wouldn't be committed?
> 
> This is no problem, as an error returned by delnode_sub() will result in
> corrupt() being called, which in turn will correct the accounting data.

To me corrupt() is a big hammer and it feels wrong to call it when I 
think we have easier/faster way to deal with the issue. Could we instead 
call acc_commit() before returning?

> 
>> Also, I think a comment would be warrant to explain why we are 
>> returning WALK_TREE_ERROR_STOP here when...
>>
>>> +
>>>       /* In case of error stop the walk. */
>>>       if (!ret && do_tdb_delete(conn, &key, &node->acc))
>>>           return WALK_TREE_SUCCESS_STOP;
>>
>> ... this is not the case when do_tdb_delete() fails for some reasons.
> 
> The main idea was that the remove is working from the leafs towards the 
> root.
> In case one entry can't be removed, we should just stop.
> 
> OTOH returning WALK_TREE_ERROR_STOP might be cleaner, as this would make 
> sure
> that accounting data is repaired afterwards. Even if do_tdb_delete() can't
> really fail in normal configurations, as the only failure reasons are:
> 
> - the node isn't found (quite unlikely, as we just read it before entering
>    delnode_sub()), or
> - writing the updated data base failed (in normal configurations it is in
>    already allocated memory, so no way to fail that)
> 
> I think I'll switch to return WALK_TREE_ERROR_STOP here.

See above for a different proposal.

Cheers,

-- 
Julien Grall

Re: [PATCH v5 05/14] tools/xenstore: use accounting buffering for node accounting

[Juergen Gross]

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

On 10.05.23 23:31, Julien Grall wrote:
> Hi,
> 
> On 10/05/2023 13:54, Juergen Gross wrote:
>> On 09.05.23 20:46, Julien Grall wrote:
>>> Hi Juergen,
>>>
>>> On 08/05/2023 12:47, Juergen Gross wrote:
>>>> Add the node accounting to the accounting information buffering in
>>>> order to avoid having to undo it in case of failure.
>>>>
>>>> This requires to call domain_nbentry_dec() before any changes to the
>>>> data base, as it can return an error now.
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>> ---
>>>> V5:
>>>> - add error handling after domain_nbentry_dec() calls (Julien Grall)
>>>> ---
>>>>   tools/xenstore/xenstored_core.c   | 29 +++++++----------------------
>>>>   tools/xenstore/xenstored_domain.h |  4 ++--
>>>>   2 files changed, 9 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
>>>> index 8392bdec9b..22da434e2a 100644
>>>> --- a/tools/xenstore/xenstored_core.c
>>>> +++ b/tools/xenstore/xenstored_core.c
>>>> @@ -1454,7 +1454,6 @@ static void destroy_node_rm(struct connection *conn, 
>>>> struct node *node)
>>>>   static int destroy_node(struct connection *conn, struct node *node)
>>>>   {
>>>>       destroy_node_rm(conn, node);
>>>> -    domain_nbentry_dec(conn, get_node_owner(node));
>>>>       /*
>>>>        * It is not possible to easily revert the changes in a transaction.
>>>> @@ -1645,6 +1644,9 @@ static int delnode_sub(const void *ctx, struct 
>>>> connection *conn,
>>>>       if (ret > 0)
>>>>           return WALK_TREE_SUCCESS_STOP;
>>>> +    if (domain_nbentry_dec(conn, get_node_owner(node)))
>>>> +        return WALK_TREE_ERROR_STOP;
>>>
>>> I think there is a potential issue with the buffering here. In case of 
>>> failure, the node could have been removed, but the quota would not be 
>>> properly accounted.
>>
>> You mean the case where another node has been deleted and due to accounting
>> buffering the corrected accounting data wouldn't be committed?
>>
>> This is no problem, as an error returned by delnode_sub() will result in
>> corrupt() being called, which in turn will correct the accounting data.
> 
> To me corrupt() is a big hammer and it feels wrong to call it when I think we 
> have easier/faster way to deal with the issue. Could we instead call 
> acc_commit() before returning?

You are aware that this is a very problematic situation we are in?

We couldn't allocate a small amount of memory (around 64 bytes)! Xenstored
will probably die within milliseconds. Using the big hammer in such a
situation is fine IMO. It will maybe result in solving the problem by
freeing of memory (quite unlikely, though), but it won't leave xenstored
in a worse state than with your suggestion.

And calling acc_commit() here wouldn't really help, as accounting data
couldn't be recorded, so there are missing updates anyway due to the failed
call of domain_nbentry_dec().

>>> Also, I think a comment would be warrant to explain why we are returning 
>>> WALK_TREE_ERROR_STOP here when...
>>>
>>>> +
>>>>       /* In case of error stop the walk. */
>>>>       if (!ret && do_tdb_delete(conn, &key, &node->acc))
>>>>           return WALK_TREE_SUCCESS_STOP;
>>>
>>> ... this is not the case when do_tdb_delete() fails for some reasons.
>>
>> The main idea was that the remove is working from the leafs towards the root.
>> In case one entry can't be removed, we should just stop.
>>
>> OTOH returning WALK_TREE_ERROR_STOP might be cleaner, as this would make sure
>> that accounting data is repaired afterwards. Even if do_tdb_delete() can't
>> really fail in normal configurations, as the only failure reasons are:
>>
>> - the node isn't found (quite unlikely, as we just read it before entering
>>    delnode_sub()), or
>> - writing the updated data base failed (in normal configurations it is in
>>    already allocated memory, so no way to fail that)
>>
>> I think I'll switch to return WALK_TREE_ERROR_STOP here.
> 
> See above for a different proposal.

Without deleting the node in the data base this would be another accounting
data inconsistency, so calling corrupt() is the correct cleanup measure.


Juergen

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

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

Re: [PATCH v5 05/14] tools/xenstore: use accounting buffering for node accounting

[Julien Grall]

Hi Juergen,

On 11/05/2023 06:25, Juergen Gross wrote:
> On 10.05.23 23:31, Julien Grall wrote:
>> On 10/05/2023 13:54, Juergen Gross wrote:
>>> On 09.05.23 20:46, Julien Grall wrote:
>>>> Hi Juergen,
>>>>
>>>> On 08/05/2023 12:47, Juergen Gross wrote:
>>>>> Add the node accounting to the accounting information buffering in
>>>>> order to avoid having to undo it in case of failure.
>>>>>
>>>>> This requires to call domain_nbentry_dec() before any changes to the
>>>>> data base, as it can return an error now.
>>>>>
>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>> ---
>>>>> V5:
>>>>> - add error handling after domain_nbentry_dec() calls (Julien Grall)
>>>>> ---
>>>>>   tools/xenstore/xenstored_core.c   | 29 +++++++----------------------
>>>>>   tools/xenstore/xenstored_domain.h |  4 ++--
>>>>>   2 files changed, 9 insertions(+), 24 deletions(-)
>>>>>
>>>>> diff --git a/tools/xenstore/xenstored_core.c 
>>>>> b/tools/xenstore/xenstored_core.c
>>>>> index 8392bdec9b..22da434e2a 100644
>>>>> --- a/tools/xenstore/xenstored_core.c
>>>>> +++ b/tools/xenstore/xenstored_core.c
>>>>> @@ -1454,7 +1454,6 @@ static void destroy_node_rm(struct connection 
>>>>> *conn, struct node *node)
>>>>>   static int destroy_node(struct connection *conn, struct node *node)
>>>>>   {
>>>>>       destroy_node_rm(conn, node);
>>>>> -    domain_nbentry_dec(conn, get_node_owner(node));
>>>>>       /*
>>>>>        * It is not possible to easily revert the changes in a 
>>>>> transaction.
>>>>> @@ -1645,6 +1644,9 @@ static int delnode_sub(const void *ctx, 
>>>>> struct connection *conn,
>>>>>       if (ret > 0)
>>>>>           return WALK_TREE_SUCCESS_STOP;
>>>>> +    if (domain_nbentry_dec(conn, get_node_owner(node)))
>>>>> +        return WALK_TREE_ERROR_STOP;
>>>>
>>>> I think there is a potential issue with the buffering here. In case 
>>>> of failure, the node could have been removed, but the quota would 
>>>> not be properly accounted.
>>>
>>> You mean the case where another node has been deleted and due to 
>>> accounting
>>> buffering the corrected accounting data wouldn't be committed?
>>>
>>> This is no problem, as an error returned by delnode_sub() will result in
>>> corrupt() being called, which in turn will correct the accounting data.
>>
>> To me corrupt() is a big hammer and it feels wrong to call it when I 
>> think we have easier/faster way to deal with the issue. Could we 
>> instead call acc_commit() before returning?
> 
> You are aware that this is a very problematic situation we are in?

It is not very clear from the code. And that's why comments are always 
useful to clarify why corrupt() is the right call.

> 
> We couldn't allocate a small amount of memory (around 64 bytes)! 

So long this is the only reason then...

Xenstored
> will probably die within milliseconds. Using the big hammer in such a
> situation is fine IMO. It will maybe result in solving the problem by
> freeing of memory (quite unlikely, though), but it won't leave xenstored
> in a worse state than with your suggestion.

... this might be OK. But in the past, we had a place where corrupt() 
could be reliably triggered by a guest. If you think that's not 
possible, then it should be properly documented.

> 
> And calling acc_commit() here wouldn't really help, as accounting data
> couldn't be recorded, so there are missing updates anyway due to the failed
> call of domain_nbentry_dec().

We are removing the node after the accounting is updated. So if the 
accounting fail, then it should still be correct for anything that was 
removed before.

> 
>>>> Also, I think a comment would be warrant to explain why we are 
>>>> returning WALK_TREE_ERROR_STOP here when...
>>>>
>>>>> +
>>>>>       /* In case of error stop the walk. */
>>>>>       if (!ret && do_tdb_delete(conn, &key, &node->acc))
>>>>>           return WALK_TREE_SUCCESS_STOP;
>>>>
>>>> ... this is not the case when do_tdb_delete() fails for some reasons.
>>>
>>> The main idea was that the remove is working from the leafs towards 
>>> the root.
>>> In case one entry can't be removed, we should just stop.
>>>
>>> OTOH returning WALK_TREE_ERROR_STOP might be cleaner, as this would 
>>> make sure
>>> that accounting data is repaired afterwards. Even if do_tdb_delete() 
>>> can't
>>> really fail in normal configurations, as the only failure reasons are:
>>>
>>> - the node isn't found (quite unlikely, as we just read it before 
>>> entering
>>>    delnode_sub()), or
>>> - writing the updated data base failed (in normal configurations it 
>>> is in
>>>    already allocated memory, so no way to fail that)
>>>
>>> I think I'll switch to return WALK_TREE_ERROR_STOP here.
>>
>> See above for a different proposal.
> 
> Without deleting the node in the data base this would be another accounting
> data inconsistency, so calling corrupt() is the correct cleanup measure.

Hmmm... I read this as this is already a bug rather than one introduced 
by this patch. IIUC, then this should be done in a new commit.

Cheers,

-- 
Julien Grall

Re: [PATCH v5 05/14] tools/xenstore: use accounting buffering for node accounting

[Juergen Gross]

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

On 11.05.23 14:07, Julien Grall wrote:
> Hi Juergen,
> 
> On 11/05/2023 06:25, Juergen Gross wrote:
>> On 10.05.23 23:31, Julien Grall wrote:
>>> On 10/05/2023 13:54, Juergen Gross wrote:
>>>> On 09.05.23 20:46, Julien Grall wrote:
>>>>> Hi Juergen,
>>>>>
>>>>> On 08/05/2023 12:47, Juergen Gross wrote:
>>>>>> Add the node accounting to the accounting information buffering in
>>>>>> order to avoid having to undo it in case of failure.
>>>>>>
>>>>>> This requires to call domain_nbentry_dec() before any changes to the
>>>>>> data base, as it can return an error now.
>>>>>>
>>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>>> ---
>>>>>> V5:
>>>>>> - add error handling after domain_nbentry_dec() calls (Julien Grall)
>>>>>> ---
>>>>>>   tools/xenstore/xenstored_core.c   | 29 +++++++----------------------
>>>>>>   tools/xenstore/xenstored_domain.h |  4 ++--
>>>>>>   2 files changed, 9 insertions(+), 24 deletions(-)
>>>>>>
>>>>>> diff --git a/tools/xenstore/xenstored_core.c 
>>>>>> b/tools/xenstore/xenstored_core.c
>>>>>> index 8392bdec9b..22da434e2a 100644
>>>>>> --- a/tools/xenstore/xenstored_core.c
>>>>>> +++ b/tools/xenstore/xenstored_core.c
>>>>>> @@ -1454,7 +1454,6 @@ static void destroy_node_rm(struct connection *conn, 
>>>>>> struct node *node)
>>>>>>   static int destroy_node(struct connection *conn, struct node *node)
>>>>>>   {
>>>>>>       destroy_node_rm(conn, node);
>>>>>> -    domain_nbentry_dec(conn, get_node_owner(node));
>>>>>>       /*
>>>>>>        * It is not possible to easily revert the changes in a transaction.
>>>>>> @@ -1645,6 +1644,9 @@ static int delnode_sub(const void *ctx, struct 
>>>>>> connection *conn,
>>>>>>       if (ret > 0)
>>>>>>           return WALK_TREE_SUCCESS_STOP;
>>>>>> +    if (domain_nbentry_dec(conn, get_node_owner(node)))
>>>>>> +        return WALK_TREE_ERROR_STOP;
>>>>>
>>>>> I think there is a potential issue with the buffering here. In case of 
>>>>> failure, the node could have been removed, but the quota would not be 
>>>>> properly accounted.
>>>>
>>>> You mean the case where another node has been deleted and due to accounting
>>>> buffering the corrected accounting data wouldn't be committed?
>>>>
>>>> This is no problem, as an error returned by delnode_sub() will result in
>>>> corrupt() being called, which in turn will correct the accounting data.
>>>
>>> To me corrupt() is a big hammer and it feels wrong to call it when I think we 
>>> have easier/faster way to deal with the issue. Could we instead call 
>>> acc_commit() before returning?
>>
>> You are aware that this is a very problematic situation we are in?
> 
> It is not very clear from the code. And that's why comments are always useful to 
> clarify why corrupt() is the right call.

I agree. I'll add a comment.

> 
>>
>> We couldn't allocate a small amount of memory (around 64 bytes)! 
> 
> So long this is the only reason then...
> 
> Xenstored
>> will probably die within milliseconds. Using the big hammer in such a
>> situation is fine IMO. It will maybe result in solving the problem by
>> freeing of memory (quite unlikely, though), but it won't leave xenstored
>> in a worse state than with your suggestion.
> 
> ... this might be OK. But in the past, we had a place where corrupt() could be 
> reliably triggered by a guest. If you think that's not possible, then it should 
> be properly documented.

Okay, will do so.

> 
>>
>> And calling acc_commit() here wouldn't really help, as accounting data
>> couldn't be recorded, so there are missing updates anyway due to the failed
>> call of domain_nbentry_dec().
> 
> We are removing the node after the accounting is updated. So if the accounting 
> fail, then it should still be correct for anything that was removed before.

Oh, right.

> 
>>
>>>>> Also, I think a comment would be warrant to explain why we are returning 
>>>>> WALK_TREE_ERROR_STOP here when...
>>>>>
>>>>>> +
>>>>>>       /* In case of error stop the walk. */
>>>>>>       if (!ret && do_tdb_delete(conn, &key, &node->acc))
>>>>>>           return WALK_TREE_SUCCESS_STOP;
>>>>>
>>>>> ... this is not the case when do_tdb_delete() fails for some reasons.
>>>>
>>>> The main idea was that the remove is working from the leafs towards the root.
>>>> In case one entry can't be removed, we should just stop.
>>>>
>>>> OTOH returning WALK_TREE_ERROR_STOP might be cleaner, as this would make sure
>>>> that accounting data is repaired afterwards. Even if do_tdb_delete() can't
>>>> really fail in normal configurations, as the only failure reasons are:
>>>>
>>>> - the node isn't found (quite unlikely, as we just read it before entering
>>>>    delnode_sub()), or
>>>> - writing the updated data base failed (in normal configurations it is in
>>>>    already allocated memory, so no way to fail that)
>>>>
>>>> I think I'll switch to return WALK_TREE_ERROR_STOP here.
>>>
>>> See above for a different proposal.
>>
>> Without deleting the node in the data base this would be another accounting
>> data inconsistency, so calling corrupt() is the correct cleanup measure.
> 
> Hmmm... I read this as this is already a bug rather than one introduced by this 
> patch. IIUC, then this should be done in a new commit.

No, previously domain_nbentry_dec() couldn't fail, so this is a new situation.
Or did I misunderstand what you mean?


Juergen

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

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

[PATCH v5 06/14] tools/xenstore: add current connection to domain_memory_add() parameters

[Juergen Gross]

In order to enable switching memory accounting to the generic array
based accounting, add the current connection to the parameters of
domain_memory_add().

This requires to add the connection to some other functions, too.

Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---
 tools/xenstore/xenstored_core.c   | 28 ++++++++++++++++------------
 tools/xenstore/xenstored_domain.c |  3 ++-
 tools/xenstore/xenstored_domain.h | 14 +++++++++-----
 tools/xenstore/xenstored_watch.c  | 11 ++++++-----
 4 files changed, 33 insertions(+), 23 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 22da434e2a..4d1debeba1 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -246,7 +246,8 @@ static void free_buffered_data(struct buffered_data *out,
 		}
 	}
 
-	domain_memory_add_nochk(conn->id, -out->hdr.msg.len - sizeof(out->hdr));
+	domain_memory_add_nochk(conn, conn->id,
+				-out->hdr.msg.len - sizeof(out->hdr));
 
 	if (out->hdr.msg.type == XS_WATCH_EVENT) {
 		req = out->pend.req;
@@ -631,24 +632,25 @@ int do_tdb_write(struct connection *conn, TDB_DATA *key, TDB_DATA *data,
 	 * nodes to new owners.
 	 */
 	if (old_acc.memory)
-		domain_memory_add_nochk(old_domid,
+		domain_memory_add_nochk(conn, old_domid,
 					-old_acc.memory - key->dsize);
-	ret = domain_memory_add(new_domid, data->dsize + key->dsize,
-				no_quota_check);
+	ret = domain_memory_add(conn, new_domid,
+				data->dsize + key->dsize, no_quota_check);
 	if (ret) {
 		/* Error path, so no quota check. */
 		if (old_acc.memory)
-			domain_memory_add_nochk(old_domid,
+			domain_memory_add_nochk(conn, old_domid,
 						old_acc.memory + key->dsize);
 		return ret;
 	}
 
 	/* TDB should set errno, but doesn't even set ecode AFAICT. */
 	if (tdb_store(tdb_ctx, *key, *data, TDB_REPLACE) != 0) {
-		domain_memory_add_nochk(new_domid, -data->dsize - key->dsize);
+		domain_memory_add_nochk(conn, new_domid,
+					-data->dsize - key->dsize);
 		/* Error path, so no quota check. */
 		if (old_acc.memory)
-			domain_memory_add_nochk(old_domid,
+			domain_memory_add_nochk(conn, old_domid,
 						old_acc.memory + key->dsize);
 		errno = EIO;
 		return errno;
@@ -683,7 +685,7 @@ int do_tdb_delete(struct connection *conn, TDB_DATA *key,
 
 	if (acc->memory) {
 		domid = get_acc_domid(conn, key, acc->domid);
-		domain_memory_add_nochk(domid, -acc->memory - key->dsize);
+		domain_memory_add_nochk(conn, domid, -acc->memory - key->dsize);
 	}
 
 	return 0;
@@ -1055,11 +1057,13 @@ void send_reply(struct connection *conn, enum xsd_sockmsg_type type,
 	if (len <= DEFAULT_BUFFER_SIZE) {
 		bdata->buffer = bdata->default_buffer;
 		/* Don't check quota, path might be used for returning error. */
-		domain_memory_add_nochk(conn->id, len + sizeof(bdata->hdr));
+		domain_memory_add_nochk(conn, conn->id,
+					len + sizeof(bdata->hdr));
 	} else {
 		bdata->buffer = talloc_array(bdata, char, len);
 		if (!bdata->buffer ||
-		    domain_memory_add_chk(conn->id, len + sizeof(bdata->hdr))) {
+		    domain_memory_add_chk(conn, conn->id,
+					  len + sizeof(bdata->hdr))) {
 			send_error(conn, ENOMEM);
 			return;
 		}
@@ -1124,7 +1128,7 @@ void send_event(struct buffered_data *req, struct connection *conn,
 		}
 	}
 
-	if (domain_memory_add_chk(conn->id, len + sizeof(bdata->hdr))) {
+	if (domain_memory_add_chk(conn, conn->id, len + sizeof(bdata->hdr))) {
 		talloc_free(bdata);
 		return;
 	}
@@ -3326,7 +3330,7 @@ static void add_buffered_data(struct buffered_data *bdata,
 	 * be smaller. So ignore it. The limit will be applied for any resource
 	 * after the state has been fully restored.
 	 */
-	domain_memory_add_nochk(conn->id, len + sizeof(bdata->hdr));
+	domain_memory_add_nochk(conn, conn->id, len + sizeof(bdata->hdr));
 }
 
 void read_state_buffered_data(const void *ctx, struct connection *conn,
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index e59e40088e..7770c4f395 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -1236,7 +1236,8 @@ static bool domain_chk_quota(struct domain *domain, int mem)
 	return false;
 }
 
-int domain_memory_add(unsigned int domid, int mem, bool no_quota_check)
+int domain_memory_add(struct connection *conn, unsigned int domid, int mem,
+		      bool no_quota_check)
 {
 	struct domain *domain;
 
diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h
index 466549709f..b94548fd7d 100644
--- a/tools/xenstore/xenstored_domain.h
+++ b/tools/xenstore/xenstored_domain.h
@@ -81,25 +81,29 @@ int domain_nbentry_inc(struct connection *conn, unsigned int domid);
 int domain_nbentry_dec(struct connection *conn, unsigned int domid);
 int domain_nbentry_fix(unsigned int domid, int num, bool update);
 unsigned int domain_nbentry(struct connection *conn);
-int domain_memory_add(unsigned int domid, int mem, bool no_quota_check);
+int domain_memory_add(struct connection *conn, unsigned int domid, int mem,
+		      bool no_quota_check);
 
 /*
  * domain_memory_add_chk(): to be used when memory quota should be checked.
  * Not to be used when specifying a negative mem value, as lowering the used
  * memory should always be allowed.
  */
-static inline int domain_memory_add_chk(unsigned int domid, int mem)
+static inline int domain_memory_add_chk(struct connection *conn,
+					unsigned int domid, int mem)
 {
-	return domain_memory_add(domid, mem, false);
+	return domain_memory_add(conn, domid, mem, false);
 }
+
 /*
  * domain_memory_add_nochk(): to be used when memory quota should not be
  * checked, e.g. when lowering memory usage, or in an error case for undoing
  * a previous memory adjustment.
  */
-static inline void domain_memory_add_nochk(unsigned int domid, int mem)
+static inline void domain_memory_add_nochk(struct connection *conn,
+					   unsigned int domid, int mem)
 {
-	domain_memory_add(domid, mem, true);
+	domain_memory_add(conn, domid, mem, true);
 }
 void domain_watch_inc(struct connection *conn);
 void domain_watch_dec(struct connection *conn);
diff --git a/tools/xenstore/xenstored_watch.c b/tools/xenstore/xenstored_watch.c
index 8ad0229df6..e30cd89be3 100644
--- a/tools/xenstore/xenstored_watch.c
+++ b/tools/xenstore/xenstored_watch.c
@@ -199,7 +199,7 @@ static struct watch *add_watch(struct connection *conn, char *path, char *token,
 	watch->token = talloc_strdup(watch, token);
 	if (!watch->node || !watch->token)
 		goto nomem;
-	if (domain_memory_add(conn->id, strlen(path) + strlen(token),
+	if (domain_memory_add(conn, conn->id, strlen(path) + strlen(token),
 			      no_quota_check))
 		goto nomem;
 
@@ -274,8 +274,9 @@ int do_unwatch(const void *ctx, struct connection *conn,
 	list_for_each_entry(watch, &conn->watches, list) {
 		if (streq(watch->node, node) && streq(watch->token, vec[1])) {
 			list_del(&watch->list);
-			domain_memory_add_nochk(conn->id, -strlen(watch->node) -
-							  strlen(watch->token));
+			domain_memory_add_nochk(conn, conn->id,
+						-strlen(watch->node) -
+						strlen(watch->token));
 			talloc_free(watch);
 			domain_watch_dec(conn);
 			send_ack(conn, XS_UNWATCH);
@@ -291,8 +292,8 @@ void conn_delete_all_watches(struct connection *conn)
 
 	while ((watch = list_top(&conn->watches, struct watch, list))) {
 		list_del(&watch->list);
-		domain_memory_add_nochk(conn->id, -strlen(watch->node) -
-						  strlen(watch->token));
+		domain_memory_add_nochk(conn, conn->id, -strlen(watch->node) -
+							strlen(watch->token));
 		talloc_free(watch);
 		domain_watch_dec(conn);
 	}
-- 
2.35.3

[PATCH v5 07/14] tools/xenstore: use accounting data array for per-domain values

[Juergen Gross]

Add the accounting of per-domain usage of Xenstore memory, watches, and
outstanding requests to the array based mechanism.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V5:
- drop domid parameter from domain_outstanding_inc() (Julien Grall)
---
 tools/xenstore/xenstored_core.c   |   4 +-
 tools/xenstore/xenstored_domain.c | 109 +++++++++++-------------------
 tools/xenstore/xenstored_domain.h |   8 ++-
 3 files changed, 48 insertions(+), 73 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 4d1debeba1..e7f86f9487 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -255,7 +255,7 @@ static void free_buffered_data(struct buffered_data *out,
 			req->pend.ref.event_cnt--;
 			if (!req->pend.ref.event_cnt && !req->on_out_list) {
 				if (req->on_ref_list) {
-					domain_outstanding_domid_dec(
+					domain_outstanding_dec(conn,
 						req->pend.ref.domid);
 					list_del(&req->list);
 				}
@@ -271,7 +271,7 @@ static void free_buffered_data(struct buffered_data *out,
 		out->on_ref_list = true;
 		return;
 	} else
-		domain_outstanding_dec(conn);
+		domain_outstanding_dec(conn, conn->id);
 
 	talloc_free(out);
 }
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index 7770c4f395..a35ed97fd7 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -72,19 +72,12 @@ struct domain
 	/* Accounting data for this domain. */
 	unsigned int acc[ACC_N];
 
-	/* Amount of memory allocated for this domain. */
-	int memory;
+	/* Memory quota data for this domain. */
 	bool soft_quota_reported;
 	bool hard_quota_reported;
 	time_t mem_last_msg;
 #define MEM_WARN_MINTIME_SEC 10
 
-	/* number of watch for this domain */
-	int nbwatch;
-
-	/* Number of outstanding requests. */
-	int nboutstanding;
-
 	/* write rate limit */
 	wrl_creditt wrl_credit; /* [ -wrl_config_writecost, +_dburst ] */
 	struct wrl_timestampt wrl_timestamp;
@@ -200,14 +193,15 @@ static bool domain_can_write(struct connection *conn)
 
 static bool domain_can_read(struct connection *conn)
 {
-	struct xenstore_domain_interface *intf = conn->domain->interface;
+	struct domain *domain = conn->domain;
+	struct xenstore_domain_interface *intf = domain->interface;
 
 	if (domain_is_unprivileged(conn)) {
-		if (conn->domain->wrl_credit < 0)
+		if (domain->wrl_credit < 0)
 			return false;
-		if (conn->domain->nboutstanding >= quota_req_outstanding)
+		if (domain->acc[ACC_OUTST] >= quota_req_outstanding)
 			return false;
-		if (conn->domain->memory >= quota_memory_per_domain_hard &&
+		if (domain->acc[ACC_MEM] >= quota_memory_per_domain_hard &&
 		    quota_memory_per_domain_hard)
 			return false;
 	}
@@ -438,10 +432,10 @@ int domain_get_quota(const void *ctx, struct connection *conn,
 	if (!resp) return ENOMEM
 
 	ent(nodes, d->acc[ACC_NODES]);
-	ent(watches, d->nbwatch);
+	ent(watches, d->acc[ACC_WATCH]);
 	ent(transactions, ta);
-	ent(outstanding, d->nboutstanding);
-	ent(memory, d->memory);
+	ent(outstanding, d->acc[ACC_OUTST]);
+	ent(memory, d->acc[ACC_MEM]);
 
 #undef ent
 
@@ -1187,14 +1181,16 @@ unsigned int domain_nbentry(struct connection *conn)
 	       ? domain_acc_add(conn, conn->id, ACC_NODES, 0, true) : 0;
 }
 
-static bool domain_chk_quota(struct domain *domain, int mem)
+static bool domain_chk_quota(struct connection *conn, unsigned int mem)
 {
 	time_t now;
+	struct domain *domain;
 
-	if (!domain || !domid_is_unprivileged(domain->domid) ||
-	    (domain->conn && domain->conn->is_ignored))
+	if (!conn || !domid_is_unprivileged(conn->id) ||
+	    conn->is_ignored)
 		return false;
 
+	domain = conn->domain;
 	now = time(NULL);
 
 	if (mem >= quota_memory_per_domain_hard &&
@@ -1239,80 +1235,57 @@ static bool domain_chk_quota(struct domain *domain, int mem)
 int domain_memory_add(struct connection *conn, unsigned int domid, int mem,
 		      bool no_quota_check)
 {
-	struct domain *domain;
+	int ret;
 
-	domain = find_domain_struct(domid);
-	if (domain) {
-		/*
-		 * domain_chk_quota() will print warning and also store whether
-		 * the soft/hard quota has been hit. So check no_quota_check
-		 * *after*.
-		 */
-		if (domain_chk_quota(domain, domain->memory + mem) &&
-		    !no_quota_check)
-			return ENOMEM;
-		domain->memory += mem;
-	} else {
-		/*
-		 * The domain the memory is to be accounted for should always
-		 * exist, as accounting is done either for a domain related to
-		 * the current connection, or for the domain owning a node
-		 * (which is always existing, as the owner of the node is
-		 * tested to exist and deleted or replaced by domid 0 if not).
-		 * So not finding the related domain MUST be an error in the
-		 * data base.
-		 */
-		errno = ENOENT;
-		corrupt(NULL, "Accounting called for non-existing domain %u\n",
-			domid);
-		return ENOENT;
-	}
+	ret = domain_acc_add(conn, domid, ACC_MEM, 0, true);
+	if (ret < 0)
+		return -ret;
+
+	/*
+	 * domain_chk_quota() will print warning and also store whether the
+	 * soft/hard quota has been hit. So check no_quota_check *after*.
+	 */
+	if (domain_chk_quota(conn, ret + mem) && !no_quota_check)
+		return ENOMEM;
+
+	/*
+	 * The domain the memory is to be accounted for should always exist,
+	 * as accounting is done either for a domain related to the current
+	 * connection, or for the domain owning a node (which is always
+	 * existing, as the owner of the node is tested to exist and deleted
+	 * or replaced by domid 0 if not).
+	 * So not finding the related domain MUST be an error in the data base.
+	 */
+	domain_acc_add(conn, domid, ACC_MEM, mem, true);
 
 	return 0;
 }
 
 void domain_watch_inc(struct connection *conn)
 {
-	if (!conn || !conn->domain)
-		return;
-	conn->domain->nbwatch++;
+	domain_acc_add(conn, conn->id, ACC_WATCH, 1, true);
 }
 
 void domain_watch_dec(struct connection *conn)
 {
-	if (!conn || !conn->domain)
-		return;
-	if (conn->domain->nbwatch)
-		conn->domain->nbwatch--;
+	domain_acc_add(conn, conn->id, ACC_WATCH, -1, true);
 }
 
 int domain_watch(struct connection *conn)
 {
 	return (domain_is_unprivileged(conn))
-		? conn->domain->nbwatch
+		? domain_acc_add(conn, conn->id, ACC_WATCH, 0, true)
 		: 0;
 }
 
 void domain_outstanding_inc(struct connection *conn)
 {
-	if (!conn || !conn->domain)
-		return;
-	conn->domain->nboutstanding++;
+	domain_acc_add(conn, conn->id, ACC_OUTST, 1, true);
 }
 
-void domain_outstanding_dec(struct connection *conn)
+void domain_outstanding_dec(struct connection *conn, unsigned int domid)
 {
-	if (!conn || !conn->domain)
-		return;
-	conn->domain->nboutstanding--;
-}
-
-void domain_outstanding_domid_dec(unsigned int domid)
-{
-	struct domain *d = find_domain_by_domid(domid);
-
-	if (d)
-		d->nboutstanding--;
+	domain_acc_add(conn, domid, ACC_OUTST, -1, true);
 }
 
 static wrl_creditt wrl_config_writecost      = WRL_FACTOR;
diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h
index b94548fd7d..086133407b 100644
--- a/tools/xenstore/xenstored_domain.h
+++ b/tools/xenstore/xenstored_domain.h
@@ -29,7 +29,10 @@ enum accitem {
 	ACC_REQ_N,		/* Number of elements per request. */
 	ACC_TR_N = ACC_REQ_N,	/* Number of elements per transaction. */
 	ACC_CHD_N = ACC_TR_N,	/* max(ACC_REQ_N, ACC_TR_N), for changed dom. */
-	ACC_N = ACC_TR_N,	/* Number of elements per domain. */
+	ACC_WATCH = ACC_TR_N,
+	ACC_OUTST,
+	ACC_MEM,
+	ACC_N,			/* Number of elements per domain. */
 };
 
 void handle_event(void);
@@ -109,8 +112,7 @@ void domain_watch_inc(struct connection *conn);
 void domain_watch_dec(struct connection *conn);
 int domain_watch(struct connection *conn);
 void domain_outstanding_inc(struct connection *conn);
-void domain_outstanding_dec(struct connection *conn);
-void domain_outstanding_domid_dec(unsigned int domid);
+void domain_outstanding_dec(struct connection *conn, unsigned int domid);
 int domain_get_quota(const void *ctx, struct connection *conn,
 		     unsigned int domid);
 
-- 
2.35.3

Re: [PATCH v5 07/14] tools/xenstore: use accounting data array for per-domain values

[Julien Grall]

On 08/05/2023 12:47, Juergen Gross wrote:
> Add the accounting of per-domain usage of Xenstore memory, watches, and
> outstanding requests to the array based mechanism.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Acked-by: Julien Grall <jgrall@amazon.com>

> ---
> V5:
> - drop domid parameter from domain_outstanding_inc() (Julien Grall)
> ---
>   tools/xenstore/xenstored_core.c   |   4 +-
>   tools/xenstore/xenstored_domain.c | 109 +++++++++++-------------------
>   tools/xenstore/xenstored_domain.h |   8 ++-
>   3 files changed, 48 insertions(+), 73 deletions(-)
> 
> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> index 4d1debeba1..e7f86f9487 100644
> --- a/tools/xenstore/xenstored_core.c
> +++ b/tools/xenstore/xenstored_core.c
> @@ -255,7 +255,7 @@ static void free_buffered_data(struct buffered_data *out,
>   			req->pend.ref.event_cnt--;
>   			if (!req->pend.ref.event_cnt && !req->on_out_list) {
>   				if (req->on_ref_list) {
> -					domain_outstanding_domid_dec(
> +					domain_outstanding_dec(conn,
>   						req->pend.ref.domid);
>   					list_del(&req->list);
>   				}
> @@ -271,7 +271,7 @@ static void free_buffered_data(struct buffered_data *out,
>   		out->on_ref_list = true;
>   		return;
>   	} else
> -		domain_outstanding_dec(conn);
> +		domain_outstanding_dec(conn, conn->id);
>   
>   	talloc_free(out);
>   }
> diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
> index 7770c4f395..a35ed97fd7 100644
> --- a/tools/xenstore/xenstored_domain.c
> +++ b/tools/xenstore/xenstored_domain.c
> @@ -72,19 +72,12 @@ struct domain
>   	/* Accounting data for this domain. */
>   	unsigned int acc[ACC_N];
>   
> -	/* Amount of memory allocated for this domain. */
> -	int memory;
> +	/* Memory quota data for this domain. */
>   	bool soft_quota_reported;
>   	bool hard_quota_reported;
>   	time_t mem_last_msg;
>   #define MEM_WARN_MINTIME_SEC 10
>   
> -	/* number of watch for this domain */
> -	int nbwatch;
> -
> -	/* Number of outstanding requests. */
> -	int nboutstanding;
> -
>   	/* write rate limit */
>   	wrl_creditt wrl_credit; /* [ -wrl_config_writecost, +_dburst ] */
>   	struct wrl_timestampt wrl_timestamp;
> @@ -200,14 +193,15 @@ static bool domain_can_write(struct connection *conn)
>   
>   static bool domain_can_read(struct connection *conn)
>   {
> -	struct xenstore_domain_interface *intf = conn->domain->interface;
> +	struct domain *domain = conn->domain;
> +	struct xenstore_domain_interface *intf = domain->interface;
>   
>   	if (domain_is_unprivileged(conn)) {
> -		if (conn->domain->wrl_credit < 0)
> +		if (domain->wrl_credit < 0)
>   			return false;
> -		if (conn->domain->nboutstanding >= quota_req_outstanding)
> +		if (domain->acc[ACC_OUTST] >= quota_req_outstanding)
>   			return false;
> -		if (conn->domain->memory >= quota_memory_per_domain_hard &&
> +		if (domain->acc[ACC_MEM] >= quota_memory_per_domain_hard &&
>   		    quota_memory_per_domain_hard)
>   			return false;
>   	}
> @@ -438,10 +432,10 @@ int domain_get_quota(const void *ctx, struct connection *conn,
>   	if (!resp) return ENOMEM
>   
>   	ent(nodes, d->acc[ACC_NODES]);
> -	ent(watches, d->nbwatch);
> +	ent(watches, d->acc[ACC_WATCH]);
>   	ent(transactions, ta);
> -	ent(outstanding, d->nboutstanding);
> -	ent(memory, d->memory);
> +	ent(outstanding, d->acc[ACC_OUTST]);
> +	ent(memory, d->acc[ACC_MEM]);
>   
>   #undef ent
>   
> @@ -1187,14 +1181,16 @@ unsigned int domain_nbentry(struct connection *conn)
>   	       ? domain_acc_add(conn, conn->id, ACC_NODES, 0, true) : 0;
>   }
>   
> -static bool domain_chk_quota(struct domain *domain, int mem)
> +static bool domain_chk_quota(struct connection *conn, unsigned int mem)
>   {
>   	time_t now;
> +	struct domain *domain;
>   
> -	if (!domain || !domid_is_unprivileged(domain->domid) ||
> -	    (domain->conn && domain->conn->is_ignored))
> +	if (!conn || !domid_is_unprivileged(conn->id) ||
> +	    conn->is_ignored)
>   		return false;
>   
> +	domain = conn->domain;
>   	now = time(NULL);
>   
>   	if (mem >= quota_memory_per_domain_hard &&
> @@ -1239,80 +1235,57 @@ static bool domain_chk_quota(struct domain *domain, int mem)
>   int domain_memory_add(struct connection *conn, unsigned int domid, int mem,
>   		      bool no_quota_check)
>   {
> -	struct domain *domain;
> +	int ret;
>   
> -	domain = find_domain_struct(domid);
> -	if (domain) {
> -		/*
> -		 * domain_chk_quota() will print warning and also store whether
> -		 * the soft/hard quota has been hit. So check no_quota_check
> -		 * *after*.
> -		 */
> -		if (domain_chk_quota(domain, domain->memory + mem) &&
> -		    !no_quota_check)
> -			return ENOMEM;
> -		domain->memory += mem;
> -	} else {
> -		/*
> -		 * The domain the memory is to be accounted for should always
> -		 * exist, as accounting is done either for a domain related to
> -		 * the current connection, or for the domain owning a node
> -		 * (which is always existing, as the owner of the node is
> -		 * tested to exist and deleted or replaced by domid 0 if not).
> -		 * So not finding the related domain MUST be an error in the
> -		 * data base.
> -		 */
> -		errno = ENOENT;
> -		corrupt(NULL, "Accounting called for non-existing domain %u\n",
> -			domid);
> -		return ENOENT;
> -	}
> +	ret = domain_acc_add(conn, domid, ACC_MEM, 0, true);
> +	if (ret < 0)
> +		return -ret;
> +
> +	/*
> +	 * domain_chk_quota() will print warning and also store whether the
> +	 * soft/hard quota has been hit. So check no_quota_check *after*.
> +	 */
> +	if (domain_chk_quota(conn, ret + mem) && !no_quota_check)
> +		return ENOMEM;
> +
> +	/*
> +	 * The domain the memory is to be accounted for should always exist,
> +	 * as accounting is done either for a domain related to the current
> +	 * connection, or for the domain owning a node (which is always
> +	 * existing, as the owner of the node is tested to exist and deleted
> +	 * or replaced by domid 0 if not).
> +	 * So not finding the related domain MUST be an error in the data base.
> +	 */
> +	domain_acc_add(conn, domid, ACC_MEM, mem, true);
>   
>   	return 0;
>   }
>   
>   void domain_watch_inc(struct connection *conn)
>   {
> -	if (!conn || !conn->domain)
> -		return;
> -	conn->domain->nbwatch++;
> +	domain_acc_add(conn, conn->id, ACC_WATCH, 1, true);
>   }
>   
>   void domain_watch_dec(struct connection *conn)
>   {
> -	if (!conn || !conn->domain)
> -		return;
> -	if (conn->domain->nbwatch)
> -		conn->domain->nbwatch--;
> +	domain_acc_add(conn, conn->id, ACC_WATCH, -1, true);
>   }
>   
>   int domain_watch(struct connection *conn)
>   {
>   	return (domain_is_unprivileged(conn))
> -		? conn->domain->nbwatch
> +		? domain_acc_add(conn, conn->id, ACC_WATCH, 0, true)
>   		: 0;
>   }
>   
>   void domain_outstanding_inc(struct connection *conn)
>   {
> -	if (!conn || !conn->domain)
> -		return;
> -	conn->domain->nboutstanding++;
> +	domain_acc_add(conn, conn->id, ACC_OUTST, 1, true);
>   }
>   
> -void domain_outstanding_dec(struct connection *conn)
> +void domain_outstanding_dec(struct connection *conn, unsigned int domid)
>   {
> -	if (!conn || !conn->domain)
> -		return;
> -	conn->domain->nboutstanding--;
> -}
> -
> -void domain_outstanding_domid_dec(unsigned int domid)
> -{
> -	struct domain *d = find_domain_by_domid(domid);
> -
> -	if (d)
> -		d->nboutstanding--;
> +	domain_acc_add(conn, domid, ACC_OUTST, -1, true);
>   }
>   
>   static wrl_creditt wrl_config_writecost      = WRL_FACTOR;
> diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h
> index b94548fd7d..086133407b 100644
> --- a/tools/xenstore/xenstored_domain.h
> +++ b/tools/xenstore/xenstored_domain.h
> @@ -29,7 +29,10 @@ enum accitem {
>   	ACC_REQ_N,		/* Number of elements per request. */
>   	ACC_TR_N = ACC_REQ_N,	/* Number of elements per transaction. */
>   	ACC_CHD_N = ACC_TR_N,	/* max(ACC_REQ_N, ACC_TR_N), for changed dom. */
> -	ACC_N = ACC_TR_N,	/* Number of elements per domain. */
> +	ACC_WATCH = ACC_TR_N,
> +	ACC_OUTST,
> +	ACC_MEM,
> +	ACC_N,			/* Number of elements per domain. */
>   };
>   
>   void handle_event(void);
> @@ -109,8 +112,7 @@ void domain_watch_inc(struct connection *conn);
>   void domain_watch_dec(struct connection *conn);
>   int domain_watch(struct connection *conn);
>   void domain_outstanding_inc(struct connection *conn);
> -void domain_outstanding_dec(struct connection *conn);
> -void domain_outstanding_domid_dec(unsigned int domid);
> +void domain_outstanding_dec(struct connection *conn, unsigned int domid);
>   int domain_get_quota(const void *ctx, struct connection *conn,
>   		     unsigned int domid);
>   

-- 
Julien Grall

[PATCH v5 08/14] tools/xenstore: add accounting trace support

[Juergen Gross]

Add a new trace switch "acc" and the related trace calls.

The "acc" switch is off per default.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
 tools/xenstore/xenstored_core.c   |  2 +-
 tools/xenstore/xenstored_core.h   |  1 +
 tools/xenstore/xenstored_domain.c | 10 ++++++++++
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index e7f86f9487..15654730a6 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -2750,7 +2750,7 @@ static void set_quota(const char *arg, bool soft)
 
 /* Sorted by bit values of TRACE_* flags. Flag is (1u << index). */
 const char *const trace_switches[] = {
-	"obj", "io", "wrl",
+	"obj", "io", "wrl", "acc",
 	NULL
 };
 
diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
index 1f811f38cb..3e0734a6c6 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -302,6 +302,7 @@ extern unsigned int trace_flags;
 #define TRACE_OBJ	0x00000001
 #define TRACE_IO	0x00000002
 #define TRACE_WRL	0x00000004
+#define TRACE_ACC	0x00000008
 extern const char *const trace_switches[];
 int set_trace_switch(const char *arg);
 
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index a35ed97fd7..03825ca24b 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -538,6 +538,12 @@ static struct domain *find_domain_by_domid(unsigned int domid)
 	return (d && d->introduced) ? d : NULL;
 }
 
+#define trace_acc(...)				\
+do {						\
+	if (trace_flags & TRACE_ACC)		\
+		trace("acc: " __VA_ARGS__);	\
+} while (0)
+
 int acc_fix_domains(struct list_head *head, bool chk_quota, bool update)
 {
 	struct changed_domain *cd;
@@ -601,6 +607,8 @@ static int acc_add_changed_dom(const void *ctx, struct list_head *head,
 		return 0;
 
 	errno = 0;
+	trace_acc("local change domid %u: what=%u %d add %d\n", domid, what,
+		  cd->acc[what], val);
 	cd->acc[what] += val;
 
 	return cd->acc[what];
@@ -1112,6 +1120,8 @@ static int domain_acc_add(struct connection *conn, unsigned int domid,
 		return domain_acc_add_valid(d, what, ret);
 	}
 
+	trace_acc("global change domid %u: what=%u %u add %d\n", domid, what,
+		  d->acc[what], add);
 	d->acc[what] = domain_acc_add_valid(d, what, add);
 
 	return d->acc[what];
-- 
2.35.3

[PATCH v5 09/14] tools/xenstore: add TDB access trace support

[Juergen Gross]

Add a new trace switch "tdb" and the related trace calls.

The "tdb" switch is off per default.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
 tools/xenstore/xenstored_core.c        | 8 +++++++-
 tools/xenstore/xenstored_core.h        | 7 +++++++
 tools/xenstore/xenstored_transaction.c | 7 ++++++-
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 15654730a6..3caf9e45dc 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -589,6 +589,8 @@ static void get_acc_data(TDB_DATA *key, struct node_account_data *acc)
 		if (old_data.dptr == NULL) {
 			acc->memory = 0;
 		} else {
+			trace_tdb("read %s size %zu\n", key->dptr,
+				  old_data.dsize + key->dsize);
 			hdr = (void *)old_data.dptr;
 			acc->memory = old_data.dsize;
 			acc->domid = hdr->perms[0].id;
@@ -655,6 +657,7 @@ int do_tdb_write(struct connection *conn, TDB_DATA *key, TDB_DATA *data,
 		errno = EIO;
 		return errno;
 	}
+	trace_tdb("store %s size %zu\n", key->dptr, data->dsize + key->dsize);
 
 	if (acc) {
 		/* Don't use new_domid, as it might be a transaction node. */
@@ -682,6 +685,7 @@ int do_tdb_delete(struct connection *conn, TDB_DATA *key,
 		errno = EIO;
 		return errno;
 	}
+	trace_tdb("delete %s\n", key->dptr);
 
 	if (acc->memory) {
 		domid = get_acc_domid(conn, key, acc->domid);
@@ -731,6 +735,8 @@ struct node *read_node(struct connection *conn, const void *ctx,
 		goto error;
 	}
 
+	trace_tdb("read %s size %zu\n", key.dptr, data.dsize + key.dsize);
+
 	node->parent = NULL;
 	talloc_steal(node, data.dptr);
 
@@ -2750,7 +2756,7 @@ static void set_quota(const char *arg, bool soft)
 
 /* Sorted by bit values of TRACE_* flags. Flag is (1u << index). */
 const char *const trace_switches[] = {
-	"obj", "io", "wrl", "acc",
+	"obj", "io", "wrl", "acc", "tdb",
 	NULL
 };
 
diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
index 3e0734a6c6..5a11dc1231 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -303,9 +303,16 @@ extern unsigned int trace_flags;
 #define TRACE_IO	0x00000002
 #define TRACE_WRL	0x00000004
 #define TRACE_ACC	0x00000008
+#define TRACE_TDB	0x00000010
 extern const char *const trace_switches[];
 int set_trace_switch(const char *arg);
 
+#define trace_tdb(...)				\
+do {						\
+	if (trace_flags & TRACE_TDB)		\
+		trace("tdb: " __VA_ARGS__);	\
+} while (0)
+
 extern TDB_CONTEXT *tdb_ctx;
 extern int dom0_domid;
 extern int dom0_event;
diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c
index 2b15506953..11c8bcec84 100644
--- a/tools/xenstore/xenstored_transaction.c
+++ b/tools/xenstore/xenstored_transaction.c
@@ -374,8 +374,11 @@ static int finalize_transaction(struct connection *conn,
 				if (tdb_error(tdb_ctx) != TDB_ERR_NOEXIST)
 					return EIO;
 				gen = NO_GENERATION;
-			} else
+			} else {
+				trace_tdb("read %s size %zu\n", key.dptr,
+					  key.dsize + data.dsize);
 				gen = hdr->generation;
+			}
 			talloc_free(data.dptr);
 			if (i->generation != gen)
 				return EAGAIN;
@@ -399,6 +402,8 @@ static int finalize_transaction(struct connection *conn,
 			set_tdb_key(i->trans_name, &ta_key);
 			data = tdb_fetch(tdb_ctx, ta_key);
 			if (data.dptr) {
+				trace_tdb("read %s size %zu\n", ta_key.dptr,
+					  ta_key.dsize + data.dsize);
 				hdr = (void *)data.dptr;
 				hdr->generation = ++generation;
 				*is_corrupt |= do_tdb_write(conn, &key, &data,
-- 
2.35.3

[PATCH v5 10/14] tools/xenstore: switch transaction accounting to generic accounting

[Juergen Gross]

As transaction accounting is active for unprivileged domains only, it
can easily be added to the generic per-domain accounting.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V5:
- use list_empty(&conn->transaction_list) for detection of "no
  transaction active" (Julien Grall)
---
 tools/xenstore/xenstored_core.c        |  3 +--
 tools/xenstore/xenstored_core.h        |  1 -
 tools/xenstore/xenstored_domain.c      | 21 ++++++++++++++++++---
 tools/xenstore/xenstored_domain.h      |  4 ++++
 tools/xenstore/xenstored_transaction.c | 14 ++++++--------
 5 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 3caf9e45dc..c98d30561f 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -2087,7 +2087,7 @@ static void consider_message(struct connection *conn)
 	 * stalled. This will ignore new requests until Live-Update happened
 	 * or it was aborted.
 	 */
-	if (lu_is_pending() && conn->transaction_started == 0 &&
+	if (lu_is_pending() && list_empty(&conn->transaction_list) &&
 	    conn->in->hdr.msg.type == XS_TRANSACTION_START) {
 		trace("Delaying transaction start for connection %p req_id %u\n",
 		      conn, conn->in->hdr.msg.req_id);
@@ -2194,7 +2194,6 @@ struct connection *new_connection(const struct interface_funcs *funcs)
 	new->funcs = funcs;
 	new->is_ignored = false;
 	new->is_stalled = false;
-	new->transaction_started = 0;
 	INIT_LIST_HEAD(&new->out_list);
 	INIT_LIST_HEAD(&new->acc_list);
 	INIT_LIST_HEAD(&new->ref_list);
diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
index 5a11dc1231..3564d85d7d 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -151,7 +151,6 @@ struct connection
 	/* List of in-progress transactions. */
 	struct list_head transaction_list;
 	uint32_t next_transaction_id;
-	unsigned int transaction_started;
 	time_t ta_start_time;
 
 	/* List of delayed requests. */
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index 03825ca24b..25c6d20036 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -417,12 +417,10 @@ int domain_get_quota(const void *ctx, struct connection *conn,
 {
 	struct domain *d = find_domain_struct(domid);
 	char *resp;
-	int ta;
 
 	if (!d)
 		return ENOENT;
 
-	ta = d->conn ? d->conn->transaction_started : 0;
 	resp = talloc_asprintf(ctx, "Domain %u:\n", domid);
 	if (!resp)
 		return ENOMEM;
@@ -433,7 +431,7 @@ int domain_get_quota(const void *ctx, struct connection *conn,
 
 	ent(nodes, d->acc[ACC_NODES]);
 	ent(watches, d->acc[ACC_WATCH]);
-	ent(transactions, ta);
+	ent(transactions, d->acc[ACC_TRANS]);
 	ent(outstanding, d->acc[ACC_OUTST]);
 	ent(memory, d->acc[ACC_MEM]);
 
@@ -1298,6 +1296,23 @@ void domain_outstanding_dec(struct connection *conn, unsigned int domid)
 	domain_acc_add(conn, domid, ACC_OUTST, -1, true);
 }
 
+void domain_transaction_inc(struct connection *conn)
+{
+	domain_acc_add(conn, conn->id, ACC_TRANS, 1, true);
+}
+
+void domain_transaction_dec(struct connection *conn)
+{
+	domain_acc_add(conn, conn->id, ACC_TRANS, -1, true);
+}
+
+unsigned int domain_transaction_get(struct connection *conn)
+{
+	return (domain_is_unprivileged(conn))
+		? domain_acc_add(conn, conn->id, ACC_TRANS, 0, true)
+		: 0;
+}
+
 static wrl_creditt wrl_config_writecost      = WRL_FACTOR;
 static wrl_creditt wrl_config_rate           = WRL_RATE   * WRL_FACTOR;
 static wrl_creditt wrl_config_dburst         = WRL_DBURST * WRL_FACTOR;
diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h
index 086133407b..01b6f1861b 100644
--- a/tools/xenstore/xenstored_domain.h
+++ b/tools/xenstore/xenstored_domain.h
@@ -32,6 +32,7 @@ enum accitem {
 	ACC_WATCH = ACC_TR_N,
 	ACC_OUTST,
 	ACC_MEM,
+	ACC_TRANS,
 	ACC_N,			/* Number of elements per domain. */
 };
 
@@ -113,6 +114,9 @@ void domain_watch_dec(struct connection *conn);
 int domain_watch(struct connection *conn);
 void domain_outstanding_inc(struct connection *conn);
 void domain_outstanding_dec(struct connection *conn, unsigned int domid);
+void domain_transaction_inc(struct connection *conn);
+void domain_transaction_dec(struct connection *conn);
+unsigned int domain_transaction_get(struct connection *conn);
 int domain_get_quota(const void *ctx, struct connection *conn,
 		     unsigned int domid);
 
diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c
index 11c8bcec84..b9e9d76a1f 100644
--- a/tools/xenstore/xenstored_transaction.c
+++ b/tools/xenstore/xenstored_transaction.c
@@ -479,8 +479,7 @@ int do_transaction_start(const void *ctx, struct connection *conn,
 	if (conn->transaction)
 		return EBUSY;
 
-	if (domain_is_unprivileged(conn) &&
-	    conn->transaction_started > quota_max_transaction)
+	if (domain_transaction_get(conn) > quota_max_transaction)
 		return ENOSPC;
 
 	/* Attach transaction to ctx for autofree until it's complete */
@@ -502,12 +501,12 @@ int do_transaction_start(const void *ctx, struct connection *conn,
 	} while (!IS_ERR(exists));
 
 	/* Now we own it. */
+	if (list_empty(&conn->transaction_list))
+		conn->ta_start_time = time(NULL);
 	list_add_tail(&trans->list, &conn->transaction_list);
 	talloc_steal(conn, trans);
 	talloc_set_destructor(trans, destroy_transaction);
-	if (!conn->transaction_started)
-		conn->ta_start_time = time(NULL);
-	conn->transaction_started++;
+	domain_transaction_inc(conn);
 	wrl_ntransactions++;
 
 	snprintf(id_str, sizeof(id_str), "%u", trans->id);
@@ -533,8 +532,8 @@ int do_transaction_end(const void *ctx, struct connection *conn,
 
 	conn->transaction = NULL;
 	list_del(&trans->list);
-	conn->transaction_started--;
-	if (!conn->transaction_started)
+	domain_transaction_dec(conn);
+	if (list_empty(&conn->transaction_list))
 		conn->ta_start_time = 0;
 
 	chk_quota = trans->node_created && domain_is_unprivileged(conn);
@@ -588,7 +587,6 @@ void conn_delete_all_transactions(struct connection *conn)
 
 	assert(conn->transaction == NULL);
 
-	conn->transaction_started = 0;
 	conn->ta_start_time = 0;
 }
 
-- 
2.35.3

Re: [PATCH v5 10/14] tools/xenstore: switch transaction accounting to generic accounting

[Julien Grall]

Hi Juergen,

On 08/05/2023 12:47, Juergen Gross wrote:
> As transaction accounting is active for unprivileged domains only, it
> can easily be added to the generic per-domain accounting.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V5:
> - use list_empty(&conn->transaction_list) for detection of "no
>    transaction active" (Julien Grall)
> ---
>   tools/xenstore/xenstored_core.c        |  3 +--
>   tools/xenstore/xenstored_core.h        |  1 -
>   tools/xenstore/xenstored_domain.c      | 21 ++++++++++++++++++---
>   tools/xenstore/xenstored_domain.h      |  4 ++++
>   tools/xenstore/xenstored_transaction.c | 14 ++++++--------
>   5 files changed, 29 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> index 3caf9e45dc..c98d30561f 100644
> --- a/tools/xenstore/xenstored_core.c
> +++ b/tools/xenstore/xenstored_core.c
> @@ -2087,7 +2087,7 @@ static void consider_message(struct connection *conn)
>   	 * stalled. This will ignore new requests until Live-Update happened
>   	 * or it was aborted.
>   	 */
> -	if (lu_is_pending() && conn->transaction_started == 0 &&
> +	if (lu_is_pending() && list_empty(&conn->transaction_list) &&
>   	    conn->in->hdr.msg.type == XS_TRANSACTION_START) {
>   		trace("Delaying transaction start for connection %p req_id %u\n",
>   		      conn, conn->in->hdr.msg.req_id);
> @@ -2194,7 +2194,6 @@ struct connection *new_connection(const struct interface_funcs *funcs)
>   	new->funcs = funcs;
>   	new->is_ignored = false;
>   	new->is_stalled = false;
> -	new->transaction_started = 0;
>   	INIT_LIST_HEAD(&new->out_list);
>   	INIT_LIST_HEAD(&new->acc_list);
>   	INIT_LIST_HEAD(&new->ref_list);
> diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
> index 5a11dc1231..3564d85d7d 100644
> --- a/tools/xenstore/xenstored_core.h
> +++ b/tools/xenstore/xenstored_core.h
> @@ -151,7 +151,6 @@ struct connection
>   	/* List of in-progress transactions. */
>   	struct list_head transaction_list;
>   	uint32_t next_transaction_id;
> -	unsigned int transaction_started;
>   	time_t ta_start_time;
>   
>   	/* List of delayed requests. */
> diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
> index 03825ca24b..25c6d20036 100644
> --- a/tools/xenstore/xenstored_domain.c
> +++ b/tools/xenstore/xenstored_domain.c
> @@ -417,12 +417,10 @@ int domain_get_quota(const void *ctx, struct connection *conn,
>   {
>   	struct domain *d = find_domain_struct(domid);
>   	char *resp;
> -	int ta;
>   
>   	if (!d)
>   		return ENOENT;
>   
> -	ta = d->conn ? d->conn->transaction_started : 0;
>   	resp = talloc_asprintf(ctx, "Domain %u:\n", domid);
>   	if (!resp)
>   		return ENOMEM;
> @@ -433,7 +431,7 @@ int domain_get_quota(const void *ctx, struct connection *conn,
>   
>   	ent(nodes, d->acc[ACC_NODES]);
>   	ent(watches, d->acc[ACC_WATCH]);
> -	ent(transactions, ta);
> +	ent(transactions, d->acc[ACC_TRANS]);
>   	ent(outstanding, d->acc[ACC_OUTST]);
>   	ent(memory, d->acc[ACC_MEM]);
>   
> @@ -1298,6 +1296,23 @@ void domain_outstanding_dec(struct connection *conn, unsigned int domid)
>   	domain_acc_add(conn, domid, ACC_OUTST, -1, true);
>   }
>   
> +void domain_transaction_inc(struct connection *conn)
> +{
> +	domain_acc_add(conn, conn->id, ACC_TRANS, 1, true);
> +}
> +
> +void domain_transaction_dec(struct connection *conn)
> +{
> +	domain_acc_add(conn, conn->id, ACC_TRANS, -1, true);
> +}
> +
> +unsigned int domain_transaction_get(struct connection *conn)
> +{
> +	return (domain_is_unprivileged(conn))
> +		? domain_acc_add(conn, conn->id, ACC_TRANS, 0, true)
> +		: 0;
> +}
> +
>   static wrl_creditt wrl_config_writecost      = WRL_FACTOR;
>   static wrl_creditt wrl_config_rate           = WRL_RATE   * WRL_FACTOR;
>   static wrl_creditt wrl_config_dburst         = WRL_DBURST * WRL_FACTOR;
> diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h
> index 086133407b..01b6f1861b 100644
> --- a/tools/xenstore/xenstored_domain.h
> +++ b/tools/xenstore/xenstored_domain.h
> @@ -32,6 +32,7 @@ enum accitem {
>   	ACC_WATCH = ACC_TR_N,
>   	ACC_OUTST,
>   	ACC_MEM,
> +	ACC_TRANS,
>   	ACC_N,			/* Number of elements per domain. */
>   };
>   
> @@ -113,6 +114,9 @@ void domain_watch_dec(struct connection *conn);
>   int domain_watch(struct connection *conn);
>   void domain_outstanding_inc(struct connection *conn);
>   void domain_outstanding_dec(struct connection *conn, unsigned int domid);
> +void domain_transaction_inc(struct connection *conn);
> +void domain_transaction_dec(struct connection *conn);
> +unsigned int domain_transaction_get(struct connection *conn);
>   int domain_get_quota(const void *ctx, struct connection *conn,
>   		     unsigned int domid);
>   
> diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c
> index 11c8bcec84..b9e9d76a1f 100644
> --- a/tools/xenstore/xenstored_transaction.c
> +++ b/tools/xenstore/xenstored_transaction.c
> @@ -479,8 +479,7 @@ int do_transaction_start(const void *ctx, struct connection *conn,
>   	if (conn->transaction)
>   		return EBUSY;
>   
> -	if (domain_is_unprivileged(conn) &&
> -	    conn->transaction_started > quota_max_transaction)
> +	if (domain_transaction_get(conn) > quota_max_transaction)
>   		return ENOSPC;
>   
>   	/* Attach transaction to ctx for autofree until it's complete */
> @@ -502,12 +501,12 @@ int do_transaction_start(const void *ctx, struct connection *conn,
>   	} while (!IS_ERR(exists));
>   
>   	/* Now we own it. */

This comment now feels a bit misplaced. I think ...

> +	if (list_empty(&conn->transaction_list))
> +		conn->ta_start_time = time(NULL);
... the two lines should be added before.

With that:

Acked-by: Julien Grall <jgrall@amazon.com>

>   	list_add_tail(&trans->list, &conn->transaction_list);
>   	talloc_steal(conn, trans);
>   	talloc_set_destructor(trans, destroy_transaction);
> -	if (!conn->transaction_started)
> -		conn->ta_start_time = time(NULL);
> -	conn->transaction_started++;
> +	domain_transaction_inc(conn);
>   	wrl_ntransactions++;
>   
>   	snprintf(id_str, sizeof(id_str), "%u", trans->id);
> @@ -533,8 +532,8 @@ int do_transaction_end(const void *ctx, struct connection *conn,
>   
>   	conn->transaction = NULL;
>   	list_del(&trans->list);
> -	conn->transaction_started--;
> -	if (!conn->transaction_started)
> +	domain_transaction_dec(conn);
> +	if (list_empty(&conn->transaction_list))
>   		conn->ta_start_time = 0;
>   
>   	chk_quota = trans->node_created && domain_is_unprivileged(conn);
> @@ -588,7 +587,6 @@ void conn_delete_all_transactions(struct connection *conn)
>   
>   	assert(conn->transaction == NULL);
>   
> -	conn->transaction_started = 0;
>   	conn->ta_start_time = 0;
>   }
>   

Cheers,

-- 
Julien Grall

[PATCH v5 11/14] tools/xenstore: remember global and per domain max accounting values

[Juergen Gross]

Add saving the maximum values of the different accounting data seen
per domain and (for unprivileged domains) globally, and print those
values via the xenstore-control quota command. Add a sub-command for
resetting the global maximum values seen.

This should help for a decision how to set the related quotas.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
 docs/misc/xenstore.txt             |   5 +-
 tools/xenstore/xenstored_control.c |  22 ++++++-
 tools/xenstore/xenstored_domain.c  | 100 +++++++++++++++++++++++------
 tools/xenstore/xenstored_domain.h  |   2 +
 4 files changed, 108 insertions(+), 21 deletions(-)

diff --git a/docs/misc/xenstore.txt b/docs/misc/xenstore.txt
index d807ef0709..38015835b1 100644
--- a/docs/misc/xenstore.txt
+++ b/docs/misc/xenstore.txt
@@ -426,7 +426,7 @@ CONTROL			<command>|[<parameters>|]
 	print|<string>
 		print <string> to syslog (xenstore runs as daemon) or
 		to console (xenstore runs as stubdom)
-	quota|[set <name> <val>|<domid>]
+	quota|[set <name> <val>|<domid>|max [-r]]
 		without parameters: print the current quota settings
 		with "set <name> <val>": set the quota <name> to new value
 		<val> (The admin should make sure all the domain usage is
@@ -435,6 +435,9 @@ CONTROL			<command>|[<parameters>|]
 		violating the new quota setting isn't increased further)
 		with "<domid>": print quota related accounting data for
 		the domain <domid>
+		with "max [-r]": show global per-domain maximum values of all
+		unprivileged domains, optionally reset the values by adding
+		"-r"
 	quota-soft|[set <name> <val>]
 		like the "quota" command, but for soft-quota.
 	help			<supported-commands>
diff --git a/tools/xenstore/xenstored_control.c b/tools/xenstore/xenstored_control.c
index 403295788a..620a7da997 100644
--- a/tools/xenstore/xenstored_control.c
+++ b/tools/xenstore/xenstored_control.c
@@ -306,6 +306,22 @@ static int quota_get(const void *ctx, struct connection *conn,
 	return domain_get_quota(ctx, conn, atoi(vec[0]));
 }
 
+static int quota_max(const void *ctx, struct connection *conn,
+		     char **vec, int num)
+{
+	if (num > 1)
+		return EINVAL;
+
+	if (num == 1) {
+		if (!strcmp(vec[0], "-r"))
+			domain_reset_global_acc();
+		else
+			return EINVAL;
+	}
+
+	return domain_max_global_acc(ctx, conn);
+}
+
 static int do_control_quota(const void *ctx, struct connection *conn,
 			    char **vec, int num)
 {
@@ -315,6 +331,9 @@ static int do_control_quota(const void *ctx, struct connection *conn,
 	if (!strcmp(vec[0], "set"))
 		return quota_set(ctx, conn, vec + 1, num - 1, hard_quotas);
 
+	if (!strcmp(vec[0], "max"))
+		return quota_max(ctx, conn, vec + 1, num - 1);
+
 	return quota_get(ctx, conn, vec, num);
 }
 
@@ -978,7 +997,8 @@ static struct cmd_s cmds[] = {
 	{ "memreport", do_control_memreport, "[<file>]" },
 #endif
 	{ "print", do_control_print, "<string>" },
-	{ "quota", do_control_quota, "[set <name> <val>|<domid>]" },
+	{ "quota", do_control_quota,
+		"[set <name> <val>|<domid>|max [-r]]" },
 	{ "quota-soft", do_control_quota_s, "[set <name> <val>]" },
 	{ "help", do_control_help, "" },
 };
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index 25c6d20036..6f3a27765a 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -43,6 +43,8 @@ static evtchn_port_t virq_port;
 
 xenevtchn_handle *xce_handle = NULL;
 
+static unsigned int acc_global_max[ACC_N];
+
 struct domain
 {
 	/* The id of this domain */
@@ -70,7 +72,10 @@ struct domain
 	bool introduced;
 
 	/* Accounting data for this domain. */
-	unsigned int acc[ACC_N];
+	struct acc {
+		unsigned int val;
+		unsigned int max;
+	} acc[ACC_N];
 
 	/* Memory quota data for this domain. */
 	bool soft_quota_reported;
@@ -199,9 +204,9 @@ static bool domain_can_read(struct connection *conn)
 	if (domain_is_unprivileged(conn)) {
 		if (domain->wrl_credit < 0)
 			return false;
-		if (domain->acc[ACC_OUTST] >= quota_req_outstanding)
+		if (domain->acc[ACC_OUTST].val >= quota_req_outstanding)
 			return false;
-		if (domain->acc[ACC_MEM] >= quota_memory_per_domain_hard &&
+		if (domain->acc[ACC_MEM].val >= quota_memory_per_domain_hard &&
 		    quota_memory_per_domain_hard)
 			return false;
 	}
@@ -264,7 +269,7 @@ static int domain_tree_remove_sub(const void *ctx, struct connection *conn,
 		ret = WALK_TREE_SKIP_CHILDREN;
 	}
 
-	return domain->acc[ACC_NODES] ? ret : WALK_TREE_SUCCESS_STOP;
+	return domain->acc[ACC_NODES].val ? ret : WALK_TREE_SUCCESS_STOP;
 }
 
 static void domain_tree_remove(struct domain *domain)
@@ -272,7 +277,7 @@ static void domain_tree_remove(struct domain *domain)
 	int ret;
 	struct walk_funcs walkfuncs = { .enter = domain_tree_remove_sub };
 
-	if (domain->acc[ACC_NODES]) {
+	if (domain->acc[ACC_NODES].val) {
 		ret = walk_node_tree(domain, NULL, "/", &walkfuncs, domain);
 		if (ret == WALK_TREE_ERROR_STOP)
 			syslog(LOG_ERR,
@@ -426,14 +431,41 @@ int domain_get_quota(const void *ctx, struct connection *conn,
 		return ENOMEM;
 
 #define ent(t, e) \
-	resp = talloc_asprintf_append(resp, "%-16s: %8d\n", #t, e); \
+	resp = talloc_asprintf_append(resp, "%-16s: %8u (max: %8u\n", #t, \
+				      d->acc[e].val, d->acc[e].max); \
+	if (!resp) return ENOMEM
+
+	ent(nodes, ACC_NODES);
+	ent(watches, ACC_WATCH);
+	ent(transactions, ACC_TRANS);
+	ent(outstanding, ACC_OUTST);
+	ent(memory, ACC_MEM);
+
+#undef ent
+
+	send_reply(conn, XS_CONTROL, resp, strlen(resp) + 1);
+
+	return 0;
+}
+
+int domain_max_global_acc(const void *ctx, struct connection *conn)
+{
+	char *resp;
+
+	resp = talloc_asprintf(ctx, "Max. seen accounting values:\n");
+	if (!resp)
+		return ENOMEM;
+
+#define ent(t, e) \
+	resp = talloc_asprintf_append(resp, "%-16s: %8u\n", #t,   \
+				      acc_global_max[e]);         \
 	if (!resp) return ENOMEM
 
-	ent(nodes, d->acc[ACC_NODES]);
-	ent(watches, d->acc[ACC_WATCH]);
-	ent(transactions, d->acc[ACC_TRANS]);
-	ent(outstanding, d->acc[ACC_OUTST]);
-	ent(memory, d->acc[ACC_MEM]);
+	ent(nodes, ACC_NODES);
+	ent(watches, ACC_WATCH);
+	ent(transactions, ACC_TRANS);
+	ent(outstanding, ACC_OUTST);
+	ent(memory, ACC_MEM);
 
 #undef ent
 
@@ -1049,10 +1081,12 @@ int domain_adjust_node_perms(struct node *node)
 
 static int domain_acc_add_valid(struct domain *d, enum accitem what, int add)
 {
+	unsigned int val;
+
 	assert(what < ARRAY_SIZE(d->acc));
 
-	if ((add < 0 && -add > d->acc[what]) ||
-	    (add > 0 && (INT_MAX - d->acc[what]) < add)) {
+	if ((add < 0 && -add > d->acc[what].val) ||
+	    (add > 0 && (INT_MAX - d->acc[what].val) < add)) {
 		/*
 		 * In a transaction when a node is being added/removed AND the
 		 * same node has been added/removed outside the transaction in
@@ -1063,7 +1097,13 @@ static int domain_acc_add_valid(struct domain *d, enum accitem what, int add)
 		return (add < 0) ? 0 : INT_MAX;
 	}
 
-	return d->acc[what] + add;
+	val = d->acc[what].val + add;
+	if (val > d->acc[what].max)
+		d->acc[what].max = val;
+	if (val > acc_global_max[what] && domid_is_unprivileged(d->domid))
+		acc_global_max[what] = val;
+
+	return val;
 }
 
 static int domain_acc_add(struct connection *conn, unsigned int domid,
@@ -1119,10 +1159,10 @@ static int domain_acc_add(struct connection *conn, unsigned int domid,
 	}
 
 	trace_acc("global change domid %u: what=%u %u add %d\n", domid, what,
-		  d->acc[what], add);
-	d->acc[what] = domain_acc_add_valid(d, what, add);
+		  d->acc[what].val, add);
+	d->acc[what].val = domain_acc_add_valid(d, what, add);
 
-	return d->acc[what];
+	return d->acc[what].val;
 }
 
 void acc_drop(struct connection *conn)
@@ -1160,6 +1200,28 @@ void acc_commit(struct connection *conn)
 	conn->in = in;
 }
 
+static int domain_reset_global_acc_sub(const void *k, void *v, void *arg)
+{
+	struct domain *d = v;
+	unsigned int i;
+
+	for (i = 0; i < ACC_N; i++)
+		d->acc[i].max = d->acc[i].val;
+
+	return 0;
+}
+
+void domain_reset_global_acc(void)
+{
+	unsigned int i;
+
+	for (i = 0; i < ACC_N; i++)
+		acc_global_max[i] = 0;
+
+	/* Set current max values seen. */
+	hashtable_iterate(domhash, domain_reset_global_acc_sub, NULL);
+}
+
 int domain_nbentry_inc(struct connection *conn, unsigned int domid)
 {
 	return (domain_acc_add(conn, domid, ACC_NODES, 1, false) < 0)
@@ -1660,7 +1722,7 @@ static int domain_check_acc_init_sub(const void *k, void *v, void *arg)
 	 * If everything is correct incrementing the value for each node will
 	 * result in dom->nodes being 0 at the end.
 	 */
-	dom->nodes = -d->acc[ACC_NODES];
+	dom->nodes = -d->acc[ACC_NODES].val;
 
 	if (!hashtable_insert(domains, &dom->domid, dom)) {
 		talloc_free(dom);
@@ -1715,7 +1777,7 @@ static int domain_check_acc_cb(const void *k, void *v, void *arg)
 	if (!d)
 		return 0;
 
-	d->acc[ACC_NODES] += dom->nodes;
+	d->acc[ACC_NODES].val += dom->nodes;
 
 	return 0;
 }
diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h
index 01b6f1861b..416df25cb2 100644
--- a/tools/xenstore/xenstored_domain.h
+++ b/tools/xenstore/xenstored_domain.h
@@ -127,6 +127,8 @@ int domain_get_quota(const void *ctx, struct connection *conn,
 int acc_fix_domains(struct list_head *head, bool chk_quota, bool update);
 void acc_drop(struct connection *conn);
 void acc_commit(struct connection *conn);
+int domain_max_global_acc(const void *ctx, struct connection *conn);
+void domain_reset_global_acc(void);
 
 /* Write rate limiting */
 
-- 
2.35.3

[PATCH v5 12/14] tools/xenstore: use generic accounting for remaining quotas

[Juergen Gross]

The maxrequests, node size, number of node permissions, and path length
quota are a little bit special, as they are either active in
transactions only (maxrequests), or they are just per item instead of
count values. Nevertheless being able to know the maximum number of
those quota related values per domain would be beneficial, so add them
to the generic accounting.

The per domain value will never show current numbers other than zero,
but the maximum number seen can be gathered the same way as the number
of nodes during a transaction.

To be able to use the const qualifier for a new function switch
domain_is_unprivileged() to take a const pointer, too.

For printing the quota/max values, adapt the print format string to
the longest quota name (now 17 characters long).

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V5:
- add comment (Julien Grall)
- add missing quota printing (Julien Grall)
---
 tools/xenstore/xenstored_core.c        | 15 +++++----
 tools/xenstore/xenstored_core.h        |  2 +-
 tools/xenstore/xenstored_domain.c      | 45 +++++++++++++++++++++-----
 tools/xenstore/xenstored_domain.h      |  6 ++++
 tools/xenstore/xenstored_transaction.c |  4 +--
 tools/xenstore/xenstored_watch.c       |  2 +-
 6 files changed, 55 insertions(+), 19 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index c98d30561f..fce73b883e 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -799,8 +799,9 @@ int write_node_raw(struct connection *conn, TDB_DATA *key, struct node *node,
 		+ node->perms.num * sizeof(node->perms.p[0])
 		+ node->datalen + node->childlen;
 
-	if (!no_quota_check && domain_is_unprivileged(conn) &&
-	    data.dsize >= quota_max_entry_size) {
+	/* Call domain_max_chk() in any case in order to record max values. */
+	if (domain_max_chk(conn, ACC_NODESZ, data.dsize, quota_max_entry_size)
+	    && !no_quota_check) {
 		errno = ENOSPC;
 		return errno;
 	}
@@ -1170,7 +1171,7 @@ static bool valid_chars(const char *node)
 		       "0123456789-/_@") == strlen(node));
 }
 
-bool is_valid_nodename(const char *node)
+bool is_valid_nodename(const struct connection *conn, const char *node)
 {
 	int local_off = 0;
 	unsigned int domid;
@@ -1190,7 +1191,8 @@ bool is_valid_nodename(const char *node)
 	if (sscanf(node, "/local/domain/%5u/%n", &domid, &local_off) != 1)
 		local_off = 0;
 
-	if (strlen(node) > local_off + quota_max_path_len)
+	if (domain_max_chk(conn, ACC_PATHLEN, strlen(node) - local_off,
+			   quota_max_path_len))
 		return false;
 
 	return valid_chars(node);
@@ -1252,7 +1254,7 @@ static struct node *get_node_canonicalized(struct connection *conn,
 	*canonical_name = canonicalize(conn, ctx, name);
 	if (!*canonical_name)
 		return NULL;
-	if (!is_valid_nodename(*canonical_name)) {
+	if (!is_valid_nodename(conn, *canonical_name)) {
 		errno = EINVAL;
 		return NULL;
 	}
@@ -1778,8 +1780,7 @@ static int do_set_perms(const void *ctx, struct connection *conn,
 		return EINVAL;
 
 	perms.num--;
-	if (domain_is_unprivileged(conn) &&
-	    perms.num > quota_nb_perms_per_node)
+	if (domain_max_chk(conn, ACC_NPERM, perms.num, quota_nb_perms_per_node))
 		return ENOSPC;
 
 	permstr = in->buffer + strlen(in->buffer) + 1;
diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
index 3564d85d7d..9339820156 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -258,7 +258,7 @@ void check_store(void);
 void corrupt(struct connection *conn, const char *fmt, ...);
 
 /* Is this a valid node name? */
-bool is_valid_nodename(const char *node);
+bool is_valid_nodename(const struct connection *conn, const char *node);
 
 /* Get name of parent node. */
 char *get_parent(const void *ctx, const char *node);
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index 6f3a27765a..b3a719569e 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -431,7 +431,7 @@ int domain_get_quota(const void *ctx, struct connection *conn,
 		return ENOMEM;
 
 #define ent(t, e) \
-	resp = talloc_asprintf_append(resp, "%-16s: %8u (max: %8u\n", #t, \
+	resp = talloc_asprintf_append(resp, "%-17s: %8u (max: %8u\n", #t, \
 				      d->acc[e].val, d->acc[e].max); \
 	if (!resp) return ENOMEM
 
@@ -440,6 +440,10 @@ int domain_get_quota(const void *ctx, struct connection *conn,
 	ent(transactions, ACC_TRANS);
 	ent(outstanding, ACC_OUTST);
 	ent(memory, ACC_MEM);
+	ent(transaction-nodes, ACC_TRANSNODES);
+	ent(node-permissions, ACC_NPERM);
+	ent(path-length, ACC_PATHLEN);
+	ent(node-size, ACC_NODESZ);
 
 #undef ent
 
@@ -457,7 +461,7 @@ int domain_max_global_acc(const void *ctx, struct connection *conn)
 		return ENOMEM;
 
 #define ent(t, e) \
-	resp = talloc_asprintf_append(resp, "%-16s: %8u\n", #t,   \
+	resp = talloc_asprintf_append(resp, "%-17s: %8u\n", #t,   \
 				      acc_global_max[e]);         \
 	if (!resp) return ENOMEM
 
@@ -466,6 +470,10 @@ int domain_max_global_acc(const void *ctx, struct connection *conn)
 	ent(transactions, ACC_TRANS);
 	ent(outstanding, ACC_OUTST);
 	ent(memory, ACC_MEM);
+	ent(transaction-nodes, ACC_TRANSNODES);
+	ent(node-permissions, ACC_NPERM);
+	ent(path-length, ACC_PATHLEN);
+	ent(node-size, ACC_NODESZ);
 
 #undef ent
 
@@ -1079,12 +1087,22 @@ int domain_adjust_node_perms(struct node *node)
 	return 0;
 }
 
+static void domain_acc_valid_max(struct domain *d, enum accitem what,
+				 unsigned int val)
+{
+	assert(what < ARRAY_SIZE(d->acc));
+	assert(what < ARRAY_SIZE(acc_global_max));
+
+	if (val > d->acc[what].max)
+		d->acc[what].max = val;
+	if (val > acc_global_max[what] && domid_is_unprivileged(d->domid))
+		acc_global_max[what] = val;
+}
+
 static int domain_acc_add_valid(struct domain *d, enum accitem what, int add)
 {
 	unsigned int val;
 
-	assert(what < ARRAY_SIZE(d->acc));
-
 	if ((add < 0 && -add > d->acc[what].val) ||
 	    (add > 0 && (INT_MAX - d->acc[what].val) < add)) {
 		/*
@@ -1098,10 +1116,7 @@ static int domain_acc_add_valid(struct domain *d, enum accitem what, int add)
 	}
 
 	val = d->acc[what].val + add;
-	if (val > d->acc[what].max)
-		d->acc[what].max = val;
-	if (val > acc_global_max[what] && domid_is_unprivileged(d->domid))
-		acc_global_max[what] = val;
+	domain_acc_valid_max(d, what, val);
 
 	return val;
 }
@@ -1222,6 +1237,20 @@ void domain_reset_global_acc(void)
 	hashtable_iterate(domhash, domain_reset_global_acc_sub, NULL);
 }
 
+bool domain_max_chk(const struct connection *conn, enum accitem what,
+		    unsigned int val, unsigned int quota)
+{
+	if (!conn || !conn->domain)
+		return false;
+
+	if (domain_is_unprivileged(conn) && val > quota)
+		return true;
+
+	domain_acc_valid_max(conn->domain, what, val);
+
+	return false;
+}
+
 int domain_nbentry_inc(struct connection *conn, unsigned int domid)
 {
 	return (domain_acc_add(conn, domid, ACC_NODES, 1, false) < 0)
diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h
index 416df25cb2..78ca434531 100644
--- a/tools/xenstore/xenstored_domain.h
+++ b/tools/xenstore/xenstored_domain.h
@@ -33,6 +33,10 @@ enum accitem {
 	ACC_OUTST,
 	ACC_MEM,
 	ACC_TRANS,
+	ACC_TRANSNODES,
+	ACC_NPERM,
+	ACC_PATHLEN,
+	ACC_NODESZ,
 	ACC_N,			/* Number of elements per domain. */
 };
 
@@ -129,6 +133,8 @@ void acc_drop(struct connection *conn);
 void acc_commit(struct connection *conn);
 int domain_max_global_acc(const void *ctx, struct connection *conn);
 void domain_reset_global_acc(void);
+bool domain_max_chk(const struct connection *conn, unsigned int what,
+		    unsigned int val, unsigned int quota);
 
 /* Write rate limiting */
 
diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c
index b9e9d76a1f..cecaf27018 100644
--- a/tools/xenstore/xenstored_transaction.c
+++ b/tools/xenstore/xenstored_transaction.c
@@ -252,8 +252,8 @@ int access_node(struct connection *conn, struct node *node,
 
 	i = find_accessed_node(trans, node->name);
 	if (!i) {
-		if (trans->nodes >= quota_trans_nodes &&
-		    domain_is_unprivileged(conn)) {
+		if (domain_max_chk(conn, ACC_TRANSNODES, trans->nodes + 1,
+				   quota_trans_nodes)) {
 			ret = ENOSPC;
 			goto err;
 		}
diff --git a/tools/xenstore/xenstored_watch.c b/tools/xenstore/xenstored_watch.c
index e30cd89be3..61b1e3421e 100644
--- a/tools/xenstore/xenstored_watch.c
+++ b/tools/xenstore/xenstored_watch.c
@@ -176,7 +176,7 @@ static int check_watch_path(struct connection *conn, const void *ctx,
 		*path = canonicalize(conn, ctx, *path);
 		if (!*path)
 			return errno;
-		if (!is_valid_nodename(*path))
+		if (!is_valid_nodename(conn, *path))
 			goto inval;
 	}
 
-- 
2.35.3

Re: [PATCH v5 12/14] tools/xenstore: use generic accounting for remaining quotas

[Julien Grall]

Hi Juergen,

On 08/05/2023 12:47, Juergen Gross wrote:
> The maxrequests, node size, number of node permissions, and path length
> quota are a little bit special, as they are either active in
> transactions only (maxrequests), or they are just per item instead of
> count values. Nevertheless being able to know the maximum number of
> those quota related values per domain would be beneficial, so add them
> to the generic accounting.
> 
> The per domain value will never show current numbers other than zero,
> but the maximum number seen can be gathered the same way as the number
> of nodes during a transaction.
> 
> To be able to use the const qualifier for a new function switch
> domain_is_unprivileged() to take a const pointer, too.
> 
> For printing the quota/max values, adapt the print format string to
> the longest quota name (now 17 characters long).
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V5:
> - add comment (Julien Grall)
> - add missing quota printing (Julien Grall)
> ---
>   tools/xenstore/xenstored_core.c        | 15 +++++----
>   tools/xenstore/xenstored_core.h        |  2 +-
>   tools/xenstore/xenstored_domain.c      | 45 +++++++++++++++++++++-----
>   tools/xenstore/xenstored_domain.h      |  6 ++++
>   tools/xenstore/xenstored_transaction.c |  4 +--
>   tools/xenstore/xenstored_watch.c       |  2 +-
>   6 files changed, 55 insertions(+), 19 deletions(-)
> 
> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> index c98d30561f..fce73b883e 100644
> --- a/tools/xenstore/xenstored_core.c
> +++ b/tools/xenstore/xenstored_core.c
> @@ -799,8 +799,9 @@ int write_node_raw(struct connection *conn, TDB_DATA *key, struct node *node,
>   		+ node->perms.num * sizeof(node->perms.p[0])
>   		+ node->datalen + node->childlen;
>   
> -	if (!no_quota_check && domain_is_unprivileged(conn) &&
> -	    data.dsize >= quota_max_entry_size) {
> +	/* Call domain_max_chk() in any case in order to record max values. */
> +	if (domain_max_chk(conn, ACC_NODESZ, data.dsize, quota_max_entry_size)
> +	    && !no_quota_check) {
>   		errno = ENOSPC;
>   		return errno;
>   	}
> @@ -1170,7 +1171,7 @@ static bool valid_chars(const char *node)
>   		       "0123456789-/_@") == strlen(node));
>   }
>   
> -bool is_valid_nodename(const char *node)
> +bool is_valid_nodename(const struct connection *conn, const char *node)
>   {
>   	int local_off = 0;
>   	unsigned int domid;
> @@ -1190,7 +1191,8 @@ bool is_valid_nodename(const char *node)
>   	if (sscanf(node, "/local/domain/%5u/%n", &domid, &local_off) != 1)
>   		local_off = 0;
>   
> -	if (strlen(node) > local_off + quota_max_path_len)
> +	if (domain_max_chk(conn, ACC_PATHLEN, strlen(node) - local_off,
> +			   quota_max_path_len))
>   		return false;
>   
>   	return valid_chars(node);
> @@ -1252,7 +1254,7 @@ static struct node *get_node_canonicalized(struct connection *conn,
>   	*canonical_name = canonicalize(conn, ctx, name);
>   	if (!*canonical_name)
>   		return NULL;
> -	if (!is_valid_nodename(*canonical_name)) {
> +	if (!is_valid_nodename(conn, *canonical_name)) {
>   		errno = EINVAL;
>   		return NULL;
>   	}
> @@ -1778,8 +1780,7 @@ static int do_set_perms(const void *ctx, struct connection *conn,
>   		return EINVAL;
>   
>   	perms.num--;
> -	if (domain_is_unprivileged(conn) &&
> -	    perms.num > quota_nb_perms_per_node)
> +	if (domain_max_chk(conn, ACC_NPERM, perms.num, quota_nb_perms_per_node))
>   		return ENOSPC;
>   
>   	permstr = in->buffer + strlen(in->buffer) + 1;
> diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
> index 3564d85d7d..9339820156 100644
> --- a/tools/xenstore/xenstored_core.h
> +++ b/tools/xenstore/xenstored_core.h
> @@ -258,7 +258,7 @@ void check_store(void);
>   void corrupt(struct connection *conn, const char *fmt, ...);
>   
>   /* Is this a valid node name? */
> -bool is_valid_nodename(const char *node);
> +bool is_valid_nodename(const struct connection *conn, const char *node);
>   
>   /* Get name of parent node. */
>   char *get_parent(const void *ctx, const char *node);
> diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
> index 6f3a27765a..b3a719569e 100644
> --- a/tools/xenstore/xenstored_domain.c
> +++ b/tools/xenstore/xenstored_domain.c
> @@ -431,7 +431,7 @@ int domain_get_quota(const void *ctx, struct connection *conn,
>   		return ENOMEM;
>   
>   #define ent(t, e) \
> -	resp = talloc_asprintf_append(resp, "%-16s: %8u (max: %8u\n", #t, \
> +	resp = talloc_asprintf_append(resp, "%-17s: %8u (max: %8u\n", #t, \
>   				      d->acc[e].val, d->acc[e].max); \
>   	if (!resp) return ENOMEM
>   
> @@ -440,6 +440,10 @@ int domain_get_quota(const void *ctx, struct connection *conn,
>   	ent(transactions, ACC_TRANS);
>   	ent(outstanding, ACC_OUTST);
>   	ent(memory, ACC_MEM);
> +	ent(transaction-nodes, ACC_TRANSNODES);
> +	ent(node-permissions, ACC_NPERM);
> +	ent(path-length, ACC_PATHLEN);
> +	ent(node-size, ACC_NODESZ);
>   
>   #undef ent
>   
> @@ -457,7 +461,7 @@ int domain_max_global_acc(const void *ctx, struct connection *conn)
>   		return ENOMEM;
>   
>   #define ent(t, e) \
> -	resp = talloc_asprintf_append(resp, "%-16s: %8u\n", #t,   \
> +	resp = talloc_asprintf_append(resp, "%-17s: %8u\n", #t,   \
>   				      acc_global_max[e]);         \
>   	if (!resp) return ENOMEM
>   
> @@ -466,6 +470,10 @@ int domain_max_global_acc(const void *ctx, struct connection *conn)
>   	ent(transactions, ACC_TRANS);
>   	ent(outstanding, ACC_OUTST);
>   	ent(memory, ACC_MEM);
> +	ent(transaction-nodes, ACC_TRANSNODES);
> +	ent(node-permissions, ACC_NPERM);
> +	ent(path-length, ACC_PATHLEN);
> +	ent(node-size, ACC_NODESZ);
>   
>   #undef ent
>   
> @@ -1079,12 +1087,22 @@ int domain_adjust_node_perms(struct node *node)
>   	return 0;
>   }
>   
> +static void domain_acc_valid_max(struct domain *d, enum accitem what,
> +				 unsigned int val)
> +{
> +	assert(what < ARRAY_SIZE(d->acc));
> +	assert(what < ARRAY_SIZE(acc_global_max));
> +
> +	if (val > d->acc[what].max)
> +		d->acc[what].max = val;
> +	if (val > acc_global_max[what] && domid_is_unprivileged(d->domid))
> +		acc_global_max[what] = val;
> +}
> +
>   static int domain_acc_add_valid(struct domain *d, enum accitem what, int add)
>   {
>   	unsigned int val;
>   
> -	assert(what < ARRAY_SIZE(d->acc));
> -

I didn't have a chance to reply on your comment on the previous version. 
So doing it here:

 > Following this reasoning I'd need to put it into even more functions.

Possibly. But for now, the discussion is about not removing the existing 
one (see more below).

 > And an
assert() triggering a little bit late is no real problem, as it will abort
xenstored anyway.

Not really. Xenstored would only be aborted if the condition is false. 
If it is not, we would return an error. That said, the condition that a 
change to be true in some condition.

But now you are relying on the compiler to never optimize out the check. 
We know that compilers can remove NULL check if a pointer was 
dereferenced beforehand. I wouldn't be surprised if they can do the same 
trick with accessing an array first and then checking the bound. So the 
abort() may actually never happen.

 > Additionally with the global and the per-domain arrays now covering all
possible quotas, it would even be reasonable to drop the assert()s in
domain_acc_valid_max() completely.

I have two concerns:
   * This is the state after this series. But I don't see what would 
prevent any change in the future.
   * If I am not mistaken none of the compilers properly enforce the 
enum in C. So you could in theory pass an outside value without the 
compiler shouting at you.

So to me, this is not warrant to completely drop the assert(). If we 
discard the latter point, the ideal would be a BUILD_BUG_ON() to tie the 
enum with the array but IIRC it is not possible to use BUILD_BUG_ON() 
with an enum. Therefore, the assert() should the best at the moment.

Ideally, we should add the assert() in other places. But, this is not 
something I think should be requested here. My only request is to not 
removing the existing one.

Cheers,

-- 
Julien Grall

Re: [PATCH v5 12/14] tools/xenstore: use generic accounting for remaining quotas

[Juergen Gross]

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

On 09.05.23 21:21, Julien Grall wrote:
> Hi Juergen,
> 
> On 08/05/2023 12:47, Juergen Gross wrote:
>> The maxrequests, node size, number of node permissions, and path length
>> quota are a little bit special, as they are either active in
>> transactions only (maxrequests), or they are just per item instead of
>> count values. Nevertheless being able to know the maximum number of
>> those quota related values per domain would be beneficial, so add them
>> to the generic accounting.
>>
>> The per domain value will never show current numbers other than zero,
>> but the maximum number seen can be gathered the same way as the number
>> of nodes during a transaction.
>>
>> To be able to use the const qualifier for a new function switch
>> domain_is_unprivileged() to take a const pointer, too.
>>
>> For printing the quota/max values, adapt the print format string to
>> the longest quota name (now 17 characters long).
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V5:
>> - add comment (Julien Grall)
>> - add missing quota printing (Julien Grall)
>> ---
>>   tools/xenstore/xenstored_core.c        | 15 +++++----
>>   tools/xenstore/xenstored_core.h        |  2 +-
>>   tools/xenstore/xenstored_domain.c      | 45 +++++++++++++++++++++-----
>>   tools/xenstore/xenstored_domain.h      |  6 ++++
>>   tools/xenstore/xenstored_transaction.c |  4 +--
>>   tools/xenstore/xenstored_watch.c       |  2 +-
>>   6 files changed, 55 insertions(+), 19 deletions(-)
>>
>> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
>> index c98d30561f..fce73b883e 100644
>> --- a/tools/xenstore/xenstored_core.c
>> +++ b/tools/xenstore/xenstored_core.c
>> @@ -799,8 +799,9 @@ int write_node_raw(struct connection *conn, TDB_DATA *key, 
>> struct node *node,
>>           + node->perms.num * sizeof(node->perms.p[0])
>>           + node->datalen + node->childlen;
>> -    if (!no_quota_check && domain_is_unprivileged(conn) &&
>> -        data.dsize >= quota_max_entry_size) {
>> +    /* Call domain_max_chk() in any case in order to record max values. */
>> +    if (domain_max_chk(conn, ACC_NODESZ, data.dsize, quota_max_entry_size)
>> +        && !no_quota_check) {
>>           errno = ENOSPC;
>>           return errno;
>>       }
>> @@ -1170,7 +1171,7 @@ static bool valid_chars(const char *node)
>>                  "0123456789-/_@") == strlen(node));
>>   }
>> -bool is_valid_nodename(const char *node)
>> +bool is_valid_nodename(const struct connection *conn, const char *node)
>>   {
>>       int local_off = 0;
>>       unsigned int domid;
>> @@ -1190,7 +1191,8 @@ bool is_valid_nodename(const char *node)
>>       if (sscanf(node, "/local/domain/%5u/%n", &domid, &local_off) != 1)
>>           local_off = 0;
>> -    if (strlen(node) > local_off + quota_max_path_len)
>> +    if (domain_max_chk(conn, ACC_PATHLEN, strlen(node) - local_off,
>> +               quota_max_path_len))
>>           return false;
>>       return valid_chars(node);
>> @@ -1252,7 +1254,7 @@ static struct node *get_node_canonicalized(struct 
>> connection *conn,
>>       *canonical_name = canonicalize(conn, ctx, name);
>>       if (!*canonical_name)
>>           return NULL;
>> -    if (!is_valid_nodename(*canonical_name)) {
>> +    if (!is_valid_nodename(conn, *canonical_name)) {
>>           errno = EINVAL;
>>           return NULL;
>>       }
>> @@ -1778,8 +1780,7 @@ static int do_set_perms(const void *ctx, struct 
>> connection *conn,
>>           return EINVAL;
>>       perms.num--;
>> -    if (domain_is_unprivileged(conn) &&
>> -        perms.num > quota_nb_perms_per_node)
>> +    if (domain_max_chk(conn, ACC_NPERM, perms.num, quota_nb_perms_per_node))
>>           return ENOSPC;
>>       permstr = in->buffer + strlen(in->buffer) + 1;
>> diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
>> index 3564d85d7d..9339820156 100644
>> --- a/tools/xenstore/xenstored_core.h
>> +++ b/tools/xenstore/xenstored_core.h
>> @@ -258,7 +258,7 @@ void check_store(void);
>>   void corrupt(struct connection *conn, const char *fmt, ...);
>>   /* Is this a valid node name? */
>> -bool is_valid_nodename(const char *node);
>> +bool is_valid_nodename(const struct connection *conn, const char *node);
>>   /* Get name of parent node. */
>>   char *get_parent(const void *ctx, const char *node);
>> diff --git a/tools/xenstore/xenstored_domain.c 
>> b/tools/xenstore/xenstored_domain.c
>> index 6f3a27765a..b3a719569e 100644
>> --- a/tools/xenstore/xenstored_domain.c
>> +++ b/tools/xenstore/xenstored_domain.c
>> @@ -431,7 +431,7 @@ int domain_get_quota(const void *ctx, struct connection 
>> *conn,
>>           return ENOMEM;
>>   #define ent(t, e) \
>> -    resp = talloc_asprintf_append(resp, "%-16s: %8u (max: %8u\n", #t, \
>> +    resp = talloc_asprintf_append(resp, "%-17s: %8u (max: %8u\n", #t, \
>>                         d->acc[e].val, d->acc[e].max); \
>>       if (!resp) return ENOMEM
>> @@ -440,6 +440,10 @@ int domain_get_quota(const void *ctx, struct connection 
>> *conn,
>>       ent(transactions, ACC_TRANS);
>>       ent(outstanding, ACC_OUTST);
>>       ent(memory, ACC_MEM);
>> +    ent(transaction-nodes, ACC_TRANSNODES);
>> +    ent(node-permissions, ACC_NPERM);
>> +    ent(path-length, ACC_PATHLEN);
>> +    ent(node-size, ACC_NODESZ);
>>   #undef ent
>> @@ -457,7 +461,7 @@ int domain_max_global_acc(const void *ctx, struct 
>> connection *conn)
>>           return ENOMEM;
>>   #define ent(t, e) \
>> -    resp = talloc_asprintf_append(resp, "%-16s: %8u\n", #t,   \
>> +    resp = talloc_asprintf_append(resp, "%-17s: %8u\n", #t,   \
>>                         acc_global_max[e]);         \
>>       if (!resp) return ENOMEM
>> @@ -466,6 +470,10 @@ int domain_max_global_acc(const void *ctx, struct 
>> connection *conn)
>>       ent(transactions, ACC_TRANS);
>>       ent(outstanding, ACC_OUTST);
>>       ent(memory, ACC_MEM);
>> +    ent(transaction-nodes, ACC_TRANSNODES);
>> +    ent(node-permissions, ACC_NPERM);
>> +    ent(path-length, ACC_PATHLEN);
>> +    ent(node-size, ACC_NODESZ);
>>   #undef ent
>> @@ -1079,12 +1087,22 @@ int domain_adjust_node_perms(struct node *node)
>>       return 0;
>>   }
>> +static void domain_acc_valid_max(struct domain *d, enum accitem what,
>> +                 unsigned int val)
>> +{
>> +    assert(what < ARRAY_SIZE(d->acc));
>> +    assert(what < ARRAY_SIZE(acc_global_max));
>> +
>> +    if (val > d->acc[what].max)
>> +        d->acc[what].max = val;
>> +    if (val > acc_global_max[what] && domid_is_unprivileged(d->domid))
>> +        acc_global_max[what] = val;
>> +}
>> +
>>   static int domain_acc_add_valid(struct domain *d, enum accitem what, int add)
>>   {
>>       unsigned int val;
>> -    assert(what < ARRAY_SIZE(d->acc));
>> -
> 
> I didn't have a chance to reply on your comment on the previous version. So 
> doing it here:
> 
>  > Following this reasoning I'd need to put it into even more functions.
> 
> Possibly. But for now, the discussion is about not removing the existing one 
> (see more below).
> 
>  > And an
> assert() triggering a little bit late is no real problem, as it will abort
> xenstored anyway.
> 
> Not really. Xenstored would only be aborted if the condition is false. If it is 
> not, we would return an error. That said, the condition that a change to be true 
> in some condition.
> 
> But now you are relying on the compiler to never optimize out the check. We know 
> that compilers can remove NULL check if a pointer was dereferenced beforehand. I 
> wouldn't be surprised if they can do the same trick with accessing an array 
> first and then checking the bound. So the abort() may actually never happen.
> 
>  > Additionally with the global and the per-domain arrays now covering all
> possible quotas, it would even be reasonable to drop the assert()s in
> domain_acc_valid_max() completely.
> 
> I have two concerns:
>    * This is the state after this series. But I don't see what would prevent any 
> change in the future.
>    * If I am not mistaken none of the compilers properly enforce the enum in C. 
> So you could in theory pass an outside value without the compiler shouting at you.
> 
> So to me, this is not warrant to completely drop the assert(). If we discard the 
> latter point, the ideal would be a BUILD_BUG_ON() to tie the enum with the array 
> but IIRC it is not possible to use BUILD_BUG_ON() with an enum. Therefore, the 
> assert() should the best at the moment.
> 
> Ideally, we should add the assert() in other places. But, this is not something 
> I think should be requested here. My only request is to not removing the 
> existing one.

Okay, I'll keep it.


Juergen

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

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

[PATCH v5 13/14] tools/xenstore: switch get_optval_int() to get_optval_uint()

[Juergen Gross]

Let get_optval_int() return an unsigned value and rename it
accordingly.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V5:
- new patch, carved out from next patch in series (Julien Grall)
---
 tools/xenstore/xenstored_core.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index fce73b883e..86ec7ab446 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -2700,13 +2700,13 @@ int dom0_domid = 0;
 int dom0_event = 0;
 int priv_domid = 0;
 
-static int get_optval_int(const char *arg)
+static unsigned int get_optval_uint(const char *arg)
 {
 	char *end;
-	long val;
+	unsigned long val;
 
-	val = strtol(arg, &end, 10);
-	if (!*arg || *end || val < 0 || val > INT_MAX)
+	val = strtoul(arg, &end, 10);
+	if (!*arg || *end || val > INT_MAX)
 		barf("invalid parameter value \"%s\"\n", arg);
 
 	return val;
@@ -2726,7 +2726,7 @@ static void set_timeout(const char *arg)
 
 	if (!eq)
 		barf("quotas must be specified via <what>=<seconds>\n");
-	val = get_optval_int(eq + 1);
+	val = get_optval_uint(eq + 1);
 	if (what_matches(arg, "watch-event"))
 		timeout_watch_event_msec = val * 1000;
 	else
@@ -2740,7 +2740,7 @@ static void set_quota(const char *arg, bool soft)
 
 	if (!eq)
 		barf("quotas must be specified via <what>=<nb>\n");
-	val = get_optval_int(eq + 1);
+	val = get_optval_uint(eq + 1);
 	if (what_matches(arg, "outstanding") && !soft)
 		quota_req_outstanding = val;
 	else if (what_matches(arg, "transaction-nodes") && !soft)
-- 
2.35.3

Re: [PATCH v5 13/14] tools/xenstore: switch get_optval_int() to get_optval_uint()

[Julien Grall]

Hi Juergen,

On 08/05/2023 12:47, Juergen Gross wrote:
> Let get_optval_int() return an unsigned value and rename it
> accordingly.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

[PATCH v5 14/14] tools/xenstore: switch quota management to be table based

[Juergen Gross]

Instead of having individual quota variables switch to a table based
approach like the generic accounting. Include all the related data in
the same table and add accessor functions.

This enables to use the command line --quota parameter for setting all
possible quota values, keeping the previous parameters for
compatibility.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- new patch
One further remark: it would be rather easy to add soft-quota for all
the other quotas (similar to the memory one). This could be used as
an early warning for the need to raise global quota.
V5:
- fix what_matches() (Julien Grall)
---
 tools/xenstore/xenstored_control.c     |  43 ++------
 tools/xenstore/xenstored_core.c        |  80 +++++++-------
 tools/xenstore/xenstored_core.h        |  10 --
 tools/xenstore/xenstored_domain.c      | 138 ++++++++++++++++---------
 tools/xenstore/xenstored_domain.h      |  12 ++-
 tools/xenstore/xenstored_transaction.c |   5 +-
 tools/xenstore/xenstored_watch.c       |   2 +-
 7 files changed, 152 insertions(+), 138 deletions(-)

diff --git a/tools/xenstore/xenstored_control.c b/tools/xenstore/xenstored_control.c
index 620a7da997..ed80924ee4 100644
--- a/tools/xenstore/xenstored_control.c
+++ b/tools/xenstore/xenstored_control.c
@@ -221,35 +221,6 @@ static int do_control_log(const void *ctx, struct connection *conn,
 	return 0;
 }
 
-struct quota {
-	const char *name;
-	int *quota;
-	const char *descr;
-};
-
-static const struct quota hard_quotas[] = {
-	{ "nodes", &quota_nb_entry_per_domain, "Nodes per domain" },
-	{ "watches", &quota_nb_watch_per_domain, "Watches per domain" },
-	{ "transactions", &quota_max_transaction, "Transactions per domain" },
-	{ "outstanding", &quota_req_outstanding,
-		"Outstanding requests per domain" },
-	{ "transaction-nodes", &quota_trans_nodes,
-		"Max. number of accessed nodes per transaction" },
-	{ "memory", &quota_memory_per_domain_hard,
-		"Total Xenstore memory per domain (error level)" },
-	{ "node-size", &quota_max_entry_size, "Max. size of a node" },
-	{ "path-max", &quota_max_path_len, "Max. length of a node path" },
-	{ "permissions", &quota_nb_perms_per_node,
-		"Max. number of permissions per node" },
-	{ NULL, NULL, NULL }
-};
-
-static const struct quota soft_quotas[] = {
-	{ "memory", &quota_memory_per_domain_soft,
-		"Total Xenstore memory per domain (warning level)" },
-	{ NULL, NULL, NULL }
-};
-
 static int quota_show_current(const void *ctx, struct connection *conn,
 			      const struct quota *quotas)
 {
@@ -260,9 +231,11 @@ static int quota_show_current(const void *ctx, struct connection *conn,
 	if (!resp)
 		return ENOMEM;
 
-	for (i = 0; quotas[i].quota; i++) {
+	for (i = 0; i < ACC_N; i++) {
+		if (!quotas[i].name)
+			continue;
 		resp = talloc_asprintf_append(resp, "%-17s: %8d %s\n",
-					      quotas[i].name, *quotas[i].quota,
+					      quotas[i].name, quotas[i].val,
 					      quotas[i].descr);
 		if (!resp)
 			return ENOMEM;
@@ -274,7 +247,7 @@ static int quota_show_current(const void *ctx, struct connection *conn,
 }
 
 static int quota_set(const void *ctx, struct connection *conn,
-		     char **vec, int num, const struct quota *quotas)
+		     char **vec, int num, struct quota *quotas)
 {
 	unsigned int i;
 	int val;
@@ -286,9 +259,9 @@ static int quota_set(const void *ctx, struct connection *conn,
 	if (val < 1)
 		return EINVAL;
 
-	for (i = 0; quotas[i].quota; i++) {
-		if (!strcmp(vec[0], quotas[i].name)) {
-			*quotas[i].quota = val;
+	for (i = 0; i < ACC_N; i++) {
+		if (quotas[i].name && !strcmp(vec[0], quotas[i].name)) {
+			quotas[i].val = val;
 			send_ack(conn, XS_CONTROL);
 			return 0;
 		}
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 86ec7ab446..4ebb75e5e9 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -89,17 +89,6 @@ unsigned int trace_flags = TRACE_OBJ | TRACE_IO;
 
 static const char *sockmsg_string(enum xsd_sockmsg_type type);
 
-int quota_nb_entry_per_domain = 1000;
-int quota_nb_watch_per_domain = 128;
-int quota_max_entry_size = 2048; /* 2K */
-int quota_max_transaction = 10;
-int quota_nb_perms_per_node = 5;
-int quota_trans_nodes = 1024;
-int quota_max_path_len = XENSTORE_REL_PATH_MAX;
-int quota_req_outstanding = 20;
-int quota_memory_per_domain_soft = 2 * 1024 * 1024; /* 2 MB */
-int quota_memory_per_domain_hard = 2 * 1024 * 1024 + 512 * 1024; /* 2.5 MB */
-
 unsigned int timeout_watch_event_msec = 20000;
 
 void trace(const char *fmt, ...)
@@ -800,8 +789,7 @@ int write_node_raw(struct connection *conn, TDB_DATA *key, struct node *node,
 		+ node->datalen + node->childlen;
 
 	/* Call domain_max_chk() in any case in order to record max values. */
-	if (domain_max_chk(conn, ACC_NODESZ, data.dsize, quota_max_entry_size)
-	    && !no_quota_check) {
+	if (domain_max_chk(conn, ACC_NODESZ, data.dsize) && !no_quota_check) {
 		errno = ENOSPC;
 		return errno;
 	}
@@ -1191,8 +1179,7 @@ bool is_valid_nodename(const struct connection *conn, const char *node)
 	if (sscanf(node, "/local/domain/%5u/%n", &domid, &local_off) != 1)
 		local_off = 0;
 
-	if (domain_max_chk(conn, ACC_PATHLEN, strlen(node) - local_off,
-			   quota_max_path_len))
+	if (domain_max_chk(conn, ACC_PATHLEN, strlen(node) - local_off))
 		return false;
 
 	return valid_chars(node);
@@ -1504,7 +1491,7 @@ static struct node *create_node(struct connection *conn, const void *ctx,
 	for (i = node; i; i = i->parent) {
 		/* i->parent is set for each new node, so check quota. */
 		if (i->parent &&
-		    domain_nbentry(conn) >= quota_nb_entry_per_domain) {
+		    domain_nbentry(conn) >= hard_quotas[ACC_NODES].val) {
 			ret = ENOSPC;
 			goto err;
 		}
@@ -1780,7 +1767,7 @@ static int do_set_perms(const void *ctx, struct connection *conn,
 		return EINVAL;
 
 	perms.num--;
-	if (domain_max_chk(conn, ACC_NPERM, perms.num, quota_nb_perms_per_node))
+	if (domain_max_chk(conn, ACC_NPERM, perms.num))
 		return ENOSPC;
 
 	permstr = in->buffer + strlen(in->buffer) + 1;
@@ -2649,7 +2636,16 @@ static void usage(void)
 "                          memory: total used memory per domain for nodes,\n"
 "                                  transactions, watches and requests, above\n"
 "                                  which Xenstore will stop talking to domain\n"
+"                          nodes: number nodes owned by a domain\n"
+"                          node-permissions: number of access permissions per\n"
+"                                            node\n"
+"                          node-size: total size of a node (permissions +\n"
+"                                     children names + content)\n"
 "                          outstanding: number of outstanding requests\n"
+"                          path-length: length of a node path\n"
+"                          transactions: number of concurrent transactions\n"
+"                                        per domain\n"
+"                          watches: number of watches per domain"
 "  -q, --quota-soft <what>=<nb> set a soft quota <what> to the value <nb>,\n"
 "                          causing a warning to be issued via syslog() if the\n"
 "                          limit is violated, allowed quotas are:\n"
@@ -2714,15 +2710,19 @@ static unsigned int get_optval_uint(const char *arg)
 
 static bool what_matches(const char *arg, const char *what)
 {
-	unsigned int what_len = strlen(what);
+	unsigned int what_len;
+
+	if (!what)
+		return false;
 
+	what_len = strlen(what);
 	return !strncmp(arg, what, what_len) && arg[what_len] == '=';
 }
 
 static void set_timeout(const char *arg)
 {
 	const char *eq = strchr(arg, '=');
-	int val;
+	unsigned int val;
 
 	if (!eq)
 		barf("quotas must be specified via <what>=<seconds>\n");
@@ -2736,22 +2736,22 @@ static void set_timeout(const char *arg)
 static void set_quota(const char *arg, bool soft)
 {
 	const char *eq = strchr(arg, '=');
-	int val;
+	struct quota *q = soft ? soft_quotas : hard_quotas;
+	unsigned int val;
+	unsigned int i;
 
 	if (!eq)
 		barf("quotas must be specified via <what>=<nb>\n");
 	val = get_optval_uint(eq + 1);
-	if (what_matches(arg, "outstanding") && !soft)
-		quota_req_outstanding = val;
-	else if (what_matches(arg, "transaction-nodes") && !soft)
-		quota_trans_nodes = val;
-	else if (what_matches(arg, "memory")) {
-		if (soft)
-			quota_memory_per_domain_soft = val;
-		else
-			quota_memory_per_domain_hard = val;
-	} else
-		barf("unknown quota \"%s\"\n", arg);
+
+	for (i = 0; i < ACC_N; i++) {
+		if (what_matches(arg, q[i].name)) {
+			q[i].val = val;
+			return;
+		}
+	}
+
+	barf("unknown quota \"%s\"\n", arg);
 }
 
 /* Sorted by bit values of TRACE_* flags. Flag is (1u << index). */
@@ -2813,7 +2813,7 @@ int main(int argc, char *argv[])
 			no_domain_init = true;
 			break;
 		case 'E':
-			quota_nb_entry_per_domain = strtol(optarg, NULL, 10);
+			hard_quotas[ACC_NODES].val = strtoul(optarg, NULL, 10);
 			break;
 		case 'F':
 			pidfile = optarg;
@@ -2831,10 +2831,10 @@ int main(int argc, char *argv[])
 			recovery = false;
 			break;
 		case 'S':
-			quota_max_entry_size = strtol(optarg, NULL, 10);
+			hard_quotas[ACC_NODESZ].val = strtoul(optarg, NULL, 10);
 			break;
 		case 't':
-			quota_max_transaction = strtol(optarg, NULL, 10);
+			hard_quotas[ACC_TRANS].val = strtoul(optarg, NULL, 10);
 			break;
 		case 'T':
 			tracefile = optarg;
@@ -2854,15 +2854,17 @@ int main(int argc, char *argv[])
 			verbose = true;
 			break;
 		case 'W':
-			quota_nb_watch_per_domain = strtol(optarg, NULL, 10);
+			hard_quotas[ACC_WATCH].val = strtoul(optarg, NULL, 10);
 			break;
 		case 'A':
-			quota_nb_perms_per_node = strtol(optarg, NULL, 10);
+			hard_quotas[ACC_NPERM].val = strtoul(optarg, NULL, 10);
 			break;
 		case 'M':
-			quota_max_path_len = strtol(optarg, NULL, 10);
-			quota_max_path_len = min(XENSTORE_REL_PATH_MAX,
-						 quota_max_path_len);
+			hard_quotas[ACC_PATHLEN].val =
+				strtoul(optarg, NULL, 10);
+			hard_quotas[ACC_PATHLEN].val =
+				 min((unsigned int)XENSTORE_REL_PATH_MAX,
+				     hard_quotas[ACC_PATHLEN].val);
 			break;
 		case 'Q':
 			set_quota(optarg, false);
diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
index 9339820156..92d5b50f3c 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -316,16 +316,6 @@ extern TDB_CONTEXT *tdb_ctx;
 extern int dom0_domid;
 extern int dom0_event;
 extern int priv_domid;
-extern int quota_nb_watch_per_domain;
-extern int quota_max_transaction;
-extern int quota_max_entry_size;
-extern int quota_nb_perms_per_node;
-extern int quota_max_path_len;
-extern int quota_nb_entry_per_domain;
-extern int quota_req_outstanding;
-extern int quota_trans_nodes;
-extern int quota_memory_per_domain_soft;
-extern int quota_memory_per_domain_hard;
 extern bool keep_orphans;
 
 extern unsigned int timeout_watch_event_msec;
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index b3a719569e..04f911b111 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -43,7 +43,61 @@ static evtchn_port_t virq_port;
 
 xenevtchn_handle *xce_handle = NULL;
 
-static unsigned int acc_global_max[ACC_N];
+struct quota hard_quotas[ACC_N] = {
+	[ACC_NODES] = {
+		.name = "nodes",
+		.descr = "Nodes per domain",
+		.val = 1000,
+	},
+	[ACC_WATCH] = {
+		.name = "watches",
+		.descr = "Watches per domain",
+		.val = 128,
+	},
+	[ACC_OUTST] = {
+		.name = "outstanding",
+		.descr = "Outstanding requests per domain",
+		.val = 20,
+	},
+	[ACC_MEM] = {
+		.name = "memory",
+		.descr = "Total Xenstore memory per domain (error level)",
+		.val = 2 * 1024 * 1024 + 512 * 1024,	/* 2.5 MB */
+	},
+	[ACC_TRANS] = {
+		.name = "transactions",
+		.descr = "Active transactions per domain",
+		.val = 10,
+	},
+	[ACC_TRANSNODES] = {
+		.name = "transaction-nodes",
+		.descr = "Max. number of accessed nodes per transaction",
+		.val = 1024,
+	},
+	[ACC_NPERM] = {
+		.name = "node-permissions",
+		.descr = "Max. number of permissions per node",
+		.val = 5,
+	},
+	[ACC_PATHLEN] = {
+		.name = "path-max",
+		.descr = "Max. length of a node path",
+		.val = XENSTORE_REL_PATH_MAX,
+	},
+	[ACC_NODESZ] = {
+		.name = "node-size",
+		.descr = "Max. size of a node",
+		.val = 2048,
+	},
+};
+
+struct quota soft_quotas[ACC_N] = {
+	[ACC_MEM] = {
+		.name = "memory",
+		.descr = "Total Xenstore memory per domain (warning level)",
+		.val = 2 * 1024 * 1024,			/* 2.0 MB */
+	},
+};
 
 struct domain
 {
@@ -204,10 +258,10 @@ static bool domain_can_read(struct connection *conn)
 	if (domain_is_unprivileged(conn)) {
 		if (domain->wrl_credit < 0)
 			return false;
-		if (domain->acc[ACC_OUTST].val >= quota_req_outstanding)
+		if (domain->acc[ACC_OUTST].val >= hard_quotas[ACC_OUTST].val)
 			return false;
-		if (domain->acc[ACC_MEM].val >= quota_memory_per_domain_hard &&
-		    quota_memory_per_domain_hard)
+		if (domain->acc[ACC_MEM].val >= hard_quotas[ACC_MEM].val &&
+		    hard_quotas[ACC_MEM].val)
 			return false;
 	}
 
@@ -422,6 +476,7 @@ int domain_get_quota(const void *ctx, struct connection *conn,
 {
 	struct domain *d = find_domain_struct(domid);
 	char *resp;
+	unsigned int i;
 
 	if (!d)
 		return ENOENT;
@@ -430,22 +485,15 @@ int domain_get_quota(const void *ctx, struct connection *conn,
 	if (!resp)
 		return ENOMEM;
 
-#define ent(t, e) \
-	resp = talloc_asprintf_append(resp, "%-17s: %8u (max: %8u\n", #t, \
-				      d->acc[e].val, d->acc[e].max); \
-	if (!resp) return ENOMEM
-
-	ent(nodes, ACC_NODES);
-	ent(watches, ACC_WATCH);
-	ent(transactions, ACC_TRANS);
-	ent(outstanding, ACC_OUTST);
-	ent(memory, ACC_MEM);
-	ent(transaction-nodes, ACC_TRANSNODES);
-	ent(node-permissions, ACC_NPERM);
-	ent(path-length, ACC_PATHLEN);
-	ent(node-size, ACC_NODESZ);
-
-#undef ent
+	for (i = 0; i < ACC_N; i++) {
+		if (!hard_quotas[i].name)
+			continue;
+		resp = talloc_asprintf_append(resp, "%-17s: %8u (max %8u)\n",
+					      hard_quotas[i].name,
+					      d->acc[i].val, d->acc[i].max);
+		if (!resp)
+			return ENOMEM;
+	}
 
 	send_reply(conn, XS_CONTROL, resp, strlen(resp) + 1);
 
@@ -455,27 +503,21 @@ int domain_get_quota(const void *ctx, struct connection *conn,
 int domain_max_global_acc(const void *ctx, struct connection *conn)
 {
 	char *resp;
+	unsigned int i;
 
 	resp = talloc_asprintf(ctx, "Max. seen accounting values:\n");
 	if (!resp)
 		return ENOMEM;
 
-#define ent(t, e) \
-	resp = talloc_asprintf_append(resp, "%-17s: %8u\n", #t,   \
-				      acc_global_max[e]);         \
-	if (!resp) return ENOMEM
-
-	ent(nodes, ACC_NODES);
-	ent(watches, ACC_WATCH);
-	ent(transactions, ACC_TRANS);
-	ent(outstanding, ACC_OUTST);
-	ent(memory, ACC_MEM);
-	ent(transaction-nodes, ACC_TRANSNODES);
-	ent(node-permissions, ACC_NPERM);
-	ent(path-length, ACC_PATHLEN);
-	ent(node-size, ACC_NODESZ);
-
-#undef ent
+	for (i = 0; i < ACC_N; i++) {
+		if (!hard_quotas[i].name)
+			continue;
+		resp = talloc_asprintf_append(resp, "%-17s: %8u\n",
+					      hard_quotas[i].name,
+					      hard_quotas[i].max);
+		if (!resp)
+			return ENOMEM;
+	}
 
 	send_reply(conn, XS_CONTROL, resp, strlen(resp) + 1);
 
@@ -590,7 +632,7 @@ int acc_fix_domains(struct list_head *head, bool chk_quota, bool update)
 	list_for_each_entry(cd, head, list) {
 		cnt = domain_nbentry_fix(cd->domid, cd->acc[ACC_NODES], update);
 		if (!update) {
-			if (chk_quota && cnt >= quota_nb_entry_per_domain)
+			if (chk_quota && cnt >= hard_quotas[ACC_NODES].val)
 				return ENOSPC;
 			if (cnt < 0)
 				return ENOMEM;
@@ -1091,12 +1133,12 @@ static void domain_acc_valid_max(struct domain *d, enum accitem what,
 				 unsigned int val)
 {
 	assert(what < ARRAY_SIZE(d->acc));
-	assert(what < ARRAY_SIZE(acc_global_max));
+	assert(what < ARRAY_SIZE(hard_quotas));
 
 	if (val > d->acc[what].max)
 		d->acc[what].max = val;
-	if (val > acc_global_max[what] && domid_is_unprivileged(d->domid))
-		acc_global_max[what] = val;
+	if (val > hard_quotas[what].max && domid_is_unprivileged(d->domid))
+		hard_quotas[what].max = val;
 }
 
 static int domain_acc_add_valid(struct domain *d, enum accitem what, int add)
@@ -1231,19 +1273,19 @@ void domain_reset_global_acc(void)
 	unsigned int i;
 
 	for (i = 0; i < ACC_N; i++)
-		acc_global_max[i] = 0;
+		hard_quotas[i].max = 0;
 
 	/* Set current max values seen. */
 	hashtable_iterate(domhash, domain_reset_global_acc_sub, NULL);
 }
 
 bool domain_max_chk(const struct connection *conn, enum accitem what,
-		    unsigned int val, unsigned int quota)
+		    unsigned int val)
 {
 	if (!conn || !conn->domain)
 		return false;
 
-	if (domain_is_unprivileged(conn) && val > quota)
+	if (domain_is_unprivileged(conn) && val > hard_quotas[what].val)
 		return true;
 
 	domain_acc_valid_max(conn->domain, what, val);
@@ -1292,8 +1334,7 @@ static bool domain_chk_quota(struct connection *conn, unsigned int mem)
 	domain = conn->domain;
 	now = time(NULL);
 
-	if (mem >= quota_memory_per_domain_hard &&
-	    quota_memory_per_domain_hard) {
+	if (mem >= hard_quotas[ACC_MEM].val && hard_quotas[ACC_MEM].val) {
 		if (domain->hard_quota_reported)
 			return true;
 		syslog(LOG_ERR, "Domain %u exceeds hard memory quota, Xenstore interface to domain stalled\n",
@@ -1310,15 +1351,14 @@ static bool domain_chk_quota(struct connection *conn, unsigned int mem)
 			syslog(LOG_INFO, "Domain %u below hard memory quota again\n",
 			       domain->domid);
 		}
-		if (mem >= quota_memory_per_domain_soft &&
-		    quota_memory_per_domain_soft &&
-		    !domain->soft_quota_reported) {
+		if (mem >= soft_quotas[ACC_MEM].val &&
+		    soft_quotas[ACC_MEM].val && !domain->soft_quota_reported) {
 			domain->mem_last_msg = now;
 			domain->soft_quota_reported = true;
 			syslog(LOG_WARNING, "Domain %u exceeds soft memory quota\n",
 			       domain->domid);
 		}
-		if (mem < quota_memory_per_domain_soft &&
+		if (mem < soft_quotas[ACC_MEM].val &&
 		    domain->soft_quota_reported) {
 			domain->mem_last_msg = now;
 			domain->soft_quota_reported = false;
diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h
index 78ca434531..5123453f6c 100644
--- a/tools/xenstore/xenstored_domain.h
+++ b/tools/xenstore/xenstored_domain.h
@@ -40,6 +40,16 @@ enum accitem {
 	ACC_N,			/* Number of elements per domain. */
 };
 
+struct quota {
+	const char *name;
+	const char *descr;
+	unsigned int val;
+	unsigned int max;
+};
+
+extern struct quota hard_quotas[ACC_N];
+extern struct quota soft_quotas[ACC_N];
+
 void handle_event(void);
 
 void check_domains(void);
@@ -134,7 +144,7 @@ void acc_commit(struct connection *conn);
 int domain_max_global_acc(const void *ctx, struct connection *conn);
 void domain_reset_global_acc(void);
 bool domain_max_chk(const struct connection *conn, unsigned int what,
-		    unsigned int val, unsigned int quota);
+		    unsigned int val);
 
 /* Write rate limiting */
 
diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c
index cecaf27018..f2650f5d05 100644
--- a/tools/xenstore/xenstored_transaction.c
+++ b/tools/xenstore/xenstored_transaction.c
@@ -252,8 +252,7 @@ int access_node(struct connection *conn, struct node *node,
 
 	i = find_accessed_node(trans, node->name);
 	if (!i) {
-		if (domain_max_chk(conn, ACC_TRANSNODES, trans->nodes + 1,
-				   quota_trans_nodes)) {
+		if (domain_max_chk(conn, ACC_TRANSNODES, trans->nodes + 1)) {
 			ret = ENOSPC;
 			goto err;
 		}
@@ -479,7 +478,7 @@ int do_transaction_start(const void *ctx, struct connection *conn,
 	if (conn->transaction)
 		return EBUSY;
 
-	if (domain_transaction_get(conn) > quota_max_transaction)
+	if (domain_transaction_get(conn) > hard_quotas[ACC_TRANS].val)
 		return ENOSPC;
 
 	/* Attach transaction to ctx for autofree until it's complete */
diff --git a/tools/xenstore/xenstored_watch.c b/tools/xenstore/xenstored_watch.c
index 61b1e3421e..e8eb35de02 100644
--- a/tools/xenstore/xenstored_watch.c
+++ b/tools/xenstore/xenstored_watch.c
@@ -239,7 +239,7 @@ int do_watch(const void *ctx, struct connection *conn, struct buffered_data *in)
 			return EEXIST;
 	}
 
-	if (domain_watch(conn) > quota_nb_watch_per_domain)
+	if (domain_watch(conn) > hard_quotas[ACC_WATCH].val)
 		return E2BIG;
 
 	watch = add_watch(conn, vec[0], vec[1], relative, false);
-- 
2.35.3

Re: [PATCH v5 14/14] tools/xenstore: switch quota management to be table based

[Julien Grall]

Hi Juergen,

On 08/05/2023 12:47, Juergen Gross wrote:
> @@ -2714,15 +2710,19 @@ static unsigned int get_optval_uint(const char *arg)
>   
>   static bool what_matches(const char *arg, const char *what)
>   {
> -	unsigned int what_len = strlen(what);
> +	unsigned int what_len;
> +
> +	if (!what)
> +		return false;
>   
> +	what_len = strlen(what);

NIT: Keep the newline before the return.

Acked-by: Julien Grall <jgrall@amazon.com>

-- 
Julien Grall