[PATCH] Synchronization required before release the lock: sem_post/8-1.c

[PATCH] Synchronization required before release the lock: sem_post/8-1.c

[naresh kamboju]

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

Hi,

I have found abnormal behavior of sem_post/8-1.c test case under posix.
This test case passes in some times and failed in many times :-(

After my investigation found synchronization is missing between the
child processes.
Made a patch to fix this issue.

Patch includes
1.	Reverting back changes made by mreed on Sep 25 2006. Making sure
child has been waiting for the lock (below Refs).
2.	using sleep in while loop is not a good idea, so sleep is removed
from while loop
3.	For the synchronization I have added sleep before releasing the lock.


After applying this patch I have tested this test case 1000 times continuously.
All the times test case reported as Test Pass :-)


Signed-off-by: Naresh Kamboju < naresh.kernel@gmail.com >
---
 testcases/open_posix_testsuite/conformance/interfaces/sem_post/8-1.c
|   15    8 +     7 -     0 !
 1 file changed, 8 insertions(+), 7 deletions(-)

Index: b/testcases/open_posix_testsuite/conformance/interfaces/sem_post/8-1.c
===================================================================
--- a/testcases/open_posix_testsuite/conformance/interfaces/sem_post/8-1.c
+++ b/testcases/open_posix_testsuite/conformance/interfaces/sem_post/8-1.c
@@ -161,7 +161,6 @@ int main()
        }
        fprintf(stderr, "P: child_1:%d forked\n", c_1);

-       sleep(1);
        c_2 = fork();
        if (c_2 == 0)
        {
@@ -176,13 +175,13 @@ int main()
        }
        fprintf(stderr, "P: child_2: %d forked\n", c_2);

+        /* Step 3 Implementation */
        /* Make sure the two children has been waiting */
-       /*do {
-               sleep(1);
+       do {
                sem_getvalue(sem_1, &val);
                //printf("val = %d\n", val);
        } while (val != 1);
-       */
+
        c_3 = fork();
        if (c_3 == 0)
        {
@@ -191,13 +190,15 @@ int main()
        }
        fprintf(stderr, "P: child_3: %d forked\n", c_3);

+        /* Step 3 Implementation */
        /* Make sure child 3 has been waiting for the lock */
-       /*do {
-               sleep(1);
+       do {
                sem_getvalue(sem_1, &val);
                //printf("val = %d\n", val);
        } while (val != 0);
-       */
+
+       /* Synchronization required before release the lock */
+       sleep(1);
        /* Ok, let's release the lock */
        fprintf(stderr, "P: release lock\n");
        sem_post(sem);


Test script to test 1000 times:
/*****************************************************/
#!/bin/sh

for ((  i = 0 ;  i < 1000;  i++  ))

do

./8-1.test >> /tmp/sem-post-8-1.log
done
/*****************************************************/

Refs:
http://ltp.cvs.sourceforge.net/viewvc/ltp/ltp/testcases/open_posix_testsuite/conformance/interfaces/sem_post/8-1.c?view=log

Please review this patch and let me know if you have any issues.

Best regards
Naresh Kamboju

[-- Attachment #2: posix-sem-post-unstable-fix.patch --]
[-- Type: application/octet-stream, Size: 1383 bytes --]

---
 testcases/open_posix_testsuite/conformance/interfaces/sem_post/8-1.c |   15 	8 +	7 -	0 !
 1 file changed, 8 insertions(+), 7 deletions(-)

Index: b/testcases/open_posix_testsuite/conformance/interfaces/sem_post/8-1.c
===================================================================
--- a/testcases/open_posix_testsuite/conformance/interfaces/sem_post/8-1.c
+++ b/testcases/open_posix_testsuite/conformance/interfaces/sem_post/8-1.c
@@ -161,7 +161,6 @@ int main() 
 	}
 	fprintf(stderr, "P: child_1:%d forked\n", c_1);
 	
-	sleep(1);
 	c_2 = fork();
 	if (c_2 == 0)
 	{
@@ -176,13 +175,13 @@ int main() 
 	}
 	fprintf(stderr, "P: child_2: %d forked\n", c_2);
 
+        /* Step 3 Implementation */
 	/* Make sure the two children has been waiting */	
-	/*do { 
-		sleep(1);
+	do {
 		sem_getvalue(sem_1, &val);
 		//printf("val = %d\n", val);
 	} while (val != 1);
-	*/
+
 	c_3 = fork();
 	if (c_3 == 0)
 	{
@@ -191,13 +190,15 @@ int main() 
 	}
 	fprintf(stderr, "P: child_3: %d forked\n", c_3);
 	
+        /* Step 3 Implementation */
 	/* Make sure child 3 has been waiting for the lock */	
-	/*do { 
-		sleep(1);
+	do {
 		sem_getvalue(sem_1, &val);
 		//printf("val = %d\n", val);
 	} while (val != 0);
-	*/
+
+	/* Synchronization required before release the lock */
+	sleep(1);
 	/* Ok, let's release the lock */
 	fprintf(stderr, "P: release lock\n");
 	sem_post(sem);

Re: [LTP] [PATCH] Synchronization required before release the lock: sem_post/8-1.c

[Rishikesh K Rajak]

On Thu, Feb 25, 2010 at 07:15:42PM +0530, naresh kamboju wrote:
> Hi,
> 
> I have found abnormal behavior of sem_post/8-1.c test case under posix.
> This test case passes in some times and failed in many times :-(
> 
> After my investigation found synchronization is missing between the
> child processes.
> Made a patch to fix this issue.
> 
> Patch includes
> 1.	Reverting back changes made by mreed on Sep 25 2006. Making sure
> child has been waiting for the lock (below Refs).
> 2.	using sleep in while loop is not a good idea, so sleep is removed
> from while loop
> 3.	For the synchronization I have added sleep before releasing the lock.
> 
> 
> After applying this patch I have tested this test case 1000 times continuously.
> All the times test case reported as Test Pass :-)
> 
> 
> Signed-off-by: Naresh Kamboju < naresh.kernel@gmail.com >

Looks good to me though i needed few clarification below.

Acked-By: Rishikesh K Rajak <risrajak@linux.vnet.ibm.com>

> ---
>  testcases/open_posix_testsuite/conformance/interfaces/sem_post/8-1.c
> |   15    8 +     7 -     0 !
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> Index: b/testcases/open_posix_testsuite/conformance/interfaces/sem_post/8-1.c
> ===================================================================
> --- a/testcases/open_posix_testsuite/conformance/interfaces/sem_post/8-1.c
> +++ b/testcases/open_posix_testsuite/conformance/interfaces/sem_post/8-1.c
> @@ -161,7 +161,6 @@ int main()
>         }
>         fprintf(stderr, "P: child_1:%d forked\n", c_1);
> 
> -       sleep(1);
>         c_2 = fork();
>         if (c_2 == 0)
>         {
> @@ -176,13 +175,13 @@ int main()
>         }
>         fprintf(stderr, "P: child_2: %d forked\n", c_2);
> 
> +        /* Step 3 Implementation */
>         /* Make sure the two children has been waiting */
> -       /*do {
> -               sleep(1);
I feel before getting semaphore value, we need to sync first so here
sleep is require,though your point is valid that there is no use of
using sleep inside while loop.

> +       do {
>                 sem_getvalue(sem_1, &val);
>                 //printf("val = %d\n", val);
>         } while (val != 1);
> -       */
> +
>         c_3 = fork();
>         if (c_3 == 0)
>         {
> @@ -191,13 +190,15 @@ int main()
>         }
>         fprintf(stderr, "P: child_3: %d forked\n", c_3);
> 
> +        /* Step 3 Implementation */
>         /* Make sure child 3 has been waiting for the lock */
> -       /*do {
> -               sleep(1);
> +       do {
>                 sem_getvalue(sem_1, &val);
>                 //printf("val = %d\n", val);
>         } while (val != 0);
> -       */
> +
> +       /* Synchronization required before release the lock */
> +       sleep(1);
>         /* Ok, let's release the lock */
>         fprintf(stderr, "P: release lock\n");
>         sem_post(sem);
> 
> 
> Test script to test 1000 times:
> /*****************************************************/
> #!/bin/sh
> 
> for ((  i = 0 ;  i < 1000;  i++  ))
> 
> do
> 
> ./8-1.test >> /tmp/sem-post-8-1.log
> done
> /*****************************************************/
> 
> Refs:
> http://ltp.cvs.sourceforge.net/viewvc/ltp/ltp/testcases/open_posix_testsuite/conformance/interfaces/sem_post/8-1.c?view=log
> 
> Please review this patch and let me know if you have any issues.
> 
> Best regards
> Naresh Kamboju


