[LTP] [PATCH v1 0/2] Fix most severe problems with `rt-migrate.c`

[LTP] [PATCH v1 0/2] Fix most severe problems with `rt-migrate.c`

[Marius Kittler]

This set of patches fixes the most severe problems with
`rt-migrate.c`. I can do further improvements and refactorings if
that is wanted.

Marius Kittler (2):
  Ensure prio is within valid range in `rt-migrate.c`
  Prevent segmentation fault when negative task count specified

 .../realtime/func/rt-migrate/rt-migrate.c     | 25 +++++++++++++++----
 1 file changed, 20 insertions(+), 5 deletions(-)

-- 
2.42.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v1 1/2] Ensure prio is within valid range in `rt-migrate.c`

[Marius Kittler]

* According to the documentation the value param->sched_priority
  must lie within the range given by sched_get_priority_min(2) and
  sched_get_priority_max(2). This change ensures that this is the
  case without completely restructuring the test yet.
* See https://github.com/linux-test-project/ltp/issues/812

Signed-off-by: Marius Kittler <mkittler@suse.de>
---
 .../realtime/func/rt-migrate/rt-migrate.c     | 21 ++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/testcases/realtime/func/rt-migrate/rt-migrate.c b/testcases/realtime/func/rt-migrate/rt-migrate.c
index 97ab604c7..2554f63e2 100644
--- a/testcases/realtime/func/rt-migrate/rt-migrate.c
+++ b/testcases/realtime/func/rt-migrate/rt-migrate.c
@@ -74,6 +74,9 @@
 
 #define VERSION_STRING "V 0.4LTP"
 
+#define CLAMP(x, lower, upper) (MIN(upper, MAX(x, lower)))
+#define CLAMP_PRIO(prio) CLAMP(prio, prio_min, prio_max)
+
 int nr_tasks;
 int lfd;
 
@@ -137,7 +140,7 @@ static unsigned long long interval = INTERVAL;
 static unsigned long long run_interval = RUN_INTERVAL;
 static unsigned long long max_err = MAX_ERR;
 static int nr_runs = NR_RUNS;
-static int prio_start = PRIO_START;
+static int prio_start = PRIO_START, prio_min, prio_max;
 static int check = 1;
 static int stop;
 
@@ -284,8 +287,8 @@ static void print_results(void)
 	printf("Parent pid: %d\n", getpid());
 
 	for (t = 0; t < nr_tasks; t++) {
-		printf(" Task %d (prio %d) (pid %ld):\n", t, t + prio_start,
-		       thread_pids[t]);
+		printf(" Task %d (prio %d) (pid %ld):\n", t,
+			   CLAMP_PRIO(t + prio_start), thread_pids[t]);
 		printf("   Max: %lld us\n", tasks_max[t]);
 		printf("   Min: %lld us\n", tasks_min[t]);
 		printf("   Tot: %lld us\n", tasks_avg[t] * nr_runs);
@@ -394,6 +397,13 @@ static void stop_log(int sig)
 
 int main(int argc, char **argv)
 {
+	/*
+	 * Determine the valid priority range; subtracting one from the
+	 * maximum to reserve the highest prio for main thread.
+	 */
+	prio_min = sched_get_priority_min(SCHED_FIFO);
+	prio_max = sched_get_priority_max(SCHED_FIFO) - 1;
+
 	int *threads;
 	long i;
 	int ret;
@@ -448,7 +458,7 @@ int main(int argc, char **argv)
 
 	for (i = 0; i < nr_tasks; i++) {
 		threads[i] = create_fifo_thread(start_task, (void *)i,
-						prio_start + i);
+						CLAMP_PRIO(prio_start + i));
 	}
 
 	/*
@@ -460,7 +470,8 @@ int main(int argc, char **argv)
 
 	/* up our prio above all tasks */
 	memset(&param, 0, sizeof(param));
-	param.sched_priority = nr_tasks + prio_start;
+	param.sched_priority = CLAMP(nr_tasks + prio_start, prio_min,
+								 prio_max + 1);
 	if (sched_setscheduler(0, SCHED_FIFO, &param))
 		debug(DBG_WARN, "Warning, can't set priority of main thread!\n");
 	intv.tv_sec = INTERVAL / NS_PER_SEC;
-- 
2.42.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v1 2/2] Prevent segmentation fault when negative task count specified

[Marius Kittler]

