Re: [PATCH] Hot idle

On 13/09/2013 4:48 a.m., Alex Rousskov wrote:
> Hello,
>
> The attached patch fixes a minor bug in the event loop. The fixed
> code may work a tiny bit faster, especially when Squid is not heavily
> loaded.
>
> The current code uses "infinite" precision when it comes to deciding
> whether the next timed event is ready but uses millisecond (1e-3)
> precision when deciding how long to wait before the next event will be
> ready. This inconsistency results in the EventSchedule engine telling
> the main loop that it has 0 milliseconds to poll pending I/O, but when
> asked again (after the I/O is quickly polled), the EventSchedule engine
> often does not schedule the promised event and tells the main loop to
> poll for another 0 milliseconds again. This cycling may happen many
> times in a row (until enough time is wasted for the next event to become
> ready using higher precision).
>
> The proposed change uses the same (millisecond) precision for both
> decisions. After this change, the EventSchedule engine will schedule the
> next event that has 0 milliseconds to wait instead of telling the main
> loop to poll for 0 milliseconds.
>
> Here is a bad-case before/after illustration:
>
> * Before the change:
>
> +0.000 seconds: no events ready, poll for 0ms
> +0.000 seconds: poll returns
> +0.000 seconds: no events ready, poll for 0ms
> +0.000 seconds: poll returns
> +0.000 seconds: no events ready, poll for 0ms
> +0.000 seconds: poll returns
> +0.000 seconds: no events ready, poll for 0ms
> +0.000 seconds: poll returns
> ...
> +0.000 seconds: event(s) ready, scheduled, poll for 1000ms
> +1.000 seconds: poll returns
>
> * After the change:
>
> +0.000 seconds: event(s) ready, scheduled, poll for 1000ms
> +1.000 seconds: poll returns
>
> In lab tests, I have seen pointless iterations repeated 3 times (with
> ALL,9 logging), but I did not look hard for longer sequences, and I
> suspect the real number is often higher.
>
>
> The only potentially bad side effect of this change I can think of is
> that some events will be scheduled and fired a few microseconds before
> they are due (instead of a few microseconds after they are due). I hope
> that would not break any existing Squid code. If it does, we can adjust
> EventSchedule to round up and, hence, fire all events at or after their
> deadline.
>
>
> Context: The next libecap release will support asynchronous (e.g.,
> threaded) eCAP adapters. To support that frequently-requested feature in
> Squid, we are adding another AsyncEngine to the existing four. I was
> surprised to see the new engine called many times within a second on an
> idle Squid. The investigation uncovered the bug addressed by the
> attached patch.
>
>
> HTH,
>
> Alex.

Please do not add an assertion into the main event loop.
If you must add something to test for the case and debug display it, but
I think you can simply remove it.

Otherwise this loks okay.

And thank you, this kind of explains the mystery 100% CPU reports still
coming in and unable to be replicated.

Amos
Received on Fri Sep 13 2013 - 01:43:57 MDT