From: Matthew Wilcox <willy@infradead.org>
To: Nikolay Borisov <nborisov@suse.com>
Cc: linux-fsdevel@vger.kernel.org, viro@zeniv.linux.org.uk,
david@fromorbit.com, djwong@kernel.org
Subject: Re: [PATCH] vfs: Optimize dedupe comparison
Date: Thu, 15 Jul 2021 16:09:06 +0100 [thread overview]
Message-ID: <YPBPkupPDnsCXrLU@casper.infradead.org> (raw)
In-Reply-To: <7c4c9e73-0a8b-5621-0b74-1bf34e4b4817@suse.com>
On Thu, Jul 15, 2021 at 05:44:15PM +0300, Nikolay Borisov wrote:
> That was my first impression, here's the profile:
>
> │ Disassembly of section .text:
> │
> │ ffffffff815c6f60 <memcmp>:
> │ memcmp():
> │ test %rdx,%rdx
> │ ↓ je 22
> │ xor %ecx,%ecx
> │ ↓ jmp 12
> 49.32 │ 9: add $0x1,%rcx
> 0.03 │ cmp %rcx,%rdx
> 11.82 │ ↓ je 21
> 0.01 │12: movzbl (%rdi,%rcx,1),%eax
> 38.19 │ movzbl (%rsi,%rcx,1),%r8d
> 0.59 │ sub %r8d,%eax
> 0.04 │ ↑ je 9
That looks like a byte loop to me ...
> It's indeed on x86-64 and according to the sources it's using
> __builtin_memcmp according to arch/x86/boot/string.h
I think the 'boot' part of that path might indicate that it's not what's
actually being used by the kernel.
$ git grep __HAVE_ARCH_MEMCMP
arch/arc/include/asm/string.h:#define __HAVE_ARCH_MEMCMP
arch/arm64/include/asm/string.h:#define __HAVE_ARCH_MEMCMP
arch/csky/abiv2/inc/abi/string.h:#define __HAVE_ARCH_MEMCMP
arch/powerpc/include/asm/string.h:#define __HAVE_ARCH_MEMCMP
arch/s390/include/asm/string.h:#define __HAVE_ARCH_MEMCMP /* arch function */
arch/s390/lib/string.c:#ifdef __HAVE_ARCH_MEMCMP
arch/s390/purgatory/string.c:#define __HAVE_ARCH_MEMCMP /* arch function */
arch/sparc/include/asm/string.h:#define __HAVE_ARCH_MEMCMP
include/linux/string.h:#ifndef __HAVE_ARCH_MEMCMP
lib/string.c:#ifndef __HAVE_ARCH_MEMCMP
So I think x86-64 is using the stupid one.
> > Can this even happen? Surely we can only dedup on a block boundary and
> > blocks are required to be a power of two and at least 512 bytes in size?
>
> I was wondering the same thing, but AFAICS it seems to be possible i.e
> if userspace spaces bad offsets, while all kinds of internal fs
> synchronization ops are going to be performed on aligned offsets, that
> doesn't mean the original ones, passed from userspace are themselves
> aligned explicitly.
Ah, I thought it'd be failed before we got to this point.
But honestly, I think x86-64 needs to be fixed to either use
__builtin_memcmp() or to have a nicely written custom memcmp(). I
tried to find the gcc implementation of __builtin_memcmp() on
x86-64, but I can't.
next prev parent reply other threads:[~2021-07-15 15:09 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-15 14:13 [PATCH] vfs: Optimize dedupe comparison Nikolay Borisov
2021-07-15 14:30 ` Matthew Wilcox
2021-07-15 14:44 ` Nikolay Borisov
2021-07-15 15:09 ` Matthew Wilcox [this message]
2021-07-15 22:33 ` Dave Chinner
2021-07-20 14:58 ` Nikolay Borisov
2021-07-20 15:12 ` Matthew Wilcox
2021-07-16 12:10 ` Nikolay Borisov
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=YPBPkupPDnsCXrLU@casper.infradead.org \
--to=willy@infradead.org \
--cc=david@fromorbit.com \
--cc=djwong@kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=nborisov@suse.com \
--cc=viro@zeniv.linux.org.uk \
/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.