Re: [PATCH] xen: make multicall debug boot time selectable

[Jürgen Groß <jgross@xxxxxxxx>]

On 06.07.24 00:36, boris.ostrovsky@xxxxxxxxxx wrote:
> On 7/3/24 7:56 AM, Juergen Gross wrote:
>
> >   #define MC_BATCH    32
> > -#define MC_DEBUG    0
> > -
> >   #define MC_ARGS        (MC_BATCH * 16)
> >   struct mc_buffer {
> >       unsigned mcidx, argidx, cbidx;
> >       struct multicall_entry entries[MC_BATCH];
> > -#if MC_DEBUG
> > -    struct multicall_entry debug[MC_BATCH];
> > -    void *caller[MC_BATCH];
> > -#endif
> >       unsigned char args[MC_ARGS];
> >       struct callback {
> >           void (*fn)(void *);
> > @@ -50,13 +46,84 @@ struct mc_buffer {
> >       } callbacks[MC_BATCH];
> >   };
> > +struct mc_debug_data {
> > +    struct multicall_entry debug[MC_BATCH];
> 'entries'? It's a mc_debug_data's copy of mc_buffer's entries.
Yes, this is better.

> Also, would it be better to keep these fields as a struct of scalars and instead 
> have the percpu array of this struct? Otherwise there is a whole bunch of 
> [MC_BATCH] arrays, all of them really indexed by the same value. (And while at 
> it, there is no reason to have callbacks[MC_BATCH] sized like that -- it has 
> nothing to do with batch size and can probably be made smaller)
As today the mc_buffer's entries are copied via a single memcpy(), there
are 3 options:

- make mc_debug_data a percpu pointer to a single array, requiring to
  copy the mc_buffer's entries in a loop

- let struct mc_debug_data contain two arrays (entries[] and struct foo {}[],
  with struct foo containing the other pointers/values)

- keep the layout as in my patch

Regarding the callbacks: I think the max number of callbacks is indeed MC_BATCH,
as for each batch member one callback might be requested. So I'd rather keep it
the way it is today.

> > +    void *caller[MC_BATCH];
> > +    size_t argsz[MC_BATCH];
> > +};
> > +
> >   static DEFINE_PER_CPU(struct mc_buffer, mc_buffer);
> > +static struct mc_debug_data __percpu *mc_debug_data;
> > +static struct mc_debug_data mc_debug_data_early __initdata;
> How about (I think this should work):
>
> static struct mc_debug_data __percpu *mc_debug_data __refdata = 
> &mc_debug_data_early;
> Then you won't need get_mc_debug_ptr().
I like this idea.


Juergen