Re: [PATCH v3] filter: Optimize instruction revalidation code.

[Eric Dumazet <eric.dumazet@gmail.com>]

Le mercredi 17 novembre 2010 à 10:19 +0900, Tetsuo Handa a écrit :
> Eric Dumazet wrote:
> > I dont understand the problem...
> > Once translated, you have to test the translated code, not the original
> > one ;)
> 
> I moved the test to after translation.
> 
> > why u16 ? 
> > 
> > You store translated instructions, so u8 is OK
> 
> I chose u16 because type of filter->code is __u16.
> But I changed to use u8 as you suggested that translated code fits in u8.
> 
> > Also fix the indentation at the end of sk_chk_filter()
> > 
> > You have 3 extra tabulations :
> 
> Fixed.
> 
> Hagen Paul Pfeifer wrote:
> > Maybe I don't get it, but you increment the opcode by one, but you never
> > increment the opcode in sk_run_filter() - do I miss something? Did you test
> > the your patch (a trivial tcpdump rule should be sufficient)?
> 
> I added a comment line.
> 
> Changli Gao wrote:
> > > +               struct sock_filter *ftest = &filter[pc];
> > 
> > Why move the define here?
> 
> To suppress compiler's warning about mixed declaration.
> 
> > > +               u16 code = ftest->code;
> > >
> > > +               if (code >= ARRAY_SIZE(codes))
> > > +                       return 0;
> > 
> > return -EINVAL;
> 
> Fixed in v2. Thanks.
> 
> > But how about this:
> > 
> > enum {
> >         BPF_S_RET_K = 1,
> 
> If BPF_S_* are only for kernel internal use, I think we don't need to translate
> from the beginning because only net/core/filter.c uses BPF_S_*.
> 
> BPF_S_* are exposed to userspace via /usr/include/linux/filter.h since 2.6.36.
> Is it no problem to change?

No problem, and Changli posted patch to move them to net/core/filter.c
anyway.

> 
> Filesize change (x86_32) by this patch:
>   gcc 3.3.5: 7184 -> 5060
>   gcc 4.4.3: 7972 -> 5588
> ----------------------------------------
> From b8777ab64bc31dbbe499eb62c2ffd29add7e79c8 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Wed, 17 Nov 2010 09:46:33 +0900
> Subject: [PATCH v3] filter: Optimize instruction revalidation code.
> 
> Since repeating u16 value to u8 value conversion using switch() clause's
> case statement is wasteful, this patch introduces u16 to u8 mapping table
> and removes most of case statements. As a result, the size of net/core/filter.o
> is reduced by about 29% on x86.
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  net/core/filter.c |  231 +++++++++++++++++------------------------------------
>  1 files changed, 72 insertions(+), 159 deletions(-)
> 

Acked-by: Eric Dumazet <eric.dumazet@gmail.com>

Please repost it when Changli patch is accepted by David 
(if accepted :)), to get rid of the "+ 1"