Little bug in cyclictest

Little bug in cyclictest

[Rachid Koucha]

Hi,

First of all, many thanks for such useful tools.

In the source code of cyclictest, I can see a mistake concerning the
error checking of pthread_setaffinity_np():

Upon error, pthread functions return an error ant not -1. So, the
following code:

    if (par->cpu != -1) {
        CPU_ZERO(&mask);
        CPU_SET(par->cpu, &mask);
        thread = pthread_self();
        if (pthread_setaffinity_np(thread, sizeof(mask), &mask) == -1)
            warn("Could not set CPU affinity to CPU #%d\n",
                 par->cpu);
    }

should be fixed as follow:

int status;

    if (par->cpu != -1) {
        CPU_ZERO(&mask);
        CPU_SET(par->cpu, &mask);
        thread = pthread_self();
        if ((status = pthread_setaffinity_np(thread, sizeof(mask), &mask)) != 0)
            warn("Could not set CPU affinity to CPU #%d: %s (%d)\n",                  par->cpu, strerror(status), status);
    }


Regards,

-- 
Rachid Koucha
 

Re: Little bug in cyclictest

[John Kacur]

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



On Wed, 14 Aug 2019, Rachid Koucha wrote:

> Hi,
> 
> First of all, many thanks for such useful tools.
> 
> In the source code of cyclictest, I can see a mistake concerning the
> error checking of pthread_setaffinity_np():
> 
> Upon error, pthread functions return an error ant not -1. So, the
> following code:
> 
>     if (par->cpu != -1) {
>         CPU_ZERO(&mask);
>         CPU_SET(par->cpu, &mask);
>         thread = pthread_self();
>         if (pthread_setaffinity_np(thread, sizeof(mask), &mask) == -1)
>             warn("Could not set CPU affinity to CPU #%d\n",
>                  par->cpu);
>     }
> 
> should be fixed as follow:
> 
> int status;
> 
>     if (par->cpu != -1) {
>         CPU_ZERO(&mask);
>         CPU_SET(par->cpu, &mask);
>         thread = pthread_self();
>         if ((status = pthread_setaffinity_np(thread, sizeof(mask), &mask)) != 0)
>             warn("Could not set CPU affinity to CPU #%d: %s (%d)\n",                  par->cpu, strerror(status), status);
>     }
> 
> 
> Regards,
> 

Thanks, that looks correct. Please generate a patch and I'll apply it.

John Kacur

Re: Little bug in cyclictest

[Rachid Koucha]

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

Find attach the patch file :

$ cd rt-tests
$ patch -p1 < ../cyclictest_affinity_check.patch

On 14/08/2019 15:25, John Kacur wrote:
>
> On Wed, 14 Aug 2019, Rachid Koucha wrote:
>
>> Hi,
>>
>> First of all, many thanks for such useful tools.
>>
>> In the source code of cyclictest, I can see a mistake concerning the
>> error checking of pthread_setaffinity_np():
>>
>> Upon error, pthread functions return an error ant not -1. So, the
>> following code:
>>
>>     if (par->cpu != -1) {
>>         CPU_ZERO(&mask);
>>         CPU_SET(par->cpu, &mask);
>>         thread = pthread_self();
>>         if (pthread_setaffinity_np(thread, sizeof(mask), &mask) == -1)
>>             warn("Could not set CPU affinity to CPU #%d\n",
>>                  par->cpu);
>>     }
>>
>> should be fixed as follow:
>>
>> int status;
>>
>>     if (par->cpu != -1) {
>>         CPU_ZERO(&mask);
>>         CPU_SET(par->cpu, &mask);
>>         thread = pthread_self();
>>         if ((status = pthread_setaffinity_np(thread, sizeof(mask), &mask)) != 0)
>>             warn("Could not set CPU affinity to CPU #%d: %s (%d)\n",                  par->cpu, strerror(status), status);
>>     }
>>
>>
>> Regards,
>>
> Thanks, that looks correct. Please generate a patch and I'll apply it.
>
> John Kacur


-- 


[-- Attachment #2: cyclictest_affinity_check.patch --]
[-- Type: text/x-diff, Size: 833 bytes --]

--- old_rt-tests/src/cyclictest/cyclictest.c	2019-08-14 20:28:48.547725419 +0200
+++ new_rt-tests/src/cyclictest/cyclictest.c	2019-08-14 20:20:58.415297592 +0200
@@ -985,6 +985,7 @@
 	cpu_set_t mask;
 	pthread_t thread;
 	unsigned long smi_now, smi_old;
+        int status;
 
 	/* if we're running in numa mode, set our memory node */
 	if (par->node != -1)
@@ -994,9 +995,9 @@
 		CPU_ZERO(&mask);
 		CPU_SET(par->cpu, &mask);
 		thread = pthread_self();
-		if (pthread_setaffinity_np(thread, sizeof(mask), &mask) == -1)
-			warn("Could not set CPU affinity to CPU #%d\n",
-			     par->cpu);
+		if ((status = pthread_setaffinity_np(thread, sizeof(mask), &mask)) != 0)
+			warn("Could not set CPU affinity to CPU #%d: %s (%d)\n",
+			     par->cpu, strerror(status), status);
 	}
 
 	interval.tv_sec = par->interval / USEC_PER_SEC;

Re: Little bug in cyclictest

[John Kacur]

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



On Wed, 14 Aug 2019, Rachid Koucha wrote:

> Find attach the patch file :
> 
> $ cd rt-tests
> $ patch -p1 < ../cyclictest_affinity_check.patch
> 
> On 14/08/2019 15:25, John Kacur wrote:
> >
> > On Wed, 14 Aug 2019, Rachid Koucha wrote:
> >
> >> Hi,
> >>
> >> First of all, many thanks for such useful tools.
> >>
> >> In the source code of cyclictest, I can see a mistake concerning the
> >> error checking of pthread_setaffinity_np():
> >>
> >> Upon error, pthread functions return an error ant not -1. So, the
> >> following code:
> >>
> >>     if (par->cpu != -1) {
> >>         CPU_ZERO(&mask);
> >>         CPU_SET(par->cpu, &mask);
> >>         thread = pthread_self();
> >>         if (pthread_setaffinity_np(thread, sizeof(mask), &mask) == -1)
> >>             warn("Could not set CPU affinity to CPU #%d\n",
> >>                  par->cpu);
> >>     }
> >>
> >> should be fixed as follow:
> >>
> >> int status;
> >>
> >>     if (par->cpu != -1) {
> >>         CPU_ZERO(&mask);
> >>         CPU_SET(par->cpu, &mask);
> >>         thread = pthread_self();
> >>         if ((status = pthread_setaffinity_np(thread, sizeof(mask), &mask)) != 0)
> >>             warn("Could not set CPU affinity to CPU #%d: %s (%d)\n",                  par->cpu, strerror(status), status);
> >>     }
> >>
> >>
> >> Regards,
> >>
> > Thanks, that looks correct. Please generate a patch and I'll apply it.
> >
> > John Kacur
> 

It would be easy for me to take the attachment and apply the patch, but if 
you want to work or play in the open source world, you have to follow 
certain rules for submitting patches.

The Linux Kernel source has a lot of documentation on how to do this, look 
here if you don't know how to access that

https://www.kernel.org/doc/html/v5.0/process/submitting-patches.html

So, you're going to need to describe the changes, and give your 
signed-off-by and no attachments, you have to use plain text.

Thanks!

John Kacur