netdev
[Top] [All Lists]

Re: [PATCH/RFC] Reduce call chain length in netfilter

To: Patrick McHardy <kaber@xxxxxxxxx>
Subject: Re: [PATCH/RFC] Reduce call chain length in netfilter
From: "David S. Miller" <davem@xxxxxxxxxxxxx>
Date: Wed, 26 Jan 2005 23:18:01 -0800
Cc: bdschuym@xxxxxxxxxx, netdev@xxxxxxxxxxx, netfilter-devel@xxxxxxxxxxxxxxxxxxx, snort2004@xxxxxxx, rusty@xxxxxxxxxxxxxxx, ak@xxxxxxx, bridge@xxxxxxxx, gandalf@xxxxxxxxxxxxxx, dwmw2@xxxxxxxxxxxxx, shemminger@xxxxxxxx
In-reply-to: <41F82C6D.7020006@xxxxxxxxx>
References: <1131604877.20041218092730@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> <p73zn0ccaee.fsf@xxxxxxxxxxxxx> <1105117559.11753.34.camel@xxxxxxxxxxxxxxxxxxxxxxx> <20050107100017.454ddadc@xxxxxxxxxxxxxxxxx> <1105133241.3375.16.camel@xxxxxxxxxxxxxxxxxxxxx> <20050118135735.4b77d38d.davem@xxxxxxxxxxxxx> <1106433059.4486.11.camel@xxxxxxxxxxxxxxxxxxxxx> <1106436153.20995.42.camel@xxxxxxxxxxxxxx> <1106484019.3376.5.camel@xxxxxxxxxxxxxxxxxxxxx> <1106496509.1085.1.camel@xxxxxxxxxxxxxx> <20050125220558.6e824f8a.davem@xxxxxxxxxxxxx> <1106730510.4041.4.camel@xxxxxxxxxxxxxxxxxxxxx> <41F82C6D.7020006@xxxxxxxxx>
Sender: netdev-bounce@xxxxxxxxxxx
On Thu, 27 Jan 2005 00:49:01 +0100
Patrick McHardy <kaber@xxxxxxxxx> wrote:

> Bart De Schuymer wrote:
> 
> >Does anyone have objections to this patch, which reduces the netfilter
> >call chain length?
> >
> Looks fine to me.
> 
> Signed-off-by: Patrick McHardy <kaber@xxxxxxxxx>

Ok, I applied this.

While reviewing I thought it may be an issue that the new macros
potentially change skb.  It really isn't an issue because NF_HOOK()
calls pass ownership of the SKB over from the caller.

Although technically, someone could go:

        skb_get(skb);
        err = NF_HOOK(... skb ...);
        ... do stuff with skb ...
        kfree_skb(skb);

but that would cause other problems and I audited the entire tree
and nobody attempts anything like this currently.  'skb' always
dies at the NF_HOOK() call site.

I guess if we wanted to preserve NF_HOOK*() semantics even in such
a case we could use a local "__skb" var in the macro's basic block.

Another huge downside to this change I was worried about
was from a code generation point of view.  Since we now take the
address of "skb", gcc cannot generate tail-calls for the common
case of:

        return NF_HOOK(...);

when netfilter is enabled.  Ho hum...

Wait...

This is actually an important point!  Since gcc is generating a tail-
call for NF_HOOK() today, there is no stack savings for NF_HOOK()
created by this patch.  The only real gain is the NF_STOP stuff
for bridge netfilter.

I'm backing this out of my tree, let's think about this some more.
Perhaps it's only worth adding the NF_STOP thing and just making
nf_hook_slow() do the okfn(skb); call in that case?

<Prev in Thread] Current Thread [Next in Thread>