[PATCH 0/2] [rt-tests] Two minor fixes of cyclictest

[PATCH 0/2] [rt-tests] Two minor fixes of cyclictest

[Carsten Emde]

These patches prevent a priority < 1 and fix an incorrect help message
in cyclictest.

[PATCH 1/2] do-not-allow-prio-less-than-one-in-cyclictest.patch

[Carsten Emde]

[-- Attachment #1: do-not-allow-prio-below-one-in-cyclictest.patch --]
[-- Type: text/plain, Size: 808 bytes --]

If not in SMP testing mode, the priority may go below 1, if the specified
priority is lower than the number of threads, e.g.
# cyclictest -p2 -t3
T: 0 (21970) P: 2 [..]
T: 1 (21971) P: 1 [..]
T: 2 (21972) P: 0 [..]

Do not allow priority to go below 1.

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

Index: rt-tests/src/cyclictest/cyclictest.c
===================================================================
--- rt-tests.orig/src/cyclictest/cyclictest.c
+++ rt-tests/src/cyclictest/cyclictest.c
@@ -1304,7 +1304,7 @@ int main(int argc, char **argv)
 		}
 
 		par->prio = priority;
-		if (!sameprio)
+		if (priority > 1 && !sameprio)
 			priority--;
                 if      (priority && policy <= 1) par->policy = SCHED_FIFO;
                 else if (priority && policy == 2) par->policy = SCHED_RR;

[PATCH 2/2] remove-incorrect-options-from-smp-help-message-in-cyclictest.patch

[Carsten Emde]

[-- Attachment #1: remove-incorrect-options-from-smp-help-message-in-cyclictest.patch --]
[-- Type: text/plain, Size: 1210 bytes --]

The help message of cyclictest's -S option says that it equals -a -t -n -m -d0.
In reality, it only equals -a -t -n.

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

Index: rt-tests/src/cyclictest/cyclictest.c
===================================================================
--- rt-tests.orig/src/cyclictest/cyclictest.c
+++ rt-tests/src/cyclictest/cyclictest.c
@@ -777,8 +777,8 @@ static void display_help(int error)
                "-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"
-	       "-S       --smp             Standard SMP testing (equals -a -t -n -m -d0)\n"
-               "                           same priority on all threads.\n"
+	       "-S       --smp             Standard SMP testing: options -a -t -n and\n"
+               "                           same priority of all threads\n"
 	       "-U       --numa            Standard NUMA testing (similar to SMP option)\n"
                "                           thread data structures allocated from local node\n",
 	       tracers

Re: [PATCH 1/2] do-not-allow-prio-less-than-one-in-cyclictest.patch

[John Kacur]

On Sun, Mar 7, 2010 at 9:39 PM, Carsten Emde <C.Emde@osadl.org> wrote:
> If not in SMP testing mode, the priority may go below 1, if the specified
> priority is lower than the number of threads, e.g.
> # cyclictest -p2 -t3
> T: 0 (21970) P: 2 [..]
> T: 1 (21971) P: 1 [..]
> T: 2 (21972) P: 0 [..]
>
> Do not allow priority to go below 1.
>
> Signed-off-by: Carsten Emde <C.Emde@osadl.org>
>
> Index: rt-tests/src/cyclictest/cyclictest.c
> ===================================================================
> --- rt-tests.orig/src/cyclictest/cyclictest.c
> +++ rt-tests/src/cyclictest/cyclictest.c
> @@ -1304,7 +1304,7 @@ int main(int argc, char **argv)
>                }
>
>                par->prio = priority;
> -               if (!sameprio)
> +               if (priority > 1 && !sameprio)
>                        priority--;
>                 if      (priority && policy <= 1) par->policy = SCHED_FIFO;
>                 else if (priority && policy == 2) par->policy = SCHED_RR;
>
> --

I'm not sure about this, why not allow a priority below 1? The code
below properly sets the third thead to SCHED_OTHER.
I could imagine wanting to test that too. If you don't want to go
below 1 then just set a higher prio, p3 in the scenario you
showed.

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

Re: [PATCH 2/2] remove-incorrect-options-from-smp-help-message-in-cyclictest.patch

[John Kacur]

On Sun, Mar 7, 2010 at 9:39 PM, Carsten Emde <C.Emde@osadl.org> wrote:
> The help message of cyclictest's -S option says that it equals -a -t -n -m -d0.
> In reality, it only equals -a -t -n.
>
> Signed-off-by: Carsten Emde <C.Emde@osadl.org>
>
> Index: rt-tests/src/cyclictest/cyclictest.c
> ===================================================================
> --- rt-tests.orig/src/cyclictest/cyclictest.c
> +++ rt-tests/src/cyclictest/cyclictest.c
> @@ -777,8 +777,8 @@ static void display_help(int error)
>                "-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"
> -              "-S       --smp             Standard SMP testing (equals -a -t -n -m -d0)\n"
> -               "                           same priority on all threads.\n"
> +              "-S       --smp             Standard SMP testing: options -a -t -n and\n"
> +               "                           same priority of all threads\n"
>               "-U       --numa            Standard NUMA testing (similar to SMP option)\n"
>                "                           thread data structures allocated from local node\n",
>               tracers
>
> --

