semaphore: Fix a few regressions by yamt · Pull Request #14755 · apache/nuttx

yamt · 2024-11-13T06:01:47Z

Summary

Regressions caused by signedness issues in
"sem: change sem wait to atomic operation".
(#14465)

An alternative would be to make these atomic macros propagate signedness using the typeof() GCC/clang extension. I'm not inclined to do so because typeof is not so portable though. As we can unlikely require "real" C11 atomics in the foreseeable future, maybe we should use a different set of names from C11 to avoid confusions.

Impact

Testing

esp32s3-devkit:smp ostest, with a few unrelated local changes.

Regressions caused by signedness issues in "sem: change sem wait to atomic operation". (apache#14465) An alternative would be to make these atomic macros propagate signedness using the typeof() GCC/clang extension. I'm not inclined to do so because typeof is not so portable though. As we can unlikely require "real" C11 atomics in the foreseeable future, maybe we should use a different set of names from C11 to avoid confusions.

nuttxpr · 2024-11-13T06:03:57Z

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary of the why and how, it lacks detail in several crucial sections:

Summary: Missing "What functional part of the code is being changed?". Be specific (e.g., semaphore implementation, specific files modified).
Impact: Completely empty. This is a critical section. Address all the points, even if the answer is "NO". Justify each "NO". For example: "Impact on user: NO (Existing applications using semaphores should not require modification)."
Testing: Insufficient. While a target is mentioned, there are no "before" and "after" logs. These logs are crucial for demonstrating the fix and justifying the change. "A few unrelated local changes" are also concerning; the PR should ideally be tested in isolation. The build host details are also missing.

Specifically, the author needs to:

Clarify the scope of changes: List the affected files/functions.
Complete the Impact section: Address every point, justifying each answer.
Provide valid testing logs: Include "before" and "after" logs demonstrating the issue and the fix. Remove any unrelated local changes before generating these logs. Specify the build host details.

Without these changes, the PR is incomplete and difficult to review properly.

xiaoxiang781216 · 2024-11-13T07:35:10Z

  /* Check our assumptions */

-  DEBUGASSERT(atomic_load(NXSEM_COUNT(sem)) <= 0);
+  DEBUGASSERT((int16_t)atomic_load(NXSEM_COUNT(sem)) <= 0);


why need cast int16_t if NXSEM_COUNT return atomic_short?

atomic_load returns uint64_t.

see

nuttx/include/nuttx/lib/stdatomic.h

Lines 75 to 81 in daab676

#define atomic_load_n(obj, type) \

(sizeof(*(obj)) == 1 ? __atomic_load_1(obj, type) : \

sizeof(*(obj)) == 2 ? __atomic_load_2(obj, type) : \

sizeof(*(obj)) == 4 ? __atomic_load_4(obj, type) : \

__atomic_load_8(obj, type))

#define atomic_load(obj) atomic_load_n(obj, __ATOMIC_RELAXED)

nuttx/include/nuttx/lib/stdatomic.h

Lines 201 to 204 in daab676

uint8_t __atomic_load_1(FAR const volatile void *ptr, int memorder);

uint16_t __atomic_load_2(FAR const volatile void *ptr, int memorder);

uint32_t __atomic_load_4(FAR const volatile void *ptr, int memorder);

uint64_t __atomic_load_8(FAR const volatile void *ptr, int memorder);

but the spec requires it return the same base type:
https://en.cppreference.com/w/c/atomic/atomic_load
we should fix the implementation instead. @crafcat7

IMO, we should not pretend to have generic selection.
it's simpler to use concrete-type apis like, say, nx_atomic_load_int16.

@crafcat7 please fix this ASAP.

We can consider whether there is a better way to reorganize arch_atomic & nuttx/stdatomic.c so that they can directly go to the atomic processing of the corresponding type, in other words, it can return the same result type as the input type parameter.
This work should take some time.

IMO, we should not pretend to have generic selection. it's simpler to use concrete-type apis like, say, nx_atomic_load_int16.

Yes, but it's this is the standard defined prototype:(

my suggestion is to use nuttx-specific, non-standard prototype.

besides that, we can provide c11 stdatomic to user applications where possible.
but our own code (eg. semaphore implementation) should not use it, IMO.

This work should take some time.

that's my expection too.

in the meantime, IMO, we should make band-aid fixes (like this PR) or a revert.

Ok, let's limit the kernel to use only one type of atomic type, so we can provide a compatible implementation when the compiler doesn't provide the atomic operation. @zyfeier @crafcat7

i'd suggest to revert the change in question for now because it would take some time to fix regressions: #14804

So that it can be used in more situations. The primary motivation here is to avoid crashes introduced by apache#14722. Tested: - esp32-devkitc:wifi_smp (smp) - esp32s3-devkit:smp (ostest, smp) (with apache#14755)

This reverts commit befe298. Because a few regressions have been reported and it likely will take some time to fix them: * for some configurations, semaphore can be used on the special memory region, where atomic access is not available. cf. apache#14625 * include/nuttx/lib/stdatomic.h is not compatible with the C11 semantics, which the change in question relies on. cf. apache#14755

yamt · 2024-11-15T05:53:13Z

i marked this draft because i'm now inclined to think it's simpler to make a revert. #14804

This reverts commit befe298. Because a few regressions have been reported and it likely will take some time to fix them: * for some configurations, semaphore can be used on the special memory region, where atomic access is not available. cf. #14625 * include/nuttx/lib/stdatomic.h is not compatible with the C11 semantics, which the change in question relies on. cf. #14755

This reverts commit befe298. Because a few regressions have been reported and it likely will take some time to fix them: * for some configurations, semaphore can be used on the special memory region, where atomic access is not available. cf. apache#14625 * include/nuttx/lib/stdatomic.h is not compatible with the C11 semantics, which the change in question relies on. cf. apache#14755

github-actions Bot added Area: OS Components OS Components issues Size: S The size of the change in this PR is small labels Nov 13, 2024

yamt force-pushed the fix-semaphore-regression branch from fdafc8e to ee3f119 Compare November 13, 2024 06:02

yamt mentioned this pull request Nov 13, 2024

[BUG] ostest fails without returning the expected exit code #14749

Open

1 task

xiaoxiang781216 reviewed Nov 13, 2024

View reviewed changes

yamt mentioned this pull request Nov 13, 2024

sched/semaphore: change semcount type to int #14625

Closed

yamt mentioned this pull request Nov 13, 2024

Reapply "SYSLOG_DEFAULT: wrap up_putc/up_nputs calls with critical section" with a fix #14761

Merged

yamt mentioned this pull request Nov 15, 2024

Revert "sem: change sem wait to atomic operation" #14804

Merged

yamt marked this pull request as draft November 15, 2024 05:52

xiaoxiang781216 closed this Nov 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

semaphore: Fix a few regressions#14755

semaphore: Fix a few regressions#14755
yamt wants to merge 1 commit into
apache:masterfrom
yamt:fix-semaphore-regression

yamt commented Nov 13, 2024 •

edited

Loading

Uh oh!

nuttxpr commented Nov 13, 2024

Uh oh!

xiaoxiang781216 Nov 13, 2024 •

edited

Loading

Uh oh!

yamt Nov 13, 2024

Uh oh!

yamt Nov 13, 2024

Uh oh!

xiaoxiang781216 Nov 13, 2024 •

edited

Loading

Uh oh!

yamt Nov 13, 2024

Uh oh!

crafcat7 Nov 14, 2024 •

edited

Loading

Uh oh!

yamt Nov 14, 2024

Uh oh!

yamt Nov 14, 2024

Uh oh!

xiaoxiang781216 Nov 15, 2024 •

edited

Loading

Uh oh!

yamt Nov 15, 2024

Uh oh!

yamt commented Nov 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	#define atomic_load_n(obj, type) \
	(sizeof(*(obj)) == 1 ? __atomic_load_1(obj, type) : \
	sizeof(*(obj)) == 2 ? __atomic_load_2(obj, type) : \
	sizeof(*(obj)) == 4 ? __atomic_load_4(obj, type) : \
	__atomic_load_8(obj, type))

	#define atomic_load(obj) atomic_load_n(obj, __ATOMIC_RELAXED)

	uint8_t __atomic_load_1(FAR const volatile void *ptr, int memorder);
	uint16_t __atomic_load_2(FAR const volatile void *ptr, int memorder);
	uint32_t __atomic_load_4(FAR const volatile void *ptr, int memorder);
	uint64_t __atomic_load_8(FAR const volatile void *ptr, int memorder);

Conversation

yamt commented Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Impact

Testing

Uh oh!

nuttxpr commented Nov 13, 2024

Uh oh!

xiaoxiang781216 Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yamt Nov 13, 2024

Choose a reason for hiding this comment

Uh oh!

yamt Nov 13, 2024

Choose a reason for hiding this comment

Uh oh!

xiaoxiang781216 Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yamt Nov 13, 2024

Choose a reason for hiding this comment

Uh oh!

crafcat7 Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yamt Nov 14, 2024

Choose a reason for hiding this comment

Uh oh!

yamt Nov 14, 2024

Choose a reason for hiding this comment

Uh oh!

xiaoxiang781216 Nov 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yamt Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

yamt commented Nov 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yamt commented Nov 13, 2024 •

edited

Loading

xiaoxiang781216 Nov 13, 2024 •

edited

Loading

xiaoxiang781216 Nov 13, 2024 •

edited

Loading

crafcat7 Nov 14, 2024 •

edited

Loading

xiaoxiang781216 Nov 15, 2024 •

edited

Loading