On 2023/10/28 20:49, Masami Hiramatsu (Google) wrote:

Hi Wuqiang,

On Thu, 26 Oct 2023 19:05:51 +0800
"wuqiang.matt" [off-list ref] wrote:

quoted

On 2023/10/26 16:46, Arnd Bergmann wrote:

quoted

On Thu, Oct 26, 2023, at 09:39, wuqiang.matt wrote:

quoted

arch_cmpxchg[64]_local() are not defined for openrisc. So implement
them with generci_cmpxchg[64]_local, advised by Masami Hiramatsu.

Closes:
https://lore.kernel.org/linux-trace-kernel/169824660459.24340.14614817132696360531.stgit@devnote2 (local)
Closes:
https://lore.kernel.org/oe-kbuild-all/202310241310.Ir5uukOG-lkp@intel.com (local)

Signed-off-by: wuqiang.matt <wuqiang.matt@bytedance.com>

I think on architectures that have actual atomics, you
generally want to define this to be the same as arch_cmpxchg()
rather than the generic version.

It depends on the relative cost of doing one atomic compared
to an irq-disable/enable pair, but everyone else went with
the former if they could. The exceptions are armv4/armv5,
sparc32 and parisc, which don't have a generic cmpxchg()
or similar operation.

Sure, better native than the generic. I'll try to collect more
insights before next move.

So I will temporally remove the last change (use arch_cmpxchg_local
in objpool) until these series are rewritten with arch native code,
so that the next release will not break the kernel build.

Ok, it's fine to me. Thank you.

But this must be fixed because arch_cmpxchg_local() is required
for each arch anyway.

Yes. I'm working on the new update for arc/openrisc/hexagon. It would
be better resolve this issue first, then consider the objpool update
of using arch_cmpxchg_local.

quoted

quoted

You could do the thing that sparc64 and xtensa do, which
use the native cmpxchg for supported word sizes but the
generic version for 1- and 2-byte swaps, but that has its
own set of problems if you end up doing operations on both
the entire word and a sub-unit of the same thing.

Thank you for pointing out this. I'll do some research on these
implementations.

arc also has the LL-SC instruction but depends on the core feature,
so I think we can use it.

Right. The arc processor does have the CONFIG_ARC_HAS_LLSC option, but
I doubt the correctness of arch_cmpxchg_relaxed and arch_cmpxchg:

arch_cmpxchg_relaxed:
...
         switch(sizeof((_p_))) {
         case 4:
....

arch_cmpxchg:
...
	BUILD_BUG_ON(sizeof(_p_) != 4);		
...

_p is the address pointer, so I'm thinking it's a typo but I couldn't
yet confirm. There is not much about arc processors in the web :(

Thank you,

quoted

quoted

      Arnd

Regards,
wuqiang