* [PATCH] Reduce number of pointer dereferences in IPv4 netfilter LOG module function dump_packet() @ 2010-11-14 21:35 Jesper Juhl 2010-11-15 0:49 ` Eric Dumazet 0 siblings, 1 reply; 3+ messages in thread From: Jesper Juhl @ 2010-11-14 21:35 UTC (permalink / raw) To: Netfilter Core Team Cc: netdev, linux-kernel, netfilter, netfilter-devel, David S. Miller, Rusty Russell By adding two pointer variables to net/ipv4/netfilter/ipt_LOG.c::dump_packet() we can save 16 bytes of .text and 9 pointer dereferences. before this patch we did 20 pointer dereferences and had this object file size: text data bss dec hex filename 6233 600 3080 9913 26b9 net/ipv4/netfilter/ipt_LOG.o after this patch we do just 11 pointer dereferences and have this object file size: text data bss dec hex filename 6217 600 3080 9897 26a9 net/ipv4/netfilter/ipt_LOG.o Please Cc me on replies. Signed-off-by: Jesper Juhl <jj@chaosbits.net> --- ipt_LOG.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/net/ipv4/netfilter/ipt_LOG.c b/net/ipv4/netfilter/ipt_LOG.c index 72ffc8f..02a92de 100644 --- a/net/ipv4/netfilter/ipt_LOG.c +++ b/net/ipv4/netfilter/ipt_LOG.c @@ -39,6 +39,8 @@ static void dump_packet(struct sbuff *m, struct iphdr _iph; const struct iphdr *ih; unsigned int logflags; + struct sock *sk; + struct socket *sk_socket; if (info->type == NF_LOG_TYPE_LOG) logflags = info->u.log.logflags; @@ -335,13 +337,15 @@ static void dump_packet(struct sbuff *m, } /* Max length: 15 "UID=4294967295 " */ - if ((logflags & IPT_LOG_UID) && !iphoff && skb->sk) { - read_lock_bh(&skb->sk->sk_callback_lock); - if (skb->sk->sk_socket && skb->sk->sk_socket->file) + sk = skb->sk; + sk_socket = sk->sk_socket; + if ((logflags & IPT_LOG_UID) && !iphoff && sk) { + read_lock_bh(&sk->sk_callback_lock); + if (sk_socket && sk_socket->file) sb_add(m, "UID=%u GID=%u ", - skb->sk->sk_socket->file->f_cred->fsuid, - skb->sk->sk_socket->file->f_cred->fsgid); - read_unlock_bh(&skb->sk->sk_callback_lock); + sk_socket->file->f_cred->fsuid, + sk_socket->file->f_cred->fsgid); + read_unlock_bh(&sk->sk_callback_lock); } /* Max length: 16 "MARK=0xFFFFFFFF " */ -- Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/ Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html Plain text mails only, please. ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] Reduce number of pointer dereferences in IPv4 netfilter LOG module function dump_packet() 2010-11-14 21:35 [PATCH] Reduce number of pointer dereferences in IPv4 netfilter LOG module function dump_packet() Jesper Juhl @ 2010-11-15 0:49 ` Eric Dumazet 2010-11-15 21:48 ` Jesper Juhl 0 siblings, 1 reply; 3+ messages in thread From: Eric Dumazet @ 2010-11-15 0:49 UTC (permalink / raw) To: Jesper Juhl Cc: Netfilter Core Team, netdev, linux-kernel, netfilter, netfilter-devel, David S. Miller, Rusty Russell Le dimanche 14 novembre 2010 à 22:35 +0100, Jesper Juhl a écrit : > By adding two pointer variables to > net/ipv4/netfilter/ipt_LOG.c::dump_packet() we can save 16 bytes of .text > and 9 pointer dereferences. > > before this patch we did 20 pointer dereferences and had this object file > size: > text data bss dec hex filename > 6233 600 3080 9913 26b9 net/ipv4/netfilter/ipt_LOG.o > > after this patch we do just 11 pointer dereferences and have this object > file size: > text data bss dec hex filename > 6217 600 3080 9897 26a9 net/ipv4/netfilter/ipt_LOG.o > > > Please Cc me on replies. > > > Signed-off-by: Jesper Juhl <jj@chaosbits.net> > --- > ipt_LOG.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/net/ipv4/netfilter/ipt_LOG.c b/net/ipv4/netfilter/ipt_LOG.c > index 72ffc8f..02a92de 100644 > --- a/net/ipv4/netfilter/ipt_LOG.c > +++ b/net/ipv4/netfilter/ipt_LOG.c > @@ -39,6 +39,8 @@ static void dump_packet(struct sbuff *m, > struct iphdr _iph; > const struct iphdr *ih; > unsigned int logflags; > + struct sock *sk; > + struct socket *sk_socket; > > if (info->type == NF_LOG_TYPE_LOG) > logflags = info->u.log.logflags; > @@ -335,13 +337,15 @@ static void dump_packet(struct sbuff *m, > } > > /* Max length: 15 "UID=4294967295 " */ > - if ((logflags & IPT_LOG_UID) && !iphoff && skb->sk) { > - read_lock_bh(&skb->sk->sk_callback_lock); > - if (skb->sk->sk_socket && skb->sk->sk_socket->file) > + sk = skb->sk; > + sk_socket = sk->sk_socket; Really ? sk can be NULL, so you add a NULL dereference. > + if ((logflags & IPT_LOG_UID) && !iphoff && sk) { > + read_lock_bh(&sk->sk_callback_lock); > + if (sk_socket && sk_socket->file) > sb_add(m, "UID=%u GID=%u ", > - skb->sk->sk_socket->file->f_cred->fsuid, > - skb->sk->sk_socket->file->f_cred->fsgid); > - read_unlock_bh(&skb->sk->sk_callback_lock); > + sk_socket->file->f_cred->fsuid, > + sk_socket->file->f_cred->fsgid); > + read_unlock_bh(&sk->sk_callback_lock); > } > > /* Max length: 16 "MARK=0xFFFFFFFF " */ > > > Most of these "dereferences" are compiler optimized. You added a bug in your patch, and make ipt_LOG slower if rule is not asking for IPT_LOG_UID ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Reduce number of pointer dereferences in IPv4 netfilter LOG module function dump_packet() 2010-11-15 0:49 ` Eric Dumazet @ 2010-11-15 21:48 ` Jesper Juhl 0 siblings, 0 replies; 3+ messages in thread From: Jesper Juhl @ 2010-11-15 21:48 UTC (permalink / raw) To: Eric Dumazet Cc: Netfilter Core Team, netdev, linux-kernel, netfilter, netfilter-devel, David S. Miller, Rusty Russell [-- Attachment #1: Type: TEXT/PLAIN, Size: 4069 bytes --] On Mon, 15 Nov 2010, Eric Dumazet wrote: > Le dimanche 14 novembre 2010 à 22:35 +0100, Jesper Juhl a écrit : > > By adding two pointer variables to > > net/ipv4/netfilter/ipt_LOG.c::dump_packet() we can save 16 bytes of .text > > and 9 pointer dereferences. > > > > before this patch we did 20 pointer dereferences and had this object file > > size: > > text data bss dec hex filename > > 6233 600 3080 9913 26b9 net/ipv4/netfilter/ipt_LOG.o > > > > after this patch we do just 11 pointer dereferences and have this object > > file size: > > text data bss dec hex filename > > 6217 600 3080 9897 26a9 net/ipv4/netfilter/ipt_LOG.o > > > > > > Please Cc me on replies. > > > > > > Signed-off-by: Jesper Juhl <jj@chaosbits.net> > > --- > > ipt_LOG.c | 16 ++++++++++------ > > 1 file changed, 10 insertions(+), 6 deletions(-) > > > > diff --git a/net/ipv4/netfilter/ipt_LOG.c b/net/ipv4/netfilter/ipt_LOG.c > > index 72ffc8f..02a92de 100644 > > --- a/net/ipv4/netfilter/ipt_LOG.c > > +++ b/net/ipv4/netfilter/ipt_LOG.c [...] > > - if ((logflags & IPT_LOG_UID) && !iphoff && skb->sk) { > > - read_lock_bh(&skb->sk->sk_callback_lock); > > - if (skb->sk->sk_socket && skb->sk->sk_socket->file) > > + sk = skb->sk; > > + sk_socket = sk->sk_socket; > > Really ? sk can be NULL, so you add a NULL dereference. > Arrgh, yes, you are right. How could I miss that "&& skb->sk" test? I even modified that bit without thinking about it (I guess that's the problem, not thinking about it and it being late and me having had too much coffee and then not testing enough)... Sorry about that. [...] > > - skb->sk->sk_socket->file->f_cred->fsuid, > > - skb->sk->sk_socket->file->f_cred->fsgid); > > - read_unlock_bh(&skb->sk->sk_callback_lock); > > + sk_socket->file->f_cred->fsuid, > > + sk_socket->file->f_cred->fsgid); > > + read_unlock_bh(&sk->sk_callback_lock); > > } > > > > /* Max length: 16 "MARK=0xFFFFFFFF " */ > > > > > > > > Most of these "dereferences" are compiler optimized. > > You added a bug in your patch, and make ipt_LOG slower if rule is not > asking for IPT_LOG_UID > Yes, I see that now. Thank you for pointing it out. How about the following instead? It still manages to save 16 bytes of .text and a number of pointer derefs and it doesn't deref potentially NULL pointers and it doesn't do any extra work if IPT_LOG_UID is not asked for. And this time I didn't just compile test it but booted and ran a kernel with the patch as well. Fewer pointer derefs and smaller .text size for net/ipv4/netfilter/ipt_LOG.c::dump_packet() Signed-off-by: Jesper Juhl <jj@chaosbits.net> --- ipt_LOG.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/net/ipv4/netfilter/ipt_LOG.c b/net/ipv4/netfilter/ipt_LOG.c index 72ffc8f..539dce3 100644 --- a/net/ipv4/netfilter/ipt_LOG.c +++ b/net/ipv4/netfilter/ipt_LOG.c @@ -39,6 +39,7 @@ static void dump_packet(struct sbuff *m, struct iphdr _iph; const struct iphdr *ih; unsigned int logflags; + struct sock *sk; if (info->type == NF_LOG_TYPE_LOG) logflags = info->u.log.logflags; @@ -336,12 +337,13 @@ static void dump_packet(struct sbuff *m, /* Max length: 15 "UID=4294967295 " */ if ((logflags & IPT_LOG_UID) && !iphoff && skb->sk) { - read_lock_bh(&skb->sk->sk_callback_lock); - if (skb->sk->sk_socket && skb->sk->sk_socket->file) + sk = skb->sk; + read_lock_bh(&sk->sk_callback_lock); + if (sk->sk_socket && sk->sk_socket->file) sb_add(m, "UID=%u GID=%u ", - skb->sk->sk_socket->file->f_cred->fsuid, - skb->sk->sk_socket->file->f_cred->fsgid); - read_unlock_bh(&skb->sk->sk_callback_lock); + sk->sk_socket->file->f_cred->fsuid, + sk->sk_socket->file->f_cred->fsgid); + read_unlock_bh(&sk->sk_callback_lock); } /* Max length: 16 "MARK=0xFFFFFFFF " */ -- Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/ Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html Plain text mails only, please. ^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2010-11-15 21:48 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-11-14 21:35 [PATCH] Reduce number of pointer dereferences in IPv4 netfilter LOG module function dump_packet() Jesper Juhl 2010-11-15 0:49 ` Eric Dumazet 2010-11-15 21:48 ` Jesper Juhl
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox