patch: sockaddr instead of mbuf to carry addresses

[Tyler Retzlaff]

hi,

attached is the first of a set of patches intended to remove the use of
mbuf as a bucket to carry sockaddr structures for per-user requests.
this patch introduces the structure required and changes bind.

i've been running it myself, the atf tests work with it and there has
been some private review prior to posting.

comments welcome

[Greg Troxel]

Not objecting, but can you articulate the gain?

Is this really only about carrying them around on the call stack, or
does it extend to storage? Do you see a new pool_cache object at some
point?

What is the long-term plan (for the set of changes you are currently
envisioning)? I would like to see that written down before we start.

Will this extend to drivers eventually? If so, what's the pro/con of
that gain vs the added difficulty of porting drivers/changes from the
other BSDs?

[Tyler Retzlaff]

Hi Greg,

Post by Greg Troxel
Not objecting, but can you articulate the gain?

Maintainability of the code with the intention that it makes simpler
future improvements to the stack in general.

* No management of allocation / de-allocation. Realized when the change
is complete for all per-user requests.
* Use of static types would catch particular classes of errors at
compile time instead of run-time. i.e. why use void * when you can use
struct foo.
* It is a continuation of work that was cut-short before the -7 branch
where per-user requests were split from the monolithic proto switch.
When everything was handled by the switch mbuf pointers were used to get
around the fact that arguments were of different types depending on the
user-request. With the requests split this no longer has to be done.

Post by Greg Troxel
Is this really only about carrying them around on the call stack, or
does it extend to storage? Do you see a new pool_cache object at some
point?

Benefits as above, I do not (at this time) envision a new pool_cache object.

Post by Greg Troxel
What is the long-term plan (for the set of changes you are currently
envisioning)? I would like to see that written down before we start.

Obviously, the first set of changes is to convert all uses of mbuf to
use sockaddr. While not dependent on this change one potential piece of
work that may follow is the merging of the inet and inet6 pcb which
would remove large amounts of redundant / duplicated #ifdef'd code.

Post by Greg Troxel
Will this extend to drivers eventually? If so, what's the pro/con of
that gain vs the added difficulty of porting drivers/changes from the
other BSDs?

As far as I'm aware this change should not in any way leak into drivers,
as for changes from other BSDs our per-user request code is already
diverged as a result of the other projects making changes. For example
FreeBSD has already merged their inet/inet6 pcb structure and they
re-factored their per-user requests long ago.

tyler

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

[Mindaugas Rasiukevicius]

Post by Greg Troxel
Not objecting, but can you articulate the gain?

It is a part of wider objective to cleanup protocol user-request methods.
Historically, we had a single pr_usrreq method which was just a massive
switch statement with PRU_ constants. While such API was okay in 1980s,
it is quite far away from okay in today's standards.

So I made initial steps of splitting the pr_usrreq actions into their own
methods, defining a proper API between the socket layer and protocol layer
(something what FreeBSD and others did long time ago). Tyler volunteered
and did a great job to finish up the split.

The old pr_usrreq method abused struct mbuf and other parameters to carry
various different types. It is a massive change to fix everything in one
go, so usng *real* type of the parameters is a next step. AFAIK (although
I might be wrong) the reason why mbufs were abused to carry the parameters
was because back in the day they just had a faster memory allocator. Well,
today we prefer functions, type-safety, clear locking rules, symmetric and
just generally sane APIs. :)

[Greg Troxel]

Post by Mindaugas Rasiukevicius

Post by Greg Troxel
Not objecting, but can you articulate the gain?

It is a part of wider objective to cleanup protocol user-request methods.
Historically, we had a single pr_usrreq method which was just a massive
switch statement with PRU_ constants. While such API was okay in 1980s,
it is quite far away from okay in today's standards.
So I made initial steps of splitting the pr_usrreq actions into their own
methods, defining a proper API between the socket layer and protocol layer
(something what FreeBSD and others did long time ago). Tyler volunteered
and did a great job to finish up the split.
The old pr_usrreq method abused struct mbuf and other parameters to carry
various different types. It is a massive change to fix everything in one
go, so usng *real* type of the parameters is a next step. AFAIK (although
I might be wrong) the reason why mbufs were abused to carry the parameters
was because back in the day they just had a faster memory allocator. Well,
today we prefer functions, type-safety, clear locking rules, symmetric and
just generally sane APIs. :)

Thanks. That sounds totally reasonable.

[Mindaugas Rasiukevicius]

Post by Tyler Retzlaff
hi,
attached is the first of a set of patches intended to remove the use of
mbuf as a bucket to carry sockaddr structures for per-user requests.
this patch introduces the structure required and changes bind.
i've been running it myself, the atf tests work with it and there has
been some private review prior to posting.
comments welcome

Thanks for working on this.

