[[e-mail]: CVS commit: src/sys]

[Joerg Sonnenberger]

----- Forwarded message from Antti Kantee <***@netbsd.org> -----

Module Name: src
Committed By: pooka
Date: Tue Jan 19 22:08:18 UTC 2010

Modified Files:
[snip]
src/sys/net: bpf.c bpf.h if_arcsubr.c if_atmsubr.c if_bridge.c
if_ecosubr.c if_etherip.c if_ethersubr.c if_faith.c if_fddisubr.c
if_gif.c if_gre.c if_hippisubr.c if_ieee1394subr.c if_loop.c
if_ppp.c if_pppoe.c if_sl.c if_srt.c if_stf.c if_strip.c if_tap.c
if_tokensubr.c if_tun.c if_vlan.c ppp_tty.c

Log Message:
Redefine bpf linkage through an always present op vector, i.e.
#if NBPFILTER is no longer required in the client. This change
doesn't yet add support for loading bpf as a module, since drivers
can register before bpf is attached. However, callers of bpf can
now be modularized.

----- End forwarded message -----

I disagree with this change, at least the part about using a function
vector. This adds the overhead on an indirect call for no good reason to
a common code path for every single network packet, if there is a BPF
listener. That happens as soon as the interface is configured by DHCP.

The same benefit of being able to modularize BPF could be obtained by
pushing down the indirect calls into the actual functions. That could at
some future point also be optimised by binary patching. This only
matters of course, if someone actually wants to make BPF a module, which
I am not too sure about given the constrains already mentioned.

So please change this back to use normal function calls. It might be a
good time to change bpf_mtap and friends to be simple macros that
include the if (ifp->if_bpf) check, but that's more a cosmetic question.

Joerg

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Antti Kantee]

Post by Joerg Sonnenberger
Module Name: src
Committed By: pooka
Date: Tue Jan 19 22:08:18 UTC 2010
[snip]
src/sys/net: bpf.c bpf.h if_arcsubr.c if_atmsubr.c if_bridge.c
if_ecosubr.c if_etherip.c if_ethersubr.c if_faith.c if_fddisubr.c
if_gif.c if_gre.c if_hippisubr.c if_ieee1394subr.c if_loop.c
if_ppp.c if_pppoe.c if_sl.c if_srt.c if_stf.c if_strip.c if_tap.c
if_tokensubr.c if_tun.c if_vlan.c ppp_tty.c
Redefine bpf linkage through an always present op vector, i.e.
#if NBPFILTER is no longer required in the client. This change
doesn't yet add support for loading bpf as a module, since drivers
can register before bpf is attached. However, callers of bpf can
now be modularized.
----- End forwarded message -----
I disagree with this change, at least the part about using a function
vector. This adds the overhead on an indirect call for no good reason to
a common code path for every single network packet, if there is a BPF
listener. That happens as soon as the interface is configured by DHCP.
The same benefit of being able to modularize BPF could be obtained by
pushing down the indirect calls into the actual functions. That could at
some future point also be optimised by binary patching. This only
matters of course, if someone actually wants to make BPF a module, which
I am not too sure about given the constrains already mentioned.
So please change this back to use normal function calls. It might be a
good time to change bpf_mtap and friends to be simple macros that
include the if (ifp->if_bpf) check, but that's more a cosmetic question.

Thank you for your insightful comments. It is refreshing to see
people truly try their best to discover problems.

If you are concerned with dhcp machine performance, may I suggest
you start by optimizing the thousands of instructions that bpf
executes for every packet while trying to decide that it's not
handling dhcp input. Also, you'd need to fix bus_space on most
architectures to keep the damn nic drivers from doing *multiple*
indirect calls (start with sun2, because this project will benefit
the most from you working on that). Finally, follow up with
binary-patching away that pesky if_input method.

best regards,
antti

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[YAMAMOTO Takashi]

hi,

Post by Antti Kantee

Post by Joerg Sonnenberger
Module Name: src
Committed By: pooka
Date: Tue Jan 19 22:08:18 UTC 2010
[snip]
src/sys/net: bpf.c bpf.h if_arcsubr.c if_atmsubr.c if_bridge.c
if_ecosubr.c if_etherip.c if_ethersubr.c if_faith.c if_fddisubr.c
if_gif.c if_gre.c if_hippisubr.c if_ieee1394subr.c if_loop.c
if_ppp.c if_pppoe.c if_sl.c if_srt.c if_stf.c if_strip.c if_tap.c
if_tokensubr.c if_tun.c if_vlan.c ppp_tty.c
Redefine bpf linkage through an always present op vector, i.e.
#if NBPFILTER is no longer required in the client. This change
doesn't yet add support for loading bpf as a module, since drivers
can register before bpf is attached. However, callers of bpf can
now be modularized.
----- End forwarded message -----
I disagree with this change, at least the part about using a function
vector. This adds the overhead on an indirect call for no good reason to
a common code path for every single network packet, if there is a BPF
listener. That happens as soon as the interface is configured by DHCP.
The same benefit of being able to modularize BPF could be obtained by
pushing down the indirect calls into the actual functions. That could at
some future point also be optimised by binary patching. This only
matters of course, if someone actually wants to make BPF a module, which
I am not too sure about given the constrains already mentioned.
So please change this back to use normal function calls. It might be a
good time to change bpf_mtap and friends to be simple macros that
include the if (ifp->if_bpf) check, but that's more a cosmetic question.

Thank you for your insightful comments. It is refreshing to see
people truly try their best to discover problems.
If you are concerned with dhcp machine performance, may I suggest
you start by optimizing the thousands of instructions that bpf
executes for every packet while trying to decide that it's not
handling dhcp input. Also, you'd need to fix bus_space on most
architectures to keep the damn nic drivers from doing *multiple*
indirect calls (start with sun2, because this project will benefit
the most from you working on that). Finally, follow up with
binary-patching away that pesky if_input method.

if you prefer to expose bpf_ops to each drivers, i'd like to hear why.
or, do you mean that you simply don't care?

YAMAMOTO Takashi

Post by Antti Kantee
best regards,
antti

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Antti Kantee]

Post by YAMAMOTO Takashi
if you prefer to expose bpf_ops to each drivers, i'd like to hear why.
or, do you mean that you simply don't care?

That's pretty much it. Whoever wants to improve it measured by whatever
metric, feel free.

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Masao Uebayashi]

The code paths where bpf hooks are placed are very performance
sensitive. If macro is used, users who want to customize their own
binary can change those hooks inline. If expanded, they have to
manage bigger local diffs, which is not good for serious 3rd party
users.

(There surely exist such users. At least @ a company at Jimbo-cho.
The problem is they don't speak.)

Masao

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Joerg Sonnenberger]

Post by Antti Kantee
If you are concerned with dhcp machine performance, may I suggest
you start by optimizing the thousands of instructions that bpf
executes for every packet while trying to decide that it's not
handling dhcp input.

The call is also done for *output*, which does not run the filter it
all in the DHCP case.

Post by Antti Kantee
Also, you'd need to fix bus_space on most
architectures to keep the damn nic drivers from doing *multiple*
indirect calls (start with sun2, because this project will benefit
the most from you working on that). Finally, follow up with
binary-patching away that pesky if_input method.

So just to make sure I understand you correctly. You are introducing
overhead for a lot of use casei to make it possible to conditionally
load 20KB after some non-trivial issues are solved. You are justifying
that because some other places either actually do use the abstraction
or are on some architectures slow?

Joerg

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de