> ------------------------------------------------------------------------------
> Download Intel&#174; Parallel Studio Eval
> Try the new software tools for yourself. Speed compiling, find bugs
> proactively, and fine-tune applications for parallel performance.
> See why Intel Parallel Studio got high marks during beta.
> http://p.sf.net/sfu/intel-sw-dev

> _______________________________________________
> Ltp-list mailing list
> Ltp-list@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/ltp-list


-- 
Thanks & Regards
Rishi
LTP Maintainer
IBM, LTC, Bangalore
Please join IRC #ltp @ irc.freenode.net

Re: [LTP] [PATCH] Synchronization required before release the lock: sem_post/8-1.c

[naresh kamboju]

On Tue, Mar 2, 2010 at 2:20 PM, Rishikesh K Rajak
<risrajak@linux.vnet.ibm.com> wrote:
> On Thu, Feb 25, 2010 at 07:15:42PM +0530, naresh kamboju wrote:
>> Hi,
>>
>> I have found abnormal behavior of sem_post/8-1.c test case under posix.
>> This test case passes in some times and failed in many times :-(
>> Patch includes
>> 1.    Reverting back changes made by mreed on Sep 25 2006. Making sure
>> child has been waiting for the lock (below Refs).
>> 2.    using sleep in while loop is not a good idea, so sleep is removed
>> from while loop
>> 3.    For the synchronization I have added sleep before releasing the lock.
>>
>> Signed-off-by: Naresh Kamboju < naresh.kernel@gmail.com >
>
> Looks good to me though i needed few clarification below.
>
> Acked-By: Rishikesh K Rajak <risrajak@linux.vnet.ibm.com>
>
>> ---
>>  testcases/open_posix_testsuite/conformance/interfaces/sem_post/8-1.c
>> |   15    8 +     7 -     0 !
>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> Index: b/testcases/open_posix_testsuite/conformance/interfaces/sem_post/8-1.c
>> ===================================================================
>> --- a/testcases/open_posix_testsuite/conformance/interfaces/sem_post/8-1.c
>> +++ b/testcases/open_posix_testsuite/conformance/interfaces/sem_post/8-1.c
>> @@ -161,7 +161,6 @@ int main()
>>         }
>>         fprintf(stderr, "P: child_1:%d forked\n", c_1);
>>
>> -       sleep(1);
>>         c_2 = fork();
>>         if (c_2 == 0)
>>         {
>> @@ -176,13 +175,13 @@ int main()
>>         }
>>         fprintf(stderr, "P: child_2: %d forked\n", c_2);
>>
>> +        /* Step 3 Implementation */
>>         /* Make sure the two children has been waiting */
>> -       /*do {
>> -               sleep(1);
> I feel before getting semaphore value, we need to sync first so here
> sleep is require,though your point is valid that there is no use of
> using sleep inside while loop.
I agree with you.
AFAIU, we should call sleep() before calling getting semaphore
value.when we don't have while loop here. Because while loop condition
is depends on val so when ever we call sem_getvalue() it will get
latest value of val.
In addition to this, we are ensuring val is decremented before we do
unlock the sem by while loop condition.

Having sleep() in while loop will not effect the final output. IIUC

Best regards,
Naresh Kamboju
>> Please review this patch and let me know if you have any issues.
>>
>> Best regards
>> Naresh Kamboju
> --
> Thanks & Regards
> Rishi
> LTP Maintainer
> IBM, LTC, Bangalore
> Please join IRC #ltp @ irc.freenode.net

Re: [LTP] [PATCH] Synchronization required before release the lock: sem_post/8-1.c

[naresh kamboju]

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

On Wed, Mar 3, 2010 at 10:26 AM, Rishikesh K Rajak
<risrajak@linux.vnet.ibm.com> wrote:
> On Tue, Mar 02, 2010 at 08:38:20PM +0530, naresh kamboju wrote:
>> On Tue, Mar 2, 2010 at 2:20 PM, Rishikesh K Rajak
>> <risrajak@linux.vnet.ibm.com> wrote:
>> > I feel before getting semaphore value, we need to sync first so here
>> > sleep is require,though your point is valid that there is no use of
>> > using sleep inside while loop.
>> I agree with you.
>> AFAIU, we should call sleep() before calling getting semaphore
>> value.when we don't have while loop here. Because while loop condition
>> is depends on val so when ever we call sem_getvalue() it will get
>> latest value of val.
>> In addition to this, we are ensuring val is decremented before we do
>> unlock the sem by while loop condition.
>>
>> Having sleep() in while loop will not effect the final output. IIUC
>>
>
> Hi Naresh,
>
> I am not able to apply your patch against ltp next branch. can you
> please send me patch against next branch of ltp ?
Here I have posted and attached to this e-mail.

Signed-off-by: Naresh Kamboju < naresh.kernel@gmail.com >

---
 testcases/open_posix_testsuite/conformance/interfaces/sem_post/8-1.c
|   15 	8 +	7 -	0 !
 1 file changed, 8 insertions(+), 7 deletions(-)

Index: b/testcases/open_posix_testsuite/conformance/interfaces/sem_post/8-1.c
===================================================================
--- a/testcases/open_posix_testsuite/conformance/interfaces/sem_post/8-1.c
+++ b/testcases/open_posix_testsuite/conformance/interfaces/sem_post/8-1.c
@@ -161,7 +161,6 @@ int main()
 	}
 	fprintf(stderr, "P: child_1:%d forked\n", c_1);
 	
-	sleep(1);
 	c_2 = fork();
 	if (c_2 == 0)
 	{
@@ -176,13 +175,13 @@ int main()
 	}
 	fprintf(stderr, "P: child_2: %d forked\n", c_2);

+        /* Step 3 Implementation */
 	/* Make sure the two children has been waiting */	
