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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Wed, 2024-01-24 at 12:19 +0100, Jan Beulich wrote:
> 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.
I misunderstood your initial suggestion. In this case we can always
have the barriers. I'll drop then #ifdef CONFIG_SMP.

Thanks for clarification.

~ Oleksii
> 
> > 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