On Fri, 18 Dec 2020, Daniel Wagner wrote: > As we have a hard dependency on libnuma we can simplify the code in > cyclictest. This allows remove all the small helpers in rt_numa.h. And > with this we can remove the header and reduce the confusion with > rt-numa.h This is why I asked you to keep numa functions separate from any common library. The reason the file exists, is in the past I had two versions of each numa function, one for architectures that supported libnuma, and empty wrapper versions for ones that didn't. This implementation detail was pushed up into that header file, to make the code cleaner. Then all the architectures that we cared about seemed to support to libnuma so I removed the versions of the functions that were empty, so we were at the point that the header file could just be removed, but I never got around to that and things just worked fine. So thank you for doing this, but, there is always a but right? There are two buts. The first one, is that I see the other programs in the test suite that mimic cyclictest behaviour are getting more attention, so I could imagine as we try to strengthen their behaviour that we could add numa support to these programs as well. The other but, is that I'm seeing the need recently to add support for a lot of architectures that we didn't care so much about in the past, and not all of them have libnuma so we might want to do the same trick as in the past, add wrapper versions in a header file. So I propose, we take your patches to get rid of the file, because it will clean things up a lot, but then we may have to go back to creating something like the rt_numa.h file anew. Removing rt_numa.h first is good though, because if we don't have to create it anew the code has been cleaned-up, and if we do recreate it from scratch, it will be better for having cleaned-up how numa works in cyclictest > > While at it, I simplified the --smp vs --affinity vs --threads > logic. There is no need for additional variables to keep state. With > this we also make --affinity to behave as with the rest of > rt-tests. That is a plan -a will be the same as with -S. There is no > need for -S anymore but I think we should leave it in place for > backwards compatibility. I suspect, there must be a lot of muscle > memory out there :) This is also an example of something that was evoling in exactly this direction. When Thomas wrote cyclictest, it made sense to have smp, but if I were to rewrite everything from scratch I wouldn't have it there. Some more background, we used to have to specify numa, but we changed it so that numa was detected automatically, but you could still specify smp if you wanted to test that behaviour. This also helped simplify the alphabet soup of options by removing --numa. Note also that smp isn't just a kind of hardware for cyclictest, you can still see this is the help - it gathered up the -a and -t options and put all threads at the same prio. So, thanks again for doing this. A word of warning I spent a long time fiddling with affinity to make it finally work correctly, so I'm going to test the sh*t out of your changes to make sure they don't break anything before I accept them. A final comment, the idea of an "unstable" devel branch, the "unstable" refers not to the code being iffy, but to the fact that I wanted developers to feel free to break the old API, and not be constrained with backwards compatability, so as you clean this stuff-up if you want to remove -S, it's fine with me. Hopefully if we take away anything that users want they'll yell at us, but I think we're pretty careful about necessary functionality. Maybe after things are cleaned-up we will have to add a new flag to specify the same prio for all threads? I also maintain the rteval program which uses cyclictest for measuring performance while running loads, changes to the cyclictest API sometimes require me to tweak how rteval works. Nobody has ever told me about any other programs that use cyclictest. Anyway, changes to the cyclictest api have to be synced with rteval. rteval has develped in the background to this community and not received much attention here before, but, well, thinking about how that might change. I think I'll spin off a new version of rt-tests before accepting this set of patches. John > > Daniel Wagner (6): > cyclictest: Always use libnuma > cyclictest: Use numa API directly > cyclictest: Use affinity_mask for stearing thread placement > cyclictest: Mimik --smp behavior with --affinity > cyclictest: Simplify --smp vs --affinity vs --threads argument logic > cyclictest: Move verbose message into main > > src/cyclictest/cyclictest.c | 154 ++++++++++++++---------------------- > src/cyclictest/rt_numa.h | 98 ----------------------- > 2 files changed, 58 insertions(+), 194 deletions(-) > delete mode 100644 src/cyclictest/rt_numa.h > > -- > 2.29.2 > >
