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

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

I just committed an alternative fix to topic/johanna/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.

Johanna

That's actually not the case, and that was the problem. There can be
elements in the queue even if both pointers align; that happens if the
writer is exactly a full round ahead of the reader.

Robin

I meant compare the absolute counters (_ctr), not the pointers (_ptr). Assuming I understand how this works, read_ctr / write_ctr are absolute counts kept in order to keep track of how many times the queue has been read from / written to, are incremented in the same places that the pointers are, and are currently only used to keep statistics for the queue. Since these counts are absolute, there shouldn't be a situation where they're equal unless the queue is empty.

Also, keeping both the counters and the pointers around may be redundant. I think it'd be a pretty easy change to start using absolute values only and mod on access, and I'd imagine performance-wise it would end up being pretty comparable.

Cheers,
Gilbert

I meant compare the absolute counters (_ctr), not the pointers (_ptr).

Ah, you mean num_reads/writes. You're right, that should actually
work, I forgot that we already track these stats. I'm changing the fix
to do this, thanks!

Also, keeping both the counters and the pointers around may be
redundant. I think it'd be a pretty easy change to start using absolute
values only and mod on access, and I'd imagine performance-wise it would
end up being pretty comparable.

Yeah, probably. My main performance concern here is the queue locking,
everything else probably doesn't matter too much.

Robin