On Thu, 14 Dec 2023, John Kacur wrote:
>
>
> On Wed, 6 Dec 2023, Crystal Wood wrote:
>
> > Numerous places threw sign comparison warnings; we could fix them but
> > it's kind of an obnoxious warning that requires casts to deal with things
> > such as ARRAY_SIZE() while still being able to check for the user
> > entering a negative number.
> >
> > -Wunused-parameter is another obnoxious warning as it flags perfectly
> > reasonable code that takes unneeded parameters in order to comply with
> > a function pointer interface or similar; however, all of the instances
> > that were flagged here were actual dead parameters, so just fix them.
> >
> > Add volatile to timer_started in hackbench so that it doesn't get
> > clobbered by longjmp().
> >
> > Signed-off-by: Crystal Wood <crwood@xxxxxxxxxx>
> > --
> > Let me know if you'd rather I fix the sign warnings.
> > ---
> > Makefile | 2 +-
> > src/hackbench/hackbench.c | 2 +-
> > src/sched_deadline/cyclicdeadline.c | 6 +++---
> > src/sched_deadline/deadline_test.c | 10 +++++-----
> > src/sigwaittest/sigwaittest.c | 6 +++---
> > 5 files changed, 13 insertions(+), 13 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index 2808c212058a..ad481a73cf93 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -31,7 +31,7 @@ prefix ?= /usr/local
> > bindir ?= $(prefix)/bin
> > mandir ?= $(prefix)/share/man
> >
> > -CFLAGS ?= -Wall -Wno-nonnull -Wextra
> > +CFLAGS ?= -Wall -Wno-nonnull -Wextra -Wno-sign-compare
> > CPPFLAGS += -D_GNU_SOURCE -Isrc/include
> > LDFLAGS ?=
> >
> > diff --git a/src/hackbench/hackbench.c b/src/hackbench/hackbench.c
> > index 69dd5f087fb6..4430db0e4ed6 100644
> > --- a/src/hackbench/hackbench.c
> > +++ b/src/hackbench/hackbench.c
> > @@ -494,7 +494,7 @@ int main(int argc, char *argv[])
> > struct timeval start, stop, diff;
> > int readyfds[2], wakefds[2];
> > char dummy;
> > - int timer_started = 0;
> > + volatile int timer_started = 0;
> > struct sched_param sp;
> >
> > process_options (argc, argv);
> > diff --git a/src/sched_deadline/cyclicdeadline.c b/src/sched_deadline/cyclicdeadline.c
> > index 9bdc4b5deaf1..097e2e5d4580 100644
> > --- a/src/sched_deadline/cyclicdeadline.c
> > +++ b/src/sched_deadline/cyclicdeadline.c
> > @@ -750,7 +750,7 @@ static void print_stat(FILE *fp, struct sched_data *sd, int index, int verbose,
> > }
> > }
> >
> > -static u64 do_runtime(long tid, struct sched_data *sd, u64 period)
> > +static u64 do_runtime(struct sched_data *sd, u64 period)
> > {
> > struct thread_stat *stat = &sd->stat;
> > u64 next_period = period + sd->deadline_us;
> > @@ -833,7 +833,7 @@ void *run_deadline(void *data)
> > period = get_time_us();
> >
> > while (!shutdown) {
> > - period = do_runtime(tid, sd, period);
> > + period = do_runtime(sd, period);
> > if (tracelimit && (stat->max > tracelimit)) {
> > shutdown++;
> > pthread_mutex_lock(&break_thread_id_lock);
> > @@ -1266,7 +1266,7 @@ int main(int argc, char **argv)
> >
> > /* Make sure that we can make our deadlines */
> > start_period = get_time_us();
> > - do_runtime(gettid(), sd, start_period);
> > + do_runtime(sd, start_period);
> > end_period = get_time_us();
> > if (end_period - start_period > sd->runtime_us)
> > fatal("Failed to perform task within runtime: Missed by %lld us\n",
> > diff --git a/src/sched_deadline/deadline_test.c b/src/sched_deadline/deadline_test.c
> > index cd8ef01f7d68..ca2da476ec95 100644
> > --- a/src/sched_deadline/deadline_test.c
> > +++ b/src/sched_deadline/deadline_test.c
> > @@ -1181,7 +1181,7 @@ static int read_ctx_switches(int *vol, int *nonvol, int *migrate)
> > * @data->total_time - Total time it took to complete all loops
> > * @data->nr_periods - Number of periods that were executed.
> > */
> > -static u64 do_runtime(long tid, struct sched_data *data, u64 period)
> > +static u64 do_runtime(struct sched_data *data, u64 period)
> > {
> > u64 next_period = period + data->deadline_us;
> > u64 now = get_time_us();
> > @@ -1354,7 +1354,7 @@ void *run_deadline(void *data)
> > period = get_time_us();
> >
> > while (!done) {
> > - period = do_runtime(tid, sched_data, period);
> > + period = do_runtime(sched_data, period);
> > sched_yield();
> > }
> > ret = sched_getattr(0, &attr, sizeof(attr), 0);
> > @@ -1714,7 +1714,7 @@ static u64 calculate_loops_per_ms(u64 *overhead)
> > do_sleep(1000);
> >
> > start = get_time_us();
> > - do_runtime(0, &sd, start + sd.deadline_us);
> > + do_runtime(&sd, start + sd.deadline_us);
> > end = get_time_us();
> >
> > diff = end - start;
> > @@ -1743,7 +1743,7 @@ static u64 calculate_loops_per_ms(u64 *overhead)
> > do_sleep(1000);
> >
> > start = get_time_us();
> > - do_runtime(0, &sd, start + sd.deadline_us);
> > + do_runtime(&sd, start + sd.deadline_us);
> > end = get_time_us();
> >
> > odiff = end - start;
> > @@ -1962,7 +1962,7 @@ int main(int argc, char **argv)
> >
> > /* Make sure that we can make our deadlines */
> > start_period = get_time_us();
> > - do_runtime(gettid(), sd, start_period);
> > + do_runtime(sd, start_period);
> > end_period = get_time_us();
> > if (end_period - start_period > sd->runtime_us) {
> > printf("Failed to perform task within runtime: Missed by %lld us\n",
> > diff --git a/src/sigwaittest/sigwaittest.c b/src/sigwaittest/sigwaittest.c
> > index 55855769c63b..8c1c16fb3081 100644
> > --- a/src/sigwaittest/sigwaittest.c
> > +++ b/src/sigwaittest/sigwaittest.c
> > @@ -375,7 +375,7 @@ static void sighand(int sig __attribute__ ((unused)))
> > mustshutdown = 1;
> > }
> >
> > -static void print_stat(FILE *fp, struct params *receiver, struct params *sender,
> > +static void print_stat(struct params *receiver, struct params *sender,
> > int verbose __attribute__ ((unused)), int quiet)
> > {
> > int i;
> > @@ -644,7 +644,7 @@ int main(int argc, char *argv[])
> > sender[i].shutdown;
> >
> > if (receiver[0].samples > oldsamples || mustshutdown) {
> > - print_stat(stdout, receiver, sender, 0, quiet);
> > + print_stat(receiver, sender, 0, quiet);
> > if (!quiet)
> > printf("\033[%dA", num_threads*2);
> > }
> > @@ -664,7 +664,7 @@ int main(int argc, char *argv[])
> > if (!quiet)
> > printf("\033[%dB", num_threads*2 + 2);
> > else
> > - print_stat(stdout, receiver, sender, 0, 0);
> > + print_stat(receiver, sender, 0, 0);
> >
> > for (i = 0; i < num_threads; i++) {
> > receiver[i].shutdown = 1;
> > --
> > 2.43.0
> >
> >
> >
>
> I tested this with rteval, and it works fine
> checkpatch in the linux kernel flags a few minor things.
> Would you mind fixing those up before I apply the patch?
>
> WARNING: Improper SPDX comment style for 'src/include/histogram.h', please
> use '/*' instead
> #227: FILE: src/include/histogram.h:1:
> +// SPDX-License-Identifier: GPL-2.0-or-later
>
> (Yes, I know there are other files in rt-tests that also aren't using
> the recommended commenting style, you probably copied from there.)
>
> WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
> #227: FILE: src/include/histogram.h:1:
> +// SPDX-License-Identifier: GPL-2.0-or-later
>
> WARNING: braces {} are not necessary for any arm of this statement
> #333: FILE: src/lib/histogram.c:59:
> + if (h->oflow_magnitude + extra > h->oflow_magnitude) {
> [...]
> + } else {
> [...]
>
> ERROR: trailing whitespace
> #338: FILE: src/lib/histogram.c:64:
> +^I^I$
>
> WARNING: braces {} are not necessary for any arm of this statement
> #340: FILE: src/lib/histogram.c:66:
> + if (h->oflow_count < h->oflow_bufsize) {
> [...]
> + } else {
> [...]
>
> total: 1 errors, 5 warnings, 394 lines checked
>
> Thanks
>
> John Kacur
>
Sorry, the comments are for patch number 3
John