-	/*do {
-		sleep(1);
+	do {
 		sem_getvalue(sem_1, &val);
 		//printf("val = %d\n", val);
 	} while (val != 1);
-	*/
+
 	c_3 = fork();
 	if (c_3 == 0)
 	{
@@ -191,13 +190,15 @@ int main()
 	}
 	fprintf(stderr, "P: child_3: %d forked\n", c_3);
 	
+        /* Step 3 Implementation */
 	/* Make sure child 3 has been waiting for the lock */	
-	/*do {
-		sleep(1);
+	do {
 		sem_getvalue(sem_1, &val);
 		//printf("val = %d\n", val);
 	} while (val != 0);
-	*/
+
+	/* Synchronization required before release the lock */
+	sleep(1);
 	/* Ok, let's release the lock */
 	fprintf(stderr, "P: release lock\n");
 	sem_post(sem);

Best regards
Naresh Kamboju
>
> #git clone -b next
> git://ltp.git.sourceforge.net/gitroot/ltp/ltp-dev.git ltp
>
> And then create your patch and send it to ltp-list@
>
> --
> Thanks & Regards
> Rishi
> LTP Maintainer
> IBM, LTC, Bangalore
> Please join IRC #ltp @ irc.freenode.net
>

[-- Attachment #2: posix-sem-post-unstable-fix.patch --]
[-- Type: application/octet-stream, Size: 1383 bytes --]

---
 testcases/open_posix_testsuite/conformance/interfaces/sem_post/8-1.c |   15 	8 +	7 -	0 !
 1 file changed, 8 insertions(+), 7 deletions(-)

Index: b/testcases/open_posix_testsuite/conformance/interfaces/sem_post/8-1.c
===================================================================
--- a/testcases/open_posix_testsuite/conformance/interfaces/sem_post/8-1.c
+++ b/testcases/open_posix_testsuite/conformance/interfaces/sem_post/8-1.c
@@ -161,7 +161,6 @@ int main() 
 	}
 	fprintf(stderr, "P: child_1:%d forked\n", c_1);
 	
-	sleep(1);
 	c_2 = fork();
 	if (c_2 == 0)
 	{
@@ -176,13 +175,13 @@ int main() 
 	}
 	fprintf(stderr, "P: child_2: %d forked\n", c_2);
 
+        /* Step 3 Implementation */
 	/* Make sure the two children has been waiting */	
-	/*do { 
-		sleep(1);
+	do {
 		sem_getvalue(sem_1, &val);
 		//printf("val = %d\n", val);
 	} while (val != 1);
-	*/
+
 	c_3 = fork();
 	if (c_3 == 0)
 	{
@@ -191,13 +190,15 @@ int main() 
 	}
 	fprintf(stderr, "P: child_3: %d forked\n", c_3);
 	
+        /* Step 3 Implementation */
 	/* Make sure child 3 has been waiting for the lock */	
-	/*do { 
-		sleep(1);
+	do {
 		sem_getvalue(sem_1, &val);
 		//printf("val = %d\n", val);
 	} while (val != 0);
-	*/
+
+	/* Synchronization required before release the lock */
+	sleep(1);
 	/* Ok, let's release the lock */
 	fprintf(stderr, "P: release lock\n");
 	sem_post(sem);