[drlvm][threading] questions about wait_count and notify_flag in thread_native_fat_monitor.c

[Weldon Washburn <we...@gmail.com>]

1)
It looks like wait_count is never initialized to zero.
hythread_monitor_init_with_name()
initializes notify_flag to zero.  Thus it seems logical that wait_count
would also need to be intialized to zero.

2)
wait_count and notify_flag appear to serve the same purpose.  Are both
variables neccessary?

3)
Why does hythread_monitor_notify_all() do:

   mon_ptr->notify_flag = mon_ptr->wait_count;

and hythread_monitor_notify() do:

   if (mon_ptr->notify_flag < mon_ptr->wait_count)
         mon_ptr->notify_flag +=1;
4)
As far as I can tell, setting both wait_count and notify_flag to arbitrary
values will not impact the proper delivery of Object.notify() or
Object.notifyAll().  Is this a correct understanding?

5)
The only impact of these variables is the return value of
hythread_monitor_num_waiting().  Is this a correct understanding?

6)
I suspect hythread_monitor_num_waiting() is only used to give someone
debugging an idea of how many threads where waiting at a given object at a
point in the past.  Is this a correct understanding?  The
hythread_monitor_num_waiting() return value does not have to be the current
state of the system but a value of -75688 would probably cause the person
who is debugging to, well, report a bug in the debugger.

-- 
Weldon Washburn
Intel Enterprise Solutions Software Division

Re: [drlvm][threading] questions about wait_count and notify_flag in thread_native_fat_monitor.c

[Salikh Zakirov <Sa...@Intel.com>]

Weldon Washburn wrote:
> Your comments helped a bunch.  I now understand this code.  It looks
> correct.   It would be good if you could put more comments in the patch.
> Maybe rephrasing the below would work.   One thing to look out for is a
> race
> condition that allows a thread to miss an intended notify.
> 
> Also the "#ifdef NO_COND_VARS" is very distracting.   Morphing notify_flag
> to notify_count is a good idea.  But I don't see a reason to attempt to
> patch up the NO_COND_VARS use of notify_flag/notify_count.  How about
> tossing all the code inside #ifdef NO_COND_VARS?

tossing NO_COND_VARS is suggested as a next step in cleaning up this code,
in HARMONY-1951 subtasks: see HARMONY-2269

I will post renaming notify_flag -> notify_count and more comments as
subsequent patches on top of HARMONY-1951 and HARMONY-2269.

Re: [drlvm][threading] questions about wait_count and notify_flag in thread_native_fat_monitor.c

[Salikh Zakirov <Sa...@Intel.com>]

Weldon Washburn wrote:
> Your comments helped a bunch.  I now understand this code.  It looks
> correct.   It would be good if you could put more comments in the patch.
> Maybe rephrasing the below would work.   One thing to look out for is a
> race
> condition that allows a thread to miss an intended notify.
> 
> Also the "#ifdef NO_COND_VARS" is very distracting.   Morphing notify_flag
> to notify_count is a good idea.  But I don't see a reason to attempt to
> patch up the NO_COND_VARS use of notify_flag/notify_count.  How about
> tossing all the code inside #ifdef NO_COND_VARS?

Please find next round of improvements on top of HARMONY-1951 and HARMONY-2269
in HARMONY-2352 (also a subtask of HARMONY-1951).

Re: [drlvm][threading] questions about wait_count and notify_flag in thread_native_fat_monitor.c

[Weldon Washburn <we...@gmail.com>]

Salikh,

Your comments helped a bunch.  I now understand this code.  It looks
correct.   It would be good if you could put more comments in the patch.
Maybe rephrasing the below would work.   One thing to look out for is a race
condition that allows a thread to miss an intended notify.

Also the "#ifdef NO_COND_VARS" is very distracting.   Morphing notify_flag
to notify_count is a good idea.  But I don't see a reason to attempt to
patch up the NO_COND_VARS use of notify_flag/notify_count.  How about
tossing all the code inside #ifdef NO_COND_VARS?


On 11/27/06, Salikh Zakirov <Sa...@intel.com> wrote:
>
> Weldon, I'm assuming you are reviewing the patch
>
> use-HyThreadMonitor.notify_flag-to-prevent-spurious-interrupt-wakeups.patch(3 kb)
> from HARMONY-1951.
>
> Weldon Washburn wrote:
>
> > It looks like wait_count is never initialized to zero.
> > hythread_monitor_init_with_name()
> > initializes notify_flag to zero.  Thus it seems logical that wait_count
> > would also need to be intialized to zero.
>
> In fact, it is initialized to zero implicitly by the following line
> 50     mon = apr_pcalloc(pool, sizeof(HyThreadMonitor));
>
> "calloc" family of functions zero out the allocated memory.
>
> perhaps, the patch could be refined to remove other extraneous zero
> initializations too.
>
> > 2)
> > wait_count and notify_flag appear to serve the same purpose.  Are both
> > variables neccessary?
>
> I do not know the original purpose of wait_count, it looks like the
> function
>     hythread_monitor_num_waiting()
> was provided for sort of functional completeness.
> As far as I know, there are no users of this function.
>
> And this variable turned out to be handy to track the notify events
> in order to be able to ignore spurios wakeups.
> At any given moment of time (when the monitor lock is acquired), the
> counters
> have following meaning:
>
> wait_count = the number of threads either waiting on the monitor or queued
> to
> acquire monitor lock after wakeup.
>
> notify_flag (which should really be renamed to notify_count) = the number
> of
> notify events generated. It cannot be greater than wait_count to prevent
> piling up notify events according to the java specification.
>
> The need of notify_count comes from the fact that we cannot use wait_count
> in a
> woken up thread to decide if the wakeup was spurious or triggered by
> notify event.
>
> > 3)
> > Why does hythread_monitor_notify_all() do:
> >
> >   mon_ptr->notify_flag = mon_ptr->wait_count;
> >
> > and hythread_monitor_notify() do:
> >
> >   if (mon_ptr->notify_flag < mon_ptr->wait_count)
> >         mon_ptr->notify_flag +=1;
>
> This code ensures that notify_flag (count) never exceeds the value
> of wait_count.
> Woken up thread decreases both wait_count and notify_flag by 1,
> thus preserving the invariant.
>
> > 4)
> > As far as I can tell, setting both wait_count and notify_flag to
> arbitrary
> > values will not impact the proper delivery of Object.notify() or
> > Object.notifyAll().  Is this a correct understanding?
>
> Unfortunately, I cannot quite get what does this mean.
>
> Both wait_count and notify_flag has well-defined semantics inside of
> synchronized section (with monitor lock acquired). Setting them to
> arbitrary
> values can break the system, for example:
> * arbitrary decreasing notify_flag will lead to threads missing notify
> events
> * arbitrary increasing notify_flag may lead to spurious wakeups
> * arbitary decreasing wait_count may lead to notify() calls being ignored
> * arbitrary increasing wait_count may lead to notify() calls piled up and
> affecting threads that executed wait() _after_ notify().
>
>
> > 5)
> > The only impact of these variables is the return value of
> > hythread_monitor_num_waiting().  Is this a correct understanding?
>
> As far as I can see, wait_count has no other use before the discussed
> patch.
>
> > 6)
> > I suspect hythread_monitor_num_waiting() is only used to give someone
> > debugging an idea of how many threads where waiting at a given object at
> a
> > point in the past.  Is this a correct understanding?  The
> > hythread_monitor_num_waiting() return value does not have to be the
> current
> > state of the system but a value of -75688 would probably cause the
> person
> > who is debugging to, well, report a bug in the debugger.
>
> Using wait_count for debugging sounds plausible, though I haven't used it.
>
>


-- 
Weldon Washburn
Intel Enterprise Solutions Software Division

Re: [drlvm][threading] questions about wait_count and notify_flag in thread_native_fat_monitor.c

[Salikh Zakirov <Sa...@Intel.com>]

Weldon, I'm assuming you are reviewing the patch
use-HyThreadMonitor.notify_flag-to-prevent-spurious-interrupt-wakeups.patch (3 kb)
from HARMONY-1951.

Weldon Washburn wrote:

> It looks like wait_count is never initialized to zero.
> hythread_monitor_init_with_name()
> initializes notify_flag to zero.  Thus it seems logical that wait_count
> would also need to be intialized to zero.

In fact, it is initialized to zero implicitly by the following line
 50     mon = apr_pcalloc(pool, sizeof(HyThreadMonitor));

"calloc" family of functions zero out the allocated memory.

perhaps, the patch could be refined to remove other extraneous zero
initializations too.

> 2)
> wait_count and notify_flag appear to serve the same purpose.  Are both
> variables neccessary?

I do not know the original purpose of wait_count, it looks like the function
     hythread_monitor_num_waiting()
was provided for sort of functional completeness.
As far as I know, there are no users of this function.

And this variable turned out to be handy to track the notify events
in order to be able to ignore spurios wakeups.
At any given moment of time (when the monitor lock is acquired), the counters
have following meaning:

 wait_count = the number of threads either waiting on the monitor or queued to
acquire monitor lock after wakeup.

 notify_flag (which should really be renamed to notify_count) = the number of
notify events generated. It cannot be greater than wait_count to prevent
piling up notify events according to the java specification.

The need of notify_count comes from the fact that we cannot use wait_count in a
woken up thread to decide if the wakeup was spurious or triggered by notify event.

> 3)
> Why does hythread_monitor_notify_all() do:
> 
>   mon_ptr->notify_flag = mon_ptr->wait_count;
> 
> and hythread_monitor_notify() do:
> 
>   if (mon_ptr->notify_flag < mon_ptr->wait_count)
>         mon_ptr->notify_flag +=1;

This code ensures that notify_flag (count) never exceeds the value
of wait_count.
Woken up thread decreases both wait_count and notify_flag by 1,
thus preserving the invariant.

> 4)
> As far as I can tell, setting both wait_count and notify_flag to arbitrary
> values will not impact the proper delivery of Object.notify() or
> Object.notifyAll().  Is this a correct understanding?

Unfortunately, I cannot quite get what does this mean.

Both wait_count and notify_flag has well-defined semantics inside of
synchronized section (with monitor lock acquired). Setting them to arbitrary
values can break the system, for example:
* arbitrary decreasing notify_flag will lead to threads missing notify events
* arbitrary increasing notify_flag may lead to spurious wakeups
* arbitary decreasing wait_count may lead to notify() calls being ignored
* arbitrary increasing wait_count may lead to notify() calls piled up and
affecting threads that executed wait() _after_ notify().


> 5)
> The only impact of these variables is the return value of
> hythread_monitor_num_waiting().  Is this a correct understanding?

As far as I can see, wait_count has no other use before the discussed patch.

> 6)
> I suspect hythread_monitor_num_waiting() is only used to give someone
> debugging an idea of how many threads where waiting at a given object at a
> point in the past.  Is this a correct understanding?  The
> hythread_monitor_num_waiting() return value does not have to be the current
> state of the system but a value of -75688 would probably cause the person
> who is debugging to, well, report a bug in the debugger.

Using wait_count for debugging sounds plausible, though I haven't used it.