question about mbuf intialization

[Beverly Schwartz]

In function m_get, all fields in struct m_hdr are initialized except mh_paddr (m_paddr) and mh_len (m_len). m_paddr is initialized in a constructor provided to the pool, so that also gets initialized.

In function m_gethdr, all fields in struct pkthdr are initialized expcept segsz and len. segsz is there for TCP, so cannot be initialized in m_gethdr.

I am wondering why m_len and m_pkthdr.len are not initialized.

The man page for mbuf does not indicate that the user needs to set these fields after calling m_get or m_gethdr. (The macros calling these functions don't set the length fields either.)

As the NetBSD kernel stands now, all protocol use of mbuf's takes care of setting the length. But who is to say there may not be a protocol in the future that depends on the lengths being initialized to 0?

The functions m_copydata and m_copyback rely on these fields. If a protocol decides to use these functions to load data rather than using mtod and writing directly to the mbuf data bytes, without initialization, these functions will have unpredictable behavior.

I am using rump to test some kernel code which heavily relies on mbuf's. In that environment, I am calling mbuf functions to get mbufs and manually filling the mbufs using m_copyback. I can initialize m_len and m_pkthdr.len to 0, but this is a pain, and I have to remember to do this for each mbuf I get.

Any reason why we can't add
m->m_len = 0;
to m_get, and
m->m_pkthdr.len = 0;
to m_gethdr?

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

[Matt Thomas]

Post by Beverly Schwartz
Any reason why we can't add
m->m_len = 0;
to m_get, and
m->m_pkthdr.len = 0;
to m_gethdr?

Why not m->m_len = MLEN or MHLEN and m->m_pkthdr.len = MHLEN?

That way you know how much space is left.


But in reality, it's expected the user knows that the length
fields and/or m_data need to be adjusted.


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

[Beverly Schwartz]

Post by Matt Thomas

Post by Beverly Schwartz
Any reason why we can't add
m->m_len = 0;
to m_get, and
m->m_pkthdr.len = 0;
to m_gethdr?

Why not m->m_len = MLEN or MHLEN and m->m_pkthdr.len = MHLEN?
That way you know how much space is left.

m_len and m_pkthdr.len report on the data that is there, not on how much space exists. At initialiation, there is no data, so should be initialized to 0.

Post by Matt Thomas
But in reality, it's expected the user knows that the length
fields and/or m_data need to be adjusted.

As long as functions like m_copyback exist which provide a different way to load up data (and these function *do* update m_len and pkthdr.len), it seems that we just add a burden to the user that doesn't have to be there. At the very least, the mbuf man page should indicate that these fields need to be initialized by the user.

Is there a downside to providing the initialization? If not, why not do it? It reduces the chances of error because some user doesn't realize these fields are not initialized.

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

[Manuel Bouyer]

Post by Beverly Schwartz
[...]
Is there a downside to providing the initialization?

The one I can think it performance. You'll write these 2 fields to 0, only
to write something not long after.

[Iain Hibbert]

Post by Beverly Schwartz

Post by Matt Thomas

Post by Beverly Schwartz
Any reason why we can't add
m->m_len = 0;
to m_get, and
m->m_pkthdr.len = 0;
to m_gethdr?

Why not m->m_len = MLEN or MHLEN and m->m_pkthdr.len = MHLEN?
That way you know how much space is left.

m_len and m_pkthdr.len report on the data that is there, not on how much
space exists. At initialiation, there is no data, so should be
initialized to 0.

Weeell, it sometimes is the space rather than the data. for example,
m_copyback() will copy data into the space that is there and then possibly
extend the chain. if you give it a mbuf with zero data lengths, and ask to
copy more data? it just adds another mbuf which contains all the data..

Post by Beverly Schwartz

Post by Matt Thomas
But in reality, it's expected the user knows that the length
fields and/or m_data need to be adjusted.

this. it is difficult for the library routines to know how much space is
available (the data buffer can be external), so the caller is mostly
responsible, I think.

Post by Beverly Schwartz
As long as functions like m_copyback exist which provide a different way
to load up data (and these function *do* update m_len and pkthdr.len),
it seems that we just add a burden to the user that doesn't have to be
there. At the very least, the mbuf man page should indicate that these
fields need to be initialized by the user.
Is there a downside to providing the initialization? If not, why not do
it? It reduces the chances of error because some user doesn't realize
these fields are not initialized.

I suspect it dates from the time when lazy initialization really made a
difference to performance. perhaps on modern processors, this is not as
much of an issue (though NetBSD still runs on VAX and amiga at least..
those are not necessarily fast machines)

the documentation could probably be improved, of course..

regards,
iain

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

[Beverly Schwartz]

Post by Iain Hibbert

Post by Beverly Schwartz

Post by Matt Thomas

Post by Beverly Schwartz
Any reason why we can't add
m->m_len = 0;
to m_get, and
m->m_pkthdr.len = 0;
to m_gethdr?

Why not m->m_len = MLEN or MHLEN and m->m_pkthdr.len = MHLEN?
That way you know how much space is left.

m_len and m_pkthdr.len report on the data that is there, not on how much
space exists. At initialiation, there is no data, so should be
initialized to 0.

Weeell, it sometimes is the space rather than the data. for example,
m_copyback() will copy data into the space that is there and then possibly
extend the chain. if you give it a mbuf with zero data lengths, and ask to
copy more data? it just adds another mbuf which contains all the data..

When m_copyback extends the chain, it sets m_len in the new mbuf's to the amount of data in the mbuf. m_len is *only* used to indicate how much data is within the mbuf itself. m_pkthdr.len is also adjusted by m_copyback if it has to add to the chain.

-Bev


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

[Christos Zoulas]

Post by Beverly Schwartz
In function m_get, all fields in struct m_hdr are initialized except
mh_paddr (m_paddr) and mh_len (m_len). m_paddr is initialized in a
constructor provided to the pool, so that also gets initialized.
In function m_gethdr, all fields in struct pkthdr are initialized
expcept segsz and len. segsz is there for TCP, so cannot be initialized
in m_gethdr.
I am wondering why m_len and m_pkthdr.len are not initialized.
The man page for mbuf does not indicate that the user needs to set these
fields after calling m_get or m_gethdr. (The macros calling these
functions don't set the length fields either.)
As the NetBSD kernel stands now, all protocol use of mbuf's takes care
of setting the length. But who is to say there may not be a protocol in
the future that depends on the lengths being initialized to 0?
The functions m_copydata and m_copyback rely on these fields. If a
protocol decides to use these functions to load data rather than using
mtod and writing directly to the mbuf data bytes, without
initialization, these functions will have unpredictable behavior.
I am using rump to test some kernel code which heavily relies on mbuf's.
In that environment, I am calling mbuf functions to get mbufs and
manually filling the mbufs using m_copyback. I can initialize m_len and
m_pkthdr.len to 0, but this is a pain, and I have to remember to do this
for each mbuf I get.
Any reason why we can't add
m->m_len = 0;
to m_get, and
m->m_pkthdr.len = 0;
to m_gethdr?

Makes sense, but at the same time we should remove the superfluous zeroing
from the other places...

christos


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

[Beverly Schwartz]

Post by Christos Zoulas

Post by Beverly Schwartz
Any reason why we can't add
m->m_len = 0;
to m_get, and
m->m_pkthdr.len = 0;
to m_gethdr?

Makes sense, but at the same time we should remove the superfluous zeroing
from the other places...

A quick scan of the code shows that some of the time m_len is set to 0, but more of the time, the library using it just sets it to its ultimate value.

There are places in uipc_mbuf.c that immediately set m_len to 0 after getting an mbuf, and I think it's fair to remove those.

It would take a commitment of time to go through all architectures, devices and protocols to find every instance where there is a 0 initialization that needs to be removed. It's not a commitment of time I am willing to make.

So I guess my question is, would the list approve of a merge upstream of changes to uipc_mbuf.c that include the two intializations listed above, and removal of superflous initializations in just uipc_mbuf.c?

If the answer is yes, would someone who has authority to make changes to the source be willing to put in the change? I normally have gdt do this, but he is not available to do a checkin at this time.

-Bev



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

[Christos Zoulas]

On Sep 20, 12:59pm, ***@bbn.com (Beverly Schwartz) wrote:
-- Subject: Re: question about mbuf intialization

|
| On Sep 19, 2013, at 4:21 PM, Christos Zoulas <***@astron.com> =
| wrote:
|
| > In article <546B8CEC-0675-463F-B5C8-***@bbn.com>,
| > Beverly Schwartz <***@bbn.com> wrote:
| >>=20
| >> Any reason why we can't add
| >> m->m_len =3D 0;
| >> to m_get, and
| >> m->m_pkthdr.len =3D 0;
| >> to m_gethdr?
| >=20
| > Makes sense, but at the same time we should remove the superfluous =
| zeroing
| > from the other places...
|
| A quick scan of the code shows that some of the time m_len is set to 0, =
| but more of the time, the library using it just sets it to its ultimate =
| value.
|
| There are places in uipc_mbuf.c that immediately set m_len to 0 after =
| getting an mbuf, and I think it's fair to remove those.

Those are the ones I saw, how many more are there?

| It would take a commitment of time to go through all architectures, =
| devices and protocols to find every instance where there is a 0 =
| initialization that needs to be removed. It's not a commitment of time =
| I am willing to make.

I'll check it out.

| So I guess my question is, would the list approve of a merge upstream of =
| changes to uipc_mbuf.c that include the two intializations listed above, =
| and removal of superflous initializations in just uipc_mbuf.c?

I think that the change is good.

| If the answer is yes, would someone who has authority to make changes to =
| the source be willing to put in the change? I normally have gdt do =
| this, but he is not available to do a checkin at this time.

I would wait for more people to chime in.

christos

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

[Iain Hibbert]

Post by Beverly Schwartz

Post by Christos Zoulas

Post by Beverly Schwartz
Any reason why we can't add
m->m_len = 0;
to m_get, and
m->m_pkthdr.len = 0;
to m_gethdr?

Makes sense, but at the same time we should remove the superfluous zeroing
from the other places...

A quick scan of the code shows that some of the time m_len is set to 0,
but more of the time, the library using it just sets it to its ultimate
value.

why does it seem useful to always set it to zero, if more often than not
it will be immediately set to another value..?

iain

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

[Beverly Schwartz]

Post by Iain Hibbert

Post by Beverly Schwartz

Post by Iain Hibbert

Post by Beverly Schwartz

Post by Christos Zoulas

Post by Beverly Schwartz
Any reason why we can't add
m->m_len = 0;
to m_get, and
m->m_pkthdr.len = 0;
to m_gethdr?

Makes sense, but at the same time we should remove the superfluous zeroing
from the other places...

A quick scan of the code shows that some of the time m_len is set to 0,
but more of the time, the library using it just sets it to its ultimate
value.

why does it seem useful to always set it to zero, if more often than not
it will be immediately set to another value..?

- efficiency,
- robustness,
- maintainability
I'm concerned primarily about maintainability, with a second concern with robustness.
Not initializing one particular field in a structure when all others are
initialized is error-prone, especially since the mbuf man page doesn't
say anything about the user needing to initialize them. In addition,
there are functions in mbuf that are completely reasonable to use such
as m_copyback and m_adj that depend on m_len being set.

The manpage says that "internal data" is initialized by the allocation
routines, but it doesn't say what that is. That could be improved no
doubt. The manpage is pretty minimal anyway, which makes a steep learning
curve, and I surely recall that I had to look at the header files and/or
source a lot when I wrote the Bluetooth code, but I personally do not feel
that it is at all onerous to set the length of the data area when that is
loaded.
I would expect that if code exists that does not initialize the length
value, then it has not been tested and will not ever work. The mbufs are
likely to have been used before and *will* contain random values. (I don't
know how rump deals with allocations like this, if the data would be
zeroed by default?)

Rump uses the actual kernel code, therefore, just like the kernel,
m_len will contain what every value was last there, or some random
scribble from memory. Writing a rump unit test, there's a lot of
grabbing mbufs and trying out different things. I can set m_len and
m_pkthdr.len, but it's a nuisance to remember to have to do it with
each allocation of an mbuf.

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

[Darren Reed]

Post by Beverly Schwartz
...
Rump uses the actual kernel code, therefore, just like the kernel,
m_len will contain what every value was last there, or some random
scribble from memory. Writing a rump unit test, there's a lot of
grabbing mbufs and trying out different things. I can set m_len and
m_pkthdr.len, but it's a nuisance to remember to have to do it with
each allocation of an mbuf. -Bev

As has been mentioned earlier in this thread, setting
m_len & co to 0 doesnot make the mbuf "work". They
need to be "non-zero" and set to the sizeof data to
be sent. Or in other words, you need to set these fields
correctlyor nothing will work. Having themintialised
to 0 will not help make anything work.

It may be a better test for whatever allocates the memory
to be used by the mbuf to fill it with 0xff or 0xa55a or
some other pattern like 0xdeadbeef/0xfeedface, etc, so
that it is obvious uninitialised memory is being used.
Given that it is the responsibility of whatever is building
the packet to send to set m_len & co correctly, having
them initialised to 0 should mean that code paths that
don't set them are more easily found.

Darren


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

[Iain Hibbert]

Post by Beverly Schwartz

Post by Christos Zoulas

Post by Beverly Schwartz
Finally, using rump as a way to test kernel code in user space is really
powerful. I did a lot of exercising of mbuf code, and initially, I did
a lot of crashing and burning because I didn't realize that m_len wasn't
initialized. This became a pain to deal with - everytime I got an mbuf
I had to remember to initialize m_len and m_pkthdr.len. So I just went
and changed the source code, and this discussion has resulted.

I've been looking at the code and it seems to me that not only m_get()
needs to be changed, but also m_getcl() and MCLGET().

m_getcl (which calls MCLGET) is called with an mbuf in hand. These
just add the external buffer and does all apropriate initializations.
They do not add data, so m_len and m_pkthdr.len do not need to be
updated.

Post by Christos Zoulas
In essence, none
of the allocators currently set m_len or m_pkthdr.len, and they leave
it to the packet formation routines to do so.

Correct, and the two functions that need to change are m_get and m_gethdr.

...except of course, that they do not add any data either.

Post by Beverly Schwartz

Post by Christos Zoulas
I am just mentioning that if we do that we should fix it consistently
(if anyone can call the mbuf API consistent...)

I think it is probably inconsistent, and full of grumble.

Post by Beverly Schwartz
I think I got it right, within uipc_mbuf.c itself. I did not
go through the rest of the code base to remove every other
initialization to zero.

I personally feel that changing the underlying behaviour of a legacy API
such as this without auditing the code that uses it is the wrong thing to
do. How can this little change possibly be a problem you ask? Well I
don't know, but historically there has been abuse of mbufs (using it for
non-packet data for example) and I am not proposing to change it.

Probably the correct thing to do is to modify the documentation to make it
clear that the length field is external (note that FreeBSD and OpenBSD
have both done this), which also leaves us compatible with other mbuf
implementations.

regards,
iain

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

[Darren Reed]

Post by Iain Hibbert

Post by Beverly Schwartz

Post by Christos Zoulas

Post by Beverly Schwartz
Finally, using rump as a way to test kernel code in user space is really
powerful. I did a lot of exercising of mbuf code, and initially, I did
a lot of crashing and burning because I didn't realize that m_len wasn't
initialized. This became a pain to deal with - everytime I got an mbuf
I had to remember to initialize m_len and m_pkthdr.len. So I just went
and changed the source code, and this discussion has resulted.

I've been looking at the code and it seems to me that not only m_get()
needs to be changed, but also m_getcl() and MCLGET().

m_getcl (which calls MCLGET) is called with an mbuf in hand. These
just add the external buffer and does all apropriate initializations.
They do not add data, so m_len and m_pkthdr.len do not need to be
updated.

Post by Christos Zoulas
In essence, none
of the allocators currently set m_len or m_pkthdr.len, and they leave
it to the packet formation routines to do so.

Correct, and the two functions that need to change are m_get and m_gethdr.

...except of course, that they do not add any data either.

Post by Beverly Schwartz

Post by Christos Zoulas
I am just mentioning that if we do that we should fix it consistently
(if anyone can call the mbuf API consistent...)

I think it is probably inconsistent, and full of grumble.

Post by Beverly Schwartz
I think I got it right, within uipc_mbuf.c itself. I did not
go through the rest of the code base to remove every other
initialization to zero.

I personally feel that changing the underlying behaviour of a legacy API
such as this without auditing the code that uses it is the wrong thing to
do. How can this little change possibly be a problem you ask? Well I
don't know, but historically there has been abuse of mbufs (using it for
non-packet data for example) and I am not proposing to change it.
Probably the correct thing to do is to modify the documentation to make it
clear that the length field is external (note that FreeBSD and OpenBSD
have both done this), which also leaves us compatible with other mbuf
implementations.

