Re: [rt-tests v1 0/6] libnuma cleanups for cyclictest

[John Kacur <jkacur@redhat.com>]

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
> 
>