Discussion:
new mbuf API
(too old to reply)
Pavel Cahyna
2007-04-20 17:24:53 UTC
Permalink
Hello,

I would like to commit the new mbuf API, resulting from last year SoC
project, reasonably soon. I described it in an old message:
http://permalink.gmane.org/gmane.os.netbsd.devel.network/7959
so I won't repeat much here.

Since then, I've only swapped the arguments "datatype" with "off" and "len"
with "off", as suggested in that message.

What I've missed there was a description of "guaranteed contiguous
data". The problem is that one function can make part of the mbuf chanin
contiguous (like m_pulldown or one of the new functions), but a
subsequent call can fragment the chain again. To prevent this, I have
introduced the notion of "guaranteed contiguous data". If a function
makes a range of a mbuf chain contiguous, it also marks it as a
guaranteed contiguous region and subsequent calls to mbuf rearranging
functions must keep it contiguous.

Unfortunately m_pulldown can't respect it in all cases: if the
guaranteed contiguous region extends say from byte 0 to byte 20 and
m_pulldown is called to make data contiguous between 10 and 40, it may
need to copy to a new mbuf and it should copy everything between 0 and
40 to respect the former guaranteed contiguous region. But it can't copy
the data between 0 and 10 because m_pulldown is not supposed to touch
the data before the offset "off" where is it told to start (10 in this
case). It is expected that code needing the new feature will use the new
functions instead of m_pulldown.

Another improvement is that m_copyback_cow now supports mbuf chain
expansion, and a bug in this function has been fixed.

The code is at
http://netbsd-soc.sourceforge.net/projects/mbuf/userland/mbuf_impl/sys/,
the two .c files belong under src/sys/kern, mbuf.h under src/sys/sys. As
this changes the implementation of some currently used functions, I would
appreciate some testing, especially of IPv6 and IPSEC.

Pavel

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Greg Troxel
2007-04-20 17:40:16 UTC
Permalink
What I've missed there was a description of "guaranteed contiguous
data". The problem is that one function can make part of the mbuf chanin
contiguous (like m_pulldown or one of the new functions), but a
subsequent call can fragment the chain again. To prevent this, I have
introduced the notion of "guaranteed contiguous data". If a function
makes a range of a mbuf chain contiguous, it also marks it as a
guaranteed contiguous region and subsequent calls to mbuf rearranging
functions must keep it contiguous.

If it's not safe for a caller to assume it's still true, then the word
guaranteed shouldn't be used. What you're describing is more like an
efficiency hint. This also seems complicated, and if a caller can't
safely omit checks, I'm not sure how much is gained.

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Liam J. Foy
2007-04-20 17:49:11 UTC
Permalink
Post by Pavel Cahyna
The code is at
http://netbsd-soc.sourceforge.net/projects/mbuf/userland/mbuf_impl/sys/,
the two .c files belong under src/sys/kern, mbuf.h under src/sys/sys. As
this changes the implementation of some currently used functions, I would
appreciate some testing, especially of IPv6 and IPSEC.
Pavel
I have been using the patch set for a little while now. It has been
general usage with IPv6 and IPv4 forwarding. I'll try and play around
with the patch set and IPSEC this weekend for you.
--
--
Liam J. Foy
<***@netbsd.org>

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Pavel Cahyna
2007-04-20 17:50:22 UTC
Permalink
Post by Pavel Cahyna
What I've missed there was a description of "guaranteed contiguous
data". The problem is that one function can make part of the mbuf chanin
contiguous (like m_pulldown or one of the new functions), but a
subsequent call can fragment the chain again. To prevent this, I have
introduced the notion of "guaranteed contiguous data". If a function
makes a range of a mbuf chain contiguous, it also marks it as a
guaranteed contiguous region and subsequent calls to mbuf rearranging
functions must keep it contiguous.
If it's not safe for a caller to assume it's still true, then the word
guaranteed shouldn't be used. What you're describing is more like an
efficiency hint. This also seems complicated, and if a caller can't
safely omit checks, I'm not sure how much is gained.
The purpose is exactly to be able to omit checks.

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Greg Troxel
2007-04-20 17:54:33 UTC
Permalink
Post by Pavel Cahyna
Post by Pavel Cahyna
What I've missed there was a description of "guaranteed contiguous
data". The problem is that one function can make part of the mbuf chanin
contiguous (like m_pulldown or one of the new functions), but a
subsequent call can fragment the chain again. To prevent this, I have
introduced the notion of "guaranteed contiguous data". If a function
makes a range of a mbuf chain contiguous, it also marks it as a
guaranteed contiguous region and subsequent calls to mbuf rearranging
functions must keep it contiguous.
If it's not safe for a caller to assume it's still true, then the word
guaranteed shouldn't be used. What you're describing is more like an
efficiency hint. This also seems complicated, and if a caller can't
safely omit checks, I'm not sure how much is gained.
The purpose is exactly to be able to omit checks.
Then you have to specify very carefully under what circumstances the
checks can be omitted; you said that the "guarantee" couldn't always be
honored.


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Pavel Cahyna
2007-04-20 18:03:58 UTC
Permalink
Post by Greg Troxel
Post by Pavel Cahyna
Post by Pavel Cahyna
What I've missed there was a description of "guaranteed contiguous
data". The problem is that one function can make part of the mbuf chanin
contiguous (like m_pulldown or one of the new functions), but a
subsequent call can fragment the chain again. To prevent this, I have
introduced the notion of "guaranteed contiguous data". If a function
makes a range of a mbuf chain contiguous, it also marks it as a
guaranteed contiguous region and subsequent calls to mbuf rearranging
functions must keep it contiguous.
If it's not safe for a caller to assume it's still true, then the word
guaranteed shouldn't be used. What you're describing is more like an
efficiency hint. This also seems complicated, and if a caller can't
safely omit checks, I'm not sure how much is gained.
The purpose is exactly to be able to omit checks.
Then you have to specify very carefully under what circumstances the
checks can be omitted; you said that the "guarantee" couldn't always be
honored.
Yes, you should not call m_pulldown... and if there are more restrictions
like that, they should be of course documented.