Signed-off-by: Marius Kittler <mkittler@suse.de>
---
 testcases/realtime/func/rt-migrate/rt-migrate.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/testcases/realtime/func/rt-migrate/rt-migrate.c b/testcases/realtime/func/rt-migrate/rt-migrate.c
index 2554f63e2..252e77e6a 100644
--- a/testcases/realtime/func/rt-migrate/rt-migrate.c
+++ b/testcases/realtime/func/rt-migrate/rt-migrate.c
@@ -419,6 +419,10 @@ int main(int argc, char **argv)
 		numcpus = sysconf(_SC_NPROCESSORS_ONLN);
 		nr_tasks = numcpus + 1;
 	}
+	if (nr_tasks < 0) {
+		printf("The number of tasks must not be negative.\n");
+		exit(EXIT_FAILURE);
+	}
 
 	intervals = malloc(sizeof(stats_container_t) * nr_tasks);
 	if (!intervals)
-- 
2.42.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1 2/2] Prevent segmentation fault when negative task count specified

[Andrea Cervesato via ltp]

Hi!

patch is simple enough to be applied, but this test really needs to be 
refactored, as it's almost untouched for 15 years now and we moved to 
new API long time ago.

Reviewed-by: Andrea Cervesato <andrea.cervesato@suse.com>

On 9/12/23 16:43, Marius Kittler wrote:
> Signed-off-by: Marius Kittler <mkittler@suse.de>
> ---
>   testcases/realtime/func/rt-migrate/rt-migrate.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/testcases/realtime/func/rt-migrate/rt-migrate.c b/testcases/realtime/func/rt-migrate/rt-migrate.c
> index 2554f63e2..252e77e6a 100644
> --- a/testcases/realtime/func/rt-migrate/rt-migrate.c
> +++ b/testcases/realtime/func/rt-migrate/rt-migrate.c
> @@ -419,6 +419,10 @@ int main(int argc, char **argv)
>   		numcpus = sysconf(_SC_NPROCESSORS_ONLN);
>   		nr_tasks = numcpus + 1;
>   	}
> +	if (nr_tasks < 0) {
> +		printf("The number of tasks must not be negative.\n");
> +		exit(EXIT_FAILURE);
> +	}
>   
>   	intervals = malloc(sizeof(stats_container_t) * nr_tasks);
>   	if (!intervals)

Andrea


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1 1/2] Ensure prio is within valid range in `rt-migrate.c`

[Andrea Cervesato via ltp]

Hi!

  I generally suggest to refactor test into new API if a structural 
change is needed. Here we are in a grey zone, because the test is old 
enough to be refactored and patch isn't so big. Still, test is bugged 
and we need to fix _at least_ a small portion of it.

I would suggest to take a look at the code a bit closer and to guess how 
much effort we should put in order to rewrite it with new API. The test 
seems pretty simple, if we get rid of all the old stuff (options, stdout 
management) as we supposed to do anyway.

Regards,
Andrea

