Re: [PATCH v4 10/13] tools/xenstore: switch transaction accounting to generic accounting

[Juergen Gross <jgross@xxxxxxxx>]

On 02.05.23 21:19, Julien Grall wrote:
> Hi,
>
> On 05/04/2023 08:03, 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@xxxxxxxx>
> > ---
> >   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 | 12 +++++-------
> >   5 files changed, 28 insertions(+), 13 deletions(-)
> >
> > diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> > index 2d481fcad9..88c569b7d5 100644
> > --- a/tools/xenstore/xenstored_core.c
> > +++ b/tools/xenstore/xenstored_core.c
> > @@ -2083,7 +2083,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() && conn->ta_start_time == 0 &&
> NIT: I know there are some places in the code checking for conn->ta_start_time 
> == 0. But it feels like a better replacement to "conn->transaction_started" is 
> "list_empty(...)".
Fine with me.

> I agree this is going to be more expensive. But you are switching the 
> transaction accounting to a generic infrastructure which is pretty heavy compare 
> to a simple addition/substraction. So I think a "list_empty()" would be OK here.
> >           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);
> > @@ -2190,7 +2190,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 1caa60bb14..40bcc1dbfa 100644
> > --- a/tools/xenstore/xenstored_domain.c
> > +++ b/tools/xenstore/xenstored_domain.c
> > @@ -419,12 +419,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;
> > @@ -435,7 +433,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]);
> > @@ -1297,6 +1295,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 0d61bf4344..abc766f343 100644
> > --- a/tools/xenstore/xenstored_domain.h
> > +++ b/tools/xenstore/xenstored_domain.h
> > @@ -31,6 +31,7 @@ enum accitem {
> >       ACC_WATCH = ACC_TR_N,
> >       ACC_OUTST,
> >       ACC_MEM,
> > +    ACC_TRANS,
> >       ACC_N,            /* Number of elements per domain. */
> >   };
> > @@ -112,6 +113,9 @@ void domain_watch_dec(struct connection *conn);
> >   int domain_watch(struct connection *conn);
> >   void domain_outstanding_inc(struct connection *conn, unsigned int domid);
> >   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..1c14b8579a 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 */
> > @@ -505,9 +504,9 @@ int do_transaction_start(const void *ctx, struct 
> > connection *conn,
> >       list_add_tail(&trans->list, &conn->transaction_list);
> >       talloc_steal(conn, trans);
> >       talloc_set_destructor(trans, destroy_transaction);
> > -    if (!conn->transaction_started)
> > +    if (!conn->ta_start_time)
> I think it would make more sense to move this if just before the list_add_tail() 
> and use (list_empty()) for the checked. This would make the code more consistent...
Okay.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature