Rtapi cleanup v2

[hdiethelm]

Continuation of #3908 reverted in #3918

Target: Move some classes out of the huge uspace_rtapi_app.cc

uspace_rtapi_main: Contains the main function and helpers
uspace_rtapi_app: Contains the RtapiApp class
uspace_posix: Contains the PosixApp class

Other fixes:

• Don't start master just for exit: 371793c
• Remove unused rtapi_task::ratio: 3097578
  • Set but only read to force a fixed ratio which probably was not the intention
• Slightly different lock to avoid needing reference to instance: c8af827
• All real time implementations are now library's and handled the same way: 62e5ea6
• No hard-coded library paths e1bbaf4
• ... and many more that came up during review

[hdiethelm]

Done:

• libuspace-xxx.so.0 -> liblinuxcnc-uspace-xxx.so.0
  • Longest name is liblinuxcnc-uspace-xenomai-evl.so.0

Still open:

• Check if slightly different lock to avoid needing reference to instance: c8af827 is a good idea
• Review

And

In the Submakefile it reads:

$(call TOOBJSDEPS, $(USPACE_POSIX_SRCS)): EXTRAFLAGS += -pthread -fPIC

Shouldn't that be (not adding to EXTRAFLAGS):

$(call TOOBJSDEPS, $(USPACE_POSIX_SRCS)): EXTRAFLAGS = -pthread -fPIC

Hmm, I don't understand makefiles in depth. I just copied what was already there a few lines below and edited it to match my lib. As much as I understand this, it just adds this flags to the global EXTRAFLAGS in Makefile for one compile command only. I did not see any duplicated flags.

[NTULINUX]

Works here on Gentoo, rip, system install, clang and gcc builds, whole shebang!

edit: Have not yet tested with RTAI.

[hdiethelm]

Thanks for testing!

I renamed the library's to:

liblinuxcnc-uspace-posix.so.0 liblinuxcnc-uspace-xenomai-evl.so.0
liblinuxcnc-uspace-rtai.so.0    liblinuxcnc-uspace-xenomai.so.0

A bit long names, but I think it is fine. But i'm open for other suggestions.

Additionally, I reduced the globals.

I tested all 5 configurations in a VM and they all work. There are two issues but I don't think they are due to this PR:

• LXRT doesn't start on an isolated CPU. Workaround: RTAPI_CPU_NUMBER=1 linuxcnc
• RTAI doesn't unload the modules in the correct order and fails to unload all

[hdiethelm]

@NTULINUX: Do you have a test setup you can share or is it all just manually setup? A series of docker files would be nice, so different OS can be tested if something starts. I messed my VM up slightly by using make install and just after the fact figured out that there is no make uninstall but I was able to restore it.
If there is nothing yet, I can create some, should not be to involved. I often use podman for similar things where I don't want to mess up my host. Podman is similar to docker but it doesn't need root.

[NTULINUX]

I have VM images up right now but they're in flux. I'm going to post new VMs with all the right fixes in a bit. Currently tracking this PR here:

#3925

My ebuilds at the moment are broken but we're in the middle of sorting it all out for good. Will share link to new VM soon with everything tied together, cleanly.

[andypugh]

Some discussion in the Sunday video meet-up has suggested that we should look at incorporating this into the rtapi cleanup:

#918

[hdiethelm]

I commented and corrected the things I have changed.
Now do we really want do do all of that in this MR?

I always prefer to have refactoring (moving code around) and bug fixing/functional changes as separated as possible. That makes review and testing easier, you just check that the moved code arrived as it was and so avoids having merge conflicts due to the branch staying open for to long.

But I have to admit, I also did some changes that I just was not able to leave it as it was and which simplified moving code due to removed dependency's and of course created a bug doing so.

[BsAtHome]

See: https://www.man7.org/linux/man-pages/man7/signal-safety.7.html for the callyou are allowed to make in signal handlers.

[BsAtHome]

I commented and corrected the things I have changed. Now do we really want do do all of that in this MR?

Well, that is a good question. You are doing "cleanup" and that would imply refactor and fixes, IMO.

I always prefer to have refactoring (moving code around) and bug fixing/functional changes as separated as possible. That makes review and testing easier, you just check that the moved code arrived as it was and so avoids having merge conflicts due to the branch staying open for to long.

That is a possibility too, but there are soooo (key got stuck) many problems with this code that it is a good question why not hit those two birds with one stone two lines with one keypress.

But I have to admit, I also did some changes that I just was not able to leave it as it was and which simplified moving code due to removed dependency's and of course created a bug doing so.

And I appreciate the changes! They are very necessary. Before we're done it needs to be tested,of course. But when the code gets better structured, then that should also become easier, I hope.

But, if you want to split it, then I think we need to have a very clear split where you move without actual changes and then refactor. My opinion is that moving code also classifies as refactoring and therefore it would be a missed opportunity if not fixed in one go.

[BsAtHome]

BTW, I've been wanting to vacuum this code for a long time but have not gotten arround to that point yet. You know, INI-file reader just done, HAL types/access revamp in the pipeline, tcl9 stuff that still needs fixing, build system cleanup, and so on.

(and also got CI's -Werror merged)

[hdiethelm]

I commented and corrected the things I have changed. Now do we really want do do all of that in this MR?

Well, that is a good question. You are doing "cleanup" and that would imply refactor and fixes, IMO.

Might be I did not specify my (initial) intentions well enough. I just found the rtapi_uspace hard to manage while implementing xenomai and wanted to split it up without changing the existing code if not needed. But that diverged anyway already a bit.

With the new structure, it should also be easier to implement rtapi_udp_sendto() or something similar needed for RTNet.

I always prefer to have refactoring (moving code around) and bug fixing/functional changes as separated as possible. That makes review and testing easier, you just check that the moved code arrived as it was and so avoids having merge conflicts due to the branch staying open for to long.

That is a possibility too, but there are soooo (key got stuck) many problems with this code that it is a good question why not hit those two birds with one stone two lines with one keypress.

But I have to admit, I also did some changes that I just was not able to leave it as it was and which simplified moving code due to removed dependency's and of course created a bug doing so.

And I appreciate the changes! They are very necessary. Before we're done it needs to be tested,of course. But when the code gets better structured, then that should also become easier, I hope.

But, if you want to split it, then I think we need to have a very clear split where you move without actual changes and then refactor. My opinion is that moving code also classifies as refactoring and therefore it would be a missed opportunity if not fixed in one go.

I have to look into it a bit more next week if I find time. But from my point of view it would make sense to split the more involved changes in a new PR or also two.

But there is also always some testing effort behind which I don't know how much you have to do from your side.

Also there is: #3925
Just randomly, I found also that DLSYM halpr_find_comp_by_name https://github.com/hdiethelm/linuxcnc-fork/blob/rtapi_cleanup_v2/src/rtapi/uspace_rtapi_main.cc#L112 does not match the signature (char* vs const char*),

I was now wondering if there is not a better solution for this. Often I use the following pattern to define the function and the matching type in the same header and then from then on only FUN_T* for function pointers. So you naturally change both. By some define magic, it should be possible do do both in one, but then it gets hard to read.

typedef int FUN_T(void *, void *);
int fun(void *, void *);

You know of a good way to test if both definitions match?

It would also be nicer to check at compile time if a .so has the needed main/exit functions.

[BsAtHome]

I'll need to make a new look over it after all these changes to see if I missed stuff.

[hdiethelm]

I'll need to make a new look over it after all these changes to see if I missed stuff.

Yes, I changed again a lot based on your review...

I rewrote the socket serialize/deserialize anyway. After looking for to long at this read_strings, I could not let it be that way. It's still a bit handwavy but I also did not want to import a new library just for this.

Still open from your review:

• The signal handling. I don't really see the intent behind. Due to it is in harden_rt() it could have some influence on real time. But otherwise, I would just delete it all together. I see no point in a graceful exit if a signal is received except for debug purpose. And for this there are better solutions.

I would like to postpone this to separate PR's:

• Master/slave issue
• Rootless
• Might be better solution for function pointers?

Of course except you see something else/new.

[hdiethelm]

I quickly tested the signal handler by doing a segfault on purpose in the load command handler:

../bin/rtapi_app load test
Note: Using POSIX realtime
rtapi_app: caught signal 11 - dumping core

Of course there is no core dump what so ever, also not on master.

[hdiethelm]

So lacking better options, I went for: cd41d50
Do you think that's acceptable?
An option would be to add:

@@ -661,7 +662,10 @@ static void signal_handler(int sig, siginfo_t * /*si*/, void * /*uctx*/) {
         WRITE_STDERR_STR("rtapi_app: UNKNOWN - shutting down\n");
         break;
     }
-    
+    rtapi_msg_queue.consume_all([](const message_t &m) {
+        WRITE_STDERR_STR(m.msg);
+    });
+
     _exit(-1);
 }

so the last messages are shown. But I am not sure if this is signal signal-safety conform.

However, it works. I blocked the normal consumer thread and then did killall rtapi_app to get messages printed in the signal handler:

Note: Using POSIX realtime
rtapi_app: SIGTERM - shutting down
Unexpected realtime delay on task 0 with period 25000
This Message will only display once per session.
Run the Latency Test and resolve before continuing.

[BsAtHome]

I quickly tested the signal handler by doing a segfault on purpose in the load command handler:
rtapi_app: caught signal 11 - dumping core
Of course there is no core dump what so ever, also not on master.

If you run on a system with systemd (most likely) you may want to use coredumpctl to access the coredump (run coredumpctl debug to start gdb with the dump).

[BsAtHome]

WRITE_STDERR_STR("rtapi_app: UNKNOWN - shutting down\n");

That should work too. As long as the string is a constant and strlen is replaced by the compiler builtin, then the length expands as a constant too. Just a minor detail, you should put the expansion of a macro in parentheses.

However, it works. I blocked the normal consumer thread and then did killall rtapi_app to get messages printed in the signal handler:
[snip]

You are now catching more signals than the original. That is wrong. You actually want to coredump on "bad" signals (anything not with explicit meaning for the running program). The system should behave predictable and if that does not happen you want to be able to backtrack why it didn't behave. That is why it codedumps, so you can backtrack.

[hdiethelm]

If you run on a system with systemd (most likely) you may want to use coredumpctl to access the coredump (run coredumpctl debug to start gdb with the dump).

Ah now I get a coredump. It did work on master and I broke it. But I also told you I am not sure what I am doing. A fix will follow.

If not setid: ulimit -c unlimited

If setuid, prctl(PR_SET_DUMPABLE, 1) is used in harden_rt() but echo 1 | sudo tee /proc/sys/fs/suid_dumpable is needed. I don't have coredumpctl installed but just to test if it works, I don't need it.

References:
https://linuxvox.com/blog/linux-enable-core-dump/
https://access.redhat.com/solutions/4896

[hdiethelm]

So, next try. If you want to take over for the signal part, feel free. The last time I used coredumps is like 20 years ago and for signals like 10.

I now just call abort() inside the handler which works and is allowed. But I don't know if it is good practice.

I added the message queue write out also. As long as there is no malloc/free in consume_all(), that is fine. And I don't think there is due to the queue is fixed size but I won't bother reviewing the boost library. I can remove it again if you think it's to dangerous.

For testing, I just run killall -SIGFPE rtapi_app in a second terminal.
rtapi_app itselv behaves fine now:

../bin/rtapi_app 
Note: Using POSIX realtime
rtapi_app: SIGFPE - aborting
Aborted (core dumped)

Coredump is there.

However, latency-test not. No clue why but it's the same on master. :

../scripts/latency-test 
Note: Using POSIX realtime
Unexpected realtime delay on task 0 with period 25000
This Message will only display once per session.
Run the Latency Test and resolve before continuing.
rtapi_app: SIGFPE - aborting

No coredump.

[BsAtHome]

So, next try. If you want to take over for the signal part, feel free. The last time I used coredumps is like 20 years ago and for signals like 10.

Most of it is now fine :-)

I now just call abort() inside the handler which works and is allowed. But I don't know if it is good practice.

Well, you are allowed to call abort() in the signal handler. The existing solution (using kill()) re-raises the caught signal with the handler to default. Using abort() seems the better choice.

[hdiethelm]

I tested the actual state of this PR both with latency-histogram and on my machine. No issues so far except #3946.

[hdiethelm]

@BsAtHome Thanks already for the detailed review.

Still open points:

• Rtapi cleanup v2 #3919 (comment)
• Socket error EINTR handling: I would ignore it but if you insist on it, I can add it, no big deal

Anything else?

[hdiethelm]

Rebased to master quick retest: latency-historgram / latency-test / linuxcnc axis_mm
All fine

[BsAtHome]

Then I think this is finally done, cooked and ready :-)