Re: [PATCH 01/12] tools/xenstore: manage per-transaction domain accounting data in an array

[Julien Grall <julien@xxxxxxx>]

Hi Juergen,

On 01/11/2022 16:04, Juergen Gross wrote:
> In order to prepare keeping accounting data in an array instead of
> using independent fields, switch the struct changed_domain accounting
> data to that scheme, for now only using an array with one element.
>
> In order to be able to extend this scheme add the needed indexing enum
> to xenstored_domain.h.
>
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> ---
>   tools/xenstore/xenstored_domain.c | 17 +++++++++--------
>   tools/xenstore/xenstored_domain.h |  5 +++++
>   2 files changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/tools/xenstore/xenstored_domain.c 
> b/tools/xenstore/xenstored_domain.c
> index 582b78b0ee..399fc920bb 100644
> --- a/tools/xenstore/xenstored_domain.c
> +++ b/tools/xenstore/xenstored_domain.c
> @@ -99,8 +99,8 @@ struct changed_domain
>         /* Identifier of the changed domain. */
>         unsigned int domid;
>   
> -	/* Amount by which this domain's nbentry field has changed. */
> -       int nbentry;
> +       /* Accounting data. */
> +       int acc[ACC_TR_N];
>   };
>   
>   static struct hashtable *domhash;
> @@ -558,7 +558,7 @@ int acc_fix_domains(struct list_head *head, bool update)
>         int cnt;
>   
>   	list_for_each_entry(cd, head, list) {
> -               cnt = domain_nbentry_fix(cd->domid, cd->nbentry, update);
> +               cnt = domain_nbentry_fix(cd->domid, cd->acc[ACC_NODES], update);
>                 if (!update) {
>                         if (cnt >= quota_nb_entry_per_domain)
>                                 return ENOSPC;
> @@ -603,8 +603,8 @@ static struct changed_domain *acc_get_changed_domain(const 
> void *ctx,
>         return cd;
>   }
>   
> -static int acc_add_dom_nbentry(const void *ctx, struct list_head *head, int val,
> -                              unsigned int domid)
> +static int acc_add_changed_dom(const void *ctx, struct list_head *head,
> +                              unsigned int what, int val, unsigned int domid)
AFAIU "what" will be a value from the enum. So let's do some hardening 
and use the type "enum ...".
>   {
>         struct changed_domain *cd;
>   
> @@ -613,9 +613,9 @@ static int acc_add_dom_nbentry(const void *ctx, struct list_head *head, int val,
>                 return 0;
>   
>   	errno = 0;
> -       cd->nbentry += val;
I would consider to adding an assert() (or possibly runtime check) to 
confirm the 'what' is smaller than the array size.
> +       cd->acc[what] += val;
>   
> -	return cd->nbentry;
> +       return cd->acc[what];
>   }
>   
>   static void domain_conn_reset(struct domain *domain)
> @@ -1076,7 +1076,8 @@ static int domain_nbentry_add(struct connection *conn, 
> unsigned int domid,
>   
>   	if (conn && conn->transaction) {
>                 head = transaction_get_changed_domains(conn->transaction);
> -               ret = acc_add_dom_nbentry(conn->transaction, head, add, domid);
> +               ret = acc_add_changed_dom(conn->transaction, head, ACC_NODES,
> +                                         add, domid);
>                 if (errno) {
>                         fail_transaction(conn->transaction);
>                         return -1;
> diff --git a/tools/xenstore/xenstored_domain.h 
> b/tools/xenstore/xenstored_domain.h
> index dc4660861e..124b9e1b1e 100644
> --- a/tools/xenstore/xenstored_domain.h
> +++ b/tools/xenstore/xenstored_domain.h
> @@ -19,6 +19,11 @@
>   #ifndef _XENSTORED_DOMAIN_H
>   #define _XENSTORED_DOMAIN_H
>   
> +enum {
> +       ACC_NODES,
> +       ACC_TR_N        /* Number of elements per transaction and domain. */
> +};
> +
>   void handle_event(void);
>   
>   void check_domains(void);
Cheers,

--
Julien Grall