+1.

Darren


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

[Beverly Schwartz]

Post by Iain Hibbert

Post by Beverly Schwartz

Post by Christos Zoulas

Post by Beverly Schwartz
Any reason why we can't add
m->m_len = 0;
to m_get, and
m->m_pkthdr.len = 0;
to m_gethdr?

Makes sense, but at the same time we should remove the superfluous zeroing
from the other places...

A quick scan of the code shows that some of the time m_len is set to 0,
but more of the time, the library using it just sets it to its ultimate
value.

why does it seem useful to always set it to zero, if more often than not
it will be immediately set to another value..?

There are various goals with software development:
- efficiency,
- robustness,
- maintainability

I'm concerned primarily about maintainability, with a second concern with robustness.

Not initializing one particular field in a structure when all others are initialized is error-prone, especially since the mbuf man page doesn't say anything about the user needing to initialize them. In addition, there are functions in mbuf that are completely reasonable to use such as m_copyback and m_adj that depend on m_len being set.

For anyone developing new code, they may do it the "old" way where they maintain the mbuf lists and figure out if they need extra storage and set lengths accordingly, or they may rely on m_copyback. So while this isn't a big issue for code that exists, it's a headache for future development.

I didn't painstakingly go through every architecture and device driver, but I would be willing to be that at least one is buggy because of lack of initialization of m_len and/or m_pkthdr.len.

