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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.08.2024 15:29, oleksii.kurochko@xxxxxxxxx wrote:
> On Thu, 2024-08-15 at 11:02 +0200, Jan Beulich wrote:
>> 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.
> I thought that NR_CPUS tells me how many CPU the system has.

Oh, no, that not what it says (and it really can't, being a compile time
constant) - it says how many CPUs the specific build of Xen is going to
support at most.

Jan