On 2012年07月13日 21:07, [email protected] wrote: > Hi! Hi Cyril,
Thanks for your detailed review. Please see my comments. >> This case fails on mips board routerstation randomly. The root cause is >> that when main thread call usleep(100) after signal the child threads, >> no child thread is scheduled to run. So the case fails. >> >> I update it to set main thread with a lower priority and child threads >> with a higher one, then when sched_yield is called in main thread, one >> of the child threads will be scheduled and run. >> >> Signed-off-by: Kang Kai<[email protected]> >> --- >> .../interfaces/pthread_cond_signal/1-1.c | 55 >> ++++++++++++++++++-- >> 1 files changed, 50 insertions(+), 5 deletions(-) >> >> diff --git >> a/testcases/open_posix_testsuite/conformance/interfaces/pthread_cond_signal/1-1.c >> >> b/testcases/open_posix_testsuite/conformance/interfaces/pthread_cond_signal/1-1.c >> index bf55b31..51c0cfb 100644 >> --- >> a/testcases/open_posix_testsuite/conformance/interfaces/pthread_cond_signal/1-1.c >> +++ >> b/testcases/open_posix_testsuite/conformance/interfaces/pthread_cond_signal/1-1.c >> @@ -11,6 +11,7 @@ >> */ >> >> #define _XOPEN_SOURCE 600 >> +#define _GNU_SOURCE > This is open posix testsuite keep that in mind so inserting _GNU_SOURCE > is not allowed here. _GNU_SOURCE is needed by set_affinity(). I once thought multi-processor will break the sync between main thread and children threads. Maybe I am wrong that set_affinity() is not needed. > >> #include<pthread.h> >> #include<stdio.h> >> @@ -18,6 +19,7 @@ >> #include<unistd.h> >> #include<signal.h> >> #include "posixtest.h" >> +#include "affinity.h" >> >> #define THREAD_NUM 3 >> >> @@ -77,6 +79,18 @@ int main() >> { >> int i, rc; >> struct sigaction act; >> + int pid; >> + int policy; >> + struct sched_param param; >> + pthread_attr_t attr; >> + >> + if (getuid() != 0) >> + { >> + fprintf(stderr, "Run this test as root, not as a Regular >> User\n"); >> + return PTS_UNTESTED; >> + } > Wrong coding style. The opening brace should be at the end of the if > statement. Please use checkpatch.pl on your patches. Sorry for that. > > And also I think that the check is not 100% correct, the POSIX says that > schedulling priorities and so may be changed "if calling process has > permission to...", there are capabilities and other mechanisms to check > for that. The rest of the testcases seems to deal with this just by > printing a hint after first call has that may need the priviledge > failed, for example: > > "foo failed with -1: are you root?" OK, I'll check the return value and error code of sched_* function then give user information. > >> + set_affinity(0); > I setting affinity really needed here? (I'm just asking before it wasn't > described in the commit description). > >> if (pthread_mutex_init(&td.mutex, NULL) != 0) { >> fprintf(stderr,"Fail to initialize mutex\n"); >> @@ -87,14 +101,41 @@ int main() >> return PTS_UNRESOLVED; >> } >> >> + pid = getpid(); >> + param.sched_priority = sched_get_priority_min(SCHED_FIFO); >> + if (sched_setscheduler(pid, SCHED_FIFO,¶m) != 0) { >> + fprintf(stderr, "Fail to set main thread scheduler.\n"); >> + return PTS_UNRESOLVED; >> + } >> + >> + if (pthread_attr_init(&attr) != 0) { >> + fprintf(stderr, "Fail to init child thread attribute.\n"); >> + return PTS_UNRESOLVED; >> + } >> + if (pthread_attr_setschedpolicy(&attr, SCHED_FIFO) != 0) { >> + fprintf(stderr, "Fail to set child thread schedule policy >> attribute.\n"); >> + return PTS_UNRESOLVED; >> + } >> + param.sched_priority += 10; >> + if (pthread_attr_setschedparam(&attr,¶m) != 0) { >> + fprintf(stderr, "Fail to set child thread schedule priority >> attribute.\n"); >> + return PTS_UNRESOLVED; >> + } >> + if (pthread_attr_setinheritsched(&attr, PTHREAD_EXPLICIT_SCHED) != 0) { >> + fprintf(stderr, "Fail to set child thread inheritace >> attribute.\n"); >> + return PTS_UNRESOLVED; >> + } >> + >> for (i=0; i<THREAD_NUM; i++) { /* create THREAD_NUM threads */ >> - if (pthread_create(&thread[i], NULL, thr_func, NULL) != 0) { >> + if (pthread_create(&thread[i],&attr, thr_func, NULL) != 0) { >> fprintf(stderr,"Fail to create thread[%d]\n", i); >> exit(PTS_UNRESOLVED); >> } >> } >> while (start_num< THREAD_NUM) /* waiting for all threads started */ >> - usleep(100); >> + sched_yield(); > Hmm, I've been burned by: > > while (xxx) > sched_yield(); > > code before. In certain situations this translates just to bussy loops, > so most of the time small sleep or both sleep and sched_yield may be > better. usleep() works here, I just want to unified the code. > >> + pthread_attr_destroy(&attr); >> >> /* Acquire the mutex to make sure that all waiters are currently >> blocked on pthread_cond_wait */ >> @@ -114,7 +155,7 @@ int main() >> fprintf(stderr,"[Main thread] failed to signal the >> condition\n"); >> exit(PTS_UNRESOLVED); >> } >> - sleep(1); >> + sched_yield(); >> if (waken_num<= 0) { >> fprintf(stderr,"[Main thread] but no waiters were wakened\n"); >> printf("Test FAILED\n"); >> @@ -142,7 +183,7 @@ int main() >> fprintf(stderr,"Main failed to signal the condition\n"); >> exit(PTS_UNRESOLVED); >> } >> - usleep(100); >> + sched_yield(); >> } >> >> if (i>= THREAD_NUM) { >> @@ -158,6 +199,10 @@ int main() >> exit(PTS_UNRESOLVED); >> } >> } >> + >> + pthread_mutex_destroy(&td.mutex); >> + pthread_cond_destroy(&td.cond);; >> + >> printf("Test PASSED\n"); >> return PTS_PASS; >> -} >> \ No newline at end of file >> +} > And as far as I remeber the original problem was that some of the > threads failed to wake up because the while loop that wakes them was too > tight. Does inserting sched_yield() there help? > Only insert sched_yield() can NOT make sure the main thread really sleep. In my test increase usleep to 1000 works. I still hope the last patch which increase the usleep value can be accept. If can't, I'll send a V2. Thanks a lot, Kai ------------------------------------------------------------------------------ Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ _______________________________________________ Ltp-list mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/ltp-list
