[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 03/13] tools/xenstore: introduce accounting data array for per-domain values
Hi Juergen,
On 20/01/2023 10:00, Juergen Gross wrote:
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@xxxxxxxx>
---
tools/xenstore/xenstored_domain.c | 71 ++++++++++++++++++-------------
tools/xenstore/xenstored_domain.h | 5 ++-
2 files changed, 45 insertions(+), 31 deletions(-)
diff --git a/tools/xenstore/xenstored_domain.c
b/tools/xenstore/xenstored_domain.c
index 44e72937fa..f459c5aabb 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,28 @@ 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_chk(struct domain *d, enum accitem what, int add,
+ unsigned int domid)
You are passing the domid but this doesn't seem to be used within the
function. Also, from just reading at this prototype, it is not clear to
me whether 'domid' is meant to correspond to the one of 'd'.
The name is also a bit confusing because below you have a function call
domain_acc_add() that will update "d->acc[what]" so I would expect this
function to also update it after a sanity get.
I would suggest to rename it to domain_acc_get_chk() or similar (see
below for some context).
+{
+ assert(what < ARRAY_SIZE(d->acc));
+
+ if ((add < 0 && -add > d->acc[what]) ||
+ (d->acc[what] + add) > INT_MAX) {
NIT: Even if I know that 'add' will unlikely be INT_MAX, it would be
better to use '(INT_MAX - d->acc[what]) < add)'. So there is no overflow
possible.
+ /*
+ * 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 +1091,49 @@ static int domain_nbentry_add(struct connection *conn,
unsigned int domid,
}
}
- if (conn && conn->transaction) {
+ if (conn && conn->transaction && what < ACC_TR_N) {
Do you have a use case where 'what' is >= ACC_TR_N and you want to
modify d->acc?
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_chk(d, what, ret, domid);
I was going to ask here why we are updating d->acc[what]. However the
function is not doing what I was expecting from the name. You are only
returning the number of entries adjusted.
}
- d->nbentry += add;
+ d->acc[what] = domain_acc_add_chk(d, what, add, domid);
- 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 +1610,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 +1665,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 6a2b76a85b..8259c114b0 100644
--- a/tools/xenstore/xenstored_domain.h
+++ b/tools/xenstore/xenstored_domain.h
@@ -21,7 +21,8 @@
enum accitem {
ACC_NODES,
- ACC_TR_N /* Number of elements per transaction and domain. */
+ ACC_TR_N, /* Number of elements per transaction and domain. */
The churn here could have been avoided if you add a "," even for the edn
element and properly indented the comment in the original patch.
Also, was the comment indented to be updated to remove "and domain"?
+ ACC_N = ACC_TR_N /* Number of elements per domain. */
};
void handle_event(void);
@@ -72,7 +73,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);
/*
Cheers,
--
Julien Grall
|
