hujun260 opened a new pull request, #21475:
URL: https://github.com/apache/nuttx/pull/21475
## Summary
* perf_gettime() folded the 64 bit clock from a 32 bit hardware
counter and an overflow count while holding a spinlock. That made
every user space caller enter the kernel, and the lock is taken on
the scheduler instrumentation path.
* This series tracks the overflow in the periodic watchdog instead and
lets readers sample the counter and the overflow count under a
seqlock, so a 64 bit value can be read from user space without a
system call.
* The series also adds the Kconfig plumbing to expose the ARMv7-A
cycle counter to user space, and fixes four pre-existing defects
found while build and run testing the result.
Commits:
1. `sched/clock: Read the perf watchdog timeout under the lock.`
2. `arch/arm: Expose up_perf_gettime() to user space on ARMv7-A.`
3. `arch: Gate the user-space perf counter behind
ARCH_PERF_EVENTS_USER_ACCESS.`
4. `sched/clock: Start the perf overflow watchdog from perf_setup().`
5. `sched/clock: Use the notrace spinlock in perf_gettime().`
6. `arch: Mark the perf counter readers noinstrument_function.`
7. `sched/clock: Track the perf counter overflow without a spinlock.`
## Impact
* Is new feature added? YES. `CONFIG_ARCH_PERF_EVENTS_USER_ACCESS`
lets an application read the 64 bit perf clock directly.
* Impact on user (will user need to adapt to change)? NO. The user
space symbol is still perf_gettime() with the same signature.
* Impact on build (will build process change)? NO, apart from the new
Kconfig option.
* Impact on hardware (will arch(s) / board(s) / driver(s) change)?
YES. arm_perf.c is now built into the user space blob on ARMv7-A,
and `struct perf_s` moved from `sched/clock/clock_perf.c` to
`include/nuttx/clock.h` so protected builds can reach it through
`struct userspace_data_s`.
* Impact on documentation (is update required / provided)? NO.
* Impact on security (any sort of implications)? NO new syscall and
no new privilege: the counter is already readable by the kernel, and
user space only gets read access to a per-system counter. The
`userspace_data_s` pointer is filled by the kernel at link time.
* Impact on compatibility (backward/forward/interoperability)? NO.
### Behaviour changes worth reviewer attention
* CONFIG_PERF_OVERFLOW_CORRECTION now depends on `!ARCH_64BIT` and
`default y`. On a 32 bit build with a perf counter the overflow
correction is on unless it is explicitly disabled.
* `CONFIG_ARCH_PERF_EVENTS_USER_ACCESS` defaults to `!BUILD_FLAT`, so
a protected or kernel build on a capable arch gets the user space
counter without touching its defconfig.
### Pre-existing defects fixed in the last commit
These were found by build and run testing this series; none of them is
introduced by it, but the series does not build or work without them.
1. `sched/Kconfig`: `PERF_OVERFLOW_CORRECTION` depended on
`SYSTEM_TIME64`, which no longer exists in the tree, so the symbol
was always `n` and the new `default y` could never take effect.
Measured with kconfiglib on a minimal Kconfig: with the
`SYSTEM_TIME64` term the value is `n`, without it `y`.
2. `include/nuttx/clock.h` included `<nuttx/percpu.h>`, which is not
present anywhere in the tree or in master, so every file that
includes clock.h failed to compile. Nothing in `struct perf_s`
uses a per-CPU variable, so the include is dropped.
3. `libs/libc/sched/Make.defs` and CMakeLists.txt listed
`clock_perf.c` twice, once unconditionally and once under
`ARCH_PERF_EVENTS_USER_ACCESS`. g_perf now lives in that file, so
dropping the unconditional entry alone gives
`undefined reference to 'g_perf'` in a flat build. The condition is
`PERF_OVERFLOW_CORRECTION || ARCH_PERF_EVENTS_USER_ACCESS`.
4. perf_gettime() sampled the hardware counter before reading
g_perf.last, so a wrap recorded by the watchdog between the two
reads was missed and the 64 bit value fell by 2^32. Reproduced, see
the testing section.
## Testing
Environment:
* Build host: Linux x86_64, `arm-none-eabi-gcc` from the Vela
prebuilts.
* Target: `qemu-armv7a:smp`, QEMU `virt`, Cortex-A7, 4 CPUs, TCG.
`qemu-armv7a` does not call `up_perf_init()`, so `PMCR.E` stays clear and
`up_perf_gettime()` returns a constant 0. To exercise the code under
test the local test build adds one call in the board bringup:
```c
#ifdef CONFIG_ARCH_PERF_EVENTS
up_perf_init((void *)(uintptr_t)1000000000);
#endif
```
This is **not** part of the pull request; it only makes the counter run
on this board. Without it the first test reports
`ERROR: counter did not advance (start=0 end=0)`, which is the stopped
counter case described in #21428.
Test configuration:
```
CONFIG_BUILD_FLAT=y
CONFIG_SMP=y
CONFIG_ARCH_PERF_EVENTS=y
CONFIG_PERF_OVERFLOW_CORRECTION=y
CONFIG_ARCH_PERF_COUNT_BITWIDTH=32
CONFIG_SCHED_CRITMONITOR=y
CONFIG_TESTING_OSTEST=y
```
### Test program
perf_seqlock.c (new, in the apps PR) is run from
`ostest`. It has two cases:
* **Test 1** busy loops for 500 ms measured with `CLOCK_MONOTONIC`,
then converts the perf_gettime() delta with perf_convert() and
checks that the converted interval is non-zero, below 1 s and above
100 ms. A counter that does not run fails here.
* **Test 2** starts 4 threads, each calling perf_gettime() 200000
times in a loop and checking that the value never decreases. A torn
or stale overflow fold shows up as a step of about -2^32.
### Testing logs before the fix
```
user_main: perf seqlock test
perf_seqlock_test: Starting
Test 1: counter advances over a 500 ms busy loop
500 ms busy loop -> 0.500 s
OK: converted interval is in range
Test 2: 4 threads sample the counter concurrently
perf_seqlock: thread 0: counter went backwards at iteration 129405:
prev=157879718338 curr=153584751512
perf_seqlock: thread 1: counter went backwards at iteration 127396:
prev=157879718530 curr=153584751485
perf_seqlock_test: FAILED (2 error(s))
```
The step is `157879718338 - 153584751512 = 4294967826`, i.e. 2^32 minus
the counter advance between the two samples. Two of four threads hit it
within 200000 iterations.
### Testing logs after the fix
```
user_main: perf seqlock test
perf_seqlock_test: Starting
Test 1: counter advances over a 500 ms busy loop
500 ms busy loop -> 0.500 s
OK: converted interval is in range
Test 2: 4 threads sample the counter concurrently
OK: no backwards step observed
perf_seqlock_test: PASSED (0 error(s))
```
### Build coverage
Both build from a clean tree with `make -j20`, no warnings from the
touched files, and g_perf has exactly one definition in the ELF:
| config | `BUILD_*` | `ARCH_PERF_EVENTS_USER_ACCESS` | result |
| --- | --- | --- | --- |
| `qemu-armv7a:smp` | `BUILD_FLAT` | n | build OK, `nm` shows one g_perf |
| `qemu-armv7a:knsh` | `BUILD_KERNEL` | y (default) | build OK, `nm` shows
one g_perf |
`./tools/nxstyle` reports no issue for every touched file.
Disassembly of the user space copy confirms the seqlock variant is the
one built when `PERF_OVERFLOW_CORRECTION=y` (the `read_seqbegin()` /
`read_seqretry()` retry loop is visible around the `up_perf_gettime()`
call).
## PR verification Self-Check
* [x] This PR introduces only one functional change (the user space
perf counter and the overflow tracking it needs).
* [x] I have updated all required description fields above.
* [x] My PR adheres to Contributing Guidelines and Documentation.
* [ ] My PR is still work in progress (not ready for review).
* [x] My PR is ready for review and can be safely merged into a
codebase.
Depends on the `nuttx-apps` PR that adds
perf_seqlock.c.
--
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]
