[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CANn89i+bondpbSEbXp5jF6_keYMGNfwAS8YXQBYMNyKgGb3WEA@mail.gmail.com>
Date: Sat, 4 Dec 2021 10:33:55 -0800
From: Eric Dumazet <edumazet@...gle.com>
To: David Laight <David.Laight@...lab.com>
Cc: kernel test robot <lkp@...el.com>,
Eric Dumazet <eric.dumazet@...il.com>,
"David S . Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
"kbuild-all@...ts.01.org" <kbuild-all@...ts.01.org>,
netdev <netdev@...r.kernel.org>,
Vladimir Oltean <vladimir.oltean@....com>,
David Lebrun <dlebrun@...gle.com>
Subject: Re: [PATCH net-next] net: fix recent csum changes
On Sat, Dec 4, 2021 at 6:00 AM David Laight <David.Laight@...lab.com> wrote:
>
> From: Eric Dumazet
> > Sent: 04 December 2021 04:41
> >
> > On Fri, Dec 3, 2021 at 7:34 PM kernel test robot <lkp@...el.com> wrote:
> > >
> > > I love your patch! Perhaps something to improve:
> ...
> >
> > 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.
>
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
Powered by blists - more mailing lists