[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2d9ffe19-1663-499c-9699-c13ab7a341ee@suse.com>
Date: Mon, 8 Jul 2024 11:04:56 +0200
From: Jürgen Groß <jgross@...e.com>
To: boris.ostrovsky@...cle.com, linux-kernel@...r.kernel.org, x86@...nel.org,
linux-doc@...r.kernel.org
Cc: Jonathan Corbet <corbet@....net>, Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
Dave Hansen <dave.hansen@...ux.intel.com>, "H. Peter Anvin" <hpa@...or.com>,
xen-devel@...ts.xenproject.org
Subject: Re: [PATCH] xen: make multicall debug boot time selectable
On 06.07.24 00:36, boris.ostrovsky@...cle.com 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
Powered by blists - more mailing lists