RE: [PATCH net-next] net: fix recent csum changes

From: Eric Dumazet

Sent: 04 December 2021 18:34

On Sat, Dec 4, 2021 at 6:00 AM David Laight [off-list ref] wrote:

quoted

From: Eric Dumazet

quoted

Sent: 04 December 2021 04:41

On Fri, Dec 3, 2021 at 7:34 PM kernel test robot [off-list ref] wrote:

quoted

I love your patch! Perhaps something to improve:

...

quoted

Yes, keeping sparse happy with these checksum is not easy.

I will add and use this helper, unless someone has a better idea.

diff --git a/include/net/checksum.h b/include/net/checksum.h
index 5b96d5bd6e54532a7a82511ff5d7d4c6f18982d5..5218041e5c8f93cd369a2a3a46add3e6a5e41af7

100644

--- a/include/net/checksum.h
+++ b/include/net/checksum.h

@@ -180,4 +180,8 @@ static inline void remcsum_unadjust(__sum16 *psum,

__wsum delta)
        *psum = csum_fold(csum_sub(delta, (__force __wsum)*psum));
 }

+static inline __wsum wsum_negate(__wsum val)
+{
+       return (__force __wsum)-((__force u32)val);
+}
 #endif

I was thinking that the expression also requires some comments.
So maybe put a #define / static inline in checksum.h like:

/* Subract the checksum of a buffer.
 * The domain is __wsum is [1..~0u] (ie excludes zero)
 * so ~csum_partial() cannot be used.
 * The two's compliment gives the right answer provided the old 'csum'
 * isn't zero - which it shouldn't be. */
#define csum_partial_sub(buf, len, csum) (-csum_partial(buf, len, -(csum))

and then add the annotations there to keep sparse happy there.

will sparse accept '1 + ~csum' ? The compiler should use negate for it.
It actually makes it slightly more obvious why the code is right.

Sparse is not happy with  1 + ~csum,

So we unfortunately would need something ugly like

(__force __wsum)(1 + ~(__force u32)csum)

Which most readers of this code will not find obvious.

Sparse really isn't helping here at all.
Perhaps there should be a way of marking a function so that
sparse just ignores it.

I also rather dislike that the compiler sees the (u32)csum cast.
The sparse annotation should really be either __sparse(u32)csum
or __sparse(u32, csum) to that compiler type checking still applies.

Perhaps adding:
#define WSUM(val) (__force __wsum)(val)
#define U32(csum)  (__force u32)(csum)
before the 'static inlines' in checksum.h and #undeffing them at the end.
Then all the functions could be made a little easier to read.

	return WSUM(1 + ~U32(csum_partial(buf, len, WSUM(1 + ~U32(csum)));

is a bit better than the casts - still not nice.

	David



	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)