linux-kernel - RE: [PATCH v2] MIPS: Check __clang__ to avoid performance influence with GCC in csum_tcpudp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 17 Mar 2021 22:36:20 +0100 (CET)
From:   "Maciej W. Rozycki" <macro@...am.me.uk>
To:     David Laight <David.Laight@...LAB.COM>
cc:     Tiezhu Yang <yangtiezhu@...ngson.cn>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        "linux-mips@...r.kernel.org" <linux-mips@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Xuefeng Li <lixuefeng@...ngson.cn>
Subject: RE: [PATCH v2] MIPS: Check __clang__ to avoid performance influence
 with GCC in csum_tcpudp_nofold()

On Wed, 17 Mar 2021, David Laight wrote:

> > > > Not that I grok the mips opcodes.
> > > > But that code has horridness on its side.
> > 
> >  It's a 32-bit one's-complement addition.  The use of 64-bit operations
> > reduces the number of calculations as any 32-bit carries accumulate in the
> > high 32-bit word allowing one instruction to be saved total compared to
> > the 32-bit variant.  Nothing particularly unusual for me here; I've seen
> > worse stuff with x86.
> 
> The 'problem' is that mips doesn't have a carry flag.
> So the 64-bit maths is 'tricky'.
> It may well be that a loop based on:
> 	do {
> 		val = *ptr++;
> 		sum += val;
> 		carry += sum < val;
> 	} while (ptr != limit)
> will generate much better code.

 This piece of assembly appears to me as good as you can get, but it is 
somewhat dated, going back to 1999 and LMO commit 0458ce25ec4e ("Fix 
MIPS64 IP checksums.") as far as the 64-bit variant is concerned, with a 
much later bug fix applied for a corner case with commit 66fd848cadaa 
("MIPS: Fix special case in 64 bit IP checksumming.") back in 2017 (!), 
which added two instructions.  It may well be that GCC has since improved 
and would produce code of similar or better quality.  Anyone is welcome to 
try it of course and submit a patch if it turns out beneficial.

 NB I note there's an earlyclobber specifier missing on the output 
operand, lost with the merge of the separate mips and mips64 ports with 
LMO commit a69fb3990ea9 ("Goodbye mips64.  31704 lines of code bite the 
dust."), back in 2003, which I previously added myself, back in 2002 with 
LMO commit acc75ed18471 ("Bug fixes for constraints and type casts.").  
Sigh.  I think the overlap with operand #1 has prevented damage from 
happening as a result of the missing specifier, but IMO it would best be 
reinstated just in case.

> I think there is a 'setlt' instruction for the compare.

 Yes and it's used in the piece of code quoted (SLT).

> It certainly would on the nios (which is mips-like).
> That is (probably) 6 instructions for 4 bytes.
> I suspect there may be a data stall after the memory read.
> So an interleaved unroll would remove that stall.
> That would be 10 clocks for 8 bytes.

 There's no memory read involved in this code; of course there could be in 
the paster of the inline function as shown in the piece shown by Tiezhu.  
There are a couple of instructions in between though, which should keep 
the pipeline occupied as long as data retrieved is hot in the cache 
(prefetch instructions could be used to assist with that).

 OTOH if data were to come from the main memory, I doubt anything could 
help here.

> The x86-64 code is 'interesting'.
> It has repeated 'add carry' instructions.
> On Intel cpus prior to (at least) Haswell they take two clocks each.
> So the code is no faster than adding 32bit values to a 64bit sum.

 GCC has DFA pipeline descriptions for various MIPS processors, so the 
details may vary, but it is assumed overall that plain ALU operations 
cause no pipeline stalls or slips, so nominally they take 1 pipeline clock 
each for scalar implementations.  This includes all the instructions in 
the piece of code discussed here.

 (Actual clock counts will of course depend on the number of pipeline 
stages a given microarchitecture implements and are not a concern here. 
The earliest MIPS chips had as few as four stages only and the inclusion 
of branch and load delay slots prevented pipeline slips from occuring or 
even actually being implemented, as in what MIPS used to stand for, that 
is Microprocessor without Interlocked Pipeline Stages.  Stalls could still 
happen of course in the case of cache misses.)

  Maciej