Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 45638 Build 42526: 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.
| sys/sys/mbuf.h | ||
|---|---|---|
| 204 | This seems to be misaligned compared where it used to be based on the previous #defines... Should these two lines not also be part of a struct {}, or are these uses all mutually exclusive and the new assignment is ok? | |
| sys/sys/mbuf.h | ||
|---|---|---|
| 204 | Richard, just pushed newer version before reading your comment. Now can't understand to what lines you refer. Sorry. Can you please explain with updated patch? | |
To pencil down my remarks for posterity: I like the idea of having gdb access to the field names properly, with the union/structs, rather than the #defines. However, I don't want to claim that I fully comprehend the details of bz's argument around kernel/user space and common headers, and what may break if these changes are committed. I would really like to verbally hear bz reasoning to learn the background of the issues raised.
| sys/sys/mbuf.h | ||
|---|---|---|
| 204 | Your new revision fixed my concern. Previously, "lro_nsegs" mapped to sixteen[0] instead of sixteen[1], which the new struct {tso_segsz ; lro_nsegs} addresses. Thx. | |
Ignoring user space for the moment. I think the two concerns I have is that
(1) mbuf.h will eventually no longer be generic but become protocol agnostic/dependent unless we will force only basic types here. It's a path that seems to have started a while ago.
(2) even with (1) it'll mean that every time someone adds a new use case to these unions we will touch mbuf.h.
I can already foresee the M_PROTO* cases no longer being re-defined but named and all put in here too even though mbuf.h was agnostic to all kinds of ULPs and tunnel devices and ... we'll simply lose that.
Should sys/dev/cxgbe/t4_sge.c, sys/dev/rtwn/usb/rtwn_usb_rx.c, sys/netinet/igmp.c, sys/netinet6/mld6.c, sys/netinet/ip_reass.c, LinuxKPI, .. use cases all be put into mbuf.h and the next driver and the next driver and the next protocol change, and ... ?
Bjoern @bz , me and TCP folks are willing to go forward with this change. Is that possible for you to discuss that on the bi-weekly TCP call if you have strong objections?
Last week I spent a fair amount of time due to PH_per aliasing and no single point of documentation of that aliasing. I'm going to forward and commit updated version of this revision.