On Fri, Jan 27, 2006 at 08:05:23AM -0500, jamal wrote:
>
> the core changes
Thanks for the patches Jamal. I agree with the basic idea.
> +extern u32 sysctl_xfrm_aevent_on;
I'd prefer for this to be automatically determined. Indeed, this is
a generic netlink problem. We want to be easily determine at run time
whether there are netlink sockets subscribed to a given multicast group.
Here is an idea, in addition to mc_list, we keep a bit mask of the groups
that exist on mc_list. This bit mask can then be used to quickly determine
if a netlink skb needs to be generated or not.
This mask would be updated at bind(2) time. Updating this is O(n) where
n is the number of members on mc_list. This should be fine since
netlink_broadcast itself which occurs much more often is also O(n).
> +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).
> 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?
> @@ -104,6 +109,20 @@ static inline unsigned long make_jiffies
> return secs*HZ;
> }
>
> +void xfrm_replay_notify(struct xfrm_state *, int);
Duplicate prototype.
> @@ -762,6 +792,50 @@ out:
> }
> EXPORT_SYMBOL(xfrm_state_walk);
>
> +
> +void xfrm_replay_notify(struct xfrm_state *x, int event)
> +{
> + struct km_event c;
> + /* we send notify messages in case
> + * 1. we updated on of the sequence numbers, and the seqno difference
> + * is at least x->replay_maxdiff, in this case we also update the
> + * timeout of our timer function
> + * 2. if x->replay_maxage has elapsed since last update,
> + * and there were changes
> + *
> + * The state structure must be locked!
> + */
> +
> + switch (event) {
> + case XFRM_REPLAY_UPDATE:
> + if (x->replay_maxdiff &&
> + (x->replay.seq - x->preplay.seq < x->replay_maxdiff) &&
> + (x->replay.oseq - x->preplay.oseq < x->replay_maxdiff))
> + return;
> +
> + break;
> +
> + case XFRM_REPLAY_TIMEOUT:
> + if ((x->replay.seq == x->preplay.seq) &&
> + (x->replay.bitmap == x->preplay.bitmap) &&
> + (x->replay.oseq == x->preplay.oseq))
> + goto resched;
> +
> + break;
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.
> @@ -805,8 +879,10 @@ void xfrm_replay_advance(struct xfrm_sta
> diff = x->replay.seq - seq;
> x->replay.bitmap |= (1U << diff);
> }
> +
> + if (sysctl_xfrm_aevent_on)
> + xfrm_replay_notify(x, XFRM_REPLAY_UPDATE);
This construct occurs many times. How about putting it into an inline
function?
> }
> -EXPORT_SYMBOL(xfrm_replay_advance);
Huh?
> @@ -835,7 +911,7 @@ void km_state_notify(struct xfrm_state *
> EXPORT_SYMBOL(km_policy_notify);
> EXPORT_SYMBOL(km_state_notify);
>
> -static void km_state_expired(struct xfrm_state *x, int hard)
> +void km_state_expired(struct xfrm_state *x, int hard)
> {
> struct km_event c;
This hunk should go into the SA expire patch.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[EMAIL PROTECTED]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
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
