[PATCH] Fix scheduling policy problems for cyclictest

[PATCH] Fix scheduling policy problems for cyclictest

[John Kacur]

These patches fix some problems with the scheduling policies in cyclictest.
In particular, the patches ensure that the expected defaults as specified
in the manpages are correct.

They also ensure that nonsense priorities such as -1 are not possible.

Finally there are some clean-ups, a spelling mistake in the manpage,
spacing in cyclictest.

You can pull these changes from
git://git.kernel.org/pub/scm/linux/kernel/git/jkacur/rt-tests.git
   6214410..1263ebd  rt-tests-dev -> rt-tests-dev

Thanks

John Kacur (4):
  Revert "simplify equal priority logic for cyclictest"
  cyclictest: Use symbolic names for scheduling policy
  cyclictest: Fix spelling mistake in the man page.
  cyclictest: Make the default scheduling policy SCHED_FIFO

 src/cyclictest/cyclictest.8 |    2 +-
 src/cyclictest/cyclictest.c |   23 +++++++----------------
 2 files changed, 8 insertions(+), 17 deletions(-)

[PATCH] Revert "simplify equal priority logic for cyclictest"

[John Kacur]

Unfortunately this commit introduces a bug because the priority is not
retested, and this can result in reported priorities below 0.
For example,

sudo ./cyclictest -t3 -p1
policy: fifo: loadavg: 0.09 0.06 0.05 1/331 21732

T: 0 (21730) P: 1 I:1000 C:    593 Min:     34 Act:  155 Avg:  100 Max:     672
T: 1 (21731) P: 0 I:1500 C:    395 Min:     15 Act:   43 Avg:   72 Max:     853
T: 2 (21732) P:-1 I:2000 C:    297 Min:     21 Act:   57 Avg:   79 Max:     330

Notice that the last priority is reported as -1.

After reverting this commit, we get the correct expected behaviour.

sudo ./cyclictest -t3 -p1
policy: fifo: loadavg: 0.07 0.05 0.04 2/330 21754

T: 0 (21752) P: 1 I:1000 C:  11600 Min:     13 Act: 7072 Avg: 3593 Max:    7841
T: 1 (21753) P: 0 I:1500 C:   7737 Min:     12 Act: 1572 Avg:  516 Max:    2381
T: 2 (21754) P: 0 I:2000 C:   5804 Min:     12 Act:   53 Avg:   59 Max:     548

I think it can be argued that the original code is also clearer, although
that is somewhat subjective. With the original code I don't need to track
down exactly what "sameprio" means, and it is clear what is being tested.

Signed-off-by: John Kacur <jkacur@redhat.com>
---
 src/cyclictest/cyclictest.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
index dc86b49..d38c0a7 100644
--- a/src/cyclictest/cyclictest.c
+++ b/src/cyclictest/cyclictest.c
@@ -801,7 +801,6 @@ static int interval = 1000;
 static int distance = 500;
 static int affinity = 0;
 static int smp = 0;
-static int sameprio = 0;
 
 enum {
 	AFFINITY_UNSPECIFIED,
@@ -1043,9 +1042,7 @@ static void process_options (int argc, char *argv[])
 	if (num_threads < 1)
 		error = 1;
 
-	if (priority && (smp || numa || histogram)) 
-		sameprio = 1;
- 
+
 	if (error)
 		display_help(1);
 }
@@ -1304,7 +1301,7 @@ int main(int argc, char **argv)
 		}
 
 		par->prio = priority;
-		if (!sameprio)
+		if (priority && !histogram && !smp && !numa)
 			priority--;
                 if      (priority && policy <= 1) par->policy = SCHED_FIFO;
                 else if (priority && policy == 2) par->policy = SCHED_RR;
-- 
1.6.0.6

Re: [PATCH] Revert "simplify equal priority logic for cyclictest"

[Carsten Emde]

On 03/08/2010 02:18 AM, John Kacur wrote:
> Unfortunately this commit introduces a bug because the priority is not
> retested, and this can result in reported priorities below 0.
> [..]
> I think it can be argued that the original code is also clearer, although
> that is somewhat subjective. With the original code I don't need to track
> down exactly what "sameprio" means, and it is clear what is being tested.
> 
> Signed-off-by: John Kacur <jkacur@redhat.com>
Acked-by: Carsten Emde <C.Emde@osadl.org>

[PATCH] cyclictest: Use symbolic names for scheduling policy

[John Kacur]

- Use symbolic names for scheduling policies, that is, don't assume
	SCHED_RR is 2, use SCHED_RR instead, and so on.

- Fix the logic in handlepolicy(char *polname)
	- remove the test with the unreachable line,
	- make the default SCHED_FIFO if we don't recognize the
	requested policy.

Signed-off-by: John Kacur <jkacur@redhat.com>
---
 src/cyclictest/cyclictest.c |   15 +++++----------
 1 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
index d38c0a7..066ca79 100644
--- a/src/cyclictest/cyclictest.c
+++ b/src/cyclictest/cyclictest.c
@@ -826,13 +826,8 @@ static void handlepolicy(char *polname)
 		policy = SCHED_FIFO;
 	else if (strncasecmp(polname, "rr", 2) == 0)
 		policy = SCHED_RR;
-
-	if (policy == SCHED_FIFO || policy == SCHED_RR) {
-		if (policy == 0)
-			policy = 1;
-	}
-	else 
-		policy = 0;
+	else	/* default policy if we don't recognize the request */
+		policy = SCHED_FIFO;
 }
 
 static char *policyname(int policy)
@@ -1303,9 +1298,9 @@ int main(int argc, char **argv)
 		par->prio = priority;
 		if (priority && !histogram && !smp && !numa)
 			priority--;
-                if      (priority && policy <= 1) par->policy = SCHED_FIFO;
-                else if (priority && policy == 2) par->policy = SCHED_RR;
-                else                              par->policy = SCHED_OTHER;
+                if (priority && policy == SCHED_FIFO) par->policy = SCHED_FIFO;
+                else if (priority && policy == SCHED_RR) par->policy = SCHED_RR;
+                else  par->policy = SCHED_OTHER;
 		par->clock = clocksources[clocksel];
 		par->mode = mode;
 		par->timermode = timermode;
-- 
1.6.0.6

Re: [PATCH] cyclictest: Use symbolic names for scheduling policy

[Carsten Emde]

[-- Attachment #1: Type: text/plain, Size: 1918 bytes --]

On 03/08/2010 02:18 AM, John Kacur wrote:
> - Use symbolic names for scheduling policies, that is, don't assume
> 	SCHED_RR is 2, use SCHED_RR instead, and so on.
> 
> - Fix the logic in handlepolicy(char *polname)
> 	- remove the test with the unreachable line,
> 	- make the default SCHED_FIFO if we don't recognize the
> 	requested policy.
> 
> Signed-off-by: John Kacur <jkacur@redhat.com>
> ---
>  src/cyclictest/cyclictest.c |   15 +++++----------
>  1 files changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
> index d38c0a7..066ca79 100644
> --- a/src/cyclictest/cyclictest.c
> +++ b/src/cyclictest/cyclictest.c
> @@ -826,13 +826,8 @@ static void handlepolicy(char *polname)
>  		policy = SCHED_FIFO;
>  	else if (strncasecmp(polname, "rr", 2) == 0)
>  		policy = SCHED_RR;
> -
> -	if (policy == SCHED_FIFO || policy == SCHED_RR) {
> -		if (policy == 0)
> -			policy = 1;
> -	}
> -	else 
> -		policy = 0;
> +	else	/* default policy if we don't recognize the request */
> +		policy = SCHED_FIFO;
>  }
>  
>  static char *policyname(int policy)
> @@ -1303,9 +1298,9 @@ int main(int argc, char **argv)
>  		par->prio = priority;
>  		if (priority && !histogram && !smp && !numa)
>  			priority--;
> -                if      (priority && policy <= 1) par->policy = SCHED_FIFO;
> -                else if (priority && policy == 2) par->policy = SCHED_RR;
> -                else                              par->policy = SCHED_OTHER;
> +                if (priority && policy == SCHED_FIFO) par->policy = SCHED_FIFO;
> +                else if (priority && policy == SCHED_RR) par->policy = SCHED_RR;
> +                else  par->policy = SCHED_OTHER;
>  		par->clock = clocksources[clocksel];
>  		par->mode = mode;
>  		par->timermode = timermode;
We need to adapt the help message accordingly.

