Re: [PATCH] x86_64: new and improved memset()

[Alexey Dobriyan <adobriyan@gmail.com>]

On Sat, Sep 14, 2019 at 01:37:17PM +0200, Borislav Petkov wrote:
> On Sat, Sep 14, 2019 at 01:33:45PM +0300, Alexey Dobriyan wrote:
> > --- a/arch/x86/include/asm/string_64.h
> > +++ b/arch/x86/include/asm/string_64.h
> > @@ -15,7 +15,111 @@ extern void *memcpy(void *to, const void *from, size_t len);
> >  extern void *__memcpy(void *to, const void *from, size_t len);
> >  
> >  #define __HAVE_ARCH_MEMSET
> > +#if defined(_ARCH_X86_BOOT) || defined(CONFIG_FORTIFY_SOURCE)
> >  void *memset(void *s, int c, size_t n);
> > +#else
> > +#include <asm/alternative.h>
> > +#include <asm/cpufeatures.h>
> > +
> > +/* Internal, do not use. */
> > +static __always_inline void memset0(void *s, size_t n)
> > +{
> > +	/* Internal, do not use. */
> > +	void _memset0_mov(void);
> > +	void _memset0_rep_stosq(void);
> > +	void memset0_mov(void);
> > +	void memset0_rep_stosq(void);
> > +	void memset0_rep_stosb(void);
> > +
> > +	if (__builtin_constant_p(n) && n == 0) {
> > +	} else if (__builtin_constant_p(n) && n == 1) {
> > +		*(uint8_t *)s = 0;
> > +	} else if (__builtin_constant_p(n) && n == 2) {
> > +		*(uint16_t *)s = 0;
> > +	} else if (__builtin_constant_p(n) && n == 4) {
> > +		*(uint32_t *)s = 0;
> > +	} else if (__builtin_constant_p(n) && n == 6) {
> > +		*(uint32_t *)s = 0;
> > +		*(uint16_t *)(s + 4) = 0;
> > +	} else if (__builtin_constant_p(n) && n == 8) {
> > +		*(uint64_t *)s = 0;
> > +	} else if (__builtin_constant_p(n) && (n & 7) == 0) {
> > +		alternative_call_2(
> > +			_memset0_mov,
> > +			_memset0_rep_stosq, X86_FEATURE_REP_GOOD,
> > +			memset0_rep_stosb, X86_FEATURE_ERMS,
> > +			ASM_OUTPUT2("=D" (s), "=c" (n)),
> > +			"D" (s), "c" (n)
> > +			: "rax", "cc", "memory"
> > +		);
> > +	} else {
> > +		alternative_call_2(
> > +			memset0_mov,
> > +			memset0_rep_stosq, X86_FEATURE_REP_GOOD,
> > +			memset0_rep_stosb, X86_FEATURE_ERMS,
> > +			ASM_OUTPUT2("=D" (s), "=c" (n)),
> > +			"D" (s), "c" (n)
> > +			: "rax", "rsi", "cc", "memory"
> > +		);
> > +	}
> > +}
> > +
> > +/* Internal, do not use. */
> > +static __always_inline void memsetx(void *s, int c, size_t n)
> > +{
> > +	/* Internal, do not use. */
> > +	void _memsetx_mov(void);
> > +	void _memsetx_rep_stosq(void);
> > +	void memsetx_mov(void);
> > +	void memsetx_rep_stosq(void);
> > +	void memsetx_rep_stosb(void);
> > +
> > +	const uint64_t ccc = (uint8_t)c * 0x0101010101010101ULL;
> > +
> > +	if (__builtin_constant_p(n) && n == 0) {
> > +	} else if (__builtin_constant_p(n) && n == 1) {
> > +		*(uint8_t *)s = ccc;
> > +	} else if (__builtin_constant_p(n) && n == 2) {
> > +		*(uint16_t *)s = ccc;
> > +	} else if (__builtin_constant_p(n) && n == 4) {
> > +		*(uint32_t *)s = ccc;
> > +	} else if (__builtin_constant_p(n) && n == 8) {
> > +		*(uint64_t *)s = ccc;
> > +	} else if (__builtin_constant_p(n) && (n & 7) == 0) {
> > +		alternative_call_2(
> > +			_memsetx_mov,
> > +			_memsetx_rep_stosq, X86_FEATURE_REP_GOOD,
> > +			memsetx_rep_stosb, X86_FEATURE_ERMS,
> > +			ASM_OUTPUT2("=D" (s), "=c" (n)),
> > +			"D" (s), "c" (n), "a" (ccc)
> > +			: "cc", "memory"
> > +		);
> > +	} else {
> > +		alternative_call_2(
> > +			memsetx_mov,
> > +			memsetx_rep_stosq, X86_FEATURE_REP_GOOD,
> > +			memsetx_rep_stosb, X86_FEATURE_ERMS,
> > +			ASM_OUTPUT2("=D" (s), "=c" (n)),
> > +			"D" (s), "c" (n), "a" (ccc)
> > +			: "rsi", "cc", "memory"
> > +		);
> > +	}
> > +}
> > +
> > +static __always_inline void *memset(void *s, int c, size_t n)
> > +{
> > +	if (__builtin_constant_p(c)) {
> > +		if (c == 0) {
> > +			memset0(s, n);
> > +		} else {
> > +			memsetx(s, c, n);
> > +		}
> > +		return s;
> > +	} else {
> > +		return __builtin_memset(s, c, n);
> > +	}
> > +}
> 
> I'm willing to take something like that only when such complexity is
> justified by numbers. I.e., I'm much more inclined to capping it under
> 32 and 64 byte sizes and keeping it simple.

OK. Those small lengths were indeed annoying.

> > +ENTRY(_memset0_mov)
> > +	xor	eax, eax
> > +.globl _memsetx_mov
> > +_memsetx_mov:
> > +	add	rcx, rdi
> > +	cmp	rdi, rcx
> > +	je	1f
> > +2:
> > +	mov	[rdi], rax
> > +	add	rdi, 8
> > +	cmp	rdi, rcx
> > +	jne	2b
> > +1:
> > +	ret
> > +ENDPROC(_memset0_mov)
> > +ENDPROC(_memsetx_mov)
> > +EXPORT_SYMBOL(_memset0_mov)
> > +EXPORT_SYMBOL(_memsetx_mov)
> > +
> > +ENTRY(memset0_mov)
> > +	xor	eax, eax
> > +.globl memsetx_mov
> > +memsetx_mov:
> > +	lea	rsi, [rdi + rcx]
> > +	cmp	rdi, rsi
> > +	je	1f
> > +2:
> > +	mov	[rdi], al
> > +	add	rdi, 1
> > +	cmp	rdi, rsi
> > +	jne	2b
> > +1:
> > +	ret
> 
> Say what now? Intel syntax? You must be joking...

It is the best thing in the x86 assembler universe.

> > +ENDPROC(memset0_mov)
> > +ENDPROC(memsetx_mov)
> > +EXPORT_SYMBOL(memset0_mov)
> > +EXPORT_SYMBOL(memsetx_mov)
> 
> Too many exported symbols.

Those are technical exports. memset() remains the only developer-visible
interface.

> Again, I'll much more prefer a cleaner,
> smaller solution than one where readability suffers greatly at the
> expense of *maybe* getting a bit better performance.

Readability is red herring, I for one find AT&T syntax unreadable.