Re: Bug in pth_sched_eventmanager() -- can't yield to a thread waiting on an fd

On Friday 16 August 2002 03:13, you wrote:
> Thanks for reporting a bug and providing fix for it. Your fix will solve
> problem for an uniprocessor
> system, but may impact on SMP performance. Here is what I think a right way
> to fix for both cases:

No worries. However I do have a few questions (inline below) about your 
patch, mainly to aid in my understanding of what is going on here...

>
> ===========================================================================
>= /* Filedescriptor I/O */
> -                   if (ev->ev_type == PTH_EVENT_FD && !dopoll && (ds ==
> descr)) {
> +                   if (ev->ev_type == PTH_EVENT_FD && (ds == descr)) {

So what does the (ds == descr) do? I'm assuming it has something to do with 
SMP operation (assuming that just from your earlier comments), but I am 
wondering why it is present in the conditional test for file I/O events, but 
not in any of the other branches of the if statement, eg the branches dealing 
with PTH_EVENT_SELECT, PTH_EVENT_MUTEX? Especially in the case of 
PTH_EVENT_SELECT, which is also dealing with file descriptor.

>                         /* filedescriptors are checked later all at once.
>                            Here we only assemble them in the fd sets */
>                         if (ev->ev_goal & PTH_UNTIL_FD_READABLE)
> @@ -1034,10 +1034,6 @@
>      /* let select() wait for the read-part of the pipe */
>      FD_SET(descr->sigpipe[0], &rfds);
>
>      /* If dopoll is set, handle the thread from ready queue... */
> -    if (dopoll)
> +    if (m_to_n && dopoll)
>        return;

I'm confused here too. Why in the m_to_n case do we wish bypass the select() 
call?

Regards
M.Beach

>
>
>      if (fdmax < descr->sigpipe[0])
>          fdmax = descr->sigpipe[0];
>
> - - - - -
> Saurabh Desai
> POSIX Threading for Linux
> IBM  Linux Technology Center
> e-mail: [email protected] OR [email protected]
> phone: 512-838-2655, T/L: 678-2655
> http://oss.software.ibm.com/developerworks/opensource/pthreads
>
>
>
> Michael Beach <[email protected]>@www-124.southbury.usf.ibm.com on
> 08/15/2002 04:35:21 AM
>
> Sent by:    [email protected]
>
>
> To:    [email protected]
> cc:
> Subject:    [pthreads-devel] Bug in pth_sched_eventmanager() -- can't yield
>        to a thread waiting on an fd
>
>
> The good news or the bad news first? The bad news you say? Well...
>
> The bad news is that I've just recently been bitten by what I believe to be
> a
> bug in pth_sched_eventmanager().
>
> The good news is that I think I've fixed it!
>
> Ok, some more details. I'm using NGPT 2.0.1 on a more or less stock SuSE
> 7.2
> installation, but with an appropriately patched 2.4.19 kernel.
>
> The problem relates to the way that the scheduler selects a new thread to
> run.
>
> At the end of the scheduler main loop, at around line 700 of pth_sched.c,
> pth_scheduler() calls pth_sched_eventmanager() in order to process any
> events
> which have occurred and to move those threads which are waiting on those
> threads from the WAITING queue to the READY queue. If the READY queue is
> not
> empty, then pth_sched_eventmanager() is called with a flag which indicates
> that it should only poll for any events which may have occurred, rather
> than
> waiting for one to occur. So far so good.
>
> The problem lies in the way that pth_sched_eventmanager() uses that
> "dopoll"
> flag. Ultimately, what it's used for is to decide whether or not to call
> select() with a zero or a non-zero timeout. Presumably if we already have
> threads in the READY queue, we want to poll (ie call select() with a zero
> timeout) so that if nothing turns up from the select() call we can then go
> straight on and schedule a thread that is already in the READY queue.
> However
> at several points in pth_sched_eventmanager() the "dopoll" flag is tested
> and
> used to short-circuit parts of the event checking altogether. This I think
> is
> the heart of the problem. It means that if we have threads which are
> currently on the READY queue, then some events will not be checked for,
> potentially resulting in starvation problems.
>
> The circumstances in which I experienced this problem involved a CPU bound
> thread (call it thread A) which would occasionally call pthread_yield_np()
> to
> allow other threads to run, and a thread performing a recv() from a socket
> (call it thread B). Despite the socket being ready to read (ie data was
> arriving from the network), and thread A yielding quite regularly, thread B
>
> would not return from the recv() call. Eventually I tracked this down to
> the
> problem in the scheduler. When thread A yields, it goes back on the READY
> queue, so when it comes time for the scheduler to service events it sets
> the
> "dopoll" flag since the READY queue is non-empty. However the "dopoll" flag
>
> was causing checking of file descriptor related events to be
> short-circuited,
> so the socket event which should have woken thread B was never being
> checked.
> Thus the scheduler just ran thread A again. And again. And again.
>
> I have a simple test case to illustrate this problem. This program creates
> a
> CPU bound thread (in this case just a tight loop) which regularly yields,
> and
> a thread which reads from stdin and in reponse writes stuff to stdout. With
>
> the stock standard NGPT 2.0.1, the thread which reads from stdin never
> seems
> to run, as no output is ever generated.
>
> ==================================================================
>
> #include <pthread.h>
>
>
> void *cpu_bound_thread_entry(void *arg)
> {
>     while (1) pthread_yield_np();
> }
>
>
> void *stdin_reading_thread_entry(void *arg)
> {
>     while (1)
>     {
>         ssize_t n;
>         const size_t bufLen = 20;
>         char buf[bufLen];
>         n = read(0, buf, bufLen);
>         if (n > 0)
>         {
>             write(1, "Read some data ==> ", 19);
>             write(1, buf, n);
>             write(1, "\n", 1);
>         }
>         else if (n == 0)
>         {
>             write(1, "*EOF*\n", 6);
>             break;
>         }
>         else
>         {
>             write(1, "*ERROR*\n", 8);
>             break;
>         }
>     }
> }
>
>
> int main(int argc, char *argv[])
> {
>     pthread_t cpu_bound_thread;
>     pthread_t stdin_reading_thread;
>
>     pthread_create(&stdin_reading_thread, 0, stdin_reading_thread_entry,
> 0);
>     pthread_create(&cpu_bound_thread, 0, cpu_bound_thread_entry, 0);
>
>     pthread_join(stdin_reading_thread, 0);
>     pthread_join(cpu_bound_thread, 0);
> }
>
> ==================================================================
>
> However if the NGPT library is patched to remove the short-circuiting of
> the
> event tests based on "dopoll" this program echos the data read as expected.
>
> The patch I applied is as follows...
>
> ==================================================================
>
> --- ngpt-2.0.1/pth_sched.c      Sat Aug 10 05:31:32 2002
> +++ ngpt-2.0.1-hacks/pth_sched.c        Thu Aug 15 18:17:32 2002
> @@ -840,7 +840,7 @@
>                     this_occurred = FALSE;
>
>                     /* Filedescriptor I/O */
> -                   if (ev->ev_type == PTH_EVENT_FD && !dopoll && (ds ==
> descr)) {
> +                   if (ev->ev_type == PTH_EVENT_FD) {
>                         /* filedescriptors are checked later all at once.
>                            Here we only assemble them in the fd sets */
>                         if (ev->ev_goal & PTH_UNTIL_FD_READABLE)
> @@ -1034,10 +1034,6 @@
>      /* let select() wait for the read-part of the pipe */
>      FD_SET(descr->sigpipe[0], &rfds);
>
> -    /* If dopoll is set, handle the thread from ready queue... */
> -    if (dopoll)
> -       return;
> -
>      if (fdmax < descr->sigpipe[0])
>          fdmax = descr->sigpipe[0];
>
> ==================================================================
>
> I doubt that this patch is the last word in solving this problem, largely
> because I've got no idea as to the details of how the M:N threading model
> is
> handled, and how all of that relates to bound threads etc etc. In
> particular,
> in the first change in the patch I've removed the (ds == dsecr) part of the
>
> expression too, but I've no idea if this is correct or not. Also, I've been
>
> working solely on a uniprocessor machine, so I've not been able to verify
> its
> correct operation in an SMP environment.
>
> However, it does get the test program (and my actual software project!)
> running correctly, so I'm sure it is a start!
>
> Regards
> M.Beach
> _______________________________________________
> pthreads-devel mailing list
> [email protected]
> http://www-124.ibm.com/developerworks/oss/mailman/listinfo/pthreads-devel
>
>
>
>
>
> _______________________________________________
> pthreads-devel mailing list
> [email protected]
> http://www-124.ibm.com/developerworks/oss/mailman/listinfo/pthreads-devel