On 28.05.2019 03:45, Robert Elz wrote: > Date: Mon, 27 May 2019 21:36:28 +0200 > From: Kamil Rytarowski <n54%gmx.com@localhost> > Message-ID: <882a832c-b37f-0e12-294f-9e46c145dbde%gmx.com@localhost> > > | http://netbsd.org/~kamil/patch-00115-tcp_input.2.txt > > With 2 (minor) caveats this one looks fine to me. > > The first (and lesser) is that the name get_unaligned32() is > ugly -- and assumes the data is unaligned which in practice it > almost never is - senders take care to align the data for any > rational receiver (and our implementation is rational in that way). > > The original (current) code uses > > *(u_int32_t *)optp > > so how about making the replacement be > > uint32(optp) > > which is less chars, ties the function name to the type name, > and doesn't imply that the data is unaligned. (deleting the _ > between u and int is just the more modern style, right? if not > put it back...) > > It might even be worth adding static inline functions like that > (and uint16() and the signed variants perhaps) to one of the header > files, do they're available throughout the kernel. > > > And second, before doing the change, it would be nice to work > out which implementation of fetching a 32 bit word from an > arbitrary pointer gcc optimises away best in two cases: > > 1. where the processor does not have any alignment constraints > 2. where the data bytes are already aligned > > (the 2nd of those might be irrelevant, the cost to find out whether > the pointer is aligned probably exceeds any benefit if it is). > > That is, where one implementation is the one you give, and the second > is the old stile (endian dependant ... though here since the data is > being compared to htonl() of a const, we can assume big endian - ie: > network byte order, and don't need an #ifdef to do it little endian > instead (I think, needs checking... perhaps the htonl() also would need > to go away) > > uint8_t *cp = p; > > return (p[0] << 24) | (p[1] << 16) | (p[2] << 8) | p[3]; > > Then use whichever implementation gcc is able to optimise into the > same code it currently generates on processors that have no alignment > constraints. > uint32() is a bad function name as it is a type defined in multiple places in the kernel. I would pick the name get_uint32(). I don't like this manual concatenation of bytes and this was rejected previously as prone to timing issues while memcpy() does the right thing. My previous approach of providing unaligned_get() [1] died as we can and should just pick memcpy() to get equivalent functionality. [1] http://netbsd.org/~kamil/kubsan/unaligned.h However the question is whether kUBSan caught a real bug here that something passed data that should be aligned? I don't have enough expertise in the networking stack to say anything on this. > | http://netbsd.org/~kamil/patch-00117-netinet6.txt > > It is hard ti imagine an implementation where this would make a > difference, but if that's required to meet the letter of the C > standard, then go ahead ... The IPv6 addr struct is designed to > always be packed with any even half rational compiler. Saying so > can't hurt anything. > Merged! > kre >
Attachment:
signature.asc
Description: OpenPGP digital signature