casaroli commented on code in PR #21475:
URL: https://github.com/apache/nuttx/pull/21475#discussion_r4509885246
##########
sched/clock/clock_perf.c:
##########
@@ -30,15 +30,137 @@
#include <nuttx/clock.h>
#include <nuttx/irq.h>
#include <nuttx/sched.h>
+#include <nuttx/seqlock.h>
#include <nuttx/wdog.h>
-#ifndef CONFIG_ARCH_PERF_EVENTS_USER_ACCESS
+#ifdef CONFIG_BUILD_PROTECTED
+# include <nuttx/userspace.h>
+#endif
/****************************************************************************
* Preprocessors
****************************************************************************/
-# if defined(CONFIG_PERF_OVERFLOW_CORRECTION) && ULONG_MAX != UINT64_MAX
+#if defined(CONFIG_PERF_OVERFLOW_CORRECTION) && ULONG_MAX != UINT64_MAX && \
+ !defined(CONFIG_SMP)
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+/* Without SMP there is a single counter, so the state is the struct perf_s
+ * declared in include/nuttx/perf.h. In a protected build it is embedded in
+ * the user-space data block; otherwise it is a global. The watchdog is the
+ * only writer, and the sequence counter lets readers sample the folded value
+ * without taking a lock.
+ */
+
+static struct wdog_s g_wdog;
+static unsigned int g_timeout;
+
+/* The overflow state is one global in every build mode. A flat build
+ * shares the address space with the application, so user space reads it
+ * directly; a protected or kernel build keeps it private to the kernel,
+ * which is the only writer. Either way the state lives here.
+ */
+
+#if !defined(CONFIG_BUILD_PROTECTED)
+struct perf_s g_perf =
+{
+ .lock = SEQLOCK_INITIALIZER,
+};
+#endif
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * perf_update
+ ****************************************************************************/
+
+static void perf_update(wdparm_t arg)
+{
+ FAR struct perf_s *perf = (FAR struct perf_s *)arg;
+ irqstate_t flags = write_seqlock_irqsave(&perf->lock);
+ clock_t now = up_perf_gettime();
+
+ if (now < perf->last)
+ {
+ perf->overflow++;
+ }
+
+ perf->last = now;
+ write_sequnlock_irqrestore(&perf->lock, flags);
+ wd_start_next(&g_wdog, g_timeout, perf_update, arg);
+}
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * perf_setup
+ ****************************************************************************/
+
+void perf_setup(void)
+{
+ FAR struct perf_s *perf = &g_perf;
+
+ /* Setup periodic-wdog here, this function can be called after
+ * perf_gettime(), but suggested to be called in initialization phase.
+ */
+
+ g_timeout =
+ ((clock_t)1 << (CONFIG_ARCH_PERF_COUNT_BITWIDTH - 1)) *
+ TICK_PER_SEC / up_perf_getfreq();
+
+ /* Periodic check for overflow */
+
+ wd_start(&g_wdog, g_timeout, perf_update, (wdparm_t)perf);
Review Comment:
`perf_setup()` leaves `g_perf.last` at 0, and the first `perf_update()` runs
half a period later. If the counter is in its upper half at boot (nothing
resets it: `arm_perf_start()` sets only `PMCR_E`), it wraps before that first
update, `perf_gettime()` steps back by 2^32, and the wrap is never counted. On
`qemu-armv7a:nsh` with the counter set to 0xf0000000 after `up_perf_init()`:
`back at 3 uptime=373 ms prev=0xffa9cfb8 now=0x2c1c200`, and after 12 s perf
time is 8068 ms for 12363 ms of `CLOCK_MONOTONIC`. Master sets `last` in the
first `perf_gettime()`, so it has no such step.
Fix: set `perf->last = up_perf_gettime();` (under `write_seqlock_irqsave()`)
in `perf_setup()` before `wd_start()`.
##########
libs/libc/sched/clock_perf.c:
##########
@@ -35,7 +44,46 @@
* perf_gettime
****************************************************************************/
-clock_t perf_gettime(void)
+clock_t noinstrument_function perf_gettime(void)
+{
+ clock_t overflow;
+ clock_t result;
+ uint32_t seq;
+
+ do
+ {
+ seq = read_seqbegin(&g_perf.lock);
+ clock_t now = up_perf_gettime();
+
+ if (now < g_perf.last)
+ {
+ overflow = g_perf.overflow + 1;
+ }
+ else
+ {
+ overflow = g_perf.overflow;
+ }
+
+ result = now | (overflow << CONFIG_ARCH_PERF_COUNT_BITWIDTH);
+ }
+ while (read_seqretry(&g_perf.lock, seq));
+
+ return result;
+}
+
+#elif defined(CONFIG_ARCH_PERF_EVENTS_USER_ACCESS)
Review Comment:
In a flat build `sched/clock/clock_perf.c` defines `perf_gettime()` in every
case except `PERF_OVERFLOW_CORRECTION` without SMP, so this copy collides with
it. CMake builds this file in any flat build with the option (CMakeLists.txt:51
has no `NOT CONFIG_BUILD_FLAT`), make in a flat build with
`PERF_OVERFLOW_CORRECTION`: `qemu-armv7a:smp` with `ARCH_PERF_EVENTS`,
`ARCH_PERF_EVENTS_USER_ACCESS` and `PERF_OVERFLOW_CORRECTION` does not link
with CMake: `sched/clock/clock_perf.c:232: multiple definition of
'perf_gettime';
libs/libc/libc.a(clock_perf.c.o):libs/libc/sched/clock_perf.c:86: first defined
here`.
Fix: add `&& !defined(CONFIG_BUILD_FLAT)` to this `#elif`, and use
`elseif(NOT CONFIG_BUILD_FLAT AND CONFIG_ARCH_PERF_EVENTS_USER_ACCESS)` in
CMakeLists.txt:51.
##########
arch/arm/Kconfig:
##########
@@ -1184,7 +1184,7 @@ config ARCH_ARMV7A
select ARCH_HAVE_FORK if BUILD_KERNEL && ARCH_ADDRENV
select ARCH_HAVE_CPUINFO
select ARCH_HAVE_DEBUG
- select ARCH_HAVE_PERF_EVENTS
+ select ARCH_HAVE_PERF_EVENTS_USER_ACCESS
Review Comment:
ARMv7-A has no MPU, so no protected build, and
`ARCH_PERF_EVENTS_USER_ACCESS` now depends on `!BUILD_KERNEL`. So on ARMv7-A
the option can only be on in a flat build, and the user-blob `arm_perf.c` of
armv7-a/Make.defs:112 and CMakeLists.txt:131 is never built.
Fix: drop the ARMv7-A user-space part (this `select`, the
Make.defs/CMakeLists.txt blocks and the `CONFIG_BUILD_FLAT || __KERNEL__` guard
in armv7-a/arm_perf.c), or say in the commit message that it is for a later
kernel-build reader.
--
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]
