Conversation
|
I have reviewed the CI failures and I do not think they are related to the changes I have made. |
|
I just pushed a change to the master branch here which cleaned up some compiler warnings, and the subsequent CI build jobs for RTEMS 4.9 and 4.10 both succeeded. I also re-triggered those CI builds for this PR which unfortunately both failed again, so some code change inside this PR must have caused the failures. I don't care about the pre-commit check failure, just the ability to build this module against RTEMS 4.9 and 4.10. Note that the EPICS Base 7.0 version that this CI is using at the moment doesn't include your RTEMS-6 PRs, and we expect future releases of this module to continue to build against older EPICS versions anyway — the CI jobs that build it for Linux against both 3.14 and 3.15 should hint at that. |
29a2d9e to
1a65c02
Compare
|
@anjohnson I have resolved the issue with 4.10 and I assume 4.9. It seems the define |
|
Unfortunately, when I try to compile (MVME6100 legacy RTEMS6) I get this: |
|
e.g. in devIocStats/os/RTEMS/devIocStatsOSD.h But then I get this error: |
|
ok, found this: |
|
The |
|
Works with the current RTEMS6 master(main). rtems_libio_count_open_iops is defined there. |
Thanks for confirming this. I do not know what the approval for workflows means. |
|
@simon-ess will have to speak up if he cares about the clang-format failure (those checks seem rather obnoxious to me), but this looks good for the RTEMS builds, thanks! |
|
I would prefer that the formatting check pass; otherwise it should be disabled, since we would then be just ignoring a failing test. We could discuss exactly what the best formatting is, but I don't like the idea of letting it fail. |
I had no idea there was a coding standard. I am happy to take a look.
This makes sense. I can fix it. |
1a65c02 to
a5ccfbe
Compare
|
I have pushed changes to fix the formatting. I did the changes manually from the diff in the CI log. Is this OK? |
|
I found a problem when I wanted to use iocStats for beaglebone black (arm). +int devIocStatsGetClusterUsage(int pool, int pval) { return -1; } /* This would otherwise need _KERNEL to be defined... */ |
a5ccfbe to
46633e2
Compare
|
@hjunkes thanks for the LibBSD fix. I have added it to the PR with a minor change. The I have build |
|
Ping |
|
Sorry for the long delay. I am currently trying RTEMS 7 with rtems-libbsd and EPICS R7.0.9.1-DEV with RTEMS-xilinx_zynq_a9_qemu. Unfortunately, I get a core dump on iocInit if devIocStats.dbd is included. |
|
It crashes when registering device support for 'devAiStats'. |
|
I have been able to trace this so far to 'memstat_sysctl_uma()': |
|
Error in freebsd/sys/vm/uma_core.c, uma_vm_zone_stats(struct uma_type_header *uth, uma_zone_t z, struct sbuf *sbuf, uma_zone_t z == 0 ?? |
|
Thanks @hjunkes for the detailed report and debugging. It looks like a bug in the zone allocation port to RTEMS. I have opened an issue in RTEMS https://gitlab.rtems.org/rtems/pkg/rtems-libbsd/-/issues/74 |
|
To adapt it to RTEMS7, I had to make a few adjustments. |
|
I’m currently trying to get PVXS running on RTEMS 7 and have come across a crash in iocStats. diff --git a/devIocStats/os/RTEMS/osdCpuUsage.c b/devIocStats/os/RTEMS/osdCpuUsage.c +#if RTEMS_MAJOR >= 7
|
|
While working on RTEMS 7 support for iocStats (BeagleBone Black BSP), I found and fixed an off-by-one bug in Fixed here: hjunkes/iocStats@3bf6951 Also fixed an unrelated clean-build race in Flagging in case either is relevant to this RTEMS 6/7 net-support work. |
anjohnson
left a comment
anjohnson
left a comment
There was a problem hiding this comment.
Comments/Q's from today's APS VxWorks to RTEMS meeting.
| #if RTEMS_VERSION_INT < VERSION_INT(6, 0, 0, 0) | ||
| #define rtems_bsd_reset() bsp_reset() | ||
| #else /* RTEMS_VERSION_INT < VERSION_INT(6, 0, 0, 0) */ | ||
| #define rtems_bsd_reset() bsp_reset() | ||
| #endif /* RTEMS_VERSION_INT < VERSION_INT(6, 0, 0, 0) */ |
There was a problem hiding this comment.
Why are both arms of this if() statement identical?
There was a problem hiding this comment.
Good question and it has been a while.
It looks like a why to separate the pre-6 functionality from the 6 and later function to call. It can be reverted?
There was a problem hiding this comment.
They're probably not meant to be identical — looks like a leftover from splitting the #if/#else.
A few lines further down, the RTEMS 6 reboot() redefinition uses the two-argument RTEMS 6 bsp_reset() signature:
static inline void reboot(int val) {
(void)val;
bsp_reset(RTEMS_FATAL_SOURCE_APPLICATION, 112233);
}but the RTEMS-6 arm of rtems_bsd_reset() right above it still calls the old zero-argument form. My guess is the #else branch was meant to be:
#define rtems_bsd_reset() bsp_reset(RTEMS_FATAL_SOURCE_APPLICATION, 112233)to match reboot(), and got missed when the conditional was split out. Worth double-checking against the actual RTEMS 6 bsp_reset() prototype either way.
There was a problem hiding this comment.
Should the name be rtems_bsp_reset() and not rtems_bsd_reset()?
Yes I agree. Thanks for the analysis. I checked and RTEMS 5 and I guess earlier have:
void bsp_reset(void);and RTEMS 6 and later has:
void bsp_reset( rtems_fatal_source source, rtems_fatal_code code );| static inline void reboot(int val) { | ||
| (void)val; | ||
| bsp_reset(RTEMS_FATAL_SOURCE_APPLICATION, 112233); | ||
| } |
There was a problem hiding this comment.
Can this use the RTEMS higher-level APIs instead of the BSP one?
There was a problem hiding this comment.
The bsp_reset should be a physical means of reset, ie a register with a signal or triggering a watchdog. It is called at the end of an orderly shutdown. Directly calling it mean you do not have an orderly shutdown. Using exit(something) would be an orderly shutdown however any issues in the exit processing can hang a system.
There was a problem hiding this comment.
Would rtems_shutdown_executive be appropriate here?
If we set BSP_RESET_BOARD_AT_EXIT in the config.ini file, it will reach bsp_reset(), and we can avoid handling bsp_* functions from EPICS applications/modules.
There was a problem hiding this comment.
Would
rtems_shutdown_executivebe appropriate here?If we set
BSP_RESET_BOARD_AT_EXITin theconfig.inifile, it will reachbsp_reset(), and we can avoid handlingbsp_*functions from EPICS applications/modules.
I see this setting as a per BSP or hardware specific settings combined with the system requirements. I am not comfortable having this setting be made a requirement for EPICS as it makes a fragile link between a BSP configuration, the BSP default and EPICS. And I doubt the calls are consistently implemented in all BSPs so both options work.
The option lets a BSP return to the boot loader or a reset happen. Some BSPs do not have a means to return to a boot loader as the boot process is destructive and others it is easier as a boot loader can handle a reset, ie hardware security reasons.
Should EPICS be consistent in how a device restarts? If yes then what role does a BSP reset have here in iocStats?
As an example the MVME5500 or MVME2700 return the boot loader on exit(). And on the boards I have access to once there they sit. I have no idea if a reset can be scripted and how recovery happens in a real system? Another example a Zynq has a software reset signal that is different to the power on reset (POR) signal and you often need custom hardware to manage this. The bsp_reset() is wired to the software reset.
| getrusage(RUSAGE_SELF, &stats); | ||
| curActive = (double)stats.ru_utime.tv_sec + stats.ru_utime.tv_usec / 1e6; | ||
| curIdle = (double)stats.ru_stime.tv_sec + stats.ru_stime.tv_usec / 1e6; | ||
| *idle = curIdle - oldIdleUsage; | ||
| *total = *idle + (curActive - oldActiveUsage); | ||
| oldActiveUsage = curActive; | ||
| oldIdleUsage = curIdle; |
There was a problem hiding this comment.
Vijay says there may be a proper RTEMS API coming for this, but only for RTEMS-7; if so that would be a good change to make use of for the supported RTEMS version(s).
There was a problem hiding this comment.
I was talking about rtems_cpu_task_get_usage MR here: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/1347
I haven't looked at how well it fits in here, but looks like the rtems API can be used here instead of custom coding it again. This code might get stale again if RTEMS is updated, and it will likely be easier to use an rtems api instead of implementing it here.
There was a problem hiding this comment.
What about using rtems_task_iterate() to loop over each task and sum the values of _Thread_Get_CPU_time_used(task)?
Unfortunately, _Thread_Get_CPU_time_used() seems to be a score function, but if there's no alternative yet perhaps its the best option for now. I think its been around since RTEMS 5.
There was a problem hiding this comment.
What about using rtems_task_iterate() to loop over each task and sum the values of _Thread_Get_CPU_time_used(task)?
Yes this is at the core of the approach I used in getusage back in 2024.
Unfortunately, _Thread_Get_CPU_time_used() seems to be a score function, but if there's no alternative yet perhaps its the best option for now. I think its been around since RTEMS 5.
In the early days of EPICS integration we were not strict as everyone worked to get something going. The downside of this is we are still paying the price today as we extract EPICS from deep inside RTEMS. A key part of this effort is epics-base/epics-base#905.
I was talking about
rtems_cpu_task_get_usageMR here: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/1347
Ah OK, the GSoC project. That call fills the gap of getrusage where you are asking about the CPU usage of specific thread that is not the executing thread. I thought iocStats was about the current load on the board and as RTEMS is a single process system calling getrusage with RUSAGE_SELF gives you that value?
I haven't looked at how well it fits in here, but looks like the rtems API can be used here instead of custom coding it again. This code might get stale again if RTEMS is updated, and it will likely be easier to use an rtems api instead of implementing it here.
It is per thread and yes correct.
I am little confused by this discussion as I thought I had done this and exported the result via the standards supported getrusage call back in July 2024?
There was a problem hiding this comment.
Same RTEMS-Score-API-drift theme, at the libbsd network-stack boundary this time. osdClustInfo.c: - The legacy network stack's `extern struct mbstat mbstat` doesn't exist under libbsd. Gated the old code behind `#if RTEMS_LIBBSD_STACK` / `#else`, and for the libbsd case query the same mbuf/cluster counts through libbsd's portable memstat API (memstat_mtl_alloc/memstat_sysctl_all/memstat_mtl_find + memstat_get_size/count/free), the same mechanism `netstat -m` uses. Also actually implements devIocStatsGetClusterUsage() for the libbsd case rather than returning -1 unconditionally. (epics-modules#61 took the same memstat approach for its RTEMS 6 support, but writes the mbuf_cluster stats into row [0] again instead of row [1], clobbering the mbuf row -- fixed that here.) osdIFErrors.c: - `extern struct ifnet *ifnet` walked via `if_next`, and if_ierrors/if_oerrors, are kernel-internal under libbsd (ifnet is a CK_STAILQ now, and the counters are behind the if_get_counter KPI) -- neither is meant to be reachable from outside the kernel proper, so `__RTEMS_VIOLATE_KERNEL_VISIBILITY__` doesn't save this approach. Switched to getifaddrs()/struct if_data, the portable userspace- visible way to read the same per-interface error counters (each interface contributes one AF_LINK entry whose ifa_data is its struct if_data). Both compile cleanly against the current RTEMS 7 build. Remaining unrelated pre-existing issues in this module (devIocStatsOSD.h's rtemsReboot, and -Wincompatible-pointer-types in the Analog/String/Waveform DSETs) still untouched -- out of scope here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Hardware-verified crash: memstat_sysctl_all() (used by the previous
commit's libbsd implementation of devIocStatsGetClusterInfo/Usage)
takes a data abort inside libbsd's UMA per-CPU counter stats path.
Confirmed via addr2line against the actual crash PC/LR from the RTEMS
fatal-exception dump:
PC 0x8023e148 -> _bsd_counter_u64_alloc
(rtems-libbsd/.../freebsd/sys/kern/subr_counter.c:64)
LR 0x802cc904 -> uma_vm_zone_stats
(rtems-libbsd/.../freebsd/sys/vm/uma_core.c:5718)
This is reached from a periodic devIocStats timer callback shortly
after iocInit starts (ai_clusts's driver init registers an
initHookAfterCaServerInit hook), so it crashes essentially every boot
once a CLUST_* record is loaded.
The memstat approach was adapted from epics-modules#61's
(still open, unmerged) RTEMS 6 port, which apparently was never run on
real hardware either -- it also has a row-index bug fixed in the
previous commit, independent of this crash. Something in this
RTEMS-libbsd build's counter(9)/UMA-zone-stats support is broken or
incomplete; root-causing that is future work.
Reverted to reporting "not available" (-1), matching what epics-modules#61 itself
does for the GetClusterUsage case it left unimplemented. CLUST_*
records will show INVALID/alarm rather than a real reading, but the
IOC boots and runs.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Hardware finding while porting this PR's 1. Row-index bug in the 2. More seriously: So For now (in #88) I've reverted the libbsd branch of |
|
The mbuf cluster stuff that iocStats purports to show was IIRC only useful with the old IP stack in VxWorks 5.x, in VxWorks 6 they went back to a more conventional IP stack with a single partition for network memory. I think it should be completely removed, I don't think anybody even understands it any more, so I have no problem with it not doing anything on RTEMS. |
This makes sense to me. I should point out If working with LibBSD shows a need there are options that can be added in. |
46633e2 to
a5bdfb2
Compare
6 participants
This pull request adds support for RTEMS 6 for the legacy network and libbsd.
It needs a current build of RTEMS 6 and tools.