On 2008/01/10 00:06, Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> Max Kellermann wrote:
> > Waking up daemons without a reason is sloppy design most of the time.
> > A quick look at the conntrackd code made me believe that conntrackd
> > just doesn't check when the next scheduled event is due, and instead
> > performs a check on all alarm objects in the current step 5 times a
> > second. That is easily fixable, and not only saves CPU cycles and
> > power, but also leads to better overall design.
>
> Indeed. This makes a lot sense to me. I have committed a patch to SVN to
> wake up the daemon only if there is any alarm event to process instead
> of polling. I'll do some testing of it tomorrow.
Looks much better!
15 files changed, 103 insertions(+), 208 deletions(-)
Also, the code has become smaller :)
Why did you create separate functions for setting secs and usecs?
(set_alarm_expiration_secs and set_alarm_expiration_usecs); both
functions are not prototyped in a header file. Why not add something
like this to alarm.h:
static inline void
set_alarm_expiration_secs(struct alarm_list *t, long tv_sec, long tv_usec)
{
t->tv.tv_sec = tv_sec;
t->tv.tv_usec = tv_usec;
}
Since the alarm_list struct is public anyway, this looks more elegant
and creates smaller code.
The function do_alarm_run() assumes that all alarms are sorted by
their due time, but add_alarm() does not enforce this.
I do not understand why you use random() to generate the next alarm
time in sync-alarm.c.
Why do you call INIT_LIST_HEAD() on all alarm_list objects? For
linux_list.h, that is only required on the sentinel (the global
variable "alarm_list" in this case which is already statically
initalized with the LIST_HEAD macro).
> > The whole alarm.c looks like duplicated effort, you could have used
> > libevent instead.
>
> Well, I think that libevent is too much since conntrackd handles not
> that many descriptors and the alarm implementation is enough for what
> conntrackd needs IMO.
I tend to use libevent for small projects, too; maybe libowfat is more
appropriate for smaller projects. That is a matter of taste.
> > I am using the most recent release, i.e. 0.9.5. I have no idea about
> > "alarm" or "persistent" mode, and I did not find any documentation on
> > this. I am using the "stats" example configuration from the tarball.
>
> Please, could you check out a working copy from SVN and tell me if the
> problem that you're reporting persists?
With conntrackd and libnetfilter_conntrack from SVN r7196, the crash
does not occur anymore. Please tell me as soon as you release both,
so I can update the Debian package.
But have a look at the strace:
select(6, [4 5], NULL, NULL, {1, 0}) = 0 (Timeout)
After that, it goes into an endless loop of:
select(6, [4 5], NULL, NULL, {0, 0}) = 0 (Timeout)
This is because select() modifies the timeout value, it contains the
rest time when select() returns. So timeout is zeroed after the first
select() because it times out, and do_alarm_run() never sets a new
timeout value.
I suggest you pass a NULL timeout when there is no alarm, and ensure
that you always set the correct next_alarm value in do_alarm_run().
Max
-
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
