Re: [PATCH v3 15/34] xen/riscv: introduce atomic.h

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.01.2024 10:23, Oleksii wrote:
> On Tue, 2024-01-23 at 14:30 +0100, Jan Beulich wrote:
>> On 23.01.2024 13:24, Oleksii wrote:
>>> On Tue, 2024-01-23 at 11:30 +0100, Jan Beulich wrote:
>>>> On 23.01.2024 11:21, Oleksii wrote:
>>>>> On Mon, 2024-01-22 at 17:56 +0100, Jan Beulich wrote:
>>>>>> On 22.12.2023 16:12, Oleksii Kurochko wrote:
>>>>>>> --- /dev/null
>>>>>>> +++ b/xen/arch/riscv/include/asm/fence.h
>>>>>>> @@ -0,0 +1,13 @@
>>>>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>>>>> +#ifndef _ASM_RISCV_FENCE_H
>>>>>>> +#define _ASM_RISCV_FENCE_H
>>>>>>> +
>>>>>>> +#ifdef CONFIG_SMP
>>>>>>> +#define RISCV_ACQUIRE_BARRIER          "\tfence r , rw\n"
>>>>>>> +#define RISCV_RELEASE_BARRIER          "\tfence rw,  w\n"
>>>>>>> +#else
>>>>>>> +#define RISCV_ACQUIRE_BARRIER
>>>>>>> +#define RISCV_RELEASE_BARRIER
>>>>>>> +#endif
>>>>>>
>>>>>> Do you really care about the !SMP case? On x86 at least we
>>>>>> stopped
>>>>>> special-
>>>>>> casing that configuration many years ago (the few cases where
>>>>>> for
>>>>>> typically
>>>>>> build reasons it matters, using CONFIG_NR_CPUS is
>>>>>> sufficient). If
>>>>>> you
>>>>>> care
>>>>>> about it, there needs to be somewhere you actually #define
>>>>>> CONFIG_SMP.
>>>>> Can't we use instead of CONFIG_SMP - CONFIG_NR_CPUS?
>>>>
>>>> You can. Question is whether there's a point in doing so. Do you
>>>> expect people to actually want to run Xen on single-CPU systems?
>>>> They're generally not overly well suited for virtualization ...
>>> Just to clarify.
>>>
>>> Do you mean physically single based CPU?
>>> Then I don't expect to run Xen on such systems and it is not
>>> nesessary
>>> to define *_BARRIER in this case. Should we have to add build error
>>> notification that we don't support single-CPU systems in this
>>> header?
>>>
>>> If you are speaking about we have ,let it be, 4 CPUs and only 1 CPU
>>> is
>>> currently supported by Xen then it still makes sense.
>>
>> No, that's still not what I mean. The question is: Is it useful for
>> you
>> to _special case_ the NR_CPUS=1 case? Or is it instead simpler to
>> handle
>> NR_CPUS=1 the same as NR_CPUS>1 (accepting less than ideal
>> performance,
>> on the basis that in reality nobody's expected to use such in
>> production
>> anyway)?
> NR_CPUS=1 sometimes is useful for debugging. At least, at the start I
> used that several times, but ITBO I don't remember when I used that
> case after SMP support was added and context_switch() was fixed.

And "sometimes is useful for debugging" warrants introducing special
cases? I've not suggested disallowing that configuration. I'm merely
asking whether it isn't easier to have the barriers there at all
times. Just like on x86 we now leave the LOCK prefixes in place at
all times.

> Probably, I misunderstand the real idea of NR_CPUS. Does NR_CPUS
> represent a number of logical CPUs which can be different from physical
> amount of CPU?

No.

Jan