Pavel

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
YAMAMOTO Takashi
2007-04-21 12:44:17 UTC
Permalink
Post by Pavel Cahyna
Post by Greg Troxel
Post by Pavel Cahyna
Post by Pavel Cahyna
What I've missed there was a description of "guaranteed contiguous
data". The problem is that one function can make part of the mbuf chanin
contiguous (like m_pulldown or one of the new functions), but a
subsequent call can fragment the chain again. To prevent this, I have
introduced the notion of "guaranteed contiguous data". If a function
makes a range of a mbuf chain contiguous, it also marks it as a
guaranteed contiguous region and subsequent calls to mbuf rearranging
functions must keep it contiguous.
If it's not safe for a caller to assume it's still true, then the word
guaranteed shouldn't be used. What you're describing is more like an
efficiency hint. This also seems complicated, and if a caller can't
safely omit checks, I'm not sure how much is gained.
The purpose is exactly to be able to omit checks.
Then you have to specify very carefully under what circumstances the
checks can be omitted; you said that the "guarantee" couldn't always be
honored.
Yes, you should not call m_pulldown... and if there are more restrictions
like that, they should be of course documented.
Pavel
is it worth to have such complicate semantics?

YAMAMOTO Takashi

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
YAMAMOTO Takashi
2007-04-23 08:00:33 UTC
Permalink
Post by YAMAMOTO Takashi
Post by Pavel Cahyna
Post by Greg Troxel
Post by Pavel Cahyna
Post by Pavel Cahyna
What I've missed there was a description of "guaranteed contiguous
data". The problem is that one function can make part of the mbuf chanin
contiguous (like m_pulldown or one of the new functions), but a
subsequent call can fragment the chain again. To prevent this, I have
introduced the notion of "guaranteed contiguous data". If a function
makes a range of a mbuf chain contiguous, it also marks it as a
guaranteed contiguous region and subsequent calls to mbuf rearranging
functions must keep it contiguous.
If it's not safe for a caller to assume it's still true, then the word
guaranteed shouldn't be used. What you're describing is more like an
efficiency hint. This also seems complicated, and if a caller can't
safely omit checks, I'm not sure how much is gained.
The purpose is exactly to be able to omit checks.
Then you have to specify very carefully under what circumstances the
checks can be omitted; you said that the "guarantee" couldn't always be
honored.
Yes, you should not call m_pulldown... and if there are more restrictions
like that, they should be of course documented.
Pavel
is it worth to have such complicate semantics?
YAMAMOTO Takashi
can you give me an example where the "guaranteed contiguous data" notion
is actually needed?

YAMAMOTO Takashi

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Hubert Feyrer
2007-04-20 18:26:42 UTC
Permalink
Post by Pavel Cahyna
The code is at
http://netbsd-soc.sourceforge.net/projects/mbuf/userland/mbuf_impl/sys/,
the two .c files belong under src/sys/kern, mbuf.h under src/sys/sys. As
this changes the implementation of some currently used functions, I would
appreciate some testing, especially of IPv6 and IPSEC.
Just wondering: is there any performance impact of these changes?
Did you make any tests?


