Hi,
On Saturday 28 January 2006 13:45, jamal wrote:
> > > +extern u32 sysctl_xfrm_aevent_etime;
> > > +extern u32 sysctl_xfrm_aevent_rseqth;
> >
> > Why do we need these defaults? I'd rather see these be removed and
> > just have the user-space KM always set the values (if it needs
> > aevent).
>
> This is mostly out of laziness when i started but has turned out to be
> useful. For ease of testing, i didnt want to set anything or make any
> changes to a KM.
> My initial attempt was to default to a time of 1 second and a replay
> threshold of 1/2 the replay window. I was frustrated using racoon to
> find it was by default setting the window to 4 ;-> (otoh, pluto sets it
> to a very high number). I also couldnt justify why 1 sec or 10 sec or
> 30 seconds as i kept changing them.
> For this reason i figured the admin would be the better person to make
> that decision and i picked the defaults based on what i was
> experimenting with.
> I could remove them if this doesnt sound like a good reason.
I don't really like the idea of generating events unless explicitly
requested by the KM. Once a PF_KEY interface is in place such behaviour
mightl break compatibility with racoon's libipsec which considers unknown
extension headers as errors.
Whether or not a PF_KEY interface for these events is desired is also a
good question. Initially I had a patch for PF_KEY, but only because we
used racoon and it was easier to implement a pfkey extension than adding
netlink support to racoon. Such PF_KEY extensions would just add even
more types to the growing list of non-standard PF_KEY extension headers.
To put it simple: I don't think PF_KEY is worth the hassle unless
someone comes up with an open source software utilizing that interface.
> > > diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> > > index e12d0be..2ccf87a 100644
> > > --- a/net/xfrm/xfrm_state.c
> > > +++ b/net/xfrm/xfrm_state.c
> > >
> > > @@ -62,6 +65,8 @@ static void xfrm_state_gc_destroy(struct
> > > {
> > > if (del_timer(&x->timer))
> > > BUG();
> > > + if (del_timer(&x->rtimer))
> > > + BUG();
> >
> > How about just using x->timer?
>
> This part of the patch i left as is from Krisztian's original patch.
> i have another timer i was going to add later (an idle timeout
> to emulate a certain vendor when an SA is idle) and felt i could use
> rtimer for that as well. For this reason i left it alone since it just
> worked ;->
>
> How expensive are timers these days? Lets handwave a moderately large
> number of SAs (100K). With this we would have 200K timers as opposed to
> 100K if we aggregated them.
Of course we could aggregate these timers, this is just a leftover from
the very first patch. However, this would mean that a more fine-grained
timing would be necessary for that timer instead of the current
precision of one second. Which is of course not a problem, just makes the
patch a bit more intrusive.
> > This seems to be counter-intuitive. Wouldn't it make more sense to
> > schedule a timer in the XFRM_REPLAY_UPDATE case, and not schedule one
> > in the XFRM_REPLAY_TIMEOUT case? Scheduling it in the
> > XFRM_REPLAY_TIMEOUT case means that you may be waking up every maxage
> > jiffies even when nothing at all is happening. While doing it the
> > other way means that you only schedule it when something has happened
> > and we've suppressed the event due to maxdiff.
>
> This is part of Krisztian's original patch - he would be a better
> person to respond to if we can wake him up.
>
> Waking up when nothing is happening is OK, but waking up and generating
> events when nothing is happening is not. so the rescheduling is fine as
> far as i can see but what you describe is an optimization i.e
> knowing nothing has happened and never waking up is even better.
> Krisztian?
Sure, pretty good point.
One more thing: whether or not this timer is necessary really depends on
how the sync daemon uses these events. The most simple trick of simply
adding a large-enough constant to the sequence numbers of outgoing
packets after failover does not take advantage of this feature. Of course
if someone comes up with a more exact way of handling failover cases,
then those additional update messages could be useful.
--
Regards,
Krisztian Kovacs
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
