wqueue remove enter_critical_section

[hujun260]

Summary

we use small lock to replace enter_critical_section to avoid busywait

reason:
We decouple semcount from business logic
by using an independent counting variable,
which allows us to remove critical sections in many cases.

Impact

work queue

Testing

Build Host:

OS: Ubuntu 20.04
CPU: x86_64
Compiler: GCC 9.4.0
Configuring NuttX and compile:
$ ./tools/configure.sh -l qemu-armv8a:nsh_smp
$ make
Running with qemu
$ qemu-system-aarch64 -cpu cortex-a53 -smp 4 -nographic
-machine virt,virtualization=on,gic-version=3
-net none -chardev stdio,id=con,mux=on -serial chardev:con
-mon chardev=con,mode=readline -kernel ./nuttx

[nuttxpr]

[Experimental Bot, please feedback here]

No, this PR summary does not adequately meet the NuttX requirements. Here's why and how to fix it:

Missing Information in Summary:

• What functional part of the code is being changed? While it mentions "work queue," this is too vague. Specify the exact files, functions, or modules affected (e.g., sched/workqueue.c, work_queue(), etc.).
• How does the change exactly work (what will change and how)? Saying "use small lock to replace enter_critical_section to avoid busywait" is a good start, but needs more detail. What kind of "small lock"? A mutex? A spinlock? Explain the mechanism and why it avoids busywaiting. Provide a before/after code snippet if possible.
• Related Issues: Are there any related NuttX or NuttX Apps issues this PR addresses? If so, provide links.

Missing Information in Impact:

• All Impact Sections are Incomplete: Simply saying "work queue" isn't enough. Answer ALL the questions with "YES" or "NO" and then provide details for any "YES" answers. For example:
  • Is new feature added? NO (presumably)
  • Is existing feature changed? YES (explain how the work queue behavior changes)
  • Impact on user: NO/YES (will user code using work queues need to change?)
  • Impact on build: NO/YES (are there any new configuration options or dependencies?)
  • Impact on hardware: NO/YES (does this affect specific architectures or boards?)
  • Impact on documentation: YES (at least the code comments should be updated; possibly also the official documentation) Explain what documentation changes are needed.
  • Impact on security: NO/YES (does changing the locking mechanism have any security implications? Consider race conditions, deadlocks, etc.)
  • Impact on compatibility: NO/YES (does this break compatibility with any existing drivers or applications?)

Missing Information in Testing:

• Target(s): While you mention qemu-armv8a:nsh_smp, be more specific. Which QEMU version? Which board configuration within qemu-armv8a?
• Testing logs before change: Provide actual logs demonstrating the problem this PR fixes. For example, show the busywaiting behavior.
• Testing logs after change: Provide logs demonstrating the improved behavior after applying this PR. Show that the busywaiting is eliminated or reduced. Quantify the improvement if possible (e.g., "reduced CPU usage by X%").

Example of a better Summary:

This PR replaces the use of enter_critical_section() with a lightweight mutex in the work queue implementation (specifically in sched/workqueue.c within functions work_queue() and work_cancel()). Currently, the work queue uses critical sections, which can lead to busywaiting when multiple threads contend for the work queue. This change introduces a mutex to protect access to the work queue's internal state, allowing threads to block efficiently instead of busywaiting. This improves performance, especially in multi-core systems. This PR addresses NuttX Issue #1234.

Example of a better Impact section (hypothetical):

• Is new feature added? NO
• Is existing feature changed? YES, the work queue's internal locking mechanism is changed, improving performance under contention.
• Impact on user: NO, existing user code using the work queue API should not require modification.
• Impact on build: NO
• Impact on hardware: NO
• Impact on documentation: YES, the code comments in sched/workqueue.c will be updated to reflect the new locking mechanism.
• Impact on security: NO, the new locking mechanism is designed to prevent race conditions and deadlocks.
• Impact on compatibility: YES, This change might slightly affect timing in some edge cases where extremely precise timing with work queues was relied upon. However, this is expected to be negligible in most scenarios.

By providing complete and detailed information in your PR, you make it easier for reviewers to understand your changes and ensure they meet the project's standards. This will lead to a faster and smoother review process.

[acassis]

Note that some places said work_cancel() only returns success, these comments need to be fixed. i.e.:

      ret = work_cancel(LPWORK, &priv->cbwork);
      if (ret < 0)
        {
          /* NOTE: Currently, work_cancel only returns success */
  
          lcderr("ERROR: Failed to cancel work: %d\n", ret);
        }

[hujun260]

This PR requires #14578 to be merged first.

[xiaoxiang781216]

need skip line 167 if notifier is NULL

[xiaoxiang781216]

why need check?

[hujun260]

Previously, there was no need for additional checks mainly because work_timer_expiry was within a critical section, and the cancellation also took place within the same critical section. If it was canceled, the callback for work_timer_expiry would not be triggered. However, now that we have switched to using smaller locks and the cancellation point is outside the critical section, we need to add a check.

[xiaoxiang781216]

@hujun260 many comments aren't addressed in the new reversion yet.