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)
Let's take an example :
struct file definition from 'include/linux/fs.h'
Many fields are seldom used (for sockets) :
unsigned int f_uid, f_gid; /* only by an obscure netfilter thing */
struct file_ra_state f_ra; /* only for readahead capable files (regular
files) */
struct list_head f_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
