From: Eric Dumazet <edumazet@google.com>
To: Vladimir Oltean <olteanv@gmail.com>
Cc: Eric Dumazet <eric.dumazet@gmail.com>,
"David S . Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>, netdev <netdev@vger.kernel.org>,
David Laight <David.Laight@aculab.com>
Subject: Re: [PATCH net-next 2/2] net: optimize skb_postpull_rcsum()
Date: Thu, 2 Dec 2021 06:51:47 -0800 [thread overview]
Message-ID: <CANn89iLW4kwKf0x094epVeCaKhB4GtYgbDwE2=Fp0HnW8UdKzw@mail.gmail.com> (raw)
In-Reply-To: <20211202131040.rdxzbfwh2slhftg5@skbuf>
Hi Vladimir
On Thu, Dec 2, 2021 at 5:10 AM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> Hello,
>
> On Wed, Nov 24, 2021 at 12:24:46PM -0800, Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@google.com>
> >
> > Remove one pair of add/adc instructions and their dependency
> > against carry flag.
> >
> > We can leverage third argument to csum_partial():
> >
> > X = csum_block_sub(X, csum_partial(start, len, 0), 0);
> >
> > -->
> >
> > X = csum_block_add(X, ~csum_partial(start, len, 0), 0);
> >
> > -->
> >
> > X = ~csum_partial(start, len, ~X);
> >
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > ---
> > include/linux/skbuff.h | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index eba256af64a577b458998845f2dc01a5ec80745a..eae4bd3237a41cc1b60b44c91fbfe21dfdd8f117 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -3485,7 +3485,11 @@ __skb_postpull_rcsum(struct sk_buff *skb, const void *start, unsigned int len,
> > static inline void skb_postpull_rcsum(struct sk_buff *skb,
> > const void *start, unsigned int len)
> > {
> > - __skb_postpull_rcsum(skb, start, len, 0);
> > + if (skb->ip_summed == CHECKSUM_COMPLETE)
> > + skb->csum = ~csum_partial(start, len, ~skb->csum);
> > + else if (skb->ip_summed == CHECKSUM_PARTIAL &&
> > + skb_checksum_start_offset(skb) < 0)
> > + skb->ip_summed = CHECKSUM_NONE;
> > }
> >
> > static __always_inline void
> > --
> > 2.34.0.rc2.393.gf8c9666880-goog
> >
>
> I am seeing some errors after this patch, and I am not able to
> understand why. Specifically, __skb_gro_checksum_complete() hits this
> condition:
There were two patches, one for GRO, one for skb_postpull_rcsum()
I am a bit confused by your report. Which one is causing problems ?
>
> wsum = skb_checksum(skb, skb_gro_offset(skb), skb_gro_len(skb), 0);
>
> /* NAPI_GRO_CB(skb)->csum holds pseudo checksum */
> sum = csum_fold(csum_add(NAPI_GRO_CB(skb)->csum, wsum));
> /* See comments in __skb_checksum_complete(). */
> if (likely(!sum)) {
> if (unlikely(skb->ip_summed == CHECKSUM_COMPLETE) &&
> !skb->csum_complete_sw)
> netdev_rx_csum_fault(skb->dev, skb);
> }
>
> To test, I am using a DSA switch network interface with an IPv4 address
> and pinging through it.
>
> The idea is as follows: an enetc port is attached to a switch, and that
> switch places a frame header before the Ethernet header.
> The enetc calculates the L2 payload (actually what it perceives as L2
> payload, since it has no insight into the switch header format) checksum
> and puts it in skb->csum:
> https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/freescale/enetc/enetc.c#L726
>
> Then, the switch driver packet type handler is invoked, and this strips
> that header and recalculates the checksum (then it changes skb->dev and
> this is how pinging is done through the DSA interface, but that is
> irrelevant).
> https://elixir.bootlin.com/linux/latest/source/net/dsa/tag_ocelot.c#L56
>
> There seems to be a disparity when the skb->csum is calculated by
> skb_postpull_rcsum as zero. Before, it was calculated as 0xffff.
skb->csum is 32bit, so there are about 2^16 different values for a
given Internet checksum
>
> Below is a dump added by me in skb_postpull_rcsum when the checksums
> calculated through both methods differ. I've kept the old implementation
> inside skb->csum and this is what skb_dump() sees:
>
> [ 99.891524] skb csum of 20 bytes (20 to left of skb->data) using method 1: 0x0 method 2 0xffffffff, orig 0xf470
> [ 99.901701] skb len=84 headroom=98 headlen=84 tailroom=1546
> [ 99.901701] mac=(84,-6) net=(78,0) trans=78
> [ 99.901701] shinfo(txflags=0 nr_frags=0 gso(size=0 type=0 segs=0))
> [ 99.901701] csum(0xffffffff ip_summed=2 complete_sw=0 valid=0 level=0)
> [ 99.901701] hash(0x0 sw=0 l4=0) proto=0x00f8 pkttype=3 iif=7
> [ 99.929916] dev name=eno2 feat=0x0x00020100001149a9
> [ 99.934831] skb headroom: 00000000: 00 c0 b1 a4 ff ff 00 00 00 e0 b1 a4 ff ff 00 00
> [ 99.942533] skb headroom: 00000010: 00 6f 5b 02 f8 14 ff ff 40 62 5b 02 f8 14 ff ff
> [ 99.950232] skb headroom: 00000020: 21 6f 5b 02 f8 14 ff ff 00 00 00 00 00 00 00 00
> [ 99.957931] skb headroom: 00000030: 00 00 00 00 00 00 00 00 00 40 00 00 00 00 00 00
> [ 99.965631] skb headroom: 00000040: 88 80 00 0a 00 33 9d 40 f0 41 01 80 00 00 08 0f
> [ 99.973330] skb headroom: 00000050: 00 10 00 00 00 04 9f 05 f6 28 ba ae e4 b6 2c 3d
> [ 99.981028] skb headroom: 00000060: 08 00
> [ 99.985062] skb linear: 00000000: 45 00 00 54 27 ac 00 00 40 01 09 a8 c0 a8 64 03
> [ 99.992762] skb linear: 00000010: c0 a8 64 01 00 00 10 e6 01 5c 00 04 49 30 a7 61
> [ 100.000462] skb linear: 00000020: 00 00 00 00 3d 55 01 00 00 00 00 00 10 11 12 13
> [ 100.008162] skb linear: 00000030: 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f 20 21 22 23
> [ 100.015862] skb linear: 00000040: 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f 30 31 32 33
> [ 100.023561] skb linear: 00000050: 34 35 36 37
>
> And below is the same output as above, but annotated by me with some comments:
>
> [ 99.891524] skb csum of 20 bytes (20 to left of skb->data) using method 1: 0x0 method 2 0xffffffff, orig 0xf470
> [ 99.901701] skb len=84 headroom=98 headlen=84 tailroom=1546
> [ 99.901701] mac=(84,-6) net=(78,0) trans=78
> ^^^^^^^^^^^^^^^^^^^^^^
> since the print is done from ocelot_rcv, the network and
> transport headers haven't yet been updated
>
> [ 99.901701] shinfo(txflags=0 nr_frags=0 gso(size=0 type=0 segs=0))
> [ 99.901701] csum(0xffffffff ip_summed=2 complete_sw=0 valid=0 level=0)
> [ 99.901701] hash(0x0 sw=0 l4=0) proto=0x00f8 pkttype=3 iif=7
> [ 99.929916] dev name=eno2 feat=0x0x00020100001149a9
> [ 99.934831] skb headroom: 00000000: 00 c0 b1 a4 ff ff 00 00 00 e0 b1 a4 ff ff 00 00
> [ 99.942533] skb headroom: 00000010: 00 6f 5b 02 f8 14 ff ff 40 62 5b 02 f8 14 ff ff
> [ 99.950232] skb headroom: 00000020: 21 6f 5b 02 f8 14 ff ff 00 00 00 00 00 00 00 00
> [ 99.957931] skb headroom: 00000030: 00 00 00 00 00 00 00 00 00 40 00 00 00 00 00 00
>
> here is where the enetc saw the the "start" variable (old skb->data)
> beginning of the frame points here
> v v
> [ 99.965631] skb headroom: 00000040: 88 80 00 0a 00 33 9d 40 f0 41 01 80 00 00 08 0f
>
> OCELOT_TAG_LEN bytes
> later is the real MAC header
> v
> [ 99.973330] skb headroom: 00000050: 00 10 00 00 00 04 9f 05 f6 28 ba ae e4 b6 2c 3d
> [ 99.981028] skb headroom: 00000060: 08 00
> ^
> the skb_postpull_rcsum is called from "start"
> until the first byte prior to this one
>
> [ 99.985062] skb linear: 00000000: 45 00 00 54 27 ac 00 00 40 01 09 a8 c0 a8 64 03
> [ 99.992762] skb linear: 00000010: c0 a8 64 01 00 00 10 e6 01 5c 00 04 49 30 a7 61
> [ 100.000462] skb linear: 00000020: 00 00 00 00 3d 55 01 00 00 00 00 00 10 11 12 13
> [ 100.008162] skb linear: 00000030: 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f 20 21 22 23
> [ 100.015862] skb linear: 00000040: 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f 30 31 32 33
> [ 100.023561] skb linear: 00000050: 34 35 36 37
>
> Do you have some suggestions as to what may be wrong? Thanks.
What kind of traffic is triggering the fault ? TCP, UDP, something else ?
Do you have a stack trace to provide, because it is not clear from
where the issue is detected.
Thanks.
next prev parent reply other threads:[~2021-12-02 14:52 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-24 20:24 [PATCH net-next 0/2] net: small csum optimizations Eric Dumazet
2021-11-24 20:24 ` [PATCH net-next 1/2] gro: optimize skb_gro_postpull_rcsum() Eric Dumazet
2021-11-24 20:24 ` [PATCH net-next 2/2] net: optimize skb_postpull_rcsum() Eric Dumazet
2021-11-25 9:41 ` David Laight
2021-11-25 13:32 ` Eric Dumazet
2021-11-25 14:29 ` David Laight
2021-12-02 13:10 ` Vladimir Oltean
2021-12-02 14:51 ` Eric Dumazet [this message]
2021-12-02 16:29 ` Vladimir Oltean
2021-12-02 19:32 ` Eric Dumazet
2021-12-02 20:37 ` Eric Dumazet
2021-12-02 21:07 ` Vladimir Oltean
2021-12-02 20:40 ` Vladimir Oltean
2021-12-02 20:58 ` Vladimir Oltean
2021-12-02 20:58 ` David Laight
2021-12-02 21:40 ` Vladimir Oltean
2021-12-03 14:51 ` David Laight
2021-12-03 14:57 ` David Laight
2021-12-03 16:14 ` Vladimir Oltean
2021-12-03 16:30 ` Eric Dumazet
2021-12-03 16:47 ` David Laight
2021-12-03 16:58 ` Eric Dumazet
2021-12-03 17:41 ` David Laight
2021-12-02 15:06 ` David Laight
2021-12-02 15:22 ` Eric Dumazet
2021-11-26 5:20 ` [PATCH net-next 0/2] net: small csum optimizations patchwork-bot+netdevbpf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CANn89iLW4kwKf0x094epVeCaKhB4GtYgbDwE2=Fp0HnW8UdKzw@mail.gmail.com' \
--to=edumazet@google.com \
--cc=David.Laight@aculab.com \
--cc=davem@davemloft.net \
--cc=eric.dumazet@gmail.com \
--cc=kuba@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=olteanv@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.