Wow, you're right. I wonder if the intention here was to set -m and
-d0 for -S, and if we instead need a patch to do exactly
that if the user selects -S.

Certainly a default of -d0 makes sense for smp, and it probably
doesn't make sense to let the user override it either.
However, I'm not sure if -m by default is a good choice to make for
-S. I suggest we make -d0 the default for -S, and
then see if anyone has an opinion about whether -m should be the
default for -S. My vote is no, so then we would need
to correct the documentation in that case.

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

Re: [PATCH 2/2] remove-incorrect-options-from-smp-help-message-in-cyclictest.patch

[John Kacur]

On Mon, Mar 8, 2010 at 12:10 AM, John Kacur <jkacur@redhat.com> wrote:
> On Sun, Mar 7, 2010 at 9:39 PM, Carsten Emde <C.Emde@osadl.org> wrote:
>> The help message of cyclictest's -S option says that it equals -a -t -n -m -d0.
>> In reality, it only equals -a -t -n.
>>
>> Signed-off-by: Carsten Emde <C.Emde@osadl.org>
>>
>> Index: rt-tests/src/cyclictest/cyclictest.c
>> ===================================================================
>> --- rt-tests.orig/src/cyclictest/cyclictest.c
>> +++ rt-tests/src/cyclictest/cyclictest.c
>> @@ -777,8 +777,8 @@ static void display_help(int error)
>>                "-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"
>> -              "-S       --smp             Standard SMP testing (equals -a -t -n -m -d0)\n"
>> -               "                           same priority on all threads.\n"
>> +              "-S       --smp             Standard SMP testing: options -a -t -n and\n"
>> +               "                           same priority of all threads\n"
>>               "-U       --numa            Standard NUMA testing (similar to SMP option)\n"
>>                "                           thread data structures allocated from local node\n",
>>               tracers
>>
>> --
>
> Wow, you're right. I wonder if the intention here was to set -m and
> -d0 for -S, and if we instead need a patch to do exactly
> that if the user selects -S.
>
> Certainly a default of -d0 makes sense for smp, and it probably
> doesn't make sense to let the user override it either.
> However, I'm not sure if -m by default is a good choice to make for
> -S. I suggest we make -d0 the default for -S, and
> then see if anyone has an opinion about whether -m should be the
> default for -S. My vote is no, so then we would need
> to correct the documentation in that case.
>

Okay, looking at the history of cyclictest in git,
77abded073c5d11d118112c46c22a84ddc29e771
indicates that the intention was to set just -t -a and -n, so in that case
Acked-by: John Kacur <jkacur@redhat.com>
--
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 1/2] do-not-allow-prio-less-than-one-in-cyclictest.patch

[David Sommerseth]

On 07/03/10 23:37, John Kacur wrote:
> On Sun, Mar 7, 2010 at 9:39 PM, Carsten Emde <C.Emde@osadl.org> wrote:
>> If not in SMP testing mode, the priority may go below 1, if the specified
>> priority is lower than the number of threads, e.g.
>> # cyclictest -p2 -t3
>> T: 0 (21970) P: 2 [..]
>> T: 1 (21971) P: 1 [..]
>> T: 2 (21972) P: 0 [..]
>>
>> Do not allow priority to go below 1.
>>
>> Signed-off-by: Carsten Emde <C.Emde@osadl.org>
>>
>> Index: rt-tests/src/cyclictest/cyclictest.c
>> ===================================================================
>> --- rt-tests.orig/src/cyclictest/cyclictest.c
>> +++ rt-tests/src/cyclictest/cyclictest.c
>> @@ -1304,7 +1304,7 @@ int main(int argc, char **argv)
>>                }
>>
>>                par->prio = priority;
>> -               if (!sameprio)
>> +               if (priority > 1 && !sameprio)
>>                        priority--;
>>                 if      (priority && policy <= 1) par->policy = SCHED_FIFO;
>>                 else if (priority && policy == 2) par->policy = SCHED_RR;
>>
>> --
> 
> I'm not sure about this, why not allow a priority below 1? The code
> below properly sets the third thead to SCHED_OTHER.
> I could imagine wanting to test that too. If you don't want to go
> below 1 then just set a higher prio, p3 in the scenario you
> showed.

Maybe I'm misreading and misunderstanding the patch ... but I believe
the if statement should say:

	if (priority > 0 && !sameprio)
		priority--;

Just to avoid the situation the commit log says, priority to go below 0.


kind regards,

David Sommerseth

Re: [PATCH 1/2] do-not-allow-prio-less-than-one-in-cyclictest.patch

[John Kacur]

On Tue, Mar 9, 2010 at 7:02 PM, David Sommerseth <davids@redhat.com> wrote:
> On 07/03/10 23:37, John Kacur wrote:
>> On Sun, Mar 7, 2010 at 9:39 PM, Carsten Emde <C.Emde@osadl.org> wrote:
>>> If not in SMP testing mode, the priority may go below 1, if the specified
>>> priority is lower than the number of threads, e.g.
>>> # cyclictest -p2 -t3
>>> T: 0 (21970) P: 2 [..]
>>> T: 1 (21971) P: 1 [..]
>>> T: 2 (21972) P: 0 [..]
>>>
>>> Do not allow priority to go below 1.
>>>
>>> Signed-off-by: Carsten Emde <C.Emde@osadl.org>
>>>
>>> Index: rt-tests/src/cyclictest/cyclictest.c
>>> ===================================================================
>>> --- rt-tests.orig/src/cyclictest/cyclictest.c
>>> +++ rt-tests/src/cyclictest/cyclictest.c
>>> @@ -1304,7 +1304,7 @@ int main(int argc, char **argv)
>>>                }
>>>
>>>                par->prio = priority;
>>> -               if (!sameprio)
>>> +               if (priority > 1 && !sameprio)
>>>                        priority--;
>>>                 if      (priority && policy <= 1) par->policy = SCHED_FIFO;
>>>                 else if (priority && policy == 2) par->policy = SCHED_RR;
>>>
>>> --
>>
>> I'm not sure about this, why not allow a priority below 1? The code
>> below properly sets the third thead to SCHED_OTHER.
>> I could imagine wanting to test that too. If you don't want to go
>> below 1 then just set a higher prio, p3 in the scenario you
>> showed.
>
> Maybe I'm misreading and misunderstanding the patch ... but I believe
> the if statement should say:
>
>        if (priority > 0 && !sameprio)
>                priority--;
>
> Just to avoid the situation the commit log says, priority to go below 0.
>
>
> kind regards,
>
> David Sommerseth

The code in general there was buggy, so I reverted the patch that
caused the problems in the first place.
(assuming Clark merges it).

Now the code will read (as it did before)

 if (priority && !histogram && !smp && !numa)
                        priority--;
--
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 1/2] do-not-allow-prio-less-than-one-in-cyclictest.patch

[David Sommerseth]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/03/10 19:12, John Kacur wrote:
> On Tue, Mar 9, 2010 at 7:02 PM, David Sommerseth <davids@redhat.com> wrote:
>> On 07/03/10 23:37, John Kacur wrote:
>>> On Sun, Mar 7, 2010 at 9:39 PM, Carsten Emde <C.Emde@osadl.org> wrote:
>>>> If not in SMP testing mode, the priority may go below 1, if the specified
>>>> priority is lower than the number of threads, e.g.
>>>> # cyclictest -p2 -t3
>>>> T: 0 (21970) P: 2 [..]
>>>> T: 1 (21971) P: 1 [..]
>>>> T: 2 (21972) P: 0 [..]
>>>>
>>>> Do not allow priority to go below 1.
>>>>
>>>> Signed-off-by: Carsten Emde <C.Emde@osadl.org>
>>>>
>>>> Index: rt-tests/src/cyclictest/cyclictest.c
>>>> ===================================================================
>>>> --- rt-tests.orig/src/cyclictest/cyclictest.c
>>>> +++ rt-tests/src/cyclictest/cyclictest.c
>>>> @@ -1304,7 +1304,7 @@ int main(int argc, char **argv)
>>>>                }
>>>>
>>>>                par->prio = priority;
>>>> -               if (!sameprio)
>>>> +               if (priority > 1 && !sameprio)
>>>>                        priority--;
>>>>                 if      (priority && policy <= 1) par->policy = SCHED_FIFO;
>>>>                 else if (priority && policy == 2) par->policy = SCHED_RR;
>>>>
>>>> --
>>>
>>> I'm not sure about this, why not allow a priority below 1? The code
>>> below properly sets the third thead to SCHED_OTHER.
>>> I could imagine wanting to test that too. If you don't want to go
>>> below 1 then just set a higher prio, p3 in the scenario you
>>> showed.
>>
>> Maybe I'm misreading and misunderstanding the patch ... but I believe
>> the if statement should say:
>>
>>        if (priority > 0 && !sameprio)
>>                priority--;
>>
>> Just to avoid the situation the commit log says, priority to go below 0.
>>
>>
>> kind regards,
>>
>> David Sommerseth
> 
> The code in general there was buggy, so I reverted the patch that
> caused the problems in the first place.
> (assuming Clark merges it).
> 
> Now the code will read (as it did before)
> 
>  if (priority && !histogram && !smp && !numa)
>                         priority--;

That looks better ... and guess what, I just stumbled upon those patches
right now in the mailing list ... the troubles of catching up after
holidays :)


kind regards,

David Sommerseth

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkuWkAAACgkQIIWEatLf4HeRiQCeMLgoLJHvzHwS+8yRroGBOAAX
zXwAnAmXlY4AFgIGRAzi+33nNwukXdCF
=cPhd
-----END PGP SIGNATURE-----