On Wed, Jan 26, 2022 at 11:05:51AM +0100, Claudio Jeker wrote: > On Wed, Jan 26, 2022 at 01:29:42AM +0100, Alexander Bluhm wrote: > > Hi, > > > > There were some problems with ix(4) and ixl(4) hardware checksumming > > for the output path on strict alignment architectures. > > > > I have merged jan@'s diffs and added some sanity checks and > > workarounds. > > > > - If the first mbuf is not aligned or not contigous, use m_copydata() > > to extract the IP, IPv6, TCP header. > > - If the header is in the first mbuf, use m_data for the fast path. > > - Add netstat counter for invalid header chains. This makes > > us aware when hardware checksumming fails. > > - Add netstat counter for header copies. This indicates that > > better storage allocation in the network stack is possible. > > It also allows to recognize alignment problems on non-strict > > architectures. > > - There is not risk of crashes on sparc64. > > > > Does this aproach make sense? > > I think it is overly complicated and too much data is copied around. > First of all there is no need to extract ipproto. > The code can just use the M_TCP_CSUM_OUT and M_UDP_CSUM_OUT flags (they > are not set for other protos). > Because of this only they ip_hlen needs to be accessed and this can be > done with m_getptr().
A solution with m_getptr() is where we started. It has already been rejected. The problem is that there are 6 ways to implement this feature. Every option has its drawbacks and was rejected. Options are: 1. m_getptr() and access the struct. Alignment cannot be guaranteed. 2. m_getptr() and access the byte or word. Header fields should be accessed by structs. 3. Always m_copydata. Too much overhead. 4. Always use m_data. Kernel may crash or use invalid data. 5. Combination of m_data and m_copydata. Too complex. 6. Track the fields in mbuf header. Too fragile and memory overhead. In my measurements checksum offloading gave us 10% performance boost so I want this feature. Other drivers also have it. Could we get another idea or find a consensus which option to use? > In the IP6 case even more can be skipped since ip_hlen is static for IPv6. > > In ixl(4) also the tcp header lenght needs to be extracted. Again the code > can be simplified because HW checksumming is only enabled if ip_hlen == 5 > and so the offset of the th_off field is static (for both IPv4 and IPv6). > Again m_getptr can be used to just access the byte with th_off. > > Longterm in_proto_cksum_out() should probably help provide the th_off > field. I think enforcing ip_hlen == 5 for UDP and TCP is fine, who needs > IP options on UDP and TCP? Other diffs have been rejected as they make too many assumtions how the stack works. bluhm
