[systemd-devel] [PATCH 20/28] dhcp: Add timeout and main loop support

[Lennart Poettering]

On Fri, 15.11.13 11:29, Patrik Flykt (patrik.flykt at linux.intel.com) wrote:

> > > +                err = sd_event_add_monotonic(sd_event_get(s),
> > next_timeout,
> > > +                                             10 * USEC_PER_MSEC,
> > > +                                             client_timeout_resend,
> > client,
> > > +
> > > &client->timeout_resend);
> > 
> > if you don't have a very good reason to specify the accuracy as 10ms,
> > I'd always recommend to pass 0 instead, which results in the default
> > accuracy of 250ms (I wouldn't be too surprised if 250ms is too
> > inaccurate for this usecase, so your code might be fine, just wanted
> > to
> > take the opportuntine to point this out...
> 
> I tried to figure out some reasonable accuracy for sending the DHCP
> messages so that hordes of clients would not trigger all at the same
> time. The default 250ms seems to be a too coarse interval for this, 10ms
> looked decently low enough to spread out the requests without being
> overly aggressive. At some point there will be real numbers from real
> use cases and the accuracy should be adjusted accordingly.

Just to take the opportunity to talk about this awesome feature of
sd-event: what the accuracy controls is a time range in which the event
will fire, that starts with the specified timeout time and lasts until
that timeout time plus the accuracy. Within that range sd-event will try
to find a good time to wake up in order to optimize power
consumption. For that it tries to move the wakeup across all processes
to the same point in time within each second, and if that point in time
does not lie within the desired range, then it will try to move it to
the same place within each 250ms. If the range doesn't allow that
either, then it will put the wakeup at the end of the desired range. The
point within the same second/the same 250ms is calculated from
/etc/machine-id which is randomized but constant for each system and
hopefully unique in the network, thus avoiding traffic floods. Putting
this together we should minimize wakeups, and if we do wakeup then we
should do work across all processes, but at different times on different
computers.

> > > -        return client_send_discover(client, 0);
> > > +        err = sd_event_add_monotonic(client->event,
> > now(CLOCK_MONOTONIC), 0,
> > > +                                     client_timeout_resend, client,
> > > +                                     &client->timeout_resend);
> > 
> > Hmm, so this would immediately trigger since "now" is already passed,
> > by
> > the time you read it... Note that "0" can be used as time here too, to
> > indicate that you want to be executed immediately, i.e. it indicates
> > in
> > this context the ealiest sensible time.
> 
> Bummer. I was thinking that the new event would be run immediately after
> the current one had returned to the main loop (or actually started in
> this case). 

Yes, that is what I meant with "immediately". sd-event is not recursive,
it will never dispatch an event from another event. Basically, each time
an event has been dispatched we just determine the next one to dispatch
by looking for the "oldest" one queued. If you specify 0 as a time the
event source is necessarily as "old" as an event source can be.

> The general idea was to do the resending in one place only
> and keep the new event handling clear from possible interference with
> the currently running one, if needed. 'now(CLOCK_MONOTONIC) + 1' would
> be enough in those cases, I'd guess. But in here it'd be easier to just
> call the function directly. I'll check the other related parts also.

Passing 0 for this is totally OK. My comment was about using 0 instead
of now(), since the latter is unnecessary and calls a syscall for no
point if all you need is to schedule another wakeup.

Lennart

-- 
Lennart Poettering, Red Hat