Subject: Re: CVS commit: syssrc/sys/net
To: Atsushi Onoe <onoe@sm.sony.co.jp>
From: Darren Reed <darrenr@reed.wattle.id.au>
List: tech-net
Date: 09/30/2002 21:16:56
In some email I received from Atsushi Onoe, sie wrote:
> > The question then is does this encourage similar use elsewhere and is
> > that a good/bad idea ?
> 
> Yes, it is exactly what I'd like to discuss.  Defining mbuf initializer
> in common code (e.g. sys/mbuf.h) looks encourage to use fake mbuf for me.

Hmm.  I'm tempted to suggest we should add more diagnostic code to
uipc_mbuf.c to check for things like type being within the 'correct'
range - ie [0,7] for NetBSD-current.

I do see your point, however, and I'm not sure what to do about it.

I'm not even sure we should.

> If you are proposing such initializer macro to put in net/bpf.h, I can
> agree except for the name, which should be prefixed with BPF_.

No, I am not.

> > So given that bpf_mtap() is already an explicit interface for mbufs,
> > why do we need bpf_mtap2() ?  The prototype you gave did not seem to
> > suggest it was a big advantage to have.
> 
> It just hide any possible hacks within bpf.  Within bpf code, you can
> safely use fake mbuf or anything useful since you know that the caller
> of the bpf module only refer the limited members of mbufs.

I see what you're saying - the caller only passes _real_ mbufs to the
BPF interface which is then free to do what it wants, including making
fake mbufs, for intrnal use.  Is there no way to avoid this copy ?

> If you try to expose such behaviour out of bpf module, the interface
> should be well defined to do the right thing, IMO.

Should bpf_mtap() be declared to be:
void bpf_mtap __P((caddr_t, const struct mbuf *));
?

> > I could maybe see the point in having a bpf_dlt_mtap() that had
> > the 'common code' from all of the DLT_NULL intraface calls of
> > bpf_mtap() in it.
> 
> It could be a possible solution, though I'm not sure the exact definition
> also useful for ieee1394subr.c and some wireless interface such as wi.c.

The code in if_ieee1394subr.c seems to be a special case ?

> The proposed bpf_mtap2() is quite simple and covers most of common cases.
> Adding some lower linklayer header 

Why do we need bpf_tap, bpf_mtap and bpf_mtap2 ?

The only user of bpf_tap() that I can find (if_hp.c) has this at
the top:

/* XXX THIS DRIVER IS BROKEN.  IT WILL NOT EVEN COMPILE. */

So maybe bpf_tap() should be garbage collected ?

That said, your proposal for bpf_mtap2() was:

void bpf_mtap2(caddr_t bpf, caddr_t hdr, int hdrlen, struct mbuf *m);

The first problems is that the hdr part needs to be in an mbuf,
sooner or later and putting it directly in with the code in
if_ieee1394subr.c saves having to copy that again.

[...]
> > My problem with using MGET here is it is a code path that _is_ speed
> > sensitive if you're buliding something like an IDS on top of NetBSD.
> > 
> > This is a change that would make NetBSD slower than others.
> 
> I don't object your use of fake mbuf in your userland programs.
> But it is a separate issue discussed here.

I think you're missing the point I raised.

IDS software will use BPF to get packets from the network.  Having
spoken to at least one vendor about doing that, their main gripe
was how slow BPF was.  That's the in-kernel BPF being slow.

Hence, I'm interested in knowing what we can do in a general manner
to make BPF faster, as well as better and less buggy.  I made a bunch
of changes to bpf_filter() after hearing this that made the kernel
code not use m_xword() and friends all the time (this is now in
libpcap.)

Replacing the current use of a mbuf on the stack with MGET will not
make it any quicker.

Actually, in these cases, it makes no difference :-(  libpcap generates
code that expects there to be a 4 byte DLT_NULL header, and this forces
all bpf accesses to use m_x*() because 'buflen == 0' from bpf_mtap. :(

That said, a bpf_mtap2() will not solve *that* problem either :-/

Darren