Unfortunately, I have not had time for a careful review, but from a quick
check of the key parts it looks good. One issue: why struct sockaddr_big?
I think we can and should use struct sockaddr_storage for this. Also, is
there any reason why the address parameter could not be const?

[Christos Zoulas]

Post by Mindaugas Rasiukevicius

Post by Tyler Retzlaff
hi,
attached is the first of a set of patches intended to remove the use of
mbuf as a bucket to carry sockaddr structures for per-user requests.
this patch introduces the structure required and changes bind.
i've been running it myself, the atf tests work with it and there has
been some private review prior to posting.
comments welcome

Thanks for working on this.
Unfortunately, I have not had time for a careful review, but from a quick
check of the key parts it looks good. One issue: why struct sockaddr_big?
I think we can and should use struct sockaddr_storage for this. Also, is
there any reason why the address parameter could not be const?

I think so too; the only reason we are not using sockaddr_storage for it
is because we allow sockaddr_un to be up to 253 characters long as an
extension to the spec. I kind of wished that sockaddr_storage was defined
to be 256 bytes instead of 128...

christos


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

[Tyler Retzlaff]

hey,

hey,

Post by Mindaugas Rasiukevicius
Unfortunately, I have not had time for a careful review, but from a quick
check of the key parts it looks good. One issue: why struct sockaddr_big?

struct sockaddr_storage isn't "big" enough unfortunately. Specifically
we, Linux and others have allowed the creation of AF_UNIX sockets with
paths that exceed sizeof(sockaddr_un.sun_path) up to 253 bytes. At the
moment sockaddr_storage is only 128 bytes so thus it cannot accommodate.

Rather than using sockaddr_storage everywhere except for AF_UNIX sockets
I decided it would be better if a new structure that was large enough be
created for consistency across all socket types. The name was suggested
by mrg@ since he (rightfully) suggested it be suitably distinct from
sockaddr_storage to avoid confusion.

I think the new structure is the right way to go since I can't imagine
increasing the size of sockaddr_storage would be fun.

Post by Mindaugas Rasiukevicius
I think we can and should use struct sockaddr_storage for this. Also, is
there any reason why the address parameter could not be const?

I love const but various places the structure members are passed to
non-const things e.g. kauth_authorize_network, also the size of the
structure is manipulated (sb_len). There are probably other places, but
that's just an initial glance.

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

[Tyler Retzlaff]

hey,

Post by Christos Zoulas
I think so too; the only reason we are not using sockaddr_storage for it
is because we allow sockaddr_un to be up to 253 characters long as an
extension to the spec. I kind of wished that sockaddr_storage was defined
to be 256 bytes instead of 128...

I have no particular objection to doing it this way, I just thought
there would be objection to changing the size of struct sockaddr_storage
which is why I went with the new structure.

If it is better to increase the size of struct sockaddr_storage shoot me
a preferred updated structure definition and I'll go with that.

Post by Christos Zoulas
christos

Thanks for looking.

--
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 Christos Zoulas
I think so too; the only reason we are not using sockaddr_storage for it
is because we allow sockaddr_un to be up to 253 characters long as an
extension to the spec. I kind of wished that sockaddr_storage was defined
to be 256 bytes instead of 128...

Why do we limit sockaddr_un to less than PATH_MAX? There really aren't
many reasons for it to be shorter. But if we do go that way, using
sockaddr_storage becomes even less attractive. IMO sockaddr_storage
should be the union of all network facing addresses and the potentially
largest is gone with the netiso removal.

Joerg

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

[Tyler Retzlaff]

hey,

Post by Joerg Sonnenberger
Why do we limit sockaddr_un to less than PATH_MAX? There really aren't
many reasons for it to be shorter. But if we do go that way, using
sockaddr_storage becomes even less attractive.

The limit of 253 originates from the capacity of sockkaddr_un.sun_len
being uint8_t. i.e. the maximum size of the data and members of any
sockaddr structure when its bounds are described by the len member.

Post by Joerg Sonnenberger
IMO sockaddr_storage
should be the union of all network facing addresses and the potentially
largest is gone with the netiso removal.

I initially made a similar argument (off list) that sockaddr_un
could/should grow in size to encompass what it was already being used
for and sockaddr_storage would then have to be grown too by implication
(ABI difficulties aside).

I accepted a counter argument that if you passed data bigger than a
sockaddr_un structure with family of AF_UNIX then really your not
passing a sockaddr structure anymore. If you make that leap then
sockaddr_storage as defined now is capable of holding any address family
sockaddr structure as required by susv2.

It gets interpreted as "it holds any of those defined sockaddr
structures but this data isn't one" (which conveniently ignores the type
of the parameter sockaddr *).

As for my patch in the kernel it seemed to make sense
(consistency/simplicity) to use one structure for all even if from
user-space it was a little less clear.

Post by Joerg Sonnenberger
Joerg

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