xiaoxiang781216 commented on code in PR #20043: URL: https://github.com/apache/nuttx/pull/20043#discussion_r3935540178
########## libs/libc/machine/arch_atomic64.c: ########## @@ -0,0 +1,287 @@ +/**************************************************************************** + * libs/libc/machine/arch_atomic64.c + * + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. The + * ASF licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the + * License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + * + ****************************************************************************/ + +/* 8 byte atomics are not lock free on every target. An arch may only have a + * 32 bit atomic instruction (TriCore swap.w/cmpswap.w for instance) and a + * toolchain without 64 bit support emits calls to the __atomic_*_8 helpers + * that would otherwise come from libatomic, which NuttX does not link. + * + * The helpers are implemented here on top of a single spinlock. A spinlock + * rather than a plain up_irq_save() is needed because disabling interrupts + * only excludes the local CPU: on SMP another CPU could still enter the same + * critical section and corrupt the 64 bit value. The interrupt state is + * still saved (spin_lock_irqsave) so that an ISR on this CPU cannot deadlock + * against a holder it interrupted. + * + * <arch/atomic.h> is deliberately not reused: its macros are built around + * the native word size and a 64 bit access would be silently truncated. All + * symbols are weak, so a toolchain or arch with a native 64 bit + * implementation still wins at link time. + */ + +/**************************************************************************** + * Included Files + ****************************************************************************/ + +#include <nuttx/config.h> + +#include <nuttx/compiler.h> +#include <nuttx/spinlock.h> + +#include <stdbool.h> +#include <stdint.h> + +/**************************************************************************** + * Private Data + ****************************************************************************/ + +/* Every 64 bit atomic serializes on this lock. The granularity is coarse, + * but 64 bit atomics are rare enough that a single lock is not a bottleneck. + */ + +static spinlock_t g_atomic64_lock = SP_UNLOCKED; Review Comment: should use irq_save/restore in UP, could share with 32bit implementation ########## libs/libc/machine/arch_atomic.c: ########## @@ -48,9 +48,6 @@ ARCH_ATOMIC_DEFINE(__atomic, uint16_t, 2) #ifdef ARCH_HAVE_ATOMIC_4 ARCH_ATOMIC_DEFINE(__atomic, uint32_t, 4) #endif -#ifdef ARCH_HAVE_ATOMIC_8 Review Comment: revert ########## libs/libc/machine/arch_atomic64.c: ########## @@ -0,0 +1,287 @@ +/**************************************************************************** + * libs/libc/machine/arch_atomic64.c + * + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. The + * ASF licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the + * License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + * + ****************************************************************************/ + +/* 8 byte atomics are not lock free on every target. An arch may only have a + * 32 bit atomic instruction (TriCore swap.w/cmpswap.w for instance) and a + * toolchain without 64 bit support emits calls to the __atomic_*_8 helpers + * that would otherwise come from libatomic, which NuttX does not link. + * + * The helpers are implemented here on top of a single spinlock. A spinlock + * rather than a plain up_irq_save() is needed because disabling interrupts + * only excludes the local CPU: on SMP another CPU could still enter the same + * critical section and corrupt the 64 bit value. The interrupt state is + * still saved (spin_lock_irqsave) so that an ISR on this CPU cannot deadlock + * against a holder it interrupted. + * + * <arch/atomic.h> is deliberately not reused: its macros are built around + * the native word size and a 64 bit access would be silently truncated. All + * symbols are weak, so a toolchain or arch with a native 64 bit + * implementation still wins at link time. + */ + +/**************************************************************************** + * Included Files + ****************************************************************************/ + +#include <nuttx/config.h> + +#include <nuttx/compiler.h> +#include <nuttx/spinlock.h> + +#include <stdbool.h> +#include <stdint.h> + +/**************************************************************************** + * Private Data + ****************************************************************************/ + +/* Every 64 bit atomic serializes on this lock. The granularity is coarse, + * but 64 bit atomics are rare enough that a single lock is not a bottleneck. + */ + +static spinlock_t g_atomic64_lock = SP_UNLOCKED; + +/**************************************************************************** + * Private Functions + ****************************************************************************/ + +static inline irqstate_t atomic64_lock(void) +{ + return spin_lock_irqsave(&g_atomic64_lock); Review Comment: let's inline the implementation too ########## arch/tricore/include/atomic.h: ########## @@ -112,6 +112,8 @@ ARCH_ATOMIC_FETCH_OP(prefix ## _fetch_or_ ## n, t, n, |) \ ARCH_ATOMIC_FETCH_OP(prefix ## _fetch_xor_ ## n, t, n, ^) +#define ARCH_HAVE_ATOMIC_4 Review Comment: why add here ########## libs/libc/machine/arch_atomic.c: ########## @@ -26,7 +26,7 @@ #include <nuttx/config.h> -#define ARCH_ATOMIC_SPECIFIER +#define ARCH_ATOMIC_SPECIFIER weak_function Review Comment: why need weak ########## include/nuttx/lib/arch_atomic.h: ########## @@ -255,20 +255,19 @@ static inline void atomic_unlock(irqstate_t flags) #define ARCH_HAVE_ATOMIC_1 #define ARCH_HAVE_ATOMIC_2 #define ARCH_HAVE_ATOMIC_4 -#define ARCH_HAVE_ATOMIC_8 Review Comment: let's keep, but provide the different lock/unlock for 64: 1. atomic for 64bit arch 2. irq save/restore for UP&32bit 3. spinlock for SMP&32bit ########## libs/libc/machine/arch_atomic64.c: ########## @@ -0,0 +1,287 @@ +/**************************************************************************** + * libs/libc/machine/arch_atomic64.c + * + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. The + * ASF licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the + * License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + * + ****************************************************************************/ + +/* 8 byte atomics are not lock free on every target. An arch may only have a + * 32 bit atomic instruction (TriCore swap.w/cmpswap.w for instance) and a + * toolchain without 64 bit support emits calls to the __atomic_*_8 helpers + * that would otherwise come from libatomic, which NuttX does not link. + * + * The helpers are implemented here on top of a single spinlock. A spinlock + * rather than a plain up_irq_save() is needed because disabling interrupts + * only excludes the local CPU: on SMP another CPU could still enter the same + * critical section and corrupt the 64 bit value. The interrupt state is + * still saved (spin_lock_irqsave) so that an ISR on this CPU cannot deadlock + * against a holder it interrupted. + * + * <arch/atomic.h> is deliberately not reused: its macros are built around + * the native word size and a 64 bit access would be silently truncated. All + * symbols are weak, so a toolchain or arch with a native 64 bit + * implementation still wins at link time. + */ + +/**************************************************************************** + * Included Files + ****************************************************************************/ + +#include <nuttx/config.h> + +#include <nuttx/compiler.h> +#include <nuttx/spinlock.h> + +#include <stdbool.h> +#include <stdint.h> + +/**************************************************************************** + * Private Data + ****************************************************************************/ + +/* Every 64 bit atomic serializes on this lock. The granularity is coarse, + * but 64 bit atomics are rare enough that a single lock is not a bottleneck. + */ + +static spinlock_t g_atomic64_lock = SP_UNLOCKED; + +/**************************************************************************** + * Private Functions + ****************************************************************************/ + +static inline irqstate_t atomic64_lock(void) +{ + return spin_lock_irqsave(&g_atomic64_lock); +} + +static inline void atomic64_unlock(irqstate_t flags) +{ + spin_unlock_irqrestore(&g_atomic64_lock, flags); +} + +/**************************************************************************** + * Pre-processor Definitions + ****************************************************************************/ + +#define ATOMIC64_STORE(func, t) \ Review Comment: let's reuse ATOMIC_DEFINE we can refine ATOMIC_DEFINE to accept the lock/unlock funciton -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
