[PATCH] Synchronization required before release the lock: sem_post/8-1.c
in [Kernel]
|
Prev: [PATCH -tip v3&10 17/18] perf probe: show more lines after last line
Next: mips,kgdb: kdb low level trap catch and stack trace
From: naresh kamboju on 25 Feb 2010 08:50 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(a)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
From: naresh kamboju on 2 Mar 2010 10:10 On Tue, Mar 2, 2010 at 2:20 PM, Rishikesh K Rajak <risrajak(a)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(a)gmail.com > > > Looks good to me though i needed few clarification below. > > Acked-By: Rishikesh K Rajak <risrajak(a)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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: naresh kamboju on 3 Mar 2010 01:10
On Wed, Mar 3, 2010 at 10:26 AM, Rishikesh K Rajak <risrajak(a)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(a)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(a)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 > |