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

[Juergen Gross <jgross@xxxxxxxx>]

On 20.02.23 23:50, Julien Grall wrote:
> Hi Juergen,
>
> On 20/01/2023 10:00, 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@xxxxxxxx>
> > ---
> >   tools/xenstore/xenstored_core.c   |  5 +++
> >   tools/xenstore/xenstored_core.h   |  3 ++
> >   tools/xenstore/xenstored_domain.c | 64 +++++++++++++++++++++++++++----
> >   tools/xenstore/xenstored_domain.h |  5 ++-
> >   4 files changed, 68 insertions(+), 9 deletions(-)
> >
> > diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> > index 27dfbe9593..2d10cacf35 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);
> >   }
> > @@ -1060,6 +1063,7 @@ void send_reply(struct connection *conn, enum 
> > xsd_sockmsg_type type,
> >       }
> >       conn->in = NULL;
> > +    acc_commit(conn);
> AFAIU, if send_reply() is called then we would need to commit the accounting 
> even if we can't send the reply (i.e. send_reply()). So shouldn't this be call 
> right at the beginning of send_reply()?
Oh, indeed. Good catch!

> >       /* Update relevant header fields and fill in the message body. */
> >       bdata->hdr.msg.type = type;
> > @@ -2195,6 +2199,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 f459c5aabb..33105dba8f 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[];
> Is this actually worth it? How much memory would we save?
Hmm, true. While being more generic, the saved memory is actually zero,
as ACC_TR_N and ACC_REQ_N have the same value. And even if they should
differ in future, we can just use the higher of both values here.

> >   };
> >   static struct hashtable *domhash;
> > @@ -577,6 +577,7 @@ static struct changed_domain 
> > *acc_find_changed_domain(struct list_head *head,
> >   static struct changed_domain *acc_get_changed_domain(const void *ctx,
> >                                struct list_head *head,
> > +                             enum accitem acc_sz,
> >                                unsigned int domid)
> >   {
> >       struct changed_domain *cd;
> > @@ -585,7 +586,7 @@ static struct changed_domain *acc_get_changed_domain(const 
> > void *ctx,
> >       if (cd)
> >           return cd;
> > -    cd = talloc_zero(ctx, struct changed_domain);
> > +    cd = talloc_zero_size(ctx, sizeof(*cd) + acc_sz * sizeof(cd->acc[0]));
> >       if (!cd)
> >           return NULL;
> > @@ -596,13 +597,13 @@ static struct changed_domain 
> > *acc_get_changed_domain(const void *ctx,
> >   }
> >   static int acc_add_changed_dom(const void *ctx, struct list_head *head,
> > -                   enum accitem what, int val, unsigned int domid)
> > +                   enum accitem acc_sz, 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);
> > +    assert(what < acc_sz);
> > +    cd = acc_get_changed_domain(ctx, head, acc_sz, domid);
> >       if (!cd)
> >           return 0;
> > @@ -1071,6 +1072,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;
> > @@ -1091,10 +1093,26 @@ 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, ACC_REQ_N,
> > +                      what, add, domid);
> > +        return errno ? -1 : domain_acc_add_chk(d, what, ret, domid);
> > +    }
> > +
> >       if (conn && conn->transaction && what < ACC_TR_N) {
> >           head = transaction_get_changed_domains(conn->transaction);
> > -        ret = acc_add_changed_dom(conn->transaction, head, what,
> > -                      add, domid);
> > +        ret = acc_add_changed_dom(conn->transaction, head, ACC_TR_N,
> > +                      what, add, domid);
> >           if (errno) {
> >               fail_transaction(conn->transaction);
> >               return -1;
> > @@ -1107,6 +1125,36 @@ 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))) {
> NIT: You could use list_for_each_safe();
Yes, at the cost of an additional variable.

> > +        list_del(&cd->list);
> > +        talloc_free(cd);
> > +    }
> > +}
> > +
> > +void acc_commit(struct connection *conn)
> > +{
> > +    struct changed_domain *cd;
> > +    struct buffered_data *in = conn->in;
> > +    enum accitem what;
> > +
> > +    conn->in = NULL; /* Avoid recursion. */
> I am not sure I understand this comment. Can you provide more details where the 
> recursion would happen?
domain_acc_add() might do temporary accounting if conn->in isn't NULL.

> > +    while ((cd = list_top(&conn->acc_list, struct changed_domain, list))) {
> NIT: You could use list_for_each_safe();
Like above.

> > +        list_del(&cd->list);
> > +        for (what = 0; what < ACC_REQ_N; what++)
> There is a chance that some compiler will complain about this line because 
> ACC_REQ_N = 0. So this would always be true. Therefore...
Huh? It would always be false.

> > +            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 8259c114b0..cd85bd0cad 100644
> > --- a/tools/xenstore/xenstored_domain.h
> > +++ b/tools/xenstore/xenstored_domain.h
> > @@ -20,7 +20,8 @@
> >   #define _XENSTORED_DOMAIN_H
> >   enum accitem {
> > -    ACC_NODES,
> > +    ACC_REQ_N,       /* Number of elements per request and domain. */
> > +    ACC_NODES = ACC_REQ_N,
> I would bring forward the change in patch #5. I mean:
>
> ACC_NODES,
> ACC_REQ_N
I'm not sure this is a good idea. This would activate the temporary
accounting for nodes, but keeping the error handling active. I'd rather
activate temporary accounting for nodes together with removing the
accounting correction in the error handling.

> >       ACC_TR_N,        /* Number of elements per transaction and domain. */
> >       ACC_N = ACC_TR_N /* Number of elements per domain. */
> >   };
> This enum is getting extremely confusing. There are many "number of elements per 
> ... domain". Can you clarify?
I will add some more comments to the header. Would you like it better to have
the limits indented more? something like:

enum accitem {
        ACC_NODES,
                /* Number of elements per request and domain. */
                ACC_REQ_N,
                /* Number of elements per transaction and domain. */
                ACC_TR_N = ACC_REQ_N,
        ACC_WATCH = ACC_TR_N,
        ACC_OUTST,
        ACC_MEM,
        ACC_TRANS,
        ACC_TRANSNODES,
        ACC_NPERM,
        ACC_PATHLEN,
        ACC_NODESZ,
                /* Number of elements per domain. */
                ACC_N
};


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature