[Bro-Dev] [Bro-Commits] [git/bro] master: Fix for input readers occasionally dead-locking. (c980d10)

Bernhard Amann bernhard at ICSI.Berkeley.EDU
Thu Oct 24 23:43:21 PDT 2013


I just committed an alternative fix to topic/bernhard/alternative-deadlock-fix - this removes
the random from Queue again and places another random into the threading manager. However,
this one is only called if no communication whatsoever occurs - and only by the main thread, so 
random() should be fine.

Bernhard

On Oct 24, 2013, at 9:04 PM, Clark, Gilbert <gc355804 at ohio.edu> wrote:

> // Thoughts?
> 
> /* 
> * Since we know that read_ctr is only incremented after a successful read, and write_ctr is only incremented after a successful write, the two values should be equal iff the queue is empty.  I think it could also help if these two items were 
> *  declared volatile.
> */
> bool MaybeReady() { return read_ctr != write_ctr; }
> 
> // Alternatively, replacing random() with a per-instance counter and saying the function returns true every Xth time it is called might mean you weren't having to hit the RNG every time you checked on the status of a queue.
> // Also, should the call be to random_r() instead of random() ?
> 
> // --Gilbert
> ________________________________________
> From: bro-commits-bounces at bro.org [bro-commits-bounces at bro.org] On Behalf Of Robin Sommer [robin at icir.org]
> Sent: Thursday, October 24, 2013 9:28 PM
> To: bro-commits at bro.org
> Subject: [Bro-Commits] [git/bro] master: Fix for input readers occasionally     dead-locking. (c980d10)
> 
> Repository : ssh://git@bro-ids.icir.org/bro
> 
> On branch  : master
> Link       : https://github.com/bro/bro/commit/c980d1055e1e17da4867e3fab1ee10f604b242b0
> 
>> ---------------------------------------------------------------
> 
> commit c980d1055e1e17da4867e3fab1ee10f604b242b0
> Author: Robin Sommer <robin at icir.org>
> Date:   Thu Oct 24 18:16:49 2013 -0700
> 
>    Fix for input readers occasionally dead-locking.
> 
>    Bernhard and I tracked it down we believe: the thread queue could
>    deadlock in certain cases. As a fix we tuned the heuristic for telling
>    if a queue might have input to occasionaly err on the safe side by
>    flagging "yes", so that processing will proceed.
> 
>    It's a bit unfortunate to apply this fix last minute before the
>    release as it could potentially impact performance if the heuristic
>    fails to often. We believe the chosen parmaterization should be fine ...
> 
> 
>> ---------------------------------------------------------------
> 
> c980d1055e1e17da4867e3fab1ee10f604b242b0
> CHANGES               |  4 ++++
> VERSION               |  2 +-
> src/threading/Queue.h | 10 ++++++----
> 3 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/CHANGES b/CHANGES
> index 10bc187..1ae192f 100644
> --- a/CHANGES
> +++ b/CHANGES
> @@ -1,4 +1,8 @@
> 
> +2.2-beta-152 | 2013-10-24 18:16:49 -0700
> +
> +  * Fix for input readers occasionally dead-locking. (Robin Sommer)
> +
> 2.2-beta-151 | 2013-10-24 16:52:26 -0700
> 
>   * Updating submodule(s).
> diff --git a/VERSION b/VERSION
> index 2b5702f..26d1beb 100644
> --- a/VERSION
> +++ b/VERSION
> @@ -1 +1 @@
> -2.2-beta-151
> +2.2-beta-152
> diff --git a/src/threading/Queue.h b/src/threading/Queue.h
> index 792fb63..c4f2bfa 100644
> --- a/src/threading/Queue.h
> +++ b/src/threading/Queue.h
> @@ -61,11 +61,13 @@ public:
>        bool Ready();
> 
>        /**
> -        * Returns true if the next Get() operation might succeed.
> -        * This function may occasionally return a value not
> -        * indicating the actual state, but won't do so very often.
> +        * Returns true if the next Get() operation might succeed. This
> +        * function may occasionally return a value not indicating the actual
> +        * state, but won't do so very often. Occasionally we also return a
> +        * true unconditionally to avoid a deadlock when both pointers happen
> +        * to be equal even though there's stuff queued.
>         */
> -       bool MaybeReady() { return ( ( read_ptr - write_ptr) != 0 ); }
> +       bool MaybeReady() { return (read_ptr != write_ptr) || (random() % 10000 == 0); }
> 
>        /** Wake up the reader if it's currently blocked for input. This is
>         primarily to give it a chance to check termination quickly.
> 
> _______________________________________________
> bro-commits mailing list
> bro-commits at bro.org
> http://mailman.icsi.berkeley.edu/mailman/listinfo/bro-commits
> _______________________________________________
> bro-dev mailing list
> bro-dev at bro.org
> http://mailman.icsi.berkeley.edu/mailman/listinfo/bro-dev

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4160 bytes
Desc: not available
Url : http://mailman.icsi.berkeley.edu/pipermail/bro-dev/attachments/20131024/2b56511a/attachment.bin 


More information about the bro-dev mailing list