Signed-off-by: Carsten Emde <C.Emde@osadl.org>


[-- Attachment #2: adapt-policy-help-message-for-cyclictest.patch --]
[-- Type: text/x-patch, Size: 1092 bytes --]

Index: rt-tests/src/cyclictest/cyclictest.c
===================================================================
--- rt-tests.orig/src/cyclictest/cyclictest.c
+++ rt-tests/src/cyclictest/cyclictest.c
@@ -776,8 +776,8 @@ static void display_help(int error)
 	       "                           format: n:c:v n=tasknum c=count v=value in us\n"
                "-w       --wakeup          task wakeup tracing (used with -b)\n"
                "-W       --wakeuprt        rt task wakeup tracing (used with -b)\n"
-               "-y POLI  --policy=POLI     policy of realtime thread (1:FIFO, 2:RR)\n"
-               "                           format: --policy=fifo(default) or --policy=rr\n"
+               "-y POLI  --policy=POLI     policy of realtime thread, POLI may be FIFO (default)\n"
+               "                           or RR\n"
 	       "-S       --smp             Standard SMP testing (equals -a -t -n -m -d0)\n"
                "                           same priority on all threads.\n"
 	       "-U       --numa            Standard NUMA testing (similar to SMP option)\n"

Re: [PATCH] cyclictest: Use symbolic names for scheduling policy

[John Kacur]

On Mon, Mar 8, 2010 at 9:35 AM, Carsten Emde <Carsten.Emde@osadl.org> wrote:
> On 03/08/2010 02:18 AM, John Kacur wrote:
>> - Use symbolic names for scheduling policies, that is, don't assume
>>       SCHED_RR is 2, use SCHED_RR instead, and so on.
>>
>> - Fix the logic in handlepolicy(char *polname)
>>       - remove the test with the unreachable line,
>>       - make the default SCHED_FIFO if we don't recognize the
>>       requested policy.
>>
>> Signed-off-by: John Kacur <jkacur@redhat.com>
>> ---
>>  src/cyclictest/cyclictest.c |   15 +++++----------
>>  1 files changed, 5 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
>> index d38c0a7..066ca79 100644
>> --- a/src/cyclictest/cyclictest.c
>> +++ b/src/cyclictest/cyclictest.c
>> @@ -826,13 +826,8 @@ static void handlepolicy(char *polname)
>>               policy = SCHED_FIFO;
>>       else if (strncasecmp(polname, "rr", 2) == 0)
>>               policy = SCHED_RR;
>> -
>> -     if (policy == SCHED_FIFO || policy == SCHED_RR) {
>> -             if (policy == 0)
>> -                     policy = 1;
>> -     }
>> -     else
>> -             policy = 0;
>> +     else    /* default policy if we don't recognize the request */
>> +             policy = SCHED_FIFO;
>>  }
>>
>>  static char *policyname(int policy)
>> @@ -1303,9 +1298,9 @@ int main(int argc, char **argv)
>>               par->prio = priority;
>>               if (priority && !histogram && !smp && !numa)
>>                       priority--;
>> -                if      (priority && policy <= 1) par->policy = SCHED_FIFO;
>> -                else if (priority && policy == 2) par->policy = SCHED_RR;
>> -                else                              par->policy = SCHED_OTHER;
>> +                if (priority && policy == SCHED_FIFO) par->policy = SCHED_FIFO;
>> +                else if (priority && policy == SCHED_RR) par->policy = SCHED_RR;
>> +                else  par->policy = SCHED_OTHER;
>>               par->clock = clocksources[clocksel];
>>               par->mode = mode;
>>               par->timermode = timermode;
> We need to adapt the help message accordingly.
>
> Signed-off-by: Carsten Emde <C.Emde@osadl.org>
>
>

Hmnn, not sure if I like this one. The message is not perfect as it
stands, but I don't think this is an improvement.
The code seems to parse, fifo, rr, and other.
Internally the numbers are as specified, but we're not parsing them anyway.

Thanks
--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] cyclictest: Use symbolic names for scheduling policy

