Re: [PATCH v2 10/19] tools/xenstore: change per-domain node accounting interface
On 13.01.23 10:53, Julien Grall wrote: Hi Juergen, On 12/01/2023 05:49, Juergen Gross wrote: On 11.01.23 18:48, Julien Grall wrote: Hi Juergen, On 11/01/2023 08:59, Juergen Gross wrote: ... to make sure domain_nbentry_add() is not returning a negative value. Then it would not work. A good example imagine you have a transaction removing nodes from tree but not adding any. Then the "ret" would be negative. Meanwhile the nodes are also removed outside of the transaction. So the sum of "d->nbentry + ret" would be negative resulting to a failure here. Thanks for catching this. I think the correct way to handle this is to return max(d->nbentry + ret, 0) in domain_nbentry_add(). The value might be imprecise, but always >= 0 and never wrong outside of a transaction collision. I am bit confused with your proposal. If the return value is imprecise, then what's the point of returning max(...) instead of simply 0? Please have a look at the use case especially in domain_nbentry(). Returning always 0 would clearly break quota checks. I am a bit concerned that we would have a code checking the quota based on an imprecise value. At the moment, I don't have a better suggestion. But we should at least document in the code when we think the value is imprecise and explain why bypassing the quota check is OK (IOW who will check it?). The imprecise value will never be too low, it can only be too high (i.e. 0 instead of negative), and that will only happen in a transaction which can't succeed. Adding a comment is good idea, though. Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH v2 10/19] tools/xenstore: change per-domain node accounting interface
Hi Juergen, On 12/01/2023 05:49, Juergen Gross wrote: On 11.01.23 18:48, Julien Grall wrote: Hi Juergen, On 11/01/2023 08:59, Juergen Gross wrote: ... to make sure domain_nbentry_add() is not returning a negative value. Then it would not work. A good example imagine you have a transaction removing nodes from tree but not adding any. Then the "ret" would be negative. Meanwhile the nodes are also removed outside of the transaction. So the sum of "d->nbentry + ret" would be negative resulting to a failure here. Thanks for catching this. I think the correct way to handle this is to return max(d->nbentry + ret, 0) in domain_nbentry_add(). The value might be imprecise, but always >= 0 and never wrong outside of a transaction collision. I am bit confused with your proposal. If the return value is imprecise, then what's the point of returning max(...) instead of simply 0? Please have a look at the use case especially in domain_nbentry(). Returning always 0 would clearly break quota checks. I am a bit concerned that we would have a code checking the quota based on an imprecise value. At the moment, I don't have a better suggestion. But we should at least document in the code when we think the value is imprecise and explain why bypassing the quota check is OK (IOW who will check it?). Cheers, -- Julien Grall
Re: [PATCH v2 10/19] tools/xenstore: change per-domain node accounting interface
On 11.01.23 18:48, Julien Grall wrote: Hi Juergen, On 11/01/2023 08:59, Juergen Gross wrote: ... to make sure domain_nbentry_add() is not returning a negative value. Then it would not work. A good example imagine you have a transaction removing nodes from tree but not adding any. Then the "ret" would be negative. Meanwhile the nodes are also removed outside of the transaction. So the sum of "d->nbentry + ret" would be negative resulting to a failure here. Thanks for catching this. I think the correct way to handle this is to return max(d->nbentry + ret, 0) in domain_nbentry_add(). The value might be imprecise, but always >= 0 and never wrong outside of a transaction collision. I am bit confused with your proposal. If the return value is imprecise, then what's the point of returning max(...) instead of simply 0? Please have a look at the use case especially in domain_nbentry(). Returning always 0 would clearly break quota checks. Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH v2 10/19] tools/xenstore: change per-domain node accounting interface
Hi Juergen, On 11/01/2023 08:59, Juergen Gross wrote: ... to make sure domain_nbentry_add() is not returning a negative value. Then it would not work. A good example imagine you have a transaction removing nodes from tree but not adding any. Then the "ret" would be negative. Meanwhile the nodes are also removed outside of the transaction. So the sum of "d->nbentry + ret" would be negative resulting to a failure here. Thanks for catching this. I think the correct way to handle this is to return max(d->nbentry + ret, 0) in domain_nbentry_add(). The value might be imprecise, but always >= 0 and never wrong outside of a transaction collision. I am bit confused with your proposal. If the return value is imprecise, then what's the point of returning max(...) instead of simply 0? Cheers, -- Julien Grall
Re: [PATCH v2 10/19] tools/xenstore: change per-domain node accounting interface
On 20.12.22 21:15, Julien Grall wrote:
Hi,
On 13/12/2022 16:00, Juergen Gross wrote:
Rework the interface and the internals of the per-domain node
accounting:
- rename the functions to domain_nbentry_*() in order to better match
the related counter name
- switch from node pointer to domid as interface, as all nodes have the
owner filled in
The downside is now you have may place open-coding "...->perms->p[0].id". IHMO
this is making the code more complicated. So can you introduce a few wrappers
that would take a node and then convert to the owner?
Okay.
- use a common internal function for adding a value to the counter
For the transaction case add a helper function to get the list head
of the per-transaction changed domains, enabling to eliminate the
transaction_entry_*() functions.
Signed-off-by: Juergen Gross
---
tools/xenstore/xenstored_core.c | 22 ++---
tools/xenstore/xenstored_domain.c | 122 +++--
tools/xenstore/xenstored_domain.h | 10 +-
tools/xenstore/xenstored_transaction.c | 15 +--
tools/xenstore/xenstored_transaction.h | 7 +-
5 files changed, 72 insertions(+), 104 deletions(-)
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index f96714e1b8..61569cecbb 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -1459,7 +1459,7 @@ 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_entry_dec(conn, node);
+ domain_nbentry_dec(conn, node->perms.p[0].id);
/*
* It is not possible to easily revert the changes in a transaction.
@@ -1498,7 +1498,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_entry(conn) >= quota_nb_entry_per_domain) {
+ domain_nbentry(conn) >= quota_nb_entry_per_domain) {
ret = ENOSPC;
goto err;
}
@@ -1509,7 +1509,7 @@ static struct node *create_node(struct connection *conn,
const void *ctx,
/* Account for new node */
if (i->parent) {
- if (domain_entry_inc(conn, i)) {
+ if (domain_nbentry_inc(conn, i->perms.p[0].id)) {
destroy_node_rm(conn, i);
return NULL;
}
@@ -1662,7 +1662,7 @@ 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_entry_dec(conn, node);
+ domain_nbentry_dec(conn, node->perms.p[0].id);
return WALK_TREE_RM_CHILDENTRY;
}
@@ -1802,25 +1802,25 @@ static int do_set_perms(const void *ctx, struct
connection *conn,
return EPERM;
old_perms = node->perms;
- domain_entry_dec(conn, node);
+ domain_nbentry_dec(conn, node->perms.p[0].id);
node->perms = perms;
- if (domain_entry_inc(conn, node)) {
+ if (domain_nbentry_inc(conn, node->perms.p[0].id)) {
node->perms = old_perms;
/*
* This should never fail because we had a reference on the
* domain before and Xenstored is single-threaded.
*/
- domain_entry_inc(conn, node);
+ domain_nbentry_inc(conn, node->perms.p[0].id);
return ENOMEM;
}
if (write_node(conn, node, false)) {
int saved_errno = errno;
- domain_entry_dec(conn, node);
+ domain_nbentry_dec(conn, node->perms.p[0].id);
node->perms = old_perms;
/* No failure possible as above. */
- domain_entry_inc(conn, node);
+ domain_nbentry_inc(conn, node->perms.p[0].id);
errno = saved_errno;
return errno;
@@ -2392,7 +2392,7 @@ void setup_structure(bool live_update)
manual_node("/tool/xenstored", NULL);
manual_node("@releaseDomain", NULL);
manual_node("@introduceDomain", NULL);
- domain_entry_fix(dom0_domid, 5, true);
+ domain_nbentry_fix(dom0_domid, 5, true);
}
check_store();
@@ -3400,7 +3400,7 @@ void read_state_node(const void *ctx, const void *state)
if (write_node_raw(NULL, &key, node, true))
barf("write node error restoring node");
- if (domain_entry_inc(&conn, node))
+ if (domain_nbentry_inc(&conn, node->perms.p[0].id))
barf("node accounting error restoring node");
talloc_free(node);
diff --git a/tools/xenstore/xenstored_domain.c
b/tools/xenstore/xenstored_domain.c
index 3216119e83..40b24056c5 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -249,7 +249,7 @@ static int domain_tree_remove_sub(const void *ctx, struct
connection *conn,
domain->nbentry--;
node->perms.p
Re: [PATCH v2 10/19] tools/xenstore: change per-domain node accounting interface
Hi,
On 13/12/2022 16:00, Juergen Gross wrote:
Rework the interface and the internals of the per-domain node
accounting:
- rename the functions to domain_nbentry_*() in order to better match
the related counter name
- switch from node pointer to domid as interface, as all nodes have the
owner filled in
The downside is now you have may place open-coding
"...->perms->p[0].id". IHMO this is making the code more complicated. So
can you introduce a few wrappers that would take a node and then convert
to the owner?
- use a common internal function for adding a value to the counter
For the transaction case add a helper function to get the list head
of the per-transaction changed domains, enabling to eliminate the
transaction_entry_*() functions.
Signed-off-by: Juergen Gross
---
tools/xenstore/xenstored_core.c| 22 ++---
tools/xenstore/xenstored_domain.c | 122 +++--
tools/xenstore/xenstored_domain.h | 10 +-
tools/xenstore/xenstored_transaction.c | 15 +--
tools/xenstore/xenstored_transaction.h | 7 +-
5 files changed, 72 insertions(+), 104 deletions(-)
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index f96714e1b8..61569cecbb 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -1459,7 +1459,7 @@ 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_entry_dec(conn, node);
+ domain_nbentry_dec(conn, node->perms.p[0].id);
/*
* It is not possible to easily revert the changes in a transaction.
@@ -1498,7 +1498,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_entry(conn) >= quota_nb_entry_per_domain) {
+ domain_nbentry(conn) >= quota_nb_entry_per_domain) {
ret = ENOSPC;
goto err;
}
@@ -1509,7 +1509,7 @@ static struct node *create_node(struct connection *conn,
const void *ctx,
/* Account for new node */
if (i->parent) {
- if (domain_entry_inc(conn, i)) {
+ if (domain_nbentry_inc(conn, i->perms.p[0].id)) {
destroy_node_rm(conn, i);
return NULL;
}
@@ -1662,7 +1662,7 @@ 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_entry_dec(conn, node);
+ domain_nbentry_dec(conn, node->perms.p[0].id);
return WALK_TREE_RM_CHILDENTRY;
}
@@ -1802,25 +1802,25 @@ static int do_set_perms(const void *ctx, struct
connection *conn,
return EPERM;
old_perms = node->perms;
- domain_entry_dec(conn, node);
+ domain_nbentry_dec(conn, node->perms.p[0].id);
node->perms = perms;
- if (domain_entry_inc(conn, node)) {
+ if (domain_nbentry_inc(conn, node->perms.p[0].id)) {
node->perms = old_perms;
/*
* This should never fail because we had a reference on the
* domain before and Xenstored is single-threaded.
*/
- domain_entry_inc(conn, node);
+ domain_nbentry_inc(conn, node->perms.p[0].id);
return ENOMEM;
}
if (write_node(conn, node, false)) {
int saved_errno = errno;
- domain_entry_dec(conn, node);
+ domain_nbentry_dec(conn, node->perms.p[0].id);
node->perms = old_perms;
/* No failure possible as above. */
- domain_entry_inc(conn, node);
+ domain_nbentry_inc(conn, node->perms.p[0].id);
errno = saved_errno;
return errno;
@@ -2392,7 +2392,7 @@ void setup_structure(bool live_update)
manual_node("/tool/xenstored", NULL);
manual_node("@releaseDomain", NULL);
manual_node("@introduceDomain", NULL);
- domain_entry_fix(dom0_domid, 5, true);
+ domain_nbentry_fix(dom0_domid, 5, true);
}
check_store();
@@ -3400,7 +3400,7 @@ void read_state_node(const void *ctx, const void *state)
if (write_node_raw(NULL, &key, node, true))
barf("write node error restoring node");
- if (domain_entry_inc(&conn, node))
+ if (domain_nbentry_inc(&conn, node->perms.p[0].id))
barf("node accounting error restoring node");
talloc_free(node);
diff --git a/tools/xenstore/xenstored_domain.c
b/tools/xenstore/xenstored_domain.c
index 3216119
