On Oct 16, 2011, at 8:44 AM, Keith Packard wrote:

> On Sat, 15 Oct 2011 23:09:30 -0700, Jeremy Huddleston <[email protected]> 
> wrote:
> 
>> +    if (new_queue == NULL)
>> +            FatalError("Unable to allocate memory for the event queue.\n");
> 
> This function should be boolean and return whether the queue was
> resized, instead of failing when out of memory.

Ok.

>> +    miEventQueue.tail = (miEventQueue.tail - miEventQueue.head) % 
>> miEventQueue.q_size;
> 
> Shouldn't this just be miEventQueue.q_size? If not, head and tail are
> both 'int' type, so the subtraction can be negative, in which case the %
> operator is undefined (thanks, dmr!).

No.  miEventQueue.q_size is the number of buckets.  (miEventQueue.tail - 
miEventQueue.head) is the number of filled buckets (modulo number of buckets).

% is undefined for negative numbers?  WTF?  That's the first time I've heard 
that.  It's certainly well defined in mathematics.

> 
> 
>> +                    ErrorF("[mi] EQ overflowing.  Increasing queue size to 
>> %lu to accomidate.\n", miEventQueue.q_size << 1);
> 
> accommodate.

That was removed in v2 already.

> 
>> +                    if (miEventQueue.dropped == 1) {
>> +                            ErrorF("[mi] EQ overflowing.  Maximum queue 
>> size reached.  Events will be discarded.\n");
>> +                    xorg_backtrace();
>> +                    } else if (miEventQueue.dropped % 
>> DROP_BACKTRACE_FREQUENCY == 0 &&
>> +                               miEventQueue.dropped / 
>> DROP_BACKTRACE_FREQUENCY <= DROP_BACKTRACE_MAX) {
>> +                            ErrorF("[mi] EQ overflow continuing.  %lu 
>> events have been dropped.\n", miEventQueue.dropped);
>> +                            if (miEventQueue.dropped / 
>> DROP_BACKTRACE_FREQUENCY == DROP_BACKTRACE_MAX) {
>> +                                    ErrorF("[mi] No further overflow 
>> reports will be reported until the clog is cleared.\n");
>> +                            }
>> +                    xorg_backtrace();
> 
> I'm not sure we need to be even this verbose; just a single warning when
> the queue is blocked and another one when it starts working again should
> be sufficient. I fear flooding the disk with errors if this happens
> because your cat falls asleep on the keyboard while watching dancing
> mice on the screen.

That's why we shut up after DROP_BACKTRACE_MAX reports.  What about lowering 
DROP_BACKTRACE_MAX to 5 rather than 20?

_______________________________________________
[email protected]: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Reply via email to