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.h 2006-03-13 18:30:21.000000000 +0100 > >>>> +++ b/include/linux/skbuff.h 2006-03-13 18:38:27.000000000 +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_XXXX 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. > > > > <handwaving mode=perhaps_silly> > > 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) > > </handwaving> > > > > 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
