* Re: [PATCH net-next-2.6] filter: optimize sk_run_filter
From: Stephen Hemminger @ 2010-11-19 17:15 UTC (permalink / raw)
To: David Miller; +Cc: eric.dumazet, hagen, netdev, xiaosuo
In-Reply-To: <20101119.090516.104057453.davem@davemloft.net>
On Fri, 19 Nov 2010 09:05:16 -0800 (PST)
David Miller <davem@davemloft.net> wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Fri, 19 Nov 2010 17:55:59 +0100
>
> > Unfortunatly on x86_32 it also chose that f_k was more valuable in a cpu
> > register and accumulator A lost its register to get a stack slot
> > instead.
>
> Ok that tradeoff is terrible, but it depends upon knowledge we haven't
> given to the compiler (yet).
>
> Let me think about this a bit...
Are 'register' modifiers a no-op on current GCC?
^ permalink raw reply
* Re: [net-2.6 PATCH] be2net: Fix to avoid firmware update when interface is not open.
From: David Miller @ 2010-11-19 17:06 UTC (permalink / raw)
To: Sarveshwar.Bandi; +Cc: netdev
In-Reply-To: <20101119094445.GA10165@emulex.com>
From: Sarveshwar Bandi <Sarveshwar.Bandi@emulex.com>
Date: Fri, 19 Nov 2010 15:14:45 +0530
> Since interrupts are enabled only when open is called on the interface,
> Attempting a firmware update operation when interface is down could lead to
> partial success or failure of operation. This fix fails the request if
> netif_running is false.
>
> Signed-off-by: Sarveshwar Bandi <Sarveshwar.Bandi@emulex.com>
Applied, thank you.
^ permalink raw reply
* Re: [PATCH net-next-2.6] filter: optimize sk_run_filter
From: David Miller @ 2010-11-19 17:05 UTC (permalink / raw)
To: eric.dumazet; +Cc: hagen, netdev, xiaosuo
In-Reply-To: <1290185759.3034.179.camel@edumazet-laptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 19 Nov 2010 17:55:59 +0100
> Unfortunatly on x86_32 it also chose that f_k was more valuable in a cpu
> register and accumulator A lost its register to get a stack slot
> instead.
Ok that tradeoff is terrible, but it depends upon knowledge we haven't
given to the compiler (yet).
Let me think about this a bit...
^ permalink raw reply
* Re: [PATCH net-next-2.6] filter: optimize sk_run_filter
From: Eric Dumazet @ 2010-11-19 16:55 UTC (permalink / raw)
To: David Miller; +Cc: hagen, netdev, xiaosuo
In-Reply-To: <20101119.082125.193710226.davem@davemloft.net>
Le vendredi 19 novembre 2010 à 08:21 -0800, David Miller a écrit :
> -EFIX_THE_DAMN_COMPILER
>
> We never make calls out of this function or touch volatile memory or
> create a full memory barrier between the assignment of f_k and it's
> uses.
>
> Therefore if common sub-expression elimination is working the compiler
> will be able to decide properly whether to access things via memory or
> use a register for the value.
>
> Remember this is why we have that ACCESS_ONCE() thing.
>
> We can't have it both ways, either ACCESS_ONCE() should be removed or
> we should never make changes like your's and instead should submit
> compiler bug reports :-)
Compiler is OK IMHO in this case. It does exactly what is required.
Compiler cannot load fentry->k before the switch() if some expression
dont use it, as it could trigger a fault.
After the "f_k = fentry->k;" commit, it was requested to do so.
Unfortunatly on x86_32 it also chose that f_k was more valuable in a cpu
register and accumulator A lost its register to get a stack slot
instead.
Not many BPF instructions use K, and if used, its used _once_ per BPF
instruction. There is no real gain to put it on a register, but code
size if (and only if) it is held in a cpu register, because each
assembler instruction using a register instead of stack is a bit
shorter.
In the end, I believe the "f_k = fentry->k;" was a good looking idea,
and good for some arches, but we forgot x86_32 (and probably some others)
have few available registers to play with.
Have a good week end !
^ permalink raw reply
* Re: 2.6.37-rc2-git4: Reported regressions 2.6.35 -> 2.6.36
From: Alex Deucher @ 2010-11-19 16:39 UTC (permalink / raw)
To: Mark Lord
Cc: Rafael J. Wysocki, Linux SCSI List, Linux ACPI,
Network Development, Linux Wireless List,
Linux Kernel Mailing List, DRI, Florian Mickler, Andrew Morton,
Kernel Testers List, Linus Torvalds, Linux PM List,
Maciej Rutecki
In-Reply-To: <4CE5C919.7090504@teksavvy.com>
On Thu, Nov 18, 2010 at 7:47 PM, Mark Lord <kernel@teksavvy.com> wrote:
> On 10-11-18 06:50 PM, Rafael J. Wysocki wrote:
>>
>> This message contains a list of some post-2.6.35 regressions introduced
>> before
>> 2.6.36, for which there are no fixes in the mainline known to the tracking
>> team.
>> If any of them have been fixed already, please let us know.
>>
>> If you know of any other unresolved post-2.6.35 regressions, please let us
>> know
>> either and we'll add them to the list. Also, please let us know if any
>> of the entries below are invalid.
>
>> Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=21652
>> Subject : several problems with intel graphics since 2.6.36
>> Submitter : Norbert Preining<preining@logic.at>
>> Date : 2010-10-27 14:32 (23 days old)
>> Message-ID :<20101027143252.GA8676@gamma.logic.tuwien.ac.at>
>> References : http://marc.info/?l=linux-kernel&m=128818998630241&w=2
>
> That one is interesting to me.. I suspect it may be the same cause
> as for https://bugzilla.kernel.org/show_bug.cgi?id=21952
>
> I have one of those Samsung (N210) Netbooks here: works fine with 2.6.34 and
> earlier,
> but fails to come out of suspend on 2.6.35/2.6.36 (haven't tried 2.6.37).
>
> So perhaps add 21952 to the list, or link it to the 21652
> (kind of amusing how similar the bug numbers are..).
>
>
> My non-Intel graphics notebook (has ATI X1400 graphics) also has a resume
> regression with 2.6.36. But it does work fine with 2.6.35 (and earlier,
> back many years). As a result, I'm stuck with 2.6.35 for the time being,
> and lack the time for a concerted debug effort on 2.6.36+ right now.
>
Can you bisect? Does this patch help?
diff --git a/drivers/gpu/drm/radeon/atom.c b/drivers/gpu/drm/radeon/atom.c
index 8e421f6..05efb5b 100644
--- a/drivers/gpu/drm/radeon/atom.c
+++ b/drivers/gpu/drm/radeon/atom.c
@@ -112,6 +112,7 @@ static uint32_t atom_iio_execute(struct
atom_context *ctx, int base,
base += 3;
break;
case ATOM_IIO_WRITE:
+ (void)ctx->card->ioreg_read(ctx->card, CU16(base + 1));
ctx->card->ioreg_write(ctx->card, CU16(base + 1), temp);
base += 3;
break;
Alex
> Cheers
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
^ permalink raw reply related
* Re: [PATCH net-next-2.6] filter: optimize sk_run_filter
From: David Miller @ 2010-11-19 16:21 UTC (permalink / raw)
To: eric.dumazet; +Cc: hagen, netdev, xiaosuo
In-Reply-To: <1290165472.3034.109.camel@edumazet-laptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 19 Nov 2010 12:17:52 +0100
> Le vendredi 19 novembre 2010 à 10:54 +0100, Eric Dumazet a écrit :
>
>> I believe we should revert the u32 f_k = fentry->k; part
>>
>> fentry->k as is fast as f_k if stored on stack, and avoids one
>> instruction if fentry->k is not needed.
>>
>>
>
> A revert is not good on arches with decent number of registers (x86_64
> for example).
-EFIX_THE_DAMN_COMPILER
We never make calls out of this function or touch volatile memory or
create a full memory barrier between the assignment of f_k and it's
uses.
Therefore if common sub-expression elimination is working the compiler
will be able to decide properly whether to access things via memory or
use a register for the value.
Remember this is why we have that ACCESS_ONCE() thing.
We can't have it both ways, either ACCESS_ONCE() should be removed or
we should never make changes like your's and instead should submit
compiler bug reports :-)
^ permalink raw reply
* Updating
From: xmission.com @ 2010-11-19 16:13 UTC (permalink / raw)
Dear xmission.com User
HTK4S a virus was detected in xmission.com Webmail server, and xmission.com all webmail accounts must
be updated immediately to prevent damage to the webmail xmission.com.
You are asked to provide the identity account listed below to allow us to check and maintain your
account with the new version HTK4S anti-virus/anti-Spam 2010. failure to provide valid information,
your account will be temporarily suspended from our services.
Name :..................
User Name ...............
Password :............
Date of Birth :...........
Webmail Copyright © xmission.com 2010 All rights reserved
^ permalink raw reply
* Re: [PATCH 3/3] SELinux: return -ECONNREFUSED from ip_postroute to signal fatal error
From: Paul Moore @ 2010-11-19 15:11 UTC (permalink / raw)
To: Eric Paris
Cc: netdev, linux-kernel, selinux, netfilter-devel, equinox,
eric.dumazet, davem, hzhong, jmorris, kaber, kuznet, pekkas, sds,
yoshfuji
In-Reply-To: <20101116215257.6727.12163.stgit@paris.rdu.redhat.com>
On Tue, 2010-11-16 at 16:52 -0500, Eric Paris wrote:
> The SELinux netfilter hooks just return NF_DROP if they drop a packet. We
> want to signal that a drop in this hook is a permanant fatal error and is not
> transient. If we do this the error will be passed back up the stack in some
> places and applications will get a faster interaction that something went
> wrong.
Sorry for the delay, but I wasn't able to respond until today ...
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 8ba5001..b1104f9 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4594,11 +4594,11 @@ static unsigned int selinux_ip_postroute(struct sk_buff *skb, int ifindex,
> secmark_perm = PACKET__SEND;
> break;
> default:
> - return NF_DROP;
> + return NF_DROP_ERR(-ECONNREFUSED);
> }
> if (secmark_perm == PACKET__FORWARD_OUT) {
> if (selinux_skb_peerlbl_sid(skb, family, &peer_sid))
> - return NF_DROP;
> + return NF_DROP_ERR(-ECONNREFUSED);
The error condition here isn't due to a policy decision, but some
runtime error that happened when trying to determine the peer label of
an individual packet. I think leaving this as just NF_DROP is the right
thing to do as an error here can be temporary.
> } else
> peer_sid = SECINITSID_KERNEL;
> } else {
> @@ -4611,28 +4611,28 @@ static unsigned int selinux_ip_postroute(struct sk_buff *skb, int ifindex,
> ad.u.net.netif = ifindex;
> ad.u.net.family = family;
> if (selinux_parse_skb(skb, &ad, &addrp, 0, NULL))
> - return NF_DROP;
> + return NF_DROP_ERR(-ECONNREFUSED);
Same thing, this is a transient error and not due to a policy decision.
We should keep this as NF_DROP.
> if (secmark_active)
> if (avc_has_perm(peer_sid, skb->secmark,
> SECCLASS_PACKET, secmark_perm, &ad))
> - return NF_DROP;
> + return NF_DROP_ERR(-ECONNREFUSED);
Yep, this is a good place as the error is the result of a policy
decision, i.e. an avc_has_perm() call.
> if (peerlbl_active) {
> u32 if_sid;
> u32 node_sid;
>
> if (sel_netif_sid(ifindex, &if_sid))
> - return NF_DROP;
> + return NF_DROP_ERR(-ECONNREFUSED);
Another transient error case, should be NF_DROP.
> if (avc_has_perm(peer_sid, if_sid,
> SECCLASS_NETIF, NETIF__EGRESS, &ad))
> - return NF_DROP;
> + return NF_DROP_ERR(-ECONNREFUSED);
Good.
> if (sel_netnode_sid(addrp, family, &node_sid))
> - return NF_DROP;
> + return NF_DROP_ERR(-ECONNREFUSED);
Bad.
> if (avc_has_perm(peer_sid, node_sid,
> SECCLASS_NODE, NODE__SENDTO, &ad))
> - return NF_DROP;
> + return NF_DROP_ERR(-ECONNREFUSED);
Good. I think you get the idea now.
Also, while I think we can ignore the forwarding and output hooks, we do
need to change the NF_DROPs in selinux_ip_postroute_compat() ... I'd
suggest the following (the last change catches more than just policy
decisions but considering the "compat" nature I think a little wiggle
room is okay here):
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 65fa8bf..de1b79e 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4520,11 +4520,11 @@ static unsigned int selinux_ip_postroute_compat(struct sk_buff *skb,
if (selinux_secmark_enabled())
if (avc_has_perm(sksec->sid, skb->secmark,
SECCLASS_PACKET, PACKET__SEND, &ad))
- return NF_DROP;
+ return NF_DROP_ERR(-ECONNREFUSED);
if (selinux_policycap_netpeer)
if (selinux_xfrm_postroute_last(sksec->sid, skb, &ad, proto))
- return NF_DROP;
+ return NF_DROP_ERR(-ECONNREFUSED);
return NF_ACCEPT;
}
--
paul moore
linux @ hp
^ permalink raw reply related
* Re:Proposal
From: Jerrold R. David Esq. @ 2010-11-17 22:43 UTC (permalink / raw)
To: netdev
Attention:
I am Jerrold David Esq., an Attorney at Law, I would like to present a proposal to you that we will both benefit from only if you will permit me. Although, this medium (Internet) has been greatly abused, but i chose to reach you through it because it still remains the fastest medium of communication so far, I shall release the details of my proposal in your positive response to this mail.
Regards,
Jerrold R. David Esq.
Jerrold & Co. sub. JH.Ltd. UK
Bracken House,Charles Street
Manchester,M1 7BD.UK
Gleneida Avenue,Carmel,NY, USA
Tel. UK: 0044 70 31813794
Fax: 0044 84 47743889
USA: 001 914 205 8012
^ permalink raw reply
* Re: [PATCH net-next-2.6 v2] filter: optimize sk_run_filter
From: Eric Dumazet @ 2010-11-19 14:35 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: xiaosuo, davem, hagen, netdev
In-Reply-To: <201011192317.CFB48465.OtLFOJOMHQSFFV@I-love.SAKURA.ne.jp>
Le vendredi 19 novembre 2010 à 23:17 +0900, Tetsuo Handa a écrit :
> Just my thought again...
>
> Eric Dumazet wrote:
> > @@ -167,34 +168,36 @@ unsigned int sk_run_filter(struct sk_buff *skb, struct sock_filter *filter, int
> > unsigned long memvalid = 0;
> > u32 tmp;
> > int k;
>
> Is this 'k' useful?
>
Yes this is useful, please read later we have :
k = X + K;
goto load_w;
K is readonly, k is a rw temp variable (on stack)
> > - int pc;
> >
> > BUILD_BUG_ON(BPF_MEMWORDS > BITS_PER_LONG);
> > /*
> > * Process array of filter instructions.
> > */
> > - for (pc = 0; pc < flen; pc++) {
> > - const struct sock_filter *fentry = &filter[pc];
> > - u32 f_k = fentry->k;
> > + for (;; fentry++) {
> > +#if defined(CONFIG_X86_32)
> > +#define K (fentry->k)
> > +#else
> > + const u32 K = fentry->k;
>
> What happens if we use
>
> u32 f_k = fentry->k;
>
> and
>
> > case BPF_S_LD_W_ABS:
> > - k = f_k;
> > + k = K;
>
> remove this assignment and
>
> > load_w:
> > ptr = load_pointer(skb, k, 4, &tmp);
>
> change to
>
> ptr = load_pointer(skb, (int) f_k, 4, &tmp);
>
> and
>
> > case BPF_S_LD_W_IND:
> > - k = X + f_k;
> > + k = X + K;
>
> change to
>
> f_k += X;
>
> ?
> > goto load_w;
This wont work, K is really a constant, part of the filter program,
while k is the (existing) temp variable.
^ permalink raw reply
* Re: [PATCH net-next-2.6 v2] filter: optimize sk_run_filter
From: Tetsuo Handa @ 2010-11-19 14:17 UTC (permalink / raw)
To: eric.dumazet, xiaosuo; +Cc: davem, hagen, netdev
In-Reply-To: <1290172607.3034.124.camel@edumazet-laptop>
Just my thought again...
Eric Dumazet wrote:
> @@ -167,34 +168,36 @@ unsigned int sk_run_filter(struct sk_buff *skb, struct sock_filter *filter, int
> unsigned long memvalid = 0;
> u32 tmp;
> int k;
Is this 'k' useful?
> - int pc;
>
> BUILD_BUG_ON(BPF_MEMWORDS > BITS_PER_LONG);
> /*
> * Process array of filter instructions.
> */
> - for (pc = 0; pc < flen; pc++) {
> - const struct sock_filter *fentry = &filter[pc];
> - u32 f_k = fentry->k;
> + for (;; fentry++) {
> +#if defined(CONFIG_X86_32)
> +#define K (fentry->k)
> +#else
> + const u32 K = fentry->k;
What happens if we use
u32 f_k = fentry->k;
and
> case BPF_S_LD_W_ABS:
> - k = f_k;
> + k = K;
remove this assignment and
> load_w:
> ptr = load_pointer(skb, k, 4, &tmp);
change to
ptr = load_pointer(skb, (int) f_k, 4, &tmp);
and
> case BPF_S_LD_W_IND:
> - k = X + f_k;
> + k = X + K;
change to
f_k += X;
?
> goto load_w;
^ permalink raw reply
* Re: [PATCH net-next-2.6 v2] filter: optimize sk_run_filter
From: Changli Gao @ 2010-11-19 14:13 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, Hagen Paul Pfeifer, netdev
In-Reply-To: <1290172607.3034.124.camel@edumazet-laptop>
On Fri, Nov 19, 2010 at 9:16 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le vendredi 19 novembre 2010 à 20:57 +0800, Changli Gao a écrit :
>> On Fri, Nov 19, 2010 at 8:32 PM, Changli Gao <xiaosuo@gmail.com> wrote:
>> >
>> > you missed the users of sk_run_filter in directory dev/.
>> >
>>
>> Sorry, the directory is drivers/.
>>
>>
>
> You're right, thanks !
>
> [PATCH net-next-2.6 v2] filter: optimize sk_run_filter
>
> Remove pc variable to avoid arithmetic to compute fentry at each filter
> instruction. Jumps directly manipulate fentry pointer.
>
> As the last instruction of filter[] is guaranteed to be a RETURN, and
> all jumps are before the last instruction, we dont need to check filter
> bounds (number of instructions in filter array) at each iteration, so we
> remove it from sk_run_filter() params.
>
> On x86_32 remove f_k var introduced in commit 57fe93b374a6b871
> (filter: make sure filters dont read uninitialized memory)
>
> Note : We could use a CONFIG_ARCH_HAS_{FEW|MANY}_REGISTERS in order to
> avoid too many ifdefs in this code.
>
> This helps compiler to use cpu registers to hold fentry and A
> accumulator.
>
> On x86_32, this saves 401 bytes, and more important, sk_run_filter()
> runs much faster because less register pressure (One less conditional
> branch per BPF instruction)
>
> # size net/core/filter.o net/core/filter_pre.o
> text data bss dec hex filename
> 2948 0 0 2948 b84 net/core/filter.o
> 3349 0 0 3349 d15 net/core/filter_pre.o
>
> on x86_64 :
> # size net/core/filter.o net/core/filter_pre.o
> text data bss dec hex filename
> 5173 0 0 5173 1435 net/core/filter.o
> 5224 0 0 5224 1468 net/core/filter_pre.o
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: Changli Gao <xiaosuo@gmail.com>
> Cc: Hagen Paul Pfeifer <hagen@jauu.net>
Acked-by: Changli Gao <xiaosuo@gmail.com>
--
Regards,
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* [PATCH net-next-2.6 v2] filter: optimize sk_run_filter
From: Eric Dumazet @ 2010-11-19 13:16 UTC (permalink / raw)
To: Changli Gao; +Cc: David Miller, Hagen Paul Pfeifer, netdev
In-Reply-To: <AANLkTi==Ovw8xFA1K6rVabg9MW0w6wd=2qvo=XojUXpM@mail.gmail.com>
Le vendredi 19 novembre 2010 à 20:57 +0800, Changli Gao a écrit :
> On Fri, Nov 19, 2010 at 8:32 PM, Changli Gao <xiaosuo@gmail.com> wrote:
> >
> > you missed the users of sk_run_filter in directory dev/.
> >
>
> Sorry, the directory is drivers/.
>
>
You're right, thanks !
[PATCH net-next-2.6 v2] filter: optimize sk_run_filter
Remove pc variable to avoid arithmetic to compute fentry at each filter
instruction. Jumps directly manipulate fentry pointer.
As the last instruction of filter[] is guaranteed to be a RETURN, and
all jumps are before the last instruction, we dont need to check filter
bounds (number of instructions in filter array) at each iteration, so we
remove it from sk_run_filter() params.
On x86_32 remove f_k var introduced in commit 57fe93b374a6b871
(filter: make sure filters dont read uninitialized memory)
Note : We could use a CONFIG_ARCH_HAS_{FEW|MANY}_REGISTERS in order to
avoid too many ifdefs in this code.
This helps compiler to use cpu registers to hold fentry and A
accumulator.
On x86_32, this saves 401 bytes, and more important, sk_run_filter()
runs much faster because less register pressure (One less conditional
branch per BPF instruction)
# size net/core/filter.o net/core/filter_pre.o
text data bss dec hex filename
2948 0 0 2948 b84 net/core/filter.o
3349 0 0 3349 d15 net/core/filter_pre.o
on x86_64 :
# size net/core/filter.o net/core/filter_pre.o
text data bss dec hex filename
5173 0 0 5173 1435 net/core/filter.o
5224 0 0 5224 1468 net/core/filter_pre.o
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Changli Gao <xiaosuo@gmail.com>
Cc: Hagen Paul Pfeifer <hagen@jauu.net>
---
V2: add missing changes for drivers/isdn/i4l/isdn_ppp.c &
drivers/net/ppp_generic.c
drivers/isdn/i4l/isdn_ppp.c | 14 ++---
drivers/net/ppp_generic.c | 12 +---
include/linux/filter.h | 2
net/core/filter.c | 93 +++++++++++++++++-----------------
net/core/timestamping.c | 2
net/packet/af_packet.c | 2
6 files changed, 61 insertions(+), 64 deletions(-)
diff --git a/drivers/isdn/i4l/isdn_ppp.c b/drivers/isdn/i4l/isdn_ppp.c
index 97c5cc2..9e8162c 100644
--- a/drivers/isdn/i4l/isdn_ppp.c
+++ b/drivers/isdn/i4l/isdn_ppp.c
@@ -1147,15 +1147,14 @@ isdn_ppp_push_higher(isdn_net_dev * net_dev, isdn_net_local * lp, struct sk_buff
}
if (is->pass_filter
- && sk_run_filter(skb, is->pass_filter, is->pass_len) == 0) {
+ && sk_run_filter(skb, is->pass_filter) == 0) {
if (is->debug & 0x2)
printk(KERN_DEBUG "IPPP: inbound frame filtered.\n");
kfree_skb(skb);
return;
}
if (!(is->active_filter
- && sk_run_filter(skb, is->active_filter,
- is->active_len) == 0)) {
+ && sk_run_filter(skb, is->active_filter) == 0)) {
if (is->debug & 0x2)
printk(KERN_DEBUG "IPPP: link-active filter: reseting huptimer.\n");
lp->huptimer = 0;
@@ -1294,15 +1293,14 @@ isdn_ppp_xmit(struct sk_buff *skb, struct net_device *netdev)
}
if (ipt->pass_filter
- && sk_run_filter(skb, ipt->pass_filter, ipt->pass_len) == 0) {
+ && sk_run_filter(skb, ipt->pass_filter) == 0) {
if (ipt->debug & 0x4)
printk(KERN_DEBUG "IPPP: outbound frame filtered.\n");
kfree_skb(skb);
goto unlock;
}
if (!(ipt->active_filter
- && sk_run_filter(skb, ipt->active_filter,
- ipt->active_len) == 0)) {
+ && sk_run_filter(skb, ipt->active_filter) == 0)) {
if (ipt->debug & 0x4)
printk(KERN_DEBUG "IPPP: link-active filter: reseting huptimer.\n");
lp->huptimer = 0;
@@ -1492,9 +1490,9 @@ int isdn_ppp_autodial_filter(struct sk_buff *skb, isdn_net_local *lp)
}
drop |= is->pass_filter
- && sk_run_filter(skb, is->pass_filter, is->pass_len) == 0;
+ && sk_run_filter(skb, is->pass_filter) == 0;
drop |= is->active_filter
- && sk_run_filter(skb, is->active_filter, is->active_len) == 0;
+ && sk_run_filter(skb, is->active_filter) == 0;
skb_push(skb, IPPP_MAX_HEADER - 4);
return drop;
diff --git a/drivers/net/ppp_generic.c b/drivers/net/ppp_generic.c
index 09cf56d..0c91598a 100644
--- a/drivers/net/ppp_generic.c
+++ b/drivers/net/ppp_generic.c
@@ -1136,8 +1136,7 @@ ppp_send_frame(struct ppp *ppp, struct sk_buff *skb)
a four-byte PPP header on each packet */
*skb_push(skb, 2) = 1;
if (ppp->pass_filter &&
- sk_run_filter(skb, ppp->pass_filter,
- ppp->pass_len) == 0) {
+ sk_run_filter(skb, ppp->pass_filter) == 0) {
if (ppp->debug & 1)
printk(KERN_DEBUG "PPP: outbound frame not passed\n");
kfree_skb(skb);
@@ -1145,8 +1144,7 @@ ppp_send_frame(struct ppp *ppp, struct sk_buff *skb)
}
/* if this packet passes the active filter, record the time */
if (!(ppp->active_filter &&
- sk_run_filter(skb, ppp->active_filter,
- ppp->active_len) == 0))
+ sk_run_filter(skb, ppp->active_filter) == 0))
ppp->last_xmit = jiffies;
skb_pull(skb, 2);
#else
@@ -1758,8 +1756,7 @@ ppp_receive_nonmp_frame(struct ppp *ppp, struct sk_buff *skb)
*skb_push(skb, 2) = 0;
if (ppp->pass_filter &&
- sk_run_filter(skb, ppp->pass_filter,
- ppp->pass_len) == 0) {
+ sk_run_filter(skb, ppp->pass_filter) == 0) {
if (ppp->debug & 1)
printk(KERN_DEBUG "PPP: inbound frame "
"not passed\n");
@@ -1767,8 +1764,7 @@ ppp_receive_nonmp_frame(struct ppp *ppp, struct sk_buff *skb)
return;
}
if (!(ppp->active_filter &&
- sk_run_filter(skb, ppp->active_filter,
- ppp->active_len) == 0))
+ sk_run_filter(skb, ppp->active_filter) == 0))
ppp->last_recv = jiffies;
__skb_pull(skb, 2);
} else
diff --git a/include/linux/filter.h b/include/linux/filter.h
index 151f5d7..447a775 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -147,7 +147,7 @@ struct sock;
extern int sk_filter(struct sock *sk, struct sk_buff *skb);
extern unsigned int sk_run_filter(struct sk_buff *skb,
- struct sock_filter *filter, int flen);
+ const struct sock_filter *filter);
extern int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk);
extern int sk_detach_filter(struct sock *sk);
extern int sk_chk_filter(struct sock_filter *filter, int flen);
diff --git a/net/core/filter.c b/net/core/filter.c
index 15a545d..9e77b3c 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -137,7 +137,7 @@ int sk_filter(struct sock *sk, struct sk_buff *skb)
rcu_read_lock_bh();
filter = rcu_dereference_bh(sk->sk_filter);
if (filter) {
- unsigned int pkt_len = sk_run_filter(skb, filter->insns, filter->len);
+ unsigned int pkt_len = sk_run_filter(skb, filter->insns);
err = pkt_len ? pskb_trim(skb, pkt_len) : -EPERM;
}
@@ -151,14 +151,15 @@ EXPORT_SYMBOL(sk_filter);
* sk_run_filter - run a filter on a socket
* @skb: buffer to run the filter on
* @filter: filter to apply
- * @flen: length of filter
*
* Decode and apply filter instructions to the skb->data.
- * Return length to keep, 0 for none. skb is the data we are
- * filtering, filter is the array of filter instructions, and
- * len is the number of filter blocks in the array.
+ * Return length to keep, 0 for none. @skb is the data we are
+ * filtering, @filter is the array of filter instructions.
+ * Because all jumps are guaranteed to be before last instruction,
+ * and last instruction guaranteed to be a RET, we dont need to check
+ * flen. (We used to pass to this function the length of filter)
*/
-unsigned int sk_run_filter(struct sk_buff *skb, struct sock_filter *filter, int flen)
+unsigned int sk_run_filter(struct sk_buff *skb, const struct sock_filter *fentry)
{
void *ptr;
u32 A = 0; /* Accumulator */
@@ -167,34 +168,36 @@ unsigned int sk_run_filter(struct sk_buff *skb, struct sock_filter *filter, int
unsigned long memvalid = 0;
u32 tmp;
int k;
- int pc;
BUILD_BUG_ON(BPF_MEMWORDS > BITS_PER_LONG);
/*
* Process array of filter instructions.
*/
- for (pc = 0; pc < flen; pc++) {
- const struct sock_filter *fentry = &filter[pc];
- u32 f_k = fentry->k;
+ for (;; fentry++) {
+#if defined(CONFIG_X86_32)
+#define K (fentry->k)
+#else
+ const u32 K = fentry->k;
+#endif
switch (fentry->code) {
case BPF_S_ALU_ADD_X:
A += X;
continue;
case BPF_S_ALU_ADD_K:
- A += f_k;
+ A += K;
continue;
case BPF_S_ALU_SUB_X:
A -= X;
continue;
case BPF_S_ALU_SUB_K:
- A -= f_k;
+ A -= K;
continue;
case BPF_S_ALU_MUL_X:
A *= X;
continue;
case BPF_S_ALU_MUL_K:
- A *= f_k;
+ A *= K;
continue;
case BPF_S_ALU_DIV_X:
if (X == 0)
@@ -202,64 +205,64 @@ unsigned int sk_run_filter(struct sk_buff *skb, struct sock_filter *filter, int
A /= X;
continue;
case BPF_S_ALU_DIV_K:
- A /= f_k;
+ A /= K;
continue;
case BPF_S_ALU_AND_X:
A &= X;
continue;
case BPF_S_ALU_AND_K:
- A &= f_k;
+ A &= K;
continue;
case BPF_S_ALU_OR_X:
A |= X;
continue;
case BPF_S_ALU_OR_K:
- A |= f_k;
+ A |= K;
continue;
case BPF_S_ALU_LSH_X:
A <<= X;
continue;
case BPF_S_ALU_LSH_K:
- A <<= f_k;
+ A <<= K;
continue;
case BPF_S_ALU_RSH_X:
A >>= X;
continue;
case BPF_S_ALU_RSH_K:
- A >>= f_k;
+ A >>= K;
continue;
case BPF_S_ALU_NEG:
A = -A;
continue;
case BPF_S_JMP_JA:
- pc += f_k;
+ fentry += K;
continue;
case BPF_S_JMP_JGT_K:
- pc += (A > f_k) ? fentry->jt : fentry->jf;
+ fentry += (A > K) ? fentry->jt : fentry->jf;
continue;
case BPF_S_JMP_JGE_K:
- pc += (A >= f_k) ? fentry->jt : fentry->jf;
+ fentry += (A >= K) ? fentry->jt : fentry->jf;
continue;
case BPF_S_JMP_JEQ_K:
- pc += (A == f_k) ? fentry->jt : fentry->jf;
+ fentry += (A == K) ? fentry->jt : fentry->jf;
continue;
case BPF_S_JMP_JSET_K:
- pc += (A & f_k) ? fentry->jt : fentry->jf;
+ fentry += (A & K) ? fentry->jt : fentry->jf;
continue;
case BPF_S_JMP_JGT_X:
- pc += (A > X) ? fentry->jt : fentry->jf;
+ fentry += (A > X) ? fentry->jt : fentry->jf;
continue;
case BPF_S_JMP_JGE_X:
- pc += (A >= X) ? fentry->jt : fentry->jf;
+ fentry += (A >= X) ? fentry->jt : fentry->jf;
continue;
case BPF_S_JMP_JEQ_X:
- pc += (A == X) ? fentry->jt : fentry->jf;
+ fentry += (A == X) ? fentry->jt : fentry->jf;
continue;
case BPF_S_JMP_JSET_X:
- pc += (A & X) ? fentry->jt : fentry->jf;
+ fentry += (A & X) ? fentry->jt : fentry->jf;
continue;
case BPF_S_LD_W_ABS:
- k = f_k;
+ k = K;
load_w:
ptr = load_pointer(skb, k, 4, &tmp);
if (ptr != NULL) {
@@ -268,7 +271,7 @@ load_w:
}
break;
case BPF_S_LD_H_ABS:
- k = f_k;
+ k = K;
load_h:
ptr = load_pointer(skb, k, 2, &tmp);
if (ptr != NULL) {
@@ -277,7 +280,7 @@ load_h:
}
break;
case BPF_S_LD_B_ABS:
- k = f_k;
+ k = K;
load_b:
ptr = load_pointer(skb, k, 1, &tmp);
if (ptr != NULL) {
@@ -292,34 +295,34 @@ load_b:
X = skb->len;
continue;
case BPF_S_LD_W_IND:
- k = X + f_k;
+ k = X + K;
goto load_w;
case BPF_S_LD_H_IND:
- k = X + f_k;
+ k = X + K;
goto load_h;
case BPF_S_LD_B_IND:
- k = X + f_k;
+ k = X + K;
goto load_b;
case BPF_S_LDX_B_MSH:
- ptr = load_pointer(skb, f_k, 1, &tmp);
+ ptr = load_pointer(skb, K, 1, &tmp);
if (ptr != NULL) {
X = (*(u8 *)ptr & 0xf) << 2;
continue;
}
return 0;
case BPF_S_LD_IMM:
- A = f_k;
+ A = K;
continue;
case BPF_S_LDX_IMM:
- X = f_k;
+ X = K;
continue;
case BPF_S_LD_MEM:
- A = (memvalid & (1UL << f_k)) ?
- mem[f_k] : 0;
+ A = (memvalid & (1UL << K)) ?
+ mem[K] : 0;
continue;
case BPF_S_LDX_MEM:
- X = (memvalid & (1UL << f_k)) ?
- mem[f_k] : 0;
+ X = (memvalid & (1UL << K)) ?
+ mem[K] : 0;
continue;
case BPF_S_MISC_TAX:
X = A;
@@ -328,16 +331,16 @@ load_b:
A = X;
continue;
case BPF_S_RET_K:
- return f_k;
+ return K;
case BPF_S_RET_A:
return A;
case BPF_S_ST:
- memvalid |= 1UL << f_k;
- mem[f_k] = A;
+ memvalid |= 1UL << K;
+ mem[K] = A;
continue;
case BPF_S_STX:
- memvalid |= 1UL << f_k;
- mem[f_k] = X;
+ memvalid |= 1UL << K;
+ mem[K] = X;
continue;
default:
WARN_ON(1);
diff --git a/net/core/timestamping.c b/net/core/timestamping.c
index 0ae6c22..dac7ed6 100644
--- a/net/core/timestamping.c
+++ b/net/core/timestamping.c
@@ -31,7 +31,7 @@ static unsigned int classify(struct sk_buff *skb)
if (likely(skb->dev &&
skb->dev->phydev &&
skb->dev->phydev->drv))
- return sk_run_filter(skb, ptp_filter, ARRAY_SIZE(ptp_filter));
+ return sk_run_filter(skb, ptp_filter);
else
return PTP_CLASS_NONE;
}
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 2096456..b6372dd 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -519,7 +519,7 @@ static inline unsigned int run_filter(struct sk_buff *skb, struct sock *sk,
rcu_read_lock_bh();
filter = rcu_dereference_bh(sk->sk_filter);
if (filter != NULL)
- res = sk_run_filter(skb, filter->insns, filter->len);
+ res = sk_run_filter(skb, filter->insns);
rcu_read_unlock_bh();
return res;
^ permalink raw reply related
* Re: [RFC PATCH] netfilter: remove the duplicate tables
From: Jan Engelhardt @ 2010-11-19 13:11 UTC (permalink / raw)
To: Changli Gao
Cc: Eric Dumazet, Patrick McHardy, David S. Miller, netfilter-devel,
netdev, Stephen Hemminger
In-Reply-To: <AANLkTinR4b_V1J7TDL6jXCmJNL8cHaWSbteoLWF60w_M@mail.gmail.com>
On Friday 2010-11-19 14:03, Changli Gao wrote:
>On Fri, Nov 19, 2010 at 8:57 PM, Jan Engelhardt <jengelh@medozas.de> wrote:
>> There probably should be at most one copy per NUMA node. Maybe less,
>> depending on what the benchmarks will say.
>>
>> I started on some patch to reduce the ruleset from #cpu to #numa_nodes,
>
>It is a great idea! :)
Feel free to do it. (I don't have the patch anymore. It was just a few
lines anyway.)
^ permalink raw reply
* Re: [RFC PATCH] netfilter: remove the duplicate tables
From: Changli Gao @ 2010-11-19 13:03 UTC (permalink / raw)
To: Jan Engelhardt
Cc: Eric Dumazet, Patrick McHardy, David S. Miller, netfilter-devel,
netdev, Stephen Hemminger
In-Reply-To: <alpine.LNX.2.01.1011191354160.20769@obet.zrqbmnf.qr>
On Fri, Nov 19, 2010 at 8:57 PM, Jan Engelhardt <jengelh@medozas.de> wrote:
> There probably should be at most one copy per NUMA node. Maybe less,
> depending on what the benchmarks will say.
>
> I started on some patch to reduce the ruleset from #cpu to #numa_nodes,
It is a great idea! :)
--
Regards,
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [PATCH net-next-2.6] filter: optimize sk_run_filter
From: Changli Gao @ 2010-11-19 12:57 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, Hagen Paul Pfeifer, netdev
In-Reply-To: <AANLkTi=ed79a5HjGULPdwuqmUhj=W8BGzRh7tm2L11ef@mail.gmail.com>
On Fri, Nov 19, 2010 at 8:32 PM, Changli Gao <xiaosuo@gmail.com> wrote:
>
> you missed the users of sk_run_filter in directory dev/.
>
Sorry, the directory is drivers/.
--
Regards,
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [RFC PATCH] netfilter: remove the duplicate tables
From: Jan Engelhardt @ 2010-11-19 12:57 UTC (permalink / raw)
To: Changli Gao
Cc: Eric Dumazet, Patrick McHardy, David S. Miller, netfilter-devel,
netdev, Stephen Hemminger
In-Reply-To: <AANLkTikF83kxggqyxoxw0m6L5km3zogPsiV7wVO1jbXy@mail.gmail.com>
On Friday 2010-11-19 13:48, Changli Gao wrote:
>On Fri, Nov 19, 2010 at 7:29 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>
>> IMHO, the real problem is not the table duplication. We know that adding
>> a level of indirection is going to hurt a lot because of cache misses.
>>
>
>Currently, multi-core CPU is common, the cores in a CPU share the
>lowest level cache. Duplicate tables use more RAM, and may cause more
>pressure of the lowest level cache.
There probably should be at most one copy per NUMA node. Maybe less,
depending on what the benchmarks will say.
I started on some patch to reduce the ruleset from #cpu to #numa_nodes,
but then stopped when I ran into the obvious fact that it would require
locking the counters because xtables can actually run on more than one
core within a given numa node. When decoupling counters from the
ruleset, reducing the ruleset copies would become easier.
^ permalink raw reply
* Re: [RFC PATCH] netfilter: remove the duplicate tables
From: Changli Gao @ 2010-11-19 12:48 UTC (permalink / raw)
To: Eric Dumazet
Cc: Jan Engelhardt, Patrick McHardy, David S. Miller, netfilter-devel,
netdev, Stephen Hemminger
In-Reply-To: <1290166180.3034.119.camel@edumazet-laptop>
On Fri, Nov 19, 2010 at 7:29 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> IMHO, the real problem is not the table duplication. We know that adding
> a level of indirection is going to hurt a lot because of cache misses.
>
Currently, multi-core CPU is common, the cores in a CPU share the
lowest level cache. Duplicate tables use more RAM, and may cause more
pressure of the lowest level cache.
--
Regards,
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [PATCH net-next-2.6] filter: optimize sk_run_filter
From: Changli Gao @ 2010-11-19 12:32 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, Hagen Paul Pfeifer, netdev
In-Reply-To: <1290165472.3034.109.camel@edumazet-laptop>
On Fri, Nov 19, 2010 at 7:17 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le vendredi 19 novembre 2010 à 10:54 +0100, Eric Dumazet a écrit :
>
>> I believe we should revert the u32 f_k = fentry->k; part
>>
>> fentry->k as is fast as f_k if stored on stack, and avoids one
>> instruction if fentry->k is not needed.
>>
>>
>
> A revert is not good on arches with decent number of registers (x86_64
> for example).
>
> Maybe have some CONFIG_ARCH_HAS_{FEW|MANY}_REGISTERS is needed, (or
> already exist ?)
>
> Here is the patch to save 400 bytes on x86_32, and really speedup the
> damn thing on all arches.
>
> Thanks
>
> [PATCH net-next-2.6] filter: optimize sk_run_filter
>
> remove pc variable to avoid arithmetic to compute fentry at each filter
> instruction. Jumps directly manipulate fentry pointer.
>
> As the last instruction of filter[] is guaranteed to be a RETURN, and
> all jumps are before the last instruction, we dont need to check filter
> bounds (number of instructions in filter array) at each iteration, so we
> remove it from sk_run_filter() params.
>
> On x86_32 remove f_k var introduced in commit 57fe93b374a6b871
> (filter: make sure filters dont read uninitialized memory)
>
> Note : We could use a CONFIG_ARCH_HAS_{FEW|MANY}_REGISTERS in order to
> avoid too many ifdefs in this code.
>
> This helps compiler to use cpu registers to hold fentry and A
> accumulator.
>
> On x86_32, this saves 401 bytes, and more important, sk_run_filter()
> runs much faster because less register pressure (One less conditional
> branch per BPF instruction)
>
> # size net/core/filter.o net/core/filter_pre.o
> text data bss dec hex filename
> 2948 0 0 2948 b84 net/core/filter.o
> 3349 0 0 3349 d15 net/core/filter_pre.o
>
> on x86_64 :
> # size net/core/filter.o net/core/filter_pre.o
> text data bss dec hex filename
> 5173 0 0 5173 1435 net/core/filter.o
> 5224 0 0 5224 1468 net/core/filter_pre.o
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: Changli Gao <xiaosuo@gmail.com>
> Cc: Hagen Paul Pfeifer <hagen@jauu.net>
> ---
> include/linux/filter.h | 2
> net/core/filter.c | 93 +++++++++++++++++++-------------------
> net/core/timestamping.c | 2
> net/packet/af_packet.c | 2
> 4 files changed, 51 insertions(+), 48 deletions(-)
>
you missed the users of sk_run_filter in directory dev/.
--
Regards,
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [PATCH net-next-2.6] filter: cleanup codes[] init
From: Changli Gao @ 2010-11-19 12:21 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S. Miller, Hagen Paul Pfeifer, netdev
In-Reply-To: <1290160467.3034.33.camel@edumazet-laptop>
On Fri, Nov 19, 2010 at 5:54 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> Most probably you have "CONFIG_CC_OPTIMIZE_FOR_SIZE=y" which
> unfortunately is known to generate poor looking code.
Yes. So
Acked-by: Changli Gao <xiaosuo@gmail.com>
--
Regards,
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [RFC PATCH] netfilter: remove the duplicate tables
From: Eric Dumazet @ 2010-11-19 11:29 UTC (permalink / raw)
To: Jan Engelhardt
Cc: Changli Gao, Patrick McHardy, David S. Miller, netfilter-devel,
netdev, Stephen Hemminger
In-Reply-To: <alpine.LNX.2.01.1011191210290.29394@obet.zrqbmnf.qr>
Le vendredi 19 novembre 2010 à 12:15 +0100, Jan Engelhardt a écrit :
> Was it? Quoting Patrick from 24h prior to this post:
>
> |so patches to get rid of the table duplication are highly welcome.
>
> >still you post a patch that needs our review and time ? This is crazy.
>
> You do not need to do it, but I will happily look at this.
> Of course my observations are the same as yours:
>
> >Your way of allocating a percpu counter for each counter is a pure TLB
> >and cache line blower (up to two cache lines per counter), not counting
> >the time needed to load a new table with 10.000 entries. Some people
> >still use scripts with hundred of calls to iptables.
>
> The two are statistically independent though. Even for a loaded
> ruleset, the TLB/DC miss accumulation will be desastrous - as I've found
> with linked-list rules/small allocs.
>
> >Allocating one contiguous percpu var for all counters is a must.
> >
> >Problem is : percpu alloc doesnt allow big allocations.
> >
> >#define PCPU_MIN_UNIT_SIZE PFN_ALIGN(32 << 10)
> >
> >So max allocation is 32 Kbytes, thats 2048 'xt_counters' only.
> >-> cannot really use pcpu-alloc, but a kmalloc_node() or vmalloc_node()
> >per cpu
>
> .. as is already done for jumpstack ;-)
IMHO, the real problem is not the table duplication. We know that adding
a level of indirection is going to hurt a lot because of cache misses.
Its the atomic op (spinlock) done for every packet, entering every
filter, with the conditional branch we do because of possible recursion.
per cpu variable, and spinlock... its really expensive.
Stephen tried an RCU conversion some time ago, that aborted because of
RCU drawbacks (too much memory was possibly waiting to be freed after a
grace period). Maybe RCU infrastructure is now ready to try again.
We should do what we did for u64 stats counters in network stack, using
the u64_stats_sync.h infrastructure. No more synchro between the threads
running through rules, and one gathering counters. Better latencies in
particular.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH net-next-2.6] filter: optimize sk_run_filter
From: Eric Dumazet @ 2010-11-19 11:17 UTC (permalink / raw)
To: David Miller; +Cc: Hagen Paul Pfeifer, netdev, Changli Gao
In-Reply-To: <1290160467.3034.33.camel@edumazet-laptop>
Le vendredi 19 novembre 2010 à 10:54 +0100, Eric Dumazet a écrit :
> I believe we should revert the u32 f_k = fentry->k; part
>
> fentry->k as is fast as f_k if stored on stack, and avoids one
> instruction if fentry->k is not needed.
>
>
A revert is not good on arches with decent number of registers (x86_64
for example).
Maybe have some CONFIG_ARCH_HAS_{FEW|MANY}_REGISTERS is needed, (or
already exist ?)
Here is the patch to save 400 bytes on x86_32, and really speedup the
damn thing on all arches.
Thanks
[PATCH net-next-2.6] filter: optimize sk_run_filter
remove pc variable to avoid arithmetic to compute fentry at each filter
instruction. Jumps directly manipulate fentry pointer.
As the last instruction of filter[] is guaranteed to be a RETURN, and
all jumps are before the last instruction, we dont need to check filter
bounds (number of instructions in filter array) at each iteration, so we
remove it from sk_run_filter() params.
On x86_32 remove f_k var introduced in commit 57fe93b374a6b871
(filter: make sure filters dont read uninitialized memory)
Note : We could use a CONFIG_ARCH_HAS_{FEW|MANY}_REGISTERS in order to
avoid too many ifdefs in this code.
This helps compiler to use cpu registers to hold fentry and A
accumulator.
On x86_32, this saves 401 bytes, and more important, sk_run_filter()
runs much faster because less register pressure (One less conditional
branch per BPF instruction)
# size net/core/filter.o net/core/filter_pre.o
text data bss dec hex filename
2948 0 0 2948 b84 net/core/filter.o
3349 0 0 3349 d15 net/core/filter_pre.o
on x86_64 :
# size net/core/filter.o net/core/filter_pre.o
text data bss dec hex filename
5173 0 0 5173 1435 net/core/filter.o
5224 0 0 5224 1468 net/core/filter_pre.o
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Changli Gao <xiaosuo@gmail.com>
Cc: Hagen Paul Pfeifer <hagen@jauu.net>
---
include/linux/filter.h | 2
net/core/filter.c | 93 +++++++++++++++++++-------------------
net/core/timestamping.c | 2
net/packet/af_packet.c | 2
4 files changed, 51 insertions(+), 48 deletions(-)
diff --git a/include/linux/filter.h b/include/linux/filter.h
index 151f5d7..447a775 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -147,7 +147,7 @@ struct sock;
extern int sk_filter(struct sock *sk, struct sk_buff *skb);
extern unsigned int sk_run_filter(struct sk_buff *skb,
- struct sock_filter *filter, int flen);
+ const struct sock_filter *filter);
extern int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk);
extern int sk_detach_filter(struct sock *sk);
extern int sk_chk_filter(struct sock_filter *filter, int flen);
diff --git a/net/core/filter.c b/net/core/filter.c
index 15a545d..9e77b3c 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -137,7 +137,7 @@ int sk_filter(struct sock *sk, struct sk_buff *skb)
rcu_read_lock_bh();
filter = rcu_dereference_bh(sk->sk_filter);
if (filter) {
- unsigned int pkt_len = sk_run_filter(skb, filter->insns, filter->len);
+ unsigned int pkt_len = sk_run_filter(skb, filter->insns);
err = pkt_len ? pskb_trim(skb, pkt_len) : -EPERM;
}
@@ -151,14 +151,15 @@ EXPORT_SYMBOL(sk_filter);
* sk_run_filter - run a filter on a socket
* @skb: buffer to run the filter on
* @filter: filter to apply
- * @flen: length of filter
*
* Decode and apply filter instructions to the skb->data.
- * Return length to keep, 0 for none. skb is the data we are
- * filtering, filter is the array of filter instructions, and
- * len is the number of filter blocks in the array.
+ * Return length to keep, 0 for none. @skb is the data we are
+ * filtering, @filter is the array of filter instructions.
+ * Because all jumps are guaranteed to be before last instruction,
+ * and last instruction guaranteed to be a RET, we dont need to check
+ * flen. (We used to pass to this function the length of filter)
*/
-unsigned int sk_run_filter(struct sk_buff *skb, struct sock_filter *filter, int flen)
+unsigned int sk_run_filter(struct sk_buff *skb, const struct sock_filter *fentry)
{
void *ptr;
u32 A = 0; /* Accumulator */
@@ -167,34 +168,36 @@ unsigned int sk_run_filter(struct sk_buff *skb, struct sock_filter *filter, int
unsigned long memvalid = 0;
u32 tmp;
int k;
- int pc;
BUILD_BUG_ON(BPF_MEMWORDS > BITS_PER_LONG);
/*
* Process array of filter instructions.
*/
- for (pc = 0; pc < flen; pc++) {
- const struct sock_filter *fentry = &filter[pc];
- u32 f_k = fentry->k;
+ for (;; fentry++) {
+#if defined(CONFIG_X86_32)
+#define K (fentry->k)
+#else
+ const u32 K = fentry->k;
+#endif
switch (fentry->code) {
case BPF_S_ALU_ADD_X:
A += X;
continue;
case BPF_S_ALU_ADD_K:
- A += f_k;
+ A += K;
continue;
case BPF_S_ALU_SUB_X:
A -= X;
continue;
case BPF_S_ALU_SUB_K:
- A -= f_k;
+ A -= K;
continue;
case BPF_S_ALU_MUL_X:
A *= X;
continue;
case BPF_S_ALU_MUL_K:
- A *= f_k;
+ A *= K;
continue;
case BPF_S_ALU_DIV_X:
if (X == 0)
@@ -202,64 +205,64 @@ unsigned int sk_run_filter(struct sk_buff *skb, struct sock_filter *filter, int
A /= X;
continue;
case BPF_S_ALU_DIV_K:
- A /= f_k;
+ A /= K;
continue;
case BPF_S_ALU_AND_X:
A &= X;
continue;
case BPF_S_ALU_AND_K:
- A &= f_k;
+ A &= K;
continue;
case BPF_S_ALU_OR_X:
A |= X;
continue;
case BPF_S_ALU_OR_K:
- A |= f_k;
+ A |= K;
continue;
case BPF_S_ALU_LSH_X:
A <<= X;
continue;
case BPF_S_ALU_LSH_K:
- A <<= f_k;
+ A <<= K;
continue;
case BPF_S_ALU_RSH_X:
A >>= X;
continue;
case BPF_S_ALU_RSH_K:
- A >>= f_k;
+ A >>= K;
continue;
case BPF_S_ALU_NEG:
A = -A;
continue;
case BPF_S_JMP_JA:
- pc += f_k;
+ fentry += K;
continue;
case BPF_S_JMP_JGT_K:
- pc += (A > f_k) ? fentry->jt : fentry->jf;
+ fentry += (A > K) ? fentry->jt : fentry->jf;
continue;
case BPF_S_JMP_JGE_K:
- pc += (A >= f_k) ? fentry->jt : fentry->jf;
+ fentry += (A >= K) ? fentry->jt : fentry->jf;
continue;
case BPF_S_JMP_JEQ_K:
- pc += (A == f_k) ? fentry->jt : fentry->jf;
+ fentry += (A == K) ? fentry->jt : fentry->jf;
continue;
case BPF_S_JMP_JSET_K:
- pc += (A & f_k) ? fentry->jt : fentry->jf;
+ fentry += (A & K) ? fentry->jt : fentry->jf;
continue;
case BPF_S_JMP_JGT_X:
- pc += (A > X) ? fentry->jt : fentry->jf;
+ fentry += (A > X) ? fentry->jt : fentry->jf;
continue;
case BPF_S_JMP_JGE_X:
- pc += (A >= X) ? fentry->jt : fentry->jf;
+ fentry += (A >= X) ? fentry->jt : fentry->jf;
continue;
case BPF_S_JMP_JEQ_X:
- pc += (A == X) ? fentry->jt : fentry->jf;
+ fentry += (A == X) ? fentry->jt : fentry->jf;
continue;
case BPF_S_JMP_JSET_X:
- pc += (A & X) ? fentry->jt : fentry->jf;
+ fentry += (A & X) ? fentry->jt : fentry->jf;
continue;
case BPF_S_LD_W_ABS:
- k = f_k;
+ k = K;
load_w:
ptr = load_pointer(skb, k, 4, &tmp);
if (ptr != NULL) {
@@ -268,7 +271,7 @@ load_w:
}
break;
case BPF_S_LD_H_ABS:
- k = f_k;
+ k = K;
load_h:
ptr = load_pointer(skb, k, 2, &tmp);
if (ptr != NULL) {
@@ -277,7 +280,7 @@ load_h:
}
break;
case BPF_S_LD_B_ABS:
- k = f_k;
+ k = K;
load_b:
ptr = load_pointer(skb, k, 1, &tmp);
if (ptr != NULL) {
@@ -292,34 +295,34 @@ load_b:
X = skb->len;
continue;
case BPF_S_LD_W_IND:
- k = X + f_k;
+ k = X + K;
goto load_w;
case BPF_S_LD_H_IND:
- k = X + f_k;
+ k = X + K;
goto load_h;
case BPF_S_LD_B_IND:
- k = X + f_k;
+ k = X + K;
goto load_b;
case BPF_S_LDX_B_MSH:
- ptr = load_pointer(skb, f_k, 1, &tmp);
+ ptr = load_pointer(skb, K, 1, &tmp);
if (ptr != NULL) {
X = (*(u8 *)ptr & 0xf) << 2;
continue;
}
return 0;
case BPF_S_LD_IMM:
- A = f_k;
+ A = K;
continue;
case BPF_S_LDX_IMM:
- X = f_k;
+ X = K;
continue;
case BPF_S_LD_MEM:
- A = (memvalid & (1UL << f_k)) ?
- mem[f_k] : 0;
+ A = (memvalid & (1UL << K)) ?
+ mem[K] : 0;
continue;
case BPF_S_LDX_MEM:
- X = (memvalid & (1UL << f_k)) ?
- mem[f_k] : 0;
+ X = (memvalid & (1UL << K)) ?
+ mem[K] : 0;
continue;
case BPF_S_MISC_TAX:
X = A;
@@ -328,16 +331,16 @@ load_b:
A = X;
continue;
case BPF_S_RET_K:
- return f_k;
+ return K;
case BPF_S_RET_A:
return A;
case BPF_S_ST:
- memvalid |= 1UL << f_k;
- mem[f_k] = A;
+ memvalid |= 1UL << K;
+ mem[K] = A;
continue;
case BPF_S_STX:
- memvalid |= 1UL << f_k;
- mem[f_k] = X;
+ memvalid |= 1UL << K;
+ mem[K] = X;
continue;
default:
WARN_ON(1);
diff --git a/net/core/timestamping.c b/net/core/timestamping.c
index 0ae6c22..dac7ed6 100644
--- a/net/core/timestamping.c
+++ b/net/core/timestamping.c
@@ -31,7 +31,7 @@ static unsigned int classify(struct sk_buff *skb)
if (likely(skb->dev &&
skb->dev->phydev &&
skb->dev->phydev->drv))
- return sk_run_filter(skb, ptp_filter, ARRAY_SIZE(ptp_filter));
+ return sk_run_filter(skb, ptp_filter);
else
return PTP_CLASS_NONE;
}
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 2096456..b6372dd 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -519,7 +519,7 @@ static inline unsigned int run_filter(struct sk_buff *skb, struct sock *sk,
rcu_read_lock_bh();
filter = rcu_dereference_bh(sk->sk_filter);
if (filter != NULL)
- res = sk_run_filter(skb, filter->insns, filter->len);
+ res = sk_run_filter(skb, filter->insns);
rcu_read_unlock_bh();
return res;
^ permalink raw reply related
* Re: [RFC PATCH] netfilter: remove the duplicate tables
From: Jan Engelhardt @ 2010-11-19 11:15 UTC (permalink / raw)
To: Eric Dumazet
Cc: Changli Gao, Patrick McHardy, David S. Miller, netfilter-devel,
netdev
In-Reply-To: <1290095020.2781.203.camel@edumazet-laptop>
On Thursday 2010-11-18 16:43, Eric Dumazet wrote:
>Le jeudi 18 novembre 2010 à 22:39 +0800, Changli Gao a écrit :
>> As only xt_counters are private to each CPU, we don't need to maintain
>> a whole individual table for each CPU.
>>
>> In the kernel space, we use the memory of ipt_entry.counters to save a
>> pointer to a percpu xt_counters. When iptables runs, it only update the
>> counters on its own CPU.
>>
>> On non SMP platforms, no change is made.
>>
>> Only the code of iptables is converted. Thanks for reviews.
>>
>
>Changli
>
>I answered you a (difficult) work was in progress
Was it? Quoting Patrick from 24h prior to this post:
|so patches to get rid of the table duplication are highly welcome.
>still you post a patch that needs our review and time ? This is crazy.
You do not need to do it, but I will happily look at this.
Of course my observations are the same as yours:
>Your way of allocating a percpu counter for each counter is a pure TLB
>and cache line blower (up to two cache lines per counter), not counting
>the time needed to load a new table with 10.000 entries. Some people
>still use scripts with hundred of calls to iptables.
The two are statistically independent though. Even for a loaded
ruleset, the TLB/DC miss accumulation will be desastrous - as I've found
with linked-list rules/small allocs.
>Allocating one contiguous percpu var for all counters is a must.
>
>Problem is : percpu alloc doesnt allow big allocations.
>
>#define PCPU_MIN_UNIT_SIZE PFN_ALIGN(32 << 10)
>
>So max allocation is 32 Kbytes, thats 2048 'xt_counters' only.
>-> cannot really use pcpu-alloc, but a kmalloc_node() or vmalloc_node()
>per cpu
.. as is already done for jumpstack ;-)
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: alchemy/gpr: au1000_eth regression with v2.6.37rc2
From: Florian Fainelli @ 2010-11-19 10:46 UTC (permalink / raw)
To: Wolfgang Grandegger; +Cc: Linux-MIPS, Netdev
In-Reply-To: <4CE65199.7030007@grandegger.com>
Hello Wolfgang,
On Friday 19 November 2010 11:29:45 Wolfgang Grandegger wrote:
> Hello Florian,
>
> On 11/18/2010 11:30 PM, Florian Fainelli wrote:
> > Hello Wolfgang,
> >
> > Le Thursday 18 November 2010 20:59:15, Wolfgang Grandegger a écrit :
> >> Hello,
> >>
> >> I just realized that the v2.6.37-rc2 kernel does not boot any more on
> >> the Alchemy GPR board. It works fine with v2.6.36. It hangs in the
> >> probe function of the au1000_eth driver when probing the second
> >>
> >> ethernet port (eth1):
> >> au1000_eth_mii: probed
> >> au1000-eth au1000-eth.0: (unregistered net_device): attached PHY
> >> driver
> >>
> >> [Generic PHY] (mii_bus:phy_addr=0:00, irq=-1) au1000-eth au1000-eth.0:
> >> eth0: Au1xx0 Ethernet found at 0x10500000, irq 35 au1000_eth: au1000_eth
> >> version 1.7 Pete Popov <ppopov@embeddedalley.com> ... hangs ...
> >>
> >> Similar messages should follow for eth1. I narrowed down (bisect'ed) the
> >>
> >> problem to commit:
> >> commit d0e7cb5d401695809ba8c980124ab1d8c66efc8b
> >> Author: Florian Fainelli <florian@openwrt.org>
> >> Date: Wed Sep 8 11:15:13 2010 +0000
> >>
> >> au1000-eth: remove volatiles, switch to I/O accessors
> >>
> >> Remove all the volatile keywords where they were used, switch to
> >> using
> >>
> >> the proper readl/writel accessors.
> >>
> >> Signed-off-by: Florian Fainelli <florian@openwrt.org>
> >> Signed-off-by: David S. Miller <davem@davemloft.net>
> >>
> >> The kernel actually hangs when accessing "&aup->mac->mii_control" in
> >> au1000_mdio_read(), but only for eth1. Any idea what does go wrong?
> >
> > I do not understand so far while it hangs only for eth1. My device only
> > has one ethernet MAC, so I could not notice the problem. Looking at this
> > close, there are a couple of u32 const* usages in
> > au1000_mdio_{read,write} which are looking wrong to me now. Can you try
> > to remove these?
>
> That did not help.
I suspected it, but thanks for the confirmation.
>
> >> In principle, I do not want to access the MII regs of the MAC because
> >> eth0 and eth1 are connected to switches. But that's not possible, even
> >> with "aup->phy_static_config=1" and "aup->phy_addr=0".
> >
> > If you think this is another issue, I will fix it in another patch.
>
> Accessing the MII registers of the MAC should not hang the system even
> if I do not need to. First I want to understand why. Looks like a wired
> optimizer issue.
I definitively agree, furthermore since there is a timeout for read and write
operations. I will look at the assembly and see if I can see anything
different.
>
> BTW: why do you use readl() and writel() instead of the usual au_readl()
> and au_writel() to access memory mapped cpu registers? It did not help,
> anyway.
This is just because they are generic accessors, and the au_{readl,writel}
variants were not different.
--
Florian
^ permalink raw reply
* Re: alchemy/gpr: au1000_eth regression with v2.6.37rc2
From: Wolfgang Grandegger @ 2010-11-19 10:29 UTC (permalink / raw)
To: Florian Fainelli; +Cc: Linux-MIPS, Netdev
In-Reply-To: <201011182330.08488.florian@openwrt.org>
Hello Florian,
On 11/18/2010 11:30 PM, Florian Fainelli wrote:
> Hello Wolfgang,
>
> Le Thursday 18 November 2010 20:59:15, Wolfgang Grandegger a écrit :
>> Hello,
>>
>> I just realized that the v2.6.37-rc2 kernel does not boot any more on
>> the Alchemy GPR board. It works fine with v2.6.36. It hangs in the
>> probe function of the au1000_eth driver when probing the second
>> ethernet port (eth1):
>>
>> au1000_eth_mii: probed
>> au1000-eth au1000-eth.0: (unregistered net_device): attached PHY driver
>> [Generic PHY] (mii_bus:phy_addr=0:00, irq=-1) au1000-eth au1000-eth.0:
>> eth0: Au1xx0 Ethernet found at 0x10500000, irq 35 au1000_eth: au1000_eth
>> version 1.7 Pete Popov <ppopov@embeddedalley.com> ... hangs ...
>>
>> Similar messages should follow for eth1. I narrowed down (bisect'ed) the
>> problem to commit:
>>
>> commit d0e7cb5d401695809ba8c980124ab1d8c66efc8b
>> Author: Florian Fainelli <florian@openwrt.org>
>> Date: Wed Sep 8 11:15:13 2010 +0000
>>
>> au1000-eth: remove volatiles, switch to I/O accessors
>>
>> Remove all the volatile keywords where they were used, switch to using
>> the proper readl/writel accessors.
>>
>> Signed-off-by: Florian Fainelli <florian@openwrt.org>
>> Signed-off-by: David S. Miller <davem@davemloft.net>
>>
>> The kernel actually hangs when accessing "&aup->mac->mii_control" in
>> au1000_mdio_read(), but only for eth1. Any idea what does go wrong?
>
> I do not understand so far while it hangs only for eth1. My device only has
> one ethernet MAC, so I could not notice the problem. Looking at this close,
> there are a couple of u32 const* usages in au1000_mdio_{read,write} which are
> looking wrong to me now. Can you try to remove these?
That did not help.
>> In principle, I do not want to access the MII regs of the MAC because
>> eth0 and eth1 are connected to switches. But that's not possible, even
>> with "aup->phy_static_config=1" and "aup->phy_addr=0".
>
> If you think this is another issue, I will fix it in another patch.
Accessing the MII registers of the MAC should not hang the system even
if I do not need to. First I want to understand why. Looks like a wired
optimizer issue.
BTW: why do you use readl() and writel() instead of the usual au_readl()
and au_writel() to access memory mapped cpu registers? It did not help,
anyway.
Wolfgang
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox