status of the mbuf API SoC project

Hello,

we should think about merging the results of my SoC project.
I'll give a
brief overview here.

I've added two macros to replace mtod: mptr and mptr_rw.
Their
"prototypes" are:

const datatype *
mptr(struct mbuf **mp, datatype, int off, int flags);

datatype *
mptr_rw(struct mbuf **mp, datatype, int off, int flags);

differences from mtod:

- mtod accepts the type of the resulting pointer, mptr*
accept the type
  that is being pointed to. This is to be able to apply
sizeof() to the
  type inside the macros, and in the case of mptr, to add
const.

- the new macros accept an offset, to access structures
which are not at
  the beginning of the packet.

- struct mbuf *m was changed to struct mbuf **mp, enabling
those macros to
  rearrange the mbuf chain.

The change in semantics is that the macros explicitely check
if the
pointer is valid, that is, if there is at least
sizeof(datatype)
contiguous data in the mbuf. In the case of mptr, the
writability of the
mbuf is also checked.

What happens if the check fails depends on the value of
flags. If they
include MP_PULLUP, the chain is rearranged to meet those
requirements.
This way, mptr can replace the calls to m_pullup or
m_pulldown.

Without MP_PULLUP, such calls trigger a panic.

Sometimes, passing the size implicitely through sizeof is
inconvenient, so
I've made function variants m_datarange and m_datarange_rw
which accept an
explicit size:

const void *
m_datarange(struct mbuf **mp, int len, int off, int flags);

void *
m_datarange_rw(struct mbuf **mp, int len, int off, int
flags);

In those functions, MP_PULLUP is implied. They also accept
more flags so
those function can replace the current m_copyup.

Please see the manual page at
http://netbsd-soc.sourceforge.net/projects/mbuf/man/mb
uf.9 for details.

I have converted all the net, netinet, netinet6 and net80211
to the new
functions where suitable (this resulted in a lot of
constification). The
result is that the input path should be now read-only mbuf
safe (read-only
mbufs were causing a lot of problems before). I've checked
it with a patch
to Xen provided by bouyer which maps mbuf external
storage read-only.

A possibly controversial change was done to the IP and IPv6
input
routines. They now use m_datarange to make a large (512
bytes) contiguous
region, which should be enough for all the headers. Then the
implementation of upper-layer protocols do not need to to
worry about
contiguous mbufs. A problem could arise when the IP header
is added in the
output path in a separate mbuf (for example because the data
are shared),
and then the packet is reflected back by the loopback
device. In this
case, we have to copy much more data than usually needed to
make those 512
bytes contiguous with no good use. I did not benchmark this
yet, but I
guess it will have a negative impact on performance.

Besides that, another subject for discussion is the naming
and arguments
of the proposed macros and functions. The arguments of
m_datarange were
modelled after the arguments of mptr, with datatype replaced
by the
requested size. This seemed natural. But after that I
realized that other
mbuf routines have "off" and "len"
arguments reversed (see m_copydata for
an example) so I should probably swap those arguments for
m_datarange too,
and swap "datatype" with "off" in
mptr to match.

Please have a look at http
://netbsd-soc.sourceforge.net/projects/mbuf/ if
you are interested.

Pavel

> I've added two macros to replace mtod: mptr and
mptr_rw.

> const datatype *
> mptr(struct mbuf **mp, datatype, int off, int flags);

> [...and three others...]

> differences from mtod:

> - the new macros accept an offset, to access structures
which are not
>   at the beginning of the packet.

Shouldn't a redesign of mtod also include a way to deal
with the
alignment issue?  I've seen code all over the place that
blindly
assumes a pointer returned from mtod is correctly aligned,
something
which has bit me more than once.  Yet I see no alignment
argument to
these macros.

Do they simply not address the issue at all, leaving it up
to the
caller to ensure that whatever stores the relevant number of
bytes at
the relevant offset is correctly aligned?  I can't see how
they could,
when part of the function's interface is that it may
rearrange the mbuf
chain (which mtod sidestepped by leaving that up to its
caller).

/~\ The ASCII				der Mouse
\ / Ribbon Campaign
 X  Against HTML	       mouserodents.montreal.qc.ca
/ \ Email!	     7D C8 61 52 5D E7 2D 39  4E F1 31 3E E8 B3
27 4B

On Mon, Sep 04, 2006 at 12:43:01AM -0400, der Mouse wrote:
> > I've added two macros to replace mtod: mptr and
mptr_rw.
> 
> > const datatype *
> > mptr(struct mbuf **mp, datatype, int off, int
flags);
> 
> > [...and three others...]
> 
> > differences from mtod:
> 
> > - the new macros accept an offset, to access
structures which are not
> >   at the beginning of the packet.
> 
> Shouldn't a redesign of mtod also include a way to
deal with the
> alignment issue?  I've seen code all over the place
that blindly
> assumes a pointer returned from mtod is correctly
aligned, something
> which has bit me more than once.  Yet I see no
alignment argument to
> these macros.

Good question. The m_datarange function in fact has an
alignment argument
in form of the flags - there are flags to request 16-bit,
32-bit and
64-bit alignment. The macros should IMHO do this
automatically by
analyzing the type - there is a __builtin_offsetof function
in gcc for
that. (I haven't implemented this yet.) But most of network
headers are
declared as __packed, which means that there is no alignment
required.
(Perhaps this should be changed after the ability to ensure
correct
alignment is added to the macros.)

Pavel

On Mon, Sep 04, 2006 at 12:43:01AM -0400, der Mouse wrote:
> > I've added two macros to replace mtod: mptr and
mptr_rw.
> 
> > const datatype *
> > mptr(struct mbuf **mp, datatype, int off, int
flags);
> 
> > [...and three others...]
> 
> > differences from mtod:
> 
> > - the new macros accept an offset, to access
structures which are not
> >   at the beginning of the packet.
> 
> Shouldn't a redesign of mtod also include a way to
deal with the
> alignment issue?  I've seen code all over the place
that blindly
> assumes a pointer returned from mtod is correctly
aligned, something
> which has bit me more than once.

BTW how exactly have you been byten by that, considering
that packet
header structures are declared as __packed and don't
require alignment?

Pavel

>> Shouldn't a redesign of mtod also include a way to
deal with the
>> alignment issue?  I've seen code all over the
place that blindly
>> assumes a pointer returned from mtod is correctly
aligned, something
>> which has bit me more than once.
> BTW how exactly have you been byten by that,
considering that packet
> header structures are declared as __packed and don't
require
> alignment?

I don't remember exact details; for too long now I've just
made a
practice of always treating an mbuf as an unaligned byte
buffer,
copying into and out of it as I would any other unaligned
byte buffer.

I can only assume that either (a) my bad experiences
predated the use
of __packed, or (b) my bad experiences were with structs
other than
packet header structures.  (The horrible botch that is the
CMSG_* stuff
comes to mind as a plausible candidate for (b) (ie, struct
cmsghdr, and
control message data types, like int, that require
alignment).)

/~\ The ASCII				der Mouse
\ / Ribbon Campaign
 X  Against HTML	       mouserodents.montreal.qc.ca
/ \ Email!	     7D C8 61 52 5D E7 2D 39  4E F1 31 3E E8 B3
27 4B