From patchwork Mon Nov 15 21:48:10 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Jesper Juhl X-Patchwork-Id: 71308 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id BA88BB712B for ; Tue, 16 Nov 2010 09:01:00 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933154Ab0KOWAb (ORCPT ); Mon, 15 Nov 2010 17:00:31 -0500 Received: from swampdragon.chaosbits.net ([90.184.90.115]:22473 "EHLO swampdragon.chaosbits.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758512Ab0KOWAa (ORCPT ); Mon, 15 Nov 2010 17:00:30 -0500 Received: by swampdragon.chaosbits.net (Postfix, from userid 1000) id 767A69403D; Mon, 15 Nov 2010 22:48:10 +0100 (CET) Received: from localhost (localhost [127.0.0.1]) by swampdragon.chaosbits.net (Postfix) with ESMTP id 6CCDA9403B; Mon, 15 Nov 2010 22:48:10 +0100 (CET) Date: Mon, 15 Nov 2010 22:48:10 +0100 (CET) From: Jesper Juhl To: Eric Dumazet cc: Netfilter Core Team , netdev@vger.kernel.org, linux-kernel@vger.kernel.org, netfilter@vger.kernel.org, netfilter-devel@vger.kernel.org, "David S. Miller" , Rusty Russell Subject: Re: [PATCH] Reduce number of pointer dereferences in IPv4 netfilter LOG module function dump_packet() In-Reply-To: <1289782180.2743.166.camel@edumazet-laptop> Message-ID: References: <1289782180.2743.166.camel@edumazet-laptop> User-Agent: Alpine 2.00 (LNX 1167 2008-08-23) MIME-Version: 1.0 Content-ID: Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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 > > --- > > 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 --- 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 " */