diff mbox series

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

Message ID 20230508114754.31514-11-jgross@suse.com (mailing list archive)
State Superseded
Headers show
Series tools/xenstore: rework internal accounting | expand

Commit Message

Jürgen Groß May 8, 2023, 11:47 a.m. UTC 
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(-)

Comments

Julien Grall May 9, 2023, 6:50 p.m. UTC | #1 
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,
diff mbox series

Patch

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;
 }