- Hubert

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Pavel Cahyna
2007-04-20 19:01:50 UTC
Permalink
Post by Hubert Feyrer
Post by Pavel Cahyna
The code is at
http://netbsd-soc.sourceforge.net/projects/mbuf/userland/mbuf_impl/sys/,
the two .c files belong under src/sys/kern, mbuf.h under src/sys/sys. As
this changes the implementation of some currently used functions, I would
appreciate some testing, especially of IPv6 and IPSEC.
Just wondering: is there any performance impact of these changes?
Did you make any tests?
Short answer to your second question: no.

Pavel

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
David Young
2007-04-20 23:24:00 UTC
Permalink
Post by Pavel Cahyna
Hello,
I would like to commit the new mbuf API, resulting from last year SoC
http://permalink.gmane.org/gmane.os.netbsd.devel.network/7959
so I won't repeat much here.
Great!

Do mptr() and mptr_rw() guarantee any alignment? Looks (at a glance) like
"no," but it would be easy enough to add a flag MP_ALIGN for the purpose.
What do you think?

Dave
--
David Young OJC Technologies
***@ojctech.com Urbana, IL * (217) 278-3933

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Pavel Cahyna
2007-04-21 06:25:05 UTC
Permalink
Post by David Young
Post by Pavel Cahyna
Hello,
I would like to commit the new mbuf API, resulting from last year SoC
http://permalink.gmane.org/gmane.os.netbsd.devel.network/7959
so I won't repeat much here.
Great!
Do mptr() and mptr_rw() guarantee any alignment? Looks (at a glance) like
"no," but it would be easy enough to add a flag MP_ALIGN for the purpose.
What do you think?
There are three alignment flags: MP_ALIGN_2, MP_ALIGN_4, MP_ALIGN_8. They
are intended for m_datarange* functions but should work with the mptr*
macros, too. This is undocumented because I think that the macros should
determine the needed alignment automatically, as they know automatically
the needed size. See the thread referenced above.

(http://thread.gmane.org/gmane.os.netbsd.devel.network/7959 for a threaded
view.)

Pavel

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
David Young
2007-04-23 05:38:50 UTC
Permalink
Post by Pavel Cahyna
Hello,
I would like to commit the new mbuf API, resulting from last year SoC
http://permalink.gmane.org/gmane.os.netbsd.devel.network/7959
so I won't repeat much here.
You mentioned pulling up 512 bytes of a packet into a contiguous, writable
area before passing it to either IPv4 or IPv6 stacks. You don't still
do that, do you?

Dave
--
David Young OJC Technologies
***@ojctech.com Urbana, IL * (217) 278-3933

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Pavel Cahyna
2007-04-23 06:46:57 UTC
Permalink
Post by David Young
Post by Pavel Cahyna
Hello,
I would like to commit the new mbuf API, resulting from last year SoC
http://permalink.gmane.org/gmane.os.netbsd.devel.network/7959
so I won't repeat much here.
You mentioned pulling up 512 bytes of a packet into a contiguous, writable
area before passing it to either IPv4 or IPv6 stacks. You don't still
do that, do you?
This proposal is about introducing the new API, not about converting
anything to use it or any related changes to the network stack.

I did pulling up of beginning of a packaet into a contiguous (but not
necessarily writable) area at the entry to IP protocols. But now I think
it was not a good idea.

Pavel

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
David Young
2007-04-23 05:47:04 UTC
Permalink
On Fri, Apr 20, 2007 at 05:24:53PM +0000, Pavel Cahyna wrote:
*snip snip*
Post by Pavel Cahyna
I have
introduced the notion of "guaranteed contiguous data". If a function
makes a range of a mbuf chain contiguous, it also marks it as a
guaranteed contiguous region and subsequent calls to mbuf rearranging
functions must keep it contiguous.
*snip snip*

Is there any reason we cannot both forbid m_pulldown for all new code,
and convert all old uses to the new API? Just developer time?

Dave
--
David Young OJC Technologies
***@ojctech.com Urbana, IL * (217) 278-3933

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Pavel Cahyna
2007-04-23 06:55:15 UTC
Permalink
Post by David Young
*snip snip*
Post by Pavel Cahyna
I have
introduced the notion of "guaranteed contiguous data". If a function
makes a range of a mbuf chain contiguous, it also marks it as a
guaranteed contiguous region and subsequent calls to mbuf rearranging
functions must keep it contiguous.
*snip snip*
Is there any reason we cannot both forbid m_pulldown for all new code,
and convert all old uses to the new API? Just developer time?
The plan is to gradually convert all old uses to the new API and convert
whole subsystems at once, so a subsystem would never use m_pulldown and
the new API together. That's why I don't think it is a big problem
that m_pulldown does not respect the "guaranteed contiguous area".

IIRC I converted all m_pullup/m_pulldown uses in IPv4/IPv6 and link
layer protocols in my SoC tree.

Pavel

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