Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / mirror / Atom feed
From: David Laight <david.laight.linux@gmail.com>
To: Demian Shulhan <demyansh@gmail.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Jakub Kicinski <kuba@kernel.org>,
	Nathan Chancellor <nathan@kernel.org>,
	Saeed Bishara <saeed.bishara.os@gmail.com>,
	Eric Biggers <ebiggers@kernel.org>,
	Ard Biesheuvel <ardb@kernel.org>,
	Robin Murphy <robin.murphy@arm.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Marco Elver <elver@google.com>, David Gow <davidgow@google.com>,
	Brendan Higgins <brendan.higgins@linux.dev>,
	linux-arm-kernel@lists.infradead.org, netdev@vger.kernel.org,
	kunit-dev@googlegroups.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/2] arm64: csum: Add fused copy and Internet checksum
Date: Sun, 15 Nov 2026 18:55:39 +0000	[thread overview]
Message-ID: <20261115185539.41402600@pumpkin> (raw)
In-Reply-To: <CAOLeWCvsxDZyhYdSOz7hpSbc6x++2UuP4ZzUUagJWq1f+-ro8g@mail.gmail.com>

On Sun, 15 Nov 2026 19:25:24 +0200
Demian Shulhan <demyansh@gmail.com> wrote:

> Hi David,
> 
> > Is that right?
> > I thought ldp/stp required aligned addresses.
> > The code below is using get_unaligned().
> > I think the timings below are for ldp/stp?  
> 
> Unlike 32-bit ARM (ldrd/strd), AArch64 GPR ldp/stp support unaligned
> addresses in hardware on Normal memory. Both GCC 15 and Clang 21 merge
> the adjacent get_unaligned()/put_unaligned() pairs into ldp/stp, and
> the timings are from that code. Unaligned (src+1, dst+3) is only ~5%
> slower due to cache-line splits (~28.8-29.1 GB/s vs ~30.3-31.1 GB/s).

Ok, I knew there was a problem - but it is 32-bit only.

> > That is topping out at 10 bytes/clock.
> > Each loop iteration copies 32 bytes - so must take 3 clocks.
> > Which means it must be doing 16 byte read/write in a single clock.
> > So rather begs the question of why it is limited by the loop control
> > and checksum instructions rather than the memory accesses.
> > The checksum code needs 4 ALU instructions (for 16 bytes), if the CPU
> > can execute more than that then there is half a chance of getting the
> > loop down to 2 clocks.  
> 
> For 32 bytes, the loop is 16 instructions (4x ldp/stp, 8x adds/cinc,
> 2x add, cmp, b.ne). On Neoverse-V2 (6 ALU pipes, 8-wide dispatch) 12
> ALU/branch ops plus 4 paired memory ops put the floor right around ~3
> clocks/iteration (~5.2 IPC).

The floor would be nearer 2.
There are 11 ALU and a branch to fit in two clocks.

> > Just moving one (or both) the pointer updates into the middle may help.
> > Otherwise you try using negative offsets from the end of the buffer
> 
> Moving the pointer updates into the middle does help, it shaves up to
> ~5% off across sizes.

Thought it might - you were trying to do to many alu ops at the end.

> > and 'reg + reg' addressing (will need more pointers since I don't think
> > arm has 'reg + reg + offset' addressing. 
> For 'reg + reg', ldp/stp only support immediate offsets, so indexed
> addressing would force single-register ldr/str and double the memory
> instructions.

I didn't think that was possible.
x86 can do 'reg + reg + offset' but I have a feeling it ends up using
an alu u-op for one of the adds (on intel and/or amd). Nothing else
explains why we can't get move that 12 bytes/clock for ip cksum.

> 
> 
> > > + if (unlikely(len <= 0))
> > > + return 0;  
> >
> > Hmmm...
> > Does zero actually happen often enough for that to be a gain?
> > Even with the unlikely() it is likely to slow down the normal path.
> > And that assumes that the branch isn't mispredicted and that gcc
> > doesn't ignore the unlikely().  
> 
> len == 0 would fall through fine, but since len is a signed int in the
> csum_partial_copy_nocheck() prototype, this guards against negative len
> wrapping (unsigned int)len >> 5 (matching do_csum() right above it).
> Both compilers place the return 0 block out-of-line at the end of the
> function.

You've still got a branch that can (plausibly) get predicted taken.
 
> > > + sum2 += v0;
> > > + sum3 += v1;  
> >
> > Is using sum0/1 here actually slower?
> > My 'gut feel' is that it won't be - and saves the adds below.  
> 
> For 64-128B your gut feel is right - 2 accumulators is 2-10% faster from
> saving the post-loop fold, and on GCC 15 it matches 4 accumulators on
> larger buffers too (GCC folds v0 + v2 first, keeping a 1-cycle chain in
> 18 insns). The catch is Clang 21: with 2 accumulators it chains both
> adds/cinc updates onto the same accumulator and drops ~12% for >= 512B
> (27.0 vs 30.5-31.2 GB/s).

I hate that - why can't the compilers do what they are told :-)
gcc always compiles (a + b) + (c + d) as (((a + b) + c) + d) making
the register dependency chain (which matters far more then the number
of instructions) one clock longer.

Sometimes an OPTIMIZER_HIDE_VAR() can help - but it tends to add
a register-register move.

> > It is also worth checking that gcc doesn't zero extend v0 into a
> > register and then do a full 128bit add.
> > I'm sure I looked at the code using only 64bit variables for:
> > sum0 += v0;
> > carry0 += sum0 < v0;
> > both gcc and clang used 'add 0 with carry' for the second line.  
> 
> Checked - neither compiler zero-extends v0, both emit adds + cinc
> directly on the 64-bit registers.

This week :-)
clang tends to get it right, gcc has a habit of allocating a register
for the high part.
For u64 on 32bit (effectively the same) I've seen it spill constant
zeros to stack and do multiplies by registers that are zero. 

> 
> 
> > > + s += 32;
> > > + d += 32;
> > > + } while (--n);  
> >
> > It is a shame to have to update 3 variables here.
> > They may be taking the 3rd clock.  
> 
> Switching --n to an end pointer (s != end) and moving s += 32 / d += 32
> between the loads and stores drops the 3rd update on Clang (80 -> 76
> insns total) and cleans up GCC's loop setup (88 -> 81 insns), gaining
> +0.4% to +4.9% across sizes.
> 
> Here is the resulting 16-instruction loop with an end pointer and 4
> accumulators:
> 
>   GCC 15.2:                        Clang 21.1:
>     ldp  x10, x9, [x0]               ldp  x17, x18, [x0]
>     add  x11, x11, #0x20             ldp  x3, x4, [x0, #0x10]
>     ldp  x8, x7, [x0, #16]           add  x0, x0, #0x20
>     add  x0, x0, #0x20               adds x15, x15, x17
>     stp  x10, x9, [x11, #-32]        stp  x17, x18, [x1]
>     adds x3, x10, x3                 cinc x16, x16, hs
>     stp  x8, x7, [x11, #-16]         adds x13, x13, x18
>     cinc x12, x12, cs                stp  x3, x4, [x1, #0x10]
>     adds x4, x9, x4                  cinc x14, x14, hs
>     cinc x13, x13, cs                adds x8, x8, x3
>     adds x6, x8, x6                  add  x1, x1, #0x20
>     cinc x16, x16, cs                cinc x9, x9, hs
>     adds x5, x7, x5                  adds x10, x10, x4
>     cinc x15, x15, cs                cinc x11, x11, hs
>     cmp  x14, x0                     cmp  x0, x12
>     b.ne .Loop                       b.ne .Loop

Both those manage to get the ldp early (probably good)
but there are a lot of alu instructions at the end.
If moving the 's/d += 32' moved those instructions to before
the stp, then trying to persuade the compiler to move the
sum0/1/2/3 earlier might help as well.
I was thinking on the lines of:
	v0 = get_unaligned((const u64 *)s);
	v1 = get_unaligned((const u64 *)s + 8);
	sum0 += v0;
	sum1 += v1;
	put_unaligned(v0, (u64 *)d);
	put_unaligned(v1, (u64 *)d + 8);
	s += 32;
	d += 32;
	v0 = get_unaligned((const u64 *)s + 16 - 32);
	v1 = get_unaligned((const u64 *)s + 24 - 32);
	sum2 += v0;
	sum3 += v1;
	put_unaligned(v0, (u64 *)d + 16 - 32);
	put_unaligned(v1, (u64 *)d + 24 - 32);

but it might make things worse
(and the compiler might just ignore you).

I always try to do tests in userspace.
Makes it much quicker to see the effect of changes and you can
test just the inner part of the loop separately from the tail code.

As it stands this code it reasonably generic.
Although cpu without ldp/stp probably don't need the extra unroll.

David

> 
> I'll switch to the end-pointer loop with interleaved pointer updates in
> v3.
> 
> Thanks!
> Demian


  reply	other threads:[~2026-11-15 18:55 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-11-14  3:56 [PATCH v2 0/2] arm64: csum: Add fused copy and Internet checksum Demian Shulhan
2026-11-14  3:57 ` [PATCH v2 1/2] " Demian Shulhan
2026-11-14 14:26   ` David Laight
2026-11-15 17:25     ` Demian Shulhan
2026-11-15 18:55       ` David Laight [this message]
2026-11-14  3:57 ` [PATCH v2 2/2] lib/tests: checksum: Add KUnit tests for csum_partial_copy_nocheck() Demian Shulhan

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=20261115185539.41402600@pumpkin \
    --to=david.laight.linux@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=ardb@kernel.org \
    --cc=brendan.higgins@linux.dev \
    --cc=catalin.marinas@arm.com \
    --cc=davidgow@google.com \
    --cc=demyansh@gmail.com \
    --cc=ebiggers@kernel.org \
    --cc=elver@google.com \
    --cc=kuba@kernel.org \
    --cc=kunit-dev@googlegroups.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=nathan@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=robin.murphy@arm.com \
    --cc=saeed.bishara.os@gmail.com \
    --cc=will@kernel.org \
    /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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox