On 12/03/2012 02:01 PM, Henrik Nordström wrote:
> mån 2012-12-03 klockan 09:07 -0700 skrev Alex Rousskov:
>> I was going to agree with that wholeheartedly, but then I thought about
>> signals. If we visit each non-waiting engine once, then we will only
>> process signals once per main loop step. Is that OK?
>
> Not 100% OK, but not too bad. Causes max 1 seconds delay of processing
> signals, and the only signals I know of which is not comm related (and
> also have comm events) are squid -k signals.
True (although the current small maximum select(2) delay is a result of
bugs elsewhere in the code and should not really be there). If we can do
the right thing here easily, we should (instead of adding more future
problems). And I think we can.
>> I am worried that this will result in 50% or more zero-delay select(2)
>> calls for nearly all Squids because there is usually some AsyncCall
>> activity per I/O loop (but possibly no timed-events).
> So need to make sure AsyncCall is drained last in RunOnce.
Ah, I see another problem with the "each non-waiting engine runs once"
approach. One engine may create work for other engines or for itself,
including async calls and "run this now" lightweight events. This means
we should really continue to use the sawActivity loop so that all
lightweight events are processed before I/O wait starts. This design is
more complex, but it is actually "more correct".
We just need to make sure that heavy events (certain timed events and
signals) interrupt the sawActivity loop. This will fix the issue you are
facing without introducing new problems.
The new condition for the sawActivity loop should be:
while(sawActivity && !sawHeavyEvent)
EventLoop::runOnce() will set sawHeavyEvent member to false before
looping. EventLoop::checkEngine() will set sawHeavyEvent member to true
when an engine returns AsyncEngine::EVENT_HEAVY. The event engine will
return AsyncEngine::EVENT_HEAVY when it encounters an event that
warrants ending the current main loop iteration ASAP.
Attached untested patch implements the above. I am sure it can be
improved further. For example, the loop_delay after heavy event should
probably be set to zero (because there are unprocessed events waiting
and also in anticipation of more heavy events to come -- we are too busy
to wait!).
> Is it sufficient to call dispatchCalls() or do one need to loop over it
> until no activity remains?
One call is sufficient -- the async call queue drains itself completely,
including any calls scheduled during the draining process itself, but
the dispatchCalls() call should remain inside the sawActivity loop
(because events create calls create "now" events create calls create
"now" events ...).
Does the attached patch makes sense to you? Does it solve the "I/O
starvation during rebuild" problem you found?
HTH,
Alex.
Quit event loop when a heavy event is encountered.
This is required so that slow events such as Store rebuilding step do not
force the event loop to run for a very long time, without any chance for the
select loop to run.
=== modified file 'src/AsyncEngine.h'
--- src/AsyncEngine.h 2012-09-01 14:38:36 +0000
+++ src/AsyncEngine.h 2012-12-03 22:26:44 +0000
@@ -34,40 +34,43 @@
/* Abstract interface for async engines which an event loop can utilise.
*
* Some implementations will be truely async, others like the event engine
* will be pseudo async.
*/
class AsyncEngine
{
public:
/* error codes returned from checkEvents. If the return value is not
* negative, then it is the requested delay until the next call. If it is
* negative, it is one of the following codes:
*/
enum CheckError {
/* this engine is completely idle: it has no pending events, and nothing
* registered with it that can create events
*/
EVENT_IDLE = -1,
+ /// found a slow event; we should end current main loop iteration ASAP
+ /// TODO: move after EVENT_ERROR before commit
+ EVENT_HEAVY = -3,
/* some error has occured in this engine */
EVENT_ERROR = -2
};
virtual ~AsyncEngine() {}
/* Check the engine for events. If there are events that have completed,
* the engine should at this point hand them off to their dispatcher.
* Engines that operate asynchronously - i.e. the DiskThreads engine -
* should hand events off to their dispatcher as they arrive rather than
* waiting for checkEvents to be called. Engines like poll and select should
* use this call as the time to perform their checks with the OS for new
* events.
*
* The return value is the status code of the event checking. If its a
* non-negative value then it is used as hint for the minimum requested
* time before checkEvents is called again. I.e. the event engine knows
* how long it is until the next event will be scheduled - so it will
* return that time (in milliseconds).
*
=== modified file 'src/EventLoop.cc'
--- src/EventLoop.cc 2012-09-25 16:38:36 +0000
+++ src/EventLoop.cc 2012-12-03 22:51:43 +0000
@@ -21,123 +21,129 @@
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111, USA.
*
*/
#include "squid.h"
#include "AsyncEngine.h"
#include "Debug.h"
#include "EventLoop.h"
#include "base/AsyncCallQueue.h"
#include "SquidTime.h"
EventLoop::EventLoop() : errcount(0), last_loop(false), timeService(NULL),
- primaryEngine(NULL)
+ primaryEngine(NULL), sawHeavyEvent(false)
{}
void
EventLoop::checkEngine(AsyncEngine * engine, bool const primary)
{
int requested_delay;
if (!primary)
requested_delay = engine->checkEvents(0);
else
requested_delay = engine->checkEvents(loop_delay);
if (requested_delay < 0)
switch (requested_delay) {
case AsyncEngine::EVENT_IDLE:
debugs(1, 9, "Engine " << engine << " is idle.");
break;
case AsyncEngine::EVENT_ERROR:
runOnceResult = false;
error = true;
break;
+ case AsyncEngine::EVENT_HEAVY:
+ debugs(1, 9, "Engine " << engine << " is busy.");
+ sawHeavyEvent = true;
+ break;
+
default:
fatal_dump("unknown AsyncEngine result");
}
else {
- /* not idle or error */
+ // a time delay was returned
runOnceResult = false;
if (requested_delay < loop_delay)
loop_delay = requested_delay;
}
}
void
EventLoop::prepareToRun()
{
last_loop = false;
errcount = 0;
}
void
EventLoop::registerEngine(AsyncEngine *engine)
{
engines.push_back(engine);
}
void
EventLoop::run()
{
prepareToRun();
while (!runOnce());
}
bool
EventLoop::runOnce()
{
bool sawActivity = false;
runOnceResult = true;
error = false;
loop_delay = EVENT_LOOP_TIMEOUT;
+ sawHeavyEvent = false;
AsyncEngine *waitingEngine = primaryEngine;
if (!waitingEngine && !engines.empty())
waitingEngine = engines.back();
do {
// generate calls and events
typedef engine_vector::iterator EVI;
for (EVI i = engines.begin(); i != engines.end(); ++i) {
if (*i != waitingEngine)
checkEngine(*i, false);
}
// dispatch calls accumulated so far
sawActivity = dispatchCalls();
if (sawActivity)
runOnceResult = false;
- } while (sawActivity);
+ } while (sawActivity && !sawHeavyEvent);
if (waitingEngine != NULL)
checkEngine(waitingEngine, true);
if (timeService != NULL)
timeService->tick();
// dispatch calls scheduled by waitingEngine and timeService
sawActivity = dispatchCalls();
if (sawActivity)
runOnceResult = false;
if (error) {
++errcount;
debugs(1, DBG_CRITICAL, "Select loop Error. Retry " << errcount);
} else
errcount = 0;
if (errcount == 10)
return true;
=== modified file 'src/EventLoop.h'
--- src/EventLoop.h 2012-09-20 11:28:21 +0000
+++ src/EventLoop.h 2012-12-03 22:18:32 +0000
@@ -90,23 +90,24 @@
int errcount;
private:
/** setup state variables prior to running */
void prepareToRun();
/** check an individual engine */
void checkEngine(AsyncEngine * engine, bool const primary);
/** dispatch calls and events scheduled during checkEngine() */
bool dispatchCalls();
bool last_loop;
typedef Vector<AsyncEngine *> engine_vector;
engine_vector engines;
TimeEngine * timeService;
AsyncEngine * primaryEngine;
int loop_delay; /**< the delay to be given to the primary engine */
bool error; /**< has an error occured in this loop */
bool runOnceResult; /**< the result from runOnce */
+ bool sawHeavyEvent; ///< must stop processing events ASAP and do I/O
};
#endif /* SQUID_EVENTLOOP_H */
=== modified file 'src/event.cc'
--- src/event.cc 2012-09-01 14:38:36 +0000
+++ src/event.cc 2012-12-03 22:23:18 +0000
@@ -248,44 +248,44 @@
PROF_start(eventRun);
debugs(41, 5, HERE << "checkEvents");
while ((event = tasks)) {
if (event->when > current_dtime)
break;
/* XXX assumes event->name is static memory! */
AsyncCall::Pointer call = asyncCall(41,5, event->name,
EventDialer(event->func, event->arg, event->cbdata));
ScheduleCallHere(call);
last_event_ran = event->name; // XXX: move this to AsyncCallQueue
const bool heavy = event->weight &&
(!event->cbdata || cbdataReferenceValid(event->arg));
tasks = event->next;
delete event;
- // XXX: We may be called again during the same event loop iteration.
- // Is there a point in breaking now?
- if (heavy)
- break; // do not dequeue events following a heavy event
+ if (heavy) {
+ PROF_stop(eventRun);
+ return EVENT_HEAVY; // do not dequeue events after a heavy event
+ }
}
PROF_stop(eventRun);
return checkDelay();
}
void
EventScheduler::clean()
{
while (ev_entry * event = tasks) {
tasks = event->next;
delete event;
}
tasks = NULL;
}
void
EventScheduler::dump(StoreEntry * sentry)
{
