Re: [PATCH] [NET] : get rid of 'struct sec_path *' in skbuff if ! CONFIG_XFRM
On 3/14/06, Eric Dumazet <[EMAIL PROTECTED]> wrote: > Arnaldo Carvalho de Melo a écrit : > > On 3/14/06, Eric Dumazet <[EMAIL PROTECTED]> wrote: > >> Arnaldo Carvalho de Melo a écrit : > >>> On 3/14/06, Eric Dumazet <[EMAIL PROTECTED]> wrote: > Herbert Xu a écrit : > > On Tue, Mar 14, 2006 at 07:23:05AM +0100, Eric Dumazet wrote: > >> Hum, but then we need a new macro or prototype, because n->sp is not > >> valid > >> > >> n->sp = secpath_get(skb->sp); > >> > >> would still miscompile, even if secpath_get() is a no-op > > How about just leaving sp in the structure unconditionally? > > > > Cheers, > Well, the point of this patch is to shrink a little bit 'struct sk_buff' > :) > --- a/include/linux/skbuff.h2006-03-13 18:30:21.0 +0100 > +++ b/include/linux/skbuff.h2006-03-13 18:38:27.0 +0100 > @@ -243,7 +243,9 @@ > } mac; > > struct dst_entry *dst; > +#ifdef CONFIG_XFRM > struct sec_path*sp; > +#endif > >>> Humm, we're trying to avoid/remove ifdefs in structs as most of the > >>> users will use distro kernels where most (if not all) of these config > >>> options are enabled :-\ > >> Sure, but in the case of distro kernel, as you said, CONFIG_XFRM will be > >> defined. My patch changes nothing for them. Should we delete all #ifdef > >> CONFIG_ from linux kernel sources because of distros ? > >> > >> At least for us developers, the #ifdef permits to detect some code that > >> would > >> try to access sp, while secpath_put() is not called (so a memory leak > >> happens) > >> > >> If we really want to bloat kernel structures, then we *should* change > >> __kfree_skb() and really call secpath_put() (delete the #ifdef CONFIG_XFRM) > >> > >> spinlock_t for example are (null)/(void) on UP kernels (!CONFIG_SMP) , > >> should I define a special sec_path_ref_t type that happens to be a > >> (null)/(void) if !CONFIG_XFRM ? > >> > >> Defining a type for one occurrence (in struct sk_buf) seems overkill, and > >> Linus would kill us for that. > > > > > > Humm, I was thinking more about something similar to the struct sock > > hierarchy, where we use different slabcaches for different purposes, > > extending a basic sk_buff and switching the slabcache used for > > sk_buffs when using things that requires specific non-basic sk_buff > > fields, but I'd have to come up with patches to see if this is not > > "mode=perhaps_silly" 8) > > > > > > Thats an idea (David already mentioned it), but only for simple hierarchy (2 > levels at most) I guess it was me that mentioned this in the past, but anyway, idea involves something like skb_alloc_private_area() done at module initialization time, where we'd get just an offset into the "extension" part of a sk_buff, code would use something like skb_has_private_area(id) or something like that to verify if the skb instance was suitable for its purposes, anyway, just more handwaving 8) - Arnaldo - 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
Re: [PATCH] [NET] : get rid of 'struct sec_path *' in skbuff if ! CONFIG_XFRM
Arnaldo Carvalho de Melo a écrit : On 3/14/06, Eric Dumazet <[EMAIL PROTECTED]> wrote: Arnaldo Carvalho de Melo a écrit : On 3/14/06, Eric Dumazet <[EMAIL PROTECTED]> wrote: Herbert Xu a écrit : On Tue, Mar 14, 2006 at 07:23:05AM +0100, Eric Dumazet wrote: Hum, but then we need a new macro or prototype, because n->sp is not valid n->sp = secpath_get(skb->sp); would still miscompile, even if secpath_get() is a no-op How about just leaving sp in the structure unconditionally? Cheers, Well, the point of this patch is to shrink a little bit 'struct sk_buff' :) --- a/include/linux/skbuff.h2006-03-13 18:30:21.0 +0100 +++ b/include/linux/skbuff.h2006-03-13 18:38:27.0 +0100 @@ -243,7 +243,9 @@ } mac; struct dst_entry *dst; +#ifdef CONFIG_XFRM struct sec_path*sp; +#endif Humm, we're trying to avoid/remove ifdefs in structs as most of the users will use distro kernels where most (if not all) of these config options are enabled :-\ Sure, but in the case of distro kernel, as you said, CONFIG_XFRM will be defined. My patch changes nothing for them. Should we delete all #ifdef CONFIG_ from linux kernel sources because of distros ? At least for us developers, the #ifdef permits to detect some code that would try to access sp, while secpath_put() is not called (so a memory leak happens) If we really want to bloat kernel structures, then we *should* change __kfree_skb() and really call secpath_put() (delete the #ifdef CONFIG_XFRM) spinlock_t for example are (null)/(void) on UP kernels (!CONFIG_SMP) , should I define a special sec_path_ref_t type that happens to be a (null)/(void) if !CONFIG_XFRM ? Defining a type for one occurrence (in struct sk_buf) seems overkill, and Linus would kill us for that. Humm, I was thinking more about something similar to the struct sock hierarchy, where we use different slabcaches for different purposes, extending a basic sk_buff and switching the slabcache used for sk_buffs when using things that requires specific non-basic sk_buff fields, but I'd have to come up with patches to see if this is not "mode=perhaps_silly" 8) Thats an idea (David already mentioned it), but only for simple hierarchy (2 levels at most) Let's take an example : struct file definition from 'include/linux/fs.h' Many fields are seldom used (for sockets) : unsigned intf_uid, f_gid; /* only by an obscure netfilter thing */ struct file_ra_statef_ra; /* only for readahead capable files (regular files) */ struct list_headf_ep_links; /* ony if epoll is used */ spinlock_t f_ep_lock; void*f_security; /* only if strong security */ struct fown_struct f_owner; /* ... */ The 'f_ra' is so big it would really be a win to include it only for readahead capable files... I had a patch to address this problem. Eric - 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
Re: [PATCH] [NET] : get rid of 'struct sec_path *' in skbuff if ! CONFIG_XFRM
On 3/14/06, Eric Dumazet <[EMAIL PROTECTED]> wrote: > Arnaldo Carvalho de Melo a écrit : > > On 3/14/06, Eric Dumazet <[EMAIL PROTECTED]> wrote: > >> Herbert Xu a écrit : > >>> On Tue, Mar 14, 2006 at 07:23:05AM +0100, Eric Dumazet wrote: > Hum, but then we need a new macro or prototype, because n->sp is not > valid > > n->sp = secpath_get(skb->sp); > > would still miscompile, even if secpath_get() is a no-op > >>> How about just leaving sp in the structure unconditionally? > >>> > >>> Cheers, > >> Well, the point of this patch is to shrink a little bit 'struct sk_buff' :) > > > >> --- a/include/linux/skbuff.h2006-03-13 18:30:21.0 +0100 > >> +++ b/include/linux/skbuff.h2006-03-13 18:38:27.0 +0100 > >> @@ -243,7 +243,9 @@ > >> } mac; > >> > >> struct dst_entry *dst; > >> +#ifdef CONFIG_XFRM > >> struct sec_path*sp; > >> +#endif > > > > Humm, we're trying to avoid/remove ifdefs in structs as most of the > > users will use distro kernels where most (if not all) of these config > > options are enabled :-\ > > Sure, but in the case of distro kernel, as you said, CONFIG_XFRM will be > defined. My patch changes nothing for them. Should we delete all #ifdef > CONFIG_ from linux kernel sources because of distros ? > > At least for us developers, the #ifdef permits to detect some code that would > try to access sp, while secpath_put() is not called (so a memory leak happens) > > If we really want to bloat kernel structures, then we *should* change > __kfree_skb() and really call secpath_put() (delete the #ifdef CONFIG_XFRM) > > spinlock_t for example are (null)/(void) on UP kernels (!CONFIG_SMP) , > should I define a special sec_path_ref_t type that happens to be a > (null)/(void) if !CONFIG_XFRM ? > > Defining a type for one occurrence (in struct sk_buf) seems overkill, and > Linus would kill us for that. Humm, I was thinking more about something similar to the struct sock hierarchy, where we use different slabcaches for different purposes, extending a basic sk_buff and switching the slabcache used for sk_buffs when using things that requires specific non-basic sk_buff fields, but I'd have to come up with patches to see if this is not "mode=perhaps_silly" 8) - Arnaldo - 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
Re: [PATCH] [NET] : get rid of 'struct sec_path *' in skbuff if ! CONFIG_XFRM
Arnaldo Carvalho de Melo a écrit : On 3/14/06, Eric Dumazet <[EMAIL PROTECTED]> wrote: Herbert Xu a écrit : On Tue, Mar 14, 2006 at 07:23:05AM +0100, Eric Dumazet wrote: Hum, but then we need a new macro or prototype, because n->sp is not valid n->sp = secpath_get(skb->sp); would still miscompile, even if secpath_get() is a no-op How about just leaving sp in the structure unconditionally? Cheers, Well, the point of this patch is to shrink a little bit 'struct sk_buff' :) --- a/include/linux/skbuff.h2006-03-13 18:30:21.0 +0100 +++ b/include/linux/skbuff.h2006-03-13 18:38:27.0 +0100 @@ -243,7 +243,9 @@ } mac; struct dst_entry *dst; +#ifdef CONFIG_XFRM struct sec_path*sp; +#endif Humm, we're trying to avoid/remove ifdefs in structs as most of the users will use distro kernels where most (if not all) of these config options are enabled :-\ Sure, but in the case of distro kernel, as you said, CONFIG_XFRM will be defined. My patch changes nothing for them. Should we delete all #ifdef CONFIG_ from linux kernel sources because of distros ? At least for us developers, the #ifdef permits to detect some code that would try to access sp, while secpath_put() is not called (so a memory leak happens) If we really want to bloat kernel structures, then we *should* change __kfree_skb() and really call secpath_put() (delete the #ifdef CONFIG_XFRM) spinlock_t for example are (null)/(void) on UP kernels (!CONFIG_SMP) , should I define a special sec_path_ref_t type that happens to be a (null)/(void) if !CONFIG_XFRM ? Defining a type for one occurrence (in struct sk_buf) seems overkill, and Linus would kill us for that. Eric - 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
Re: [PATCH] [NET] : get rid of 'struct sec_path *' in skbuff if ! CONFIG_XFRM
On 3/14/06, Eric Dumazet <[EMAIL PROTECTED]> wrote: > Herbert Xu a écrit : > > On Tue, Mar 14, 2006 at 07:23:05AM +0100, Eric Dumazet wrote: > >> Hum, but then we need a new macro or prototype, because n->sp is not valid > >> > >> n->sp = secpath_get(skb->sp); > >> > >> would still miscompile, even if secpath_get() is a no-op > > > > How about just leaving sp in the structure unconditionally? > > > > Cheers, > > Well, the point of this patch is to shrink a little bit 'struct sk_buff' :) > --- a/include/linux/skbuff.h2006-03-13 18:30:21.0 +0100 > +++ b/include/linux/skbuff.h2006-03-13 18:38:27.0 +0100 > @@ -243,7 +243,9 @@ > } mac; > > struct dst_entry *dst; > +#ifdef CONFIG_XFRM > struct sec_path*sp; > +#endif Humm, we're trying to avoid/remove ifdefs in structs as most of the users will use distro kernels where most (if not all) of these config options are enabled :-\ - Arnaldo - 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
Re: [PATCH] [NET] : get rid of 'struct sec_path *' in skbuff if ! CONFIG_XFRM
Herbert Xu a écrit :
On Tue, Mar 14, 2006 at 07:23:05AM +0100, Eric Dumazet wrote:
Hum, but then we need a new macro or prototype, because n->sp is not valid
n->sp = secpath_get(skb->sp);
would still miscompile, even if secpath_get() is a no-op
How about just leaving sp in the structure unconditionally?
Cheers,
Well, the point of this patch is to shrink a little bit 'struct sk_buff' :)
In this second try, I added a secpath_copy() helper, so that CONFIG_XFRM test
is done in include/net/xfrm.h file
[PATCH] [NET] : get rid of 'struct sec_path *' in sk_buff if ! CONFIG_XFRM
There is a strange glitch about secpath_put() and secpath_get() calls.
secpath_put() is called from __kfree_skb() only if CONFIG_XFRM is defined.
secpath_get() is called from skb_clone() and copy_skb_header() if CONFIG_INET
is defined (!!!)
I think the intention was to use CONFIG_XFRM everywhere ?
If yes, we can save a 'struct sec_path' pointer from 'struct sk_buff' if !
CONFIG_XFRM
Signed-off-by: Eric Dumazet <[EMAIL PROTECTED]>
--- a/include/net/xfrm.h2006-03-14 08:23:19.0 +0100
+++ b/include/net/xfrm.h2006-03-14 08:26:38.0 +0100
@@ -635,6 +635,14 @@
return sp;
}
+static inline void
+secpath_copy(struct sk_buff *dst, const struct sk_buff *src)
+{
+#ifdef CONFIG_XFRM
+ dst->sp = secpath_get(src->sp);
+#endif
+}
+
extern void __secpath_destroy(struct sec_path *sp);
static inline void
--- a/include/linux/skbuff.h2006-03-13 18:30:21.0 +0100
+++ b/include/linux/skbuff.h2006-03-13 18:38:27.0 +0100
@@ -243,7 +243,9 @@
} mac;
struct dst_entry *dst;
+#ifdef CONFIG_XFRM
struct sec_path*sp;
+#endif
/*
* This is the control buffer. It is free to use for every
--- a/net/core/skbuff.c 2006-03-13 18:31:03.0 +0100
+++ b/net/core/skbuff.c 2006-03-14 08:24:33.0 +0100
@@ -415,10 +415,7 @@
C(mac);
C(dst);
dst_clone(skb->dst);
- C(sp);
-#ifdef CONFIG_INET
- secpath_get(skb->sp);
-#endif
+ secpath_copy(n, skb);
memcpy(n->cb, skb->cb, sizeof(skb->cb));
C(len);
C(data_len);
@@ -483,9 +480,7 @@
new->priority = old->priority;
new->protocol = old->protocol;
new->dst= dst_clone(old->dst);
-#ifdef CONFIG_INET
- new->sp = secpath_get(old->sp);
-#endif
+ secpath_copy(new, old);
new->h.raw = old->h.raw + offset;
new->nh.raw = old->nh.raw + offset;
new->mac.raw= old->mac.raw + offset;
Re: [PATCH] [NET] : get rid of 'struct sec_path *' in skbuff if ! CONFIG_XFRM
On Tue, Mar 14, 2006 at 07:23:05AM +0100, Eric Dumazet wrote:
>
> Hum, but then we need a new macro or prototype, because n->sp is not valid
>
> n->sp = secpath_get(skb->sp);
>
> would still miscompile, even if secpath_get() is a no-op
How about just leaving sp in the structure unconditionally?
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
Re: [PATCH] [NET] : get rid of 'struct sec_path *' in skbuff if ! CONFIG_XFRM
Herbert Xu a écrit :
Eric Dumazet <[EMAIL PROTECTED]> wrote:
- C(sp);
-#ifdef CONFIG_INET
- secpath_get(skb->sp);
+#ifdef CONFIG_XFRM
+ n->sp = secpath_get(skb->sp);
#endif
Please move the ifdef into xfrm.h while you're at it. That is, make
secpath_get a no-op if CONFIG_XFRM is not defined.
Thanks,
Hum, but then we need a new macro or prototype, because n->sp is not valid
n->sp = secpath_get(skb->sp);
would still miscompile, even if secpath_get() is a no-op
Maybe a new secpath_copy() declared in include/net/xfrm.h ?
static inline void
secpath_copy(struct sk_buff *dst,
struct_sk_buff *src)
{
#ifdef CONFIG_XFRM
dst->sp = secpath_get(src);
#endif
}
Eric
-
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
Re: [PATCH] [NET] : get rid of 'struct sec_path *' in skbuff if ! CONFIG_XFRM
Eric Dumazet <[EMAIL PROTECTED]> wrote:
>
> - C(sp);
> -#ifdef CONFIG_INET
> - secpath_get(skb->sp);
> +#ifdef CONFIG_XFRM
> + n->sp = secpath_get(skb->sp);
> #endif
Please move the ifdef into xfrm.h while you're at it. That is, make
secpath_get a no-op if CONFIG_XFRM is not defined.
Thanks,
--
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
[PATCH] [NET] : get rid of 'struct sec_path *' in skbuff if ! CONFIG_XFRM
There is a strange glitch about secpath_put() and secpath_get() calls. secpath_put() is called from __kfree_skb() only if CONFIG_XFRM is defined. secpath_get() is called from skb_clone() and copy_skb_header() if CONFIG_INET is defined (!!!) I think the intention was to use CONFIG_XFRM everywhere ? If yes, we can save a 'struct sec_path' pointer from 'struct sk_buff' if ! CONFIG_XFRM Signed-off-by: Eric Dumazet <[EMAIL PROTECTED]> --- a/include/linux/skbuff.h2006-03-13 18:30:21.0 +0100 +++ b/include/linux/skbuff.h2006-03-13 18:38:27.0 +0100 @@ -243,7 +243,9 @@ } mac; struct dst_entry *dst; +#ifdef CONFIG_XFRM struct sec_path*sp; +#endif /* * This is the control buffer. It is free to use for every --- a/net/core/skbuff.c 2006-03-13 18:31:03.0 +0100 +++ b/net/core/skbuff.c 2006-03-13 18:39:01.0 +0100 @@ -415,9 +415,8 @@ C(mac); C(dst); dst_clone(skb->dst); - C(sp); -#ifdef CONFIG_INET - secpath_get(skb->sp); +#ifdef CONFIG_XFRM + n->sp = secpath_get(skb->sp); #endif memcpy(n->cb, skb->cb, sizeof(skb->cb)); C(len); @@ -483,7 +482,7 @@ new->priority = old->priority; new->protocol = old->protocol; new->dst= dst_clone(old->dst); -#ifdef CONFIG_INET +#ifdef CONFIG_XFRM new->sp = secpath_get(old->sp); #endif new->h.raw = old->h.raw + offset;
