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

[Rishikesh K Rajak <risrajak@linux.vnet.ibm.com>]

On Mon, Mar 08, 2010 at 04:27:36PM +0530, naresh kamboju wrote:
> Hi Rishi and Garrett,
> 
> On Wed, Mar 3, 2010 at 7:15 PM, naresh kamboju <naresh.kernel@gmail.com> wrote:
> > On Wed, Mar 3, 2010 at 5:25 PM, Garrett Cooper <yanegomi@gmail.com> wrote:
> >> On Wed, Mar 3, 2010 at 3:49 AM, naresh kamboju <naresh.kernel@gmail.com> wrote:
> >>> On Wed, Mar 3, 2010 at 5:12 PM, Garrett Cooper <yanegomi@gmail.com> wrote:
> >>>> On Wed, Mar 3, 2010 at 3:33 AM, naresh kamboju <naresh.kernel@gmail.com> wrote:
> >>>>> On Wed, Mar 3, 2010 at 3:15 PM, Rishikesh K Rajak
> >>>>> <risrajak@linux.vnet.ibm.com> wrote:
> >>>>>> On Wed, Mar 03, 2010 at 01:18:50AM -0800, Garrett Cooper wrote:
> >>>>>>> Uh, hold on a sec before we call it good...
> >>>>>>
> >>>>>> oh ok, holiding on for bit.
> >>>>>>
> >>>>>> Naresh, can you please send a patch with incorporating garret's comment
> >>>>>> ?
> >>>>>>>>> +        /* 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);
> >>>>>>>>> -     */
> >>>>>>>
> >>>>>>> Please provide another patch with a limit to this --
> >>>>>
> >>>>> Garrett,
> >>>>>
> >>>>> When sem_wait is called 'val' value will be decremented by one.
> >>>>> To ensure that sem_wait is called, we are checking 'val' value by
> >>>>> calling sem_getvalue(). in this case we don’t need to decrement the
> >>>>> values by --. IIUC.
> >>>>> (snip)
> >>>>> OTOH,
> >>>>>>> I get annoyed
> >>>>>>> with tests that have infinite loops in them because the underlying
> >>>>>>> functionality is broken.
> >>>>> I agree with you, having infinite loops in test case is not a good.
> >>>>> However, in this patch while loop is not infinite loop. It is a
> >>>>> conditional loop with finite value.
> >>>>>
> >>>>> Please let me know if you have any issues.
> >>>>
> >>>>    The problem was that it wasn't failing properly as stated in the
> >>>> manpage on mips* (was decrementing past 0) and it was blocking
> >>>> indefinitely. Hence I had to yank those tests from the default run.
> >>> do you mean, after applying above patch you have noticed these kind of behavior?
> >>
> >> Not with this patch; I've seen this kind of behavior in general under
> >> odd conditions with my former team's embedded setup running tests with
> >> POSIX semaphores, so I don't doubt that others could run into the
> >> similar functional issues given the right conditions.
> >
> > Hey Garrett,
> >
> > Thanks for you information :-)
> >
> > I have tested these on MIPS architecture and  reproduced infinite
> > waiting situation after applying this patch. (with strace no issue
> > found ex: #strace ./8-1.test)
> > As you said there may be issues in MIPS-POSIX library. it may take
> > some time to fix these issues or may not be fixed.
> > However, I’ll discuss this issue with MIPS folks in different thread.
> >
> > ATM, my patch is not a good idea for MIPS architectures.
> > So, after your comments I have modified my patch and tested.
> > Here in this latest patch i did not change any thing regarding while loop.
> > I replaced sleep() in an appropriate place.
> > It is working fine on X86, ARM and MIPS.
> 
> please review the patch.
> I have tested this patch on X86, ARM and MIPS and results are good.
> 
> If you feel ok. please commit. otherwise give me your comments.

Hi Naresh,

can you send your patch against today's git ?

-Rishi
> 
> Best regards
> Naresh Kamboju
> >
> > Signed-off-by: Naresh Kamboju < naresh.kernel@gmail.com >
> > ---
> >  testcases/open_posix_testsuite/conformance/interfaces/sem_post/8-1.c
> > |    2  1 +     1 -     0 !
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > 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)
> >        {
> > @@ -198,6 +197,7 @@ int main()
> >                //printf("val = %d\n", val);
> >        } while (val != 0);
> >        */
> > +       sleep(1);
> >        /* Ok, let's release the lock */
> >        fprintf(stderr, "P: release lock\n");
> >        sem_post(sem);
> >
> >
> > please let me know if you have any issues.
> >
> > Best regards,
> > Naresh Kamboju
> >> Thanks,
> >> -Garrett
> >>
> >



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

------------------------------------------------------------------------------
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