Re: [PATCH v4 4/7] xen/riscv: introduce functionality to work with CPU info

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.08.2024 10:55, oleksii.kurochko@xxxxxxxxx wrote:
> On Wed, 2024-08-14 at 17:22 +0200, Jan Beulich wrote:
>> On 14.08.2024 16:45, oleksii.kurochko@xxxxxxxxx wrote:
>>> On Tue, 2024-08-13 at 10:54 +0200, Jan Beulich wrote:
>>>> On 09.08.2024 18:19, Oleksii Kurochko wrote:
>>>>> --- /dev/null
>>>>> +++ b/xen/arch/riscv/smp.c
>>>>> @@ -0,0 +1,4 @@
>>>>> +#include <xen/smp.h>
>>>>> +
>>>>> +/* tp points to one of these per cpu */
>>>>> +struct pcpu_info pcpu_info[NR_CPUS];
>>>>
>>>> And they all need setting up statically? Is there a plan to make
>>>> this
>>>> dynamic (which could be recorded in a "fixme" in the comment)?
>>> I didn't plan to make allocation of this array dynamic. I don't
>>> expect
>>> that NR_CPUS will be big.
>>
>> What is this expectation of yours based on? Other architectures
>> permit
>> systems with hundreds or even thousands of CPUs; why would RISC-V be
>> different there?
> Based on available dev boards. ( what isn't really strong argument )
> 
> I checked other architectures and they are using static allocation too:
>    struct cpuinfo_x86 cpu_data[NR_CPUS];
>    
>    u32 x86_cpu_to_apicid[NR_CPUS] __read_mostly =
>       { [0 ... NR_CPUS-1] = BAD_APICID };
>    
>    ... /* Arm */
>    
>    struct cpuinfo_arm cpu_data[NR_CPUS];
> 
> I wanted to check to understand which one API should be used to
> allocate this array dynamically. xmalloc?

As of a few days ago xvmalloc() (or friends thereof), as long as ...

> And I am curious how I can use xmalloc() at this stage if page
> allocator (?) should be initialized what isn't true now.

... this happens late enough in the boot process. Indeed ...

> Or just to allocate pcpu_info only for boot cpu and for other then use
> xmalloc()?

... statically allocating space for the boot CPU only is another option.

>>> I can add "fixme" but I am not really
>>> understand what will be advantages if pcpu_info[] will be allocated
>>> dynamically.
>>
>> Where possible it's better to avoid static allocations, of which on
>> some systems only a very small part may be used. Even if you put
>> yourself
>> on the position that many take - memory being cheap - you then still
>> waste cache and TLB bandwidth. Furthermore as long as struct
>> pcpu_info
>> isn't big enough (and properly aligned) for two successive array
>> entries
>> to not share cache lines, you may end up playing cacheline ping-pong
>> when a CPU writes to its own array slot.
> Why the mentioned issues aren't work for dynamic memory? We still
> allocating memory for sizeof(pcpu_info) * NR_CPUS

Why NR_CPUS? At runtime you know how may CPUs the system has you're
running on. You only need to allocate as much then. Just like e.g.
dynamically allocated CPU mask variables (cpumask_var_t) deliberately
use less than NR_CPUS bits unless on really big iron.

Jan

> and when this
> allocated memory access it will go to cache, CPU/MMU still will use TLB
> for address translation for this region and without a proper alignment
> of pcpu_info size it still could be an issue with cache line sharing.
> 
> ~ Oleksii
>