[Carsten Emde]

Hi John,

>> [..]
>> We need to adapt the help message accordingly.
> Hmnn, not sure if I like this one. The message is not perfect as it
> stands, but I don't think this is an improvement.
> The code seems to parse, fifo, rr, and other.
> Internally the numbers are as specified, but we're not parsing them anyway.
The original help message says:

-y POLI  --policy=POLI     policy of realtime thread (1:FIFO, 2:RR)
                           format: --policy=fifo(default) or --policy=rr

This *could* be understood that "-y 2" would be a legal way to specify
SCHED_RR, but it is not. Shouldn't we at least remove the "(1:FIFO,
2:RR)" part?

	Carsten.

Re: [PATCH] cyclictest: Use symbolic names for scheduling policy

[John Kacur]

On Mon, Mar 8, 2010 at 2:01 PM, Carsten Emde <Carsten.Emde@osadl.org> wrote:
> Hi John,
>
>>> [..]
>>> We need to adapt the help message accordingly.
>> Hmnn, not sure if I like this one. The message is not perfect as it
>> stands, but I don't think this is an improvement.
>> The code seems to parse, fifo, rr, and other.
>> Internally the numbers are as specified, but we're not parsing them anyway.
> The original help message says:
>
> -y POLI  --policy=POLI     policy of realtime thread (1:FIFO, 2:RR)
>                           format: --policy=fifo(default) or --policy=rr
>
> This *could* be understood that "-y 2" would be a legal way to specify
> SCHED_RR, but it is not. Shouldn't we at least remove the "(1:FIFO,
> 2:RR)" part?
>
>        Carsten.

Yes, I agree to that, since what the internal number is, is of no
significance to
the user.

Thanks,
John
--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] cyclictest: Fix spelling mistake in the man page.

[John Kacur]

	- In the -mlockall section, change "an" to "and"

Signed-off-by: John Kacur <jkacur@redhat.com>
---
 src/cyclictest/cyclictest.8 |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/cyclictest/cyclictest.8 b/src/cyclictest/cyclictest.8
index 91e1102..a6ddb92 100644
--- a/src/cyclictest/cyclictest.8
+++ b/src/cyclictest/cyclictest.8
@@ -134,7 +134,7 @@ Set the number of test threads (default is 1). Create NUM test threads. If NUM i
 the number of available CPUs. See \-d, \-i and \-p for further information.
 .TP
 .B \-m, \-\-mlockall
-Lock current an future memory allocations to prevent being paged out
+Lock current and future memory allocations to prevent being paged out
 .TP
 .B \-v, \-\-verbose
 Output values on stdout for statistics. This option is used to gather statistical information about the latency distribution. The output is sent to stdout. The output format is:
-- 
1.6.0.6

[PATCH] cyclictest: Make the default scheduling policy SCHED_FIFO

[John Kacur]

The default scheduling policy if unspecified should be SCHED_FIFO.

Before the change for example.
 sudo ./cyclictest
 policy: other: loadavg: 0.05 0.04 0.05 1/331 22367

 T: 0 (22367) P: 0 I:1000 C:   1321 Min:     14 Act:   89 Avg:   77 Max:     942

After the change
 sudo ./cyclictest
 defaulting realtime priority to 2
 policy: fifo: loadavg: 0.03 0.04 0.05 2/331 22387

 T: 0 (22387) P: 2 I:1000 C:    713 Min:     17 Act:   41 Avg:   81 Max:     161

Signed-off-by: John Kacur <jkacur@redhat.com>
---
 src/cyclictest/cyclictest.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
index 066ca79..e4febec 100644
--- a/src/cyclictest/cyclictest.c
+++ b/src/cyclictest/cyclictest.c
@@ -792,7 +792,7 @@ static int use_nanosleep;
 static int timermode = TIMER_ABSTIME;
 static int use_system;
 static int priority;
-static int policy = 0;
+static int policy = SCHED_FIFO;	/* default policy if not specified */
 static int num_threads = 1;
 static int max_cycles;
 static int clocksel = 0;
@@ -1037,7 +1037,6 @@ static void process_options (int argc, char *argv[])
 	if (num_threads < 1)
 		error = 1;