Re: Multiple issues in Squid-3.2.3 SMP + rock + aufs + a bit of load

On 12/04/2012 01:47 AM, Henrik Nordström wrote:
> mån 2012-12-03 klockan 22:25 -0700 skrev Alex Rousskov:
>
>> I disagree with this logic. Yes, sawActivity approach supports long
>> chains, but they are not the loop's fault! If some chain is too long,
>> the bug is in that chain's code and should be fixed there. If it cannot
>> be fixed there, then the chain is not "too long", but Squid is just
>> being asked to do "too much".
>
> The event engines we talk about here is
>
> SignalEngine
> EventScheduler
> StoreEngine
>
> Of these only possibly StoreEngine may need repeated work.
>
> SignalEngine is happy if it gets polled every now and then at low
> frequency.

> EventScheduler do not want to be polled more than once per loop.

I think that would be true if we did not have any zero-delay events
mimicking async calls. I believe we still have a few of those left. If
you have reviewed all of them and believe they all will be fine with a
non-zero delay (essentially), then I agree that EventScheduler does not
need to be polled more than once (and sawActivity loop can be removed).

> StoreEngine is about store callbacks. In the past this should not be
> invoked too often per loop.

I think it makes sense to scan store events once per loop because they
are similar to Comm I/O events and those are scanned once per loop.

>> Perhaps we can agree to let the sleeping dog lie? The bug you needed to
>> fix was with the heavy events handling. My patch or your next patch fix
>> that bug. Let's not remove the sawActivity loop until its removal
>> becomes the best solution for some other bug. We can discuss the loop's
>> pros and cons in the context of that specific bug then.
>
> Ok.
>
> I would probably go for both pathes. Solves different aspects of the
> event loop timings.
>
> My patch fixes the delay calculation, both end result and readability,
> and enabling removal of sawActivity. But it does not fix the comm
> starvation issue without removal of sawActivity. Today the comm loop
> delay is often too short in current code.

My argument is that once heavy events are supported, there is no comm
starvation issue that the sawActivity loop is responsible for. Thus,
doing something to fix that issue is out of scope (and should have some
other benefit such as fixing another bug to justify risky changes).

> Your patch addresses sawActivity and heavy events. If sawActivity loop
> is removed in future then your patch have no effect but still makes code
> more readable.

Not true. My patch has an effect even without the sawActivity loop
because my changes stop EventScheduler::checkEvents() loop after the
first heavy event (IIRC, you wanted only one heavy event per main loop
iteration). And that change is all that is needed to support heavy
events after sawActivity loop is gone.

HTH,

Alex.
Received on Tue Dec 04 2012 - 15:29:50 MST