casaroli commented on code in PR #21475:
URL: https://github.com/apache/nuttx/pull/21475#discussion_r4495158015
##########
sched/clock/clock_perf.c:
##########
@@ -38,7 +40,117 @@
* 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 overflow state is the
+ * global struct perf_s declared in include/nuttx/clock.h. The watchdog
+ * is the only writer: the write side of the sequence counter tells a
+ * reader that the state is being updated, so the folded 64 bit value
+ * can be sampled without taking a lock.
+ */
+
+static struct wdog_s g_wdog;
+static unsigned int g_timeout;
+
+/* A flat build shares this single state with the application, which
+ * reads it through the definition in libs/libc/sched/clock_perf.c. In a
+ * protected build the kernel owns it and hands the pointer out through
+ * struct userspace_data_s.
+ */
+
+#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;
Review Comment:
In a protected build `g_perf` is `*USERSPACE->us_data->us_perf`, and only
`olimex-stm32-p407/kernel/stm32_userspace.c` sets `us_perf`. With
`PERF_OVERFLOW_CORRECTION` now default y, `perf_setup()` starts the watchdog
with a NULL `perf` on every other protected board (`rv-virt:pnsh`,
`mps2-an500:knsh`, `mps3-an547:knsh`). `rv-virt:pnsh` at this head panics about
2 s after boot: `Load access fault ... EPC: 80002272, MTVAL: 00000000` in
`perf_update()`; the base boots fine.
Fix: define `g_perf` in sched/clock/clock_perf.c in every build mode, and
drop `us_perf` and the `g_perf` macro in include/nuttx/clock.h.
##########
arch/Kconfig:
##########
@@ -675,11 +675,18 @@ config ARCH_HAVE_PERF_EVENTS
config ARCH_HAVE_PERF_EVENTS_USER_ACCESS
bool
- default n
select ARCH_HAVE_PERF_EVENTS
+ default n
---help---
Select if hardware allows userspace perf counter access.
+config ARCH_PERF_EVENTS_USER_ACCESS
+ bool "Enable userspace perf counter access"
+ default !BUILD_FLAT
Review Comment:
`ARCH_PERF_EVENTS_USER_ACCESS` defaults to y without `ARCH_PERF_EVENTS`, as
in `qemu-armv7a:knsh` and `sabre-6quad:knsh`. User access to PMCCNTR is enabled
only in `up_perf_init()` (`cp15_pmu_uer(PMUER_UME)`), which the boards call
only with `ARCH_PERF_EVENTS`, so a user read of the counter takes an undefined
instruction exception there.
Fix: add `depends on ARCH_PERF_EVENTS`.
##########
sched/Kconfig:
##########
@@ -259,8 +259,8 @@ config PREALLOC_TIMERS
config PERF_OVERFLOW_CORRECTION
bool "Compensate perf count overflow"
- depends on ALARM_ARCH || TIMER_ARCH || ARCH_PERF_EVENTS
- default n
+ depends on !ARCH_64BIT && (ALARM_ARCH || TIMER_ARCH || ARCH_PERF_EVENTS)
+ default y
Review Comment:
`default y` turns the correction on in 233 defconfigs. In most of them
(ALARM_ARCH/TIMER_ARCH without an arch counter) `up_perf_gettime()` is already
64 bit (`arch_alarm.c` returns nanoseconds), so the watchdog only adds a wakeup
every 2.1 s and code (hifive1-revb: +452 B text, +64 B bss in the MemBrowse
report).
Fix: `default y if ARCH_PERF_EVENTS`.
##########
sched/clock/clock_perf.c:
##########
@@ -164,6 +276,25 @@ clock_t perf_gettime(void)
# endif
#endif /* !CONFIG_ARCH_PERF_EVENTS_USER_ACCESS */
+#if defined(CONFIG_PERF_OVERFLOW_CORRECTION) && !defined(CONFIG_SMP) && \
+ defined(CONFIG_ARCH_PERF_EVENTS_USER_ACCESS)
+
+/****************************************************************************
+ * perf_setup
+ *
+ * Description:
+ * The counter is read directly from user space in this configuration,
+ * so there is no kernel overflow state to maintain. clock_initialize()
+ * still calls perf_setup(), keep it as a no-op.
+ *
+ ****************************************************************************/
+
+void perf_setup(void)
Review Comment:
With `ARCH_PERF_EVENTS_USER_ACCESS=y` (now the default in protected and
kernel builds on ARMv7-A, ARMv7-R and ARMv8-R) this `perf_setup()` is empty, so
nothing writes `g_perf` and the libc `perf_gettime()` never adds the overflow:
user space and the kernel (which now links the libc copy) get the 32 bit
counter. In a kernel build each process also has its own `g_perf`, and an
application cannot link it: on `qemu-armv7a:knsh` a program that calls
`perf_gettime()` fails with `libs/libc/sched/clock_perf.c:70: undefined
reference to 'up_perf_gettime'` (applications do not link libarch.a).
Fix: build the libc copy and this empty `perf_setup()` only where the
application and the kernel share `g_perf` (`BUILD_FLAT`), and keep
`perf_gettime()` and the watchdog in the kernel in protected and kernel builds.
##########
libs/libc/sched/clock_perf.c:
##########
@@ -26,6 +26,66 @@
#include <nuttx/arch.h>
#include <nuttx/clock.h>
+#include <nuttx/seqlock.h>
+#include <nuttx/userspace.h>
+
+#if defined(CONFIG_PERF_OVERFLOW_CORRECTION) && !defined(CONFIG_SMP)
+
+/****************************************************************************
+ * Public Data
+ ****************************************************************************/
+
+/* Without SMP the overflow state is a single global. In a protected
+ * build the application reaches the kernel copy through struct
+ * userspace_data_s, otherwise it defines its own.
+ */
+
+#undef g_perf
+#if defined(CONFIG_BUILD_PROTECTED) && defined(__KERNEL__)
+# define g_perf (*(USERSPACE->us_data->us_perf))
+#else
+struct perf_s g_perf =
+{
+ .lock = SEQLOCK_INITIALIZER,
+};
+#endif
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * perf_gettime
+ ****************************************************************************/
+
+clock_t perf_gettime(void)
Review Comment:
In a build with `ARCH_PERF_EVENTS_USER_ACCESS` this copy is also the
kernel's `perf_gettime()`, but it has no `noinstrument_function`, so the
recursion that 94caa928351 fixes for sched/clock/clock_perf.c stays here.
Fix: mark it `noinstrument_function`.
##########
libs/libc/sched/Make.defs:
##########
@@ -39,7 +39,20 @@ ifneq ($(CONFIG_BUILD_KERNEL),y)
CSRCS += task_startup.c
endif # CONFIG_BUILD_KERNEL
-ifeq ($(CONFIG_ARCH_HAVE_PERF_EVENTS_USER_ACCESS),y)
+# The libc copy reads the folded counter only in a protected or kernel
+# build without SMP: with SMP the state is per CPU and stays private to
+# the kernel, and a flat build already gets perf_gettime() from
+# sched/clock/clock_perf.c.
+
+ifneq ($(CONFIG_BUILD_FLAT),y)
+ifneq ($(CONFIG_SMP),y)
+ifeq ($(CONFIG_PERF_OVERFLOW_CORRECTION),y)
+ CSRCS += clock_perf.c
+endif
+endif
+endif
+
+ifeq ($(CONFIG_ARCH_PERF_EVENTS_USER_ACCESS),y)
Review Comment:
With `!BUILD_FLAT`, `!SMP`, `PERF_OVERFLOW_CORRECTION` and
`ARCH_PERF_EVENTS_USER_ACCESS` (`qemu-armv7r:pnsh`), both blocks add
`clock_perf.c`, so it is listed twice; the same in CMakeLists.txt:53 and :57.
Fix: add `clock_perf.c` from one block, under (`PERF_OVERFLOW_CORRECTION`
and `!SMP` and `!BUILD_FLAT`) or `ARCH_PERF_EVENTS_USER_ACCESS`.
--
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]
