Re: [PATCH v2 08/13] tools/xenstore: add accounting trace support

[Julien Grall <julien@xxxxxxx>]

Hi Juergen,

On 20/01/2023 10:00, Juergen Gross wrote:
> Add a new trace switch "acc" and the related trace calls.
>
> The "acc" switch is off per default.
>
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
With one reamrk (see below):

Reviewed-by: Julien Grall <jgrall@xxxxxxxxxx>

> ---
>   tools/xenstore/xenstored_core.c   |  2 +-
>   tools/xenstore/xenstored_core.h   |  1 +
>   tools/xenstore/xenstored_domain.c | 10 ++++++++++
>   3 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> index 6ef60179fa..558ef491b1 100644
> --- a/tools/xenstore/xenstored_core.c
> +++ b/tools/xenstore/xenstored_core.c
> @@ -2746,7 +2746,7 @@ static void set_quota(const char *arg, bool soft)
>   
>   /* Sorted by bit values of TRACE_* flags. Flag is (1u << index). */
>   const char *const trace_switches[] = {
> -       "obj", "io", "wrl",
> +       "obj", "io", "wrl", "acc",
>         NULL
>   };
>   
> diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
> index 1f811f38cb..3e0734a6c6 100644
> --- a/tools/xenstore/xenstored_core.h
> +++ b/tools/xenstore/xenstored_core.h
> @@ -302,6 +302,7 @@ extern unsigned int trace_flags;
>   #define TRACE_OBJ     0x00000001
>   #define TRACE_IO      0x00000002
>   #define TRACE_WRL     0x00000004
> +#define TRACE_ACC      0x00000008
>   extern const char *const trace_switches[];
>   int set_trace_switch(const char *arg);
>   
> diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
> index b1e29edb7e..d461fd8cc8 100644
> --- a/tools/xenstore/xenstored_domain.c
> +++ b/tools/xenstore/xenstored_domain.c
> @@ -538,6 +538,12 @@ static struct domain *find_domain_by_domid(unsigned int 
> domid)
>         return (d && d->introduced) ? d : NULL;
>   }
>   
> +#define trace_acc(...)				\
The indentation of '\' looks odd.

> +do {                                           \
> +       if (trace_flags & TRACE_ACC)                \
> +               trace("acc: " __VA_ARGS__);   \
> +} while (0)
> +
>   int acc_fix_domains(struct list_head *head, bool update)
>   {
>         struct changed_domain *cd;
> @@ -602,6 +608,8 @@ static int acc_add_changed_dom(const void *ctx, struct 
> list_head *head,
>                 return 0;
>   
>   	errno = 0;
> +       trace_acc("local change domid %u: what=%u %d add %d\n", domid, what,
> +                 cd->acc[what], val);
>         cd->acc[what] += val;
>   
>   	return cd->acc[what];
> @@ -1114,6 +1122,8 @@ static int domain_acc_add(struct connection *conn, 
> unsigned int domid,
>                 return domain_acc_add_chk(d, what, ret, domid);
>         }
>   
> +	trace_acc("global change domid %u: what=%u %u add %d\n", domid, what,
> +                 d->acc[what], add);
>         d->acc[what] = domain_acc_add_chk(d, what, add, domid);
>   
>   	return d->acc[what];
Cheers,

--
Julien Grall