Page MenuHomeFreeBSD

mbuf: unionize globally declared uses of PH_per and PH_loc
Needs ReviewPublic

Authored by glebius on Fri, May 13, 8:34 PM.

Details

Reviewers
None
Group Reviewers
network
transport

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 45569
Build 42457: arc lint + arc unit

Event Timeline

I added transport to clarify the following question. Before my change we got

#define tcp_tun_port    PH_per.sixteen[0] /* outbound */
#define tso_segsz       PH_per.sixteen[1] /* inbound after LRO */
#define lro_nsegs       tso_segsz         /* inbound after LRO */

lro_nsegs and tso_segsz should be two very different things . Why do they have the same comment? Why lro_nsegs isn't declared as PH_per.sixteen[1] directly? My guess is that tso_segsz is actually an outbound feature, hence it is stored separate from tcp_tun_port, which is also outbound feature. lro_nsegs is inbound feature, so can be safely moved into sixteen[0]. This is what my patch does.

I still do not like the idea that layer specifics are inside mbuf structs; I have not committed the @rrs approved https://reviews.freebsd.org/D34298 for reasons that I thought it'll need more discussions.

mbufs are used in user space and elsewhere and while generic types are fine and make it explicit what all these unions are used for, you do have generic layer-specific storage you are using; why does TCP LRO need to be more special than other networking code? Changing mbuf for every (implicit) field there, you could remove the PH_loc entirely but it'll mess with mbufs constantly which I think is a bad idea.

I am just saying what I am thinking; but I'll neither approve (nor reject) a change like this and I highly think that this needs a wider discussion which won't really happen here I fear.

I tend to remove as much preprocessor as possible in favor of the modern C. And I would like to move all other network protocols into this declaration, too. So that there will be a single place (properly commented) where you can see all the the possible uses and see if there are any conflicts. Yes, ideally to get rid of all these sixteen's and thirtytwo's. But I'm afraid we should leave them for external modules.

For the mbufs in the userspace the problem can be easily masked with #ifdef _KERNEL.

Actually I don't think any user space would have problems. Potentially we have only generic types and pointers to forward declared structs.