From: Al Viro <[EMAIL PROTECTED]>
Date: Sat, 7 Jan 2006 08:44:42 +0000
[ BTW Al, I noticed you subscribed to netdev, you don't need to
do that if all you want to do is make a posting and be involved
in that particular discussion. If you really are interested in
everything else that goes on here, that's fine too :-) ]
> BTW, why does csum_tcpudp_nofold() have such a prototype?
Historic baggage...
> The value we are interested in is 8bit; all callers
> pass either an explicit constant smaller than 256 or iphdr ->protocol
> (__u8). Moreover, in any arithmetics both __u8 and __u16 will be
> promoted to the same thing, so even arguments about avoiding casts
> in the function body do not apply...
Agreed.
> Even funnier, prototype depends on target; amd64 csum_tcpudp_nofold()
> takes unsigned saddr, unsigned daddr; alpha has both unsigned long...
> At the same time, amd64 has
> static inline unsigned short int
> csum_tcpudp_magic(unsigned long saddr, unsigned long daddr,
> unsigned short len, unsigned short proto, unsigned int sum)
> {
> return csum_fold(csum_tcpudp_nofold(saddr,daddr,len,proto,sum));
> }
> so we pass 32bit value to that puppy as 64bit argument, only to cut it
> back to 32bit when we pass it to csum_tcpudp_nofold()...
Arch specific typing is bad for these interfaces... But, one thing.
Richard Henderson and I always talked about keeping around a 64-bit
running checksum on 64-bit platforms so we didn't need to fold
the values so many times while building a packet. However, this idea
never materialized, so currently all platforms should bascially be
using the same types.
> That's one hell of a hot path, so I'd rather avoid messing with
> it without a very good idea of what's going on. One guaranteed-to-be-safe
> way of annotating it is
> #define csum_tcp_magic(saddr, daddr, len, proto, sum) \
> csum_tcp_magic((__force u32)(__be32)saddr, (__force u32)(__be32)daddr,\
> len, proto, sum)
> which would verify that arguments have the right types and have those casts
> leave the generated code as-is - actual arguments _are_ unsigned int from
> C point of view, so those casts will leave arguments as-is.
Not beautiful, but functional...
> The thing is, for smaller-than-int bitwise types (e.g. __be16)
> ~ is a prohibited operation. The reason is simple: integer promotion
> happens before ~, so we are guaranteed that value of ~x will _not_
> be within range of our type. I can try to teach sparse about such
> "slightly fouled" __be16, so that conversion back to our type (e.g.
> from assignment) would go without complaints, but there are some limits
> to that.
Why not just make some kind of "negate()" macro that hides away all of
the typing issues? Another way to describe the operation is as a
xor of X with an all-1's bitmask the same size of X. Maybe that helps
describe it better?
> Note that some places around checksum handling *WILL* give complaints,
> no matter how smart sparse might become. When we stuff big-endian
> 16bit values or ~ of such values into u32 array and calculate checksum
> of that, we rely on the fact that both all-0 and all-1 16bit words _and_
> any reordering of words do not affect the checksum, which is far beyond
> anything sparse could be expected to understand. For places like that
> (mostly in netfilter code) a forced cast and comment explaining what's
> going on and what we are relying upon would be the right thing, IMO.
> No need to reproduce the entire RFC1071, but reference to it would not be
> a bad thing either...
Indeed.
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