On 9/12/23 16:43, Marius Kittler wrote:
> * According to the documentation the value param->sched_priority
>    must lie within the range given by sched_get_priority_min(2) and
>    sched_get_priority_max(2). This change ensures that this is the
>    case without completely restructuring the test yet.
> * See https://github.com/linux-test-project/ltp/issues/812
>
> Signed-off-by: Marius Kittler <mkittler@suse.de>
> ---
>   .../realtime/func/rt-migrate/rt-migrate.c     | 21 ++++++++++++++-----
>   1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/testcases/realtime/func/rt-migrate/rt-migrate.c b/testcases/realtime/func/rt-migrate/rt-migrate.c
> index 97ab604c7..2554f63e2 100644
> --- a/testcases/realtime/func/rt-migrate/rt-migrate.c
> +++ b/testcases/realtime/func/rt-migrate/rt-migrate.c
> @@ -74,6 +74,9 @@
>   
>   #define VERSION_STRING "V 0.4LTP"
>   
> +#define CLAMP(x, lower, upper) (MIN(upper, MAX(x, lower)))
> +#define CLAMP_PRIO(prio) CLAMP(prio, prio_min, prio_max)
> +
>   int nr_tasks;
>   int lfd;
>   
> @@ -137,7 +140,7 @@ static unsigned long long interval = INTERVAL;
>   static unsigned long long run_interval = RUN_INTERVAL;
>   static unsigned long long max_err = MAX_ERR;
>   static int nr_runs = NR_RUNS;
> -static int prio_start = PRIO_START;
> +static int prio_start = PRIO_START, prio_min, prio_max;
>   static int check = 1;
>   static int stop;
>   
> @@ -284,8 +287,8 @@ static void print_results(void)
>   	printf("Parent pid: %d\n", getpid());
>   
>   	for (t = 0; t < nr_tasks; t++) {
> -		printf(" Task %d (prio %d) (pid %ld):\n", t, t + prio_start,
> -		       thread_pids[t]);
> +		printf(" Task %d (prio %d) (pid %ld):\n", t,
> +			   CLAMP_PRIO(t + prio_start), thread_pids[t]);
>   		printf("   Max: %lld us\n", tasks_max[t]);
>   		printf("   Min: %lld us\n", tasks_min[t]);
>   		printf("   Tot: %lld us\n", tasks_avg[t] * nr_runs);
> @@ -394,6 +397,13 @@ static void stop_log(int sig)
>   
>   int main(int argc, char **argv)
>   {
> +	/*
> +	 * Determine the valid priority range; subtracting one from the
> +	 * maximum to reserve the highest prio for main thread.
> +	 */
> +	prio_min = sched_get_priority_min(SCHED_FIFO);
> +	prio_max = sched_get_priority_max(SCHED_FIFO) - 1;
> +
>   	int *threads;
>   	long i;
>   	int ret;
> @@ -448,7 +458,7 @@ int main(int argc, char **argv)
>   
>   	for (i = 0; i < nr_tasks; i++) {
>   		threads[i] = create_fifo_thread(start_task, (void *)i,
> -						prio_start + i);
> +						CLAMP_PRIO(prio_start + i));
>   	}
>   
>   	/*
> @@ -460,7 +470,8 @@ int main(int argc, char **argv)
>   
>   	/* up our prio above all tasks */
>   	memset(&param, 0, sizeof(param));
> -	param.sched_priority = nr_tasks + prio_start;
> +	param.sched_priority = CLAMP(nr_tasks + prio_start, prio_min,
> +								 prio_max + 1);
>   	if (sched_setscheduler(0, SCHED_FIFO, &param))
>   		debug(DBG_WARN, "Warning, can't set priority of main thread!\n");
>   	intv.tv_sec = INTERVAL / NS_PER_SEC;



-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1 1/2] Ensure prio is within valid range in `rt-migrate.c`

[Marius Kittler]

Am Mittwoch, 13. September 2023, 11:11:42 CEST schrieb Andrea Cervesato:
> Hi!
> 
>   I generally suggest to refactor test into new API if a structural
> change is needed.

That is why I refrained from a structural change and implemented the fix with 
the minimum amount of change possible. This way the risk of introducing new 
bugs should be very small and the low "hanging fruit" is grabbed.

> I would suggest to take a look at the code a bit closer and to guess how
> much effort we should put in order to rewrite it with new API.

I guess it would be doable and if that's wanted I can do that as the next step 
as a separate commit. Not sure how long it'll take me, maybe a couple of hours 
or a day (since I'm still new to ltp). (There is no ticket asking about such a 
refactoring explicitly so I honestly don't know whether it is wanted or 
whether we should invest our time better elsewhere.)




-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v1 1/2] Ensure prio is within valid range in `rt-migrate.c`

[Richard Palethorpe]

Hello,

Marius Kittler <mkittler@suse.de> writes:

> Am Mittwoch, 13. September 2023, 11:11:42 CEST schrieb Andrea Cervesato:
>> Hi!
>> 
>>   I generally suggest to refactor test into new API if a structural
>> change is needed.
>
> That is why I refrained from a structural change and implemented the fix with 
> the minimum amount of change possible. This way the risk of introducing new 
> bugs should be very small and the low "hanging fruit" is grabbed.
>
>> I would suggest to take a look at the code a bit closer and to guess how
>> much effort we should put in order to rewrite it with new API.
>
> I guess it would be doable and if that's wanted I can do that as the next step 
> as a separate commit. Not sure how long it'll take me, maybe a couple of hours 
> or a day (since I'm still new to ltp). (There is no ticket asking about such a 
> refactoring explicitly so I honestly don't know whether it is wanted or 
> whether we should invest our time better elsewhere.)

I merged it and the fix looks good. Thanks!

I'm not sure it is worth applying little fixes to these tests without a
rewrite or investigating wether this test is a duplicate, but it's done
now.

-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp