From: "wuqiang.matt" <wuqiang.matt@bytedance.com>
To: Vineet Gupta <vgupta@kernel.org>,
"Masami Hiramatsu (Google)" <mhiramat@kernel.org>
Cc: Arnd Bergmann <arnd@arndb.de>, Jonas Bonn <jonas@southpole.se>,
Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>,
Stafford Horne <shorne@gmail.com>, Ingo Molnar <mingo@kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
Andi Shyti <andi.shyti@linux.intel.com>,
Palmer Dabbelt <palmer@rivosinc.com>,
Andrzej Hajda <andrzej.hajda@intel.com>,
linux-trace-kernel@vger.kernel.org, mattwu@163.com,
linux-snps-arc@lists.infradead.org
Subject: Re: [PATCH 3/3] locking/atomic: openrisc: use generic_cmpxchg[64]_local for arch_cmpxchg[64]_local
Date: Thu, 2 Nov 2023 18:05:04 +0800 [thread overview]
Message-ID: <8511e096-365f-4625-88d1-affd498685e1@bytedance.com> (raw)
In-Reply-To: <3c7cc279-d5ea-4071-ba68-88563a6a0877@bytedance.com>
On 2023/11/2 17:30, wuqiang.matt wrote:
> On 2023/11/2 12:53, Vineet Gupta wrote:
>>
>>
>> On 10/29/23 20:41, wuqiang.matt wrote:
>>>>>>
>>>>>> 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 :(
>>>>> Hmm, indeed. This seems like a bug but it depends on the 'llock %0, [%1]'
>>>>> can take a 32bit address or 32bit data register. Usually it should
>>>>> check the size of data, but need to check with ISA manual.
>>>>>
>>>>> Vineet, can you check this suspicious bug?
>>>>
>>>> ARCv2 is a 32-bit ISA and LLOCK/SCOND work on 32-bit data.
>>>> So the pointers will be 32-bit anyways. Is the issue that pointer/cmpxchg
>>>> operation could be on a smaller data type ?
>>>
>>> For ARCv2 with CONFIG_ARC_HAS_LLSC, better add the data size checking and
>>> only permit 32bit data size. Even for 32-bit system, data should can be
>>> 64bit 'long long'.
>>
>> Right, makes sense. Can you send a patch ?
>
> Okay, I will.
>
>>>
>>> And In the case that CONFIG_ARC_HAS_LLSC is undefined, in arch_cmpxchg: the
>>> pointer size checking is unnecessary, since it's using spinlock internally:
>>
>> Correct, I had the same thought.
>>
>>>
>>> https://elixir.bootlin.com/linux/v6.6-rc7/source/arch/arc/include/asm/cmpxchg.h#L60:
>>> BUILD_BUG_ON(sizeof(_p_) != 4); \
>>> \
>>> /* \
>>> * spin lock/unlock provide the needed smp_mb() before/after \
>>> */ \
>>> atomic_ops_lock(__flags); \
>>> _prev_ = *_p_; \
>>> if (_prev_ == _o_) \
>>> *_p_ = _n_; \
>>> atomic_ops_unlock(__flags);
>>
>> Can you do that fix in same patch as well ?
>
> Sure.
>
>>> Another question about the naming: arch_cmpxchg_relaxed() implemented if
>>> CONFIG_ARC_HAS_LLSC is configured and arch_cmpxchg() defined for the rest.
>>> Are there any reasons for difference names ?
>>
>> Yes the atomics API consists of _relaxed, _acquire, _release and unadorned.
>> _relaxed is the most efficient, with rest having some extra barriers.
>> If arch provides _relaxed, generic code can create the rest - assuming arch
>> hardware can support the relaxed ones.
>
> Definitely Yes! The generic cmpxchg will prefer arch_cmpxchg_relaxed rather
> than arch_cmpxchg.
It should be arch_cmpxchg (if defined) rather than arch_cmpxchg_relaxed. I
just double checked with v6.6:
...
#if defined(arch_cmpxchg)
#define raw_cmpxchg arch_cmpxchg
#elif defined(arch_cmpxchg_relaxed)
#define raw_cmpxchg(...) \
__atomic_op_fence(arch_cmpxchg, __VA_ARGS__)
#else
extern void raw_cmpxchg_not_implemented(void);
#define raw_cmpxchg(...) raw_cmpxchg_not_implemented()
#endif
...
#if defined(arch_cmpxchg_relaxed)
#define raw_cmpxchg_relaxed arch_cmpxchg_relaxed
#elif defined(arch_cmpxchg)
#define raw_cmpxchg_relaxed arch_cmpxchg
#else
extern void raw_cmpxchg_relaxed_not_implemented(void);
#define raw_cmpxchg_relaxed(...) raw_cmpxchg_relaxed_not_implemented()
#endif
...
I made the mistake since the definitions of v5.2 confused me. The followings
are for v5.2, and these conditions are removed from v5.14:
...
#if !defined(arch_cmpxchg_relaxed) || defined(arch_cmpxchg)
#define cmpxchg(ptr, ...) \
({ \
typeof(ptr) __ai_ptr = (ptr); \
instrument_atomic_write(__ai_ptr, sizeof(*__ai_ptr)); \
arch_cmpxchg(__ai_ptr, __VA_ARGS__); \
})
#endif
...
#if defined(arch_cmpxchg_relaxed)
#define cmpxchg_relaxed(ptr, ...) \
({ \
typeof(ptr) __ai_ptr = (ptr); \
instrument_atomic_write(__ai_ptr, sizeof(*__ai_ptr)); \
arch_cmpxchg_relaxed(__ai_ptr, __VA_ARGS__); \
})
#endif
...
Sorry for the noise.
>> That is true for LLSC.
>>
>> However for !LLSC, spinlock versions already have full barriers due to
>> spinlock, so relaxed variant doesn't make sense.
>>
>>>
>>> As I checked, Synopsys has released 64bit ARC processors (HS66/HS68), but
>>> I don't know the status of Linux kernel support.
>>
>> Yes there's an internal ARC64 port running but it is not ready for
>> upstreaming yet.
>
> Thanks for the update.
>
>> -Vineet
>
> Regards,
> wuqiang
_______________________________________________
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
prev parent reply other threads:[~2023-11-02 10:05 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20231026073932.702197-1-wuqiang.matt@bytedance.com>
[not found] ` <1e3aba7d-89ac-4b62-840e-992527115a70@app.fastmail.com>
[not found] ` <66f5645c-f9f5-4c53-9503-a1f8470a9bee@bytedance.com>
[not found] ` <20231028214918.e32265f1dd2ef26fd9d2d1c2@kernel.org>
2023-10-28 16:40 ` [PATCH 3/3] locking/atomic: openrisc: use generic_cmpxchg[64]_local for arch_cmpxchg[64]_local wuqiang.matt
2023-10-29 3:26 ` Masami Hiramatsu
2023-10-30 2:22 ` Vineet Gupta
2023-10-30 3:41 ` wuqiang.matt
2023-11-02 4:53 ` Vineet Gupta
2023-11-02 9:30 ` wuqiang.matt
2023-11-02 10:05 ` wuqiang.matt [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8511e096-365f-4625-88d1-affd498685e1@bytedance.com \
--to=wuqiang.matt@bytedance.com \
--cc=andi.shyti@linux.intel.com \
--cc=andrzej.hajda@intel.com \
--cc=arnd@arndb.de \
--cc=jonas@southpole.se \
--cc=linux-snps-arc@lists.infradead.org \
--cc=linux-trace-kernel@vger.kernel.org \
--cc=mattwu@163.com \
--cc=mhiramat@kernel.org \
--cc=mingo@kernel.org \
--cc=palmer@rivosinc.com \
--cc=peterz@infradead.org \
--cc=shorne@gmail.com \
--cc=stefan.kristiansson@saunalahti.fi \
--cc=vgupta@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).