Finally, using rump as a way to test kernel code in user space is really powerful. I did a lot of exercising of mbuf code, and initially, I did a lot of crashing and burning because I didn't realize that m_len wasn't initialized. This became a pain to deal with - everytime I got an mbuf I had to remember to initialize m_len and m_pkthdr.len. So I just went and changed the source code, and this discussion has resulted.

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

[Darren Reed]

So it also sounds like there is a man page bug in that the man
page for mbuf does not fully describe what needs to be done
to use them successfully, yes?

Using functions such as m_copyback and m_copydata should
be limited. Moving data around in the networking code path
is generally something people try not to do - and as a general
meme it is a good programming practice. Thus code more
often will do an m_adj or similar. When trying to get full
speed with 10GE, you don't want to initialise anything more
than you have to.

Darren


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

[Iain Hibbert]

Post by Beverly Schwartz

Post by Iain Hibbert

Post by Beverly Schwartz

Post by Christos Zoulas

Post by Beverly Schwartz
Any reason why we can't add
m->m_len = 0;
to m_get, and
m->m_pkthdr.len = 0;
to m_gethdr?

Makes sense, but at the same time we should remove the superfluous zeroing
from the other places...

A quick scan of the code shows that some of the time m_len is set to 0,
but more of the time, the library using it just sets it to its ultimate
value.

why does it seem useful to always set it to zero, if more often than not
it will be immediately set to another value..?

- efficiency,
- robustness,
- maintainability
I'm concerned primarily about maintainability, with a second concern with robustness.
Not initializing one particular field in a structure when all others are
initialized is error-prone, especially since the mbuf man page doesn't
say anything about the user needing to initialize them. In addition,
there are functions in mbuf that are completely reasonable to use such
as m_copyback and m_adj that depend on m_len being set.

The manpage says that "internal data" is initialized by the allocation
routines, but it doesn't say what that is. That could be improved no
doubt. The manpage is pretty minimal anyway, which makes a steep learning
curve, and I surely recall that I had to look at the header files and/or
source a lot when I wrote the Bluetooth code, but I personally do not feel
that it is at all onerous to set the length of the data area when that is
loaded.

I would expect that if code exists that does not initialize the length
value, then it has not been tested and will not ever work. The mbufs are
likely to have been used before and *will* contain random values. (I don't
know how rump deals with allocations like this, if the data would be
zeroed by default?)

regards,
iain

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

[Christos Zoulas]

Post by Beverly Schwartz
Finally, using rump as a way to test kernel code in user space is really
powerful. I did a lot of exercising of mbuf code, and initially, I did
a lot of crashing and burning because I didn't realize that m_len wasn't
initialized. This became a pain to deal with - everytime I got an mbuf
I had to remember to initialize m_len and m_pkthdr.len. So I just went
and changed the source code, and this discussion has resulted.

I've been looking at the code and it seems to me that not only m_get()
needs to be changed, but also m_getcl() and MCLGET(). In essence, none
of the allocators currently set m_len or m_pkthdr.len, and they leave
it to the packet formation routines to do so. I am not against setting
the length to 0 by default on construction to make things more robust
(I don't think it makes any performance difference these days); I am
just mentioning that if we do that we should fix it consistently (if
anyone can call the mbuf API consistent...)

christos


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

[Beverly Schwartz]

Post by Christos Zoulas

Post by Beverly Schwartz
Finally, using rump as a way to test kernel code in user space is really
powerful. I did a lot of exercising of mbuf code, and initially, I did
a lot of crashing and burning because I didn't realize that m_len wasn't
initialized. This became a pain to deal with - everytime I got an mbuf
I had to remember to initialize m_len and m_pkthdr.len. So I just went
and changed the source code, and this discussion has resulted.

I've been looking at the code and it seems to me that not only m_get()
needs to be changed, but also m_getcl() and MCLGET().

m_getcl (which calls MCLGET) is called with an mbuf in hand. These
just add the external buffer and does all apropriate initializations.
They do not add data, so m_len and m_pkthdr.len do not need to be
updated.

Post by Christos Zoulas
In essence, none
of the allocators currently set m_len or m_pkthdr.len, and they leave
it to the packet formation routines to do so.

Correct, and the two functions that need to change are m_get and m_gethdr.

Post by Christos Zoulas
I am not against setting
the length to 0 by default on construction to make things more robust

Thank-you.

Post by Christos Zoulas
(I don't think it makes any performance difference these days);

Agreed. Nobody objected to the previous change that set m_type to
MT_FREE for robustness sake. This is almost the same performance
hit as that. For non-pkthdr mbufs, it is the same - one field set
per mbuf. For those mbufs which have a pkthdr, then it's two
fields set per mbuf.

Post by Christos Zoulas
I am
just mentioning that if we do that we should fix it consistently (if
anyone can call the mbuf API consistent...)

I think I got it right, within uipc_mbuf.c itself. I did not
go through the rest of the code base to remove every other
initialization to zero.

Here is my diff:

Author: Bev Schwartz <***@bbn.com>
Date: Mon Sep 16 18:41:48 2013 -0400

initialize mbuf lens

Whereas all other mbuf hdr fields are initialized in m_get,
m_len is not. Iniitialize to 0.

Likewise, all pkthdr fields are initialized in m_gethdr,
but not len. Initialize to 0.

Remove superfluous m_len = 0 statements now that mbufs are
initialized

diff --git a/netbsd/src/sys/kern/uipc_mbuf.c b/netbsd/src/sys/kern/uipc_mbuf.c
index 542347a..57ebef1 100644
--- a/netbsd/src/sys/kern/uipc_mbuf.c
+++ b/netbsd/src/sys/kern/uipc_mbuf.c
@@ -505,6 +505,7 @@ m_get(int nowait, int type)
mowner_init(m, type);
m->m_ext_ref = m;
m->m_type = type;
+ m->m_len = 0;
m->m_next = NULL;
m->m_nextpkt = NULL;
m->m_data = m->m_dat;
@@ -525,6 +526,7 @@ m_gethdr(int nowait, int type)
m->m_data = m->m_pktdat;
m->m_flags = M_PKTHDR;
m->m_pkthdr.rcvif = NULL;
+ m->m_pkthdr.len = 0;
m->m_pkthdr.csum_flags = 0;
m->m_pkthdr.csum_data = 0;
SLIST_INIT(&m->m_pkthdr.tags);
@@ -692,7 +694,6 @@ m_copym0(struct mbuf *m, int off0, int len, int wait, int deep)
* copy into multiple MCLBYTES cluster mbufs.
*/
MCLGET(n, wait);
- n->m_len = 0;
n->m_len = M_TRAILINGSPACE(n);
n->m_len = min(n->m_len, len);
n->m_len = min(n->m_len, m->m_len - off);
@@ -942,7 +943,6 @@ m_ensure_contig(struct mbuf **m0, int len)
return false;
}
MCLAIM(m, n->m_owner);
- m->m_len = 0;
if (n->m_flags & M_PKTHDR) {
M_MOVE_PKTHDR(m, n);
}
@@ -1006,7 +1006,6 @@ m_copyup(struct mbuf *n, int len, int dstoff)
if (m == NULL)
goto bad;
MCLAIM(m, n->m_owner);
- m->m_len = 0;
if (n->m_flags & M_PKTHDR) {
M_MOVE_PKTHDR(m, n);
}
@@ -1387,7 +1386,6 @@ extend:
if (n == NULL) {
goto out;
}
- n->m_len = 0;
n->m_len = min(M_TRAILINGSPACE(n), off + len);
memset(mtod(n, char *), 0, min(n->m_len, off));
m->m_next = n;



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

[Darren Reed]

Post by Christos Zoulas
I've been looking at the code and it seems to me that not only m_get()
needs to be changed, but also m_getcl() and MCLGET(). In essence, none
of the allocators currently set m_len or m_pkthdr.len, and they leave
it to the packet formation routines to do so. I am not against setting
the length to 0 by default on construction to make things more robust
(I don't think it makes any performance difference these days); I am
just mentioning that if we do that we should fix it consistently (if
anyone can call the mbuf API consistent...) christos

The correct argument to me would appear to be that since the mbufs
contain no data in them when returned by m_get() that the length
fields should read as "0" in order to reflect that (if they don't already.)

Darren


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

[Christos Zoulas]

Post by Darren Reed

Post by Christos Zoulas
I've been looking at the code and it seems to me that not only m_get()
needs to be changed, but also m_getcl() and MCLGET(). In essence, none
of the allocators currently set m_len or m_pkthdr.len, and they leave
it to the packet formation routines to do so. I am not against setting
the length to 0 by default on construction to make things more robust
(I don't think it makes any performance difference these days); I am
just mentioning that if we do that we should fix it consistently (if
anyone can call the mbuf API consistent...) christos

The correct argument to me would appear to be that since the mbufs
contain no data in them when returned by m_get() that the length
fields should read as "0" in order to reflect that (if they don't already.)

I agree and this is what Beverly is proposing. The latest unix
socket bug of returning random trash in the sockaddr of accept call
from a closed unix socket that has issued a connect proves just
that: mbufs that have just been created should contain no data
(m_len == 0); otherwise they will end up containing junk from their
previous pool use and this can lead to random bugs. I am planning
to initialize m_len and m_pkthdr.len to zero in the constructors
in the next couple of days and move the extra assignments unless
I hear otherwise. We already initialize most other fields in the
mbuf, so the performance loss claims don't really make sense.

christos


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