| Message ID | 20201218141935.24151-1-dwagner@suse.de |
|---|---|
| Headers | show |
| Series | libnuma cleanups for cyclictest | expand |
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 > >
On Fri, Dec 18, 2020 at 11:02:45AM -0500, John Kacur wrote: > For the future, I'm okay with things evolving in steps, as long as what > you sent me works correctly, you can then add support to do the same kind of things > in other programs in the suite later, as long as it gets done and not > neglected. I am on the same side here. We should really try to unify the code base match as possible. This series is just one small step into this direction :) > Since you told me to wait this time though, I will wait for v2 See your inbox. Happy testing.
On Fri, Dec 18, 2020 at 7:59 AM John Kacur <jkacur@redhat.com> wrote: > 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. What is the authoritative source of rteval? I don't see it in rt-tests, in kernel source or in Debian packages. Thanks, Alison Chaiken Aurora Innovation
On Tue, 22 Dec 2020, Alison Chaiken wrote: > On Fri, Dec 18, 2020 at 7:59 AM John Kacur <jkacur@redhat.com> wrote: > > 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. > > What is the authoritative source of rteval? I don't see it in > rt-tests, in kernel source or in Debian packages. > > Thanks, > Alison Chaiken > Aurora Innovation > Clone git://git.kernel.org/pub/scm/utils/rteval/rteval.git https://git.kernel.org/pub/scm/utils/rteval/rteval.git https://kernel.googlesource.com/pub/scm/utils/rteval/rteval.git
On Fri, 18 Dec 2020, Daniel Wagner wrote: > We don't need an extra variable to track the state if a bitmask is > available or not. Check directly if the mask is usable. > > Signed-off-by: Daniel Wagner <dwagner@suse.de> > --- > src/cyclictest/cyclictest.c | 28 ++++------------------------ > 1 file changed, 4 insertions(+), 24 deletions(-) > > diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c > index a5ca764da254..0e6519125c2f 100644 > --- a/src/cyclictest/cyclictest.c > +++ b/src/cyclictest/cyclictest.c > @@ -893,7 +893,6 @@ static int interval = DEFAULT_INTERVAL; > static int distance = -1; > static struct bitmask *affinity_mask = NULL; > static int smp = 0; > -static int setaffinity = AFFINITY_UNSPECIFIED; > > static int clocksources[] = { > CLOCK_MONOTONIC, > @@ -1020,19 +1019,13 @@ static void process_options(int argc, char *argv[], int max_cpus) > break; > if (optarg) { > parse_cpumask(optarg, max_cpus, &affinity_mask); > - setaffinity = AFFINITY_SPECIFIED; > } else if (optind < argc && > (atoi(argv[optind]) || > argv[optind][0] == '0' || > argv[optind][0] == '!')) { > parse_cpumask(argv[optind], max_cpus, &affinity_mask); > - setaffinity = AFFINITY_SPECIFIED; > - } else { > - setaffinity = AFFINITY_USEALL; > } > > - if (setaffinity == AFFINITY_SPECIFIED && !affinity_mask) > - display_help(1); > if (verbose) > printf("Using %u cpus.\n", > numa_bitmask_weight(affinity_mask)); > @@ -1125,7 +1118,6 @@ static void process_options(int argc, char *argv[], int max_cpus) > case OPT_SMP: /* SMP testing */ > smp = 1; > num_threads = -1; /* update after parsing */ > - setaffinity = AFFINITY_USEALL; > break; > case 't': > case OPT_THREADS: > @@ -1194,23 +1186,16 @@ static void process_options(int argc, char *argv[], int max_cpus) > use_nanosleep = MODE_CLOCK_NANOSLEEP; > } > > - /* if smp wasn't requested, test for numa automatically */ > - if (!smp) { > - if (setaffinity == AFFINITY_UNSPECIFIED) > - setaffinity = AFFINITY_USEALL; > - } > - > if (option_affinity && smp) { > warn("-a ignored due to smp mode\n"); > if (affinity_mask) { > numa_bitmask_free(affinity_mask); > affinity_mask = NULL; > } > - setaffinity = AFFINITY_USEALL; > } > > if (smi) { > - if (setaffinity == AFFINITY_UNSPECIFIED) > + if (affinity_mask) > fatal("SMI counter relies on thread affinity\n"); > > if (!has_smi_counter()) > @@ -1756,7 +1741,7 @@ int main(int argc, char **argv) > printf("Max CPUs = %d\n", max_cpus); > > /* Restrict the main pid to the affinity specified by the user */ > - if (affinity_mask != NULL) { > + if (affinity_mask) { > int res; > > errno = 0; > @@ -1915,17 +1900,12 @@ int main(int argc, char **argv) > if (status != 0) > fatal("error from pthread_attr_init for thread %d: %s\n", i, strerror(status)); > > - switch (setaffinity) { > - case AFFINITY_UNSPECIFIED: cpu = -1; break; > - case AFFINITY_SPECIFIED: > + if (affinity_mask) { > cpu = cpu_for_thread_sp(i, max_cpus, affinity_mask); > if (verbose) > printf("Thread %d using cpu %d.\n", i, cpu); > - break; > - case AFFINITY_USEALL: > + } else { > cpu = cpu_for_thread_ua(i, max_cpus); > - break; > - default: cpu = -1; > } > > /* find the memory node associated with the cpu i */ > -- > 2.29.2 > > -Fixed spelling of "steering" Signed-off-by: John Kacur <jkacur@redhat.com>
On Tue, Jan 26, 2021 at 12:39:02AM -0500, John Kacur wrote:
> Signed-off-by: John Kacur <jkacur@redhat.com>
Is there any reason why you picked this version of the patch? It was
also part of v2?
On Tue, 26 Jan 2021, Daniel Wagner wrote: > On Tue, Jan 26, 2021 at 12:39:02AM -0500, John Kacur wrote: > > Signed-off-by: John Kacur <jkacur@redhat.com> > > Is there any reason why you picked this version of the patch? It was > also part of v2? > I reviewed the correct version of your patch, which I pulled from git, but when I went to reply to it, I replied to the wrong email, sorry for the confusion. John
On Tue, Jan 26, 2021 at 11:33:24AM -0500, John Kacur wrote: > I reviewed the correct version of your patch, which I pulled from git, > but when I went to reply to it, I replied to the wrong email, sorry for > the confusion. Ah, okay. I was very confused :)