linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "George Spelvin" <linux@horizon.com>
To: linux@horizon.com, npiggin@gmail.com
Cc: bharrosh@panasas.com, linux-arch@vger.kernel.org,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: Big git diff speedup by avoiding x86 "fast string" memcmp
Date: 19 Dec 2010 12:06:46 -0500	[thread overview]
Message-ID: <20101219170646.482.qmail@science.horizon.com> (raw)
In-Reply-To: <AANLkTikwUEBd1Ez75pQ7gw7ShZitwQOQcH6CvCdJV2np@mail.gmail.com>

> First, a byte-by-byte strcpy_from_user of the whole name string to
> kernel space. Then a byte-by-byte chunking and hashing component
> paths according to '/'. Then a byte-by-byte memcmp against the
> dentry name.

Well, you've put your finger on the obvious place to to the aligned copy,
if you want.  Looking into it, I note that __d_lookup() does a 32-bit
hash compare before doing a string compare, so the memcmp should almost
always succeed.  (Failures are due to hash collisions and rename races.)

> I'd love to do everything with 8 byte loads, do the component
> separation and hashing at the same time as copy from user, and
> have the padded and aligned component strings and their hash
> available... but complexity.

It actually doesn't seem that difficult to do in fs/namei.c:do_getname
via a heavily hacked strncpy_from_user.  Call it strhash_from_user, and
it copies the string, while accumulating the hash and the length,
until it hits a nul or /.  It has to return the length, the hash,
and the termination condition: nul, /, or space exhausted.

Then you could arrange for each component to be padded so that it
starts aligned.

(The hash and length can be stored directly in the qstr.  The length is
only otherwise needed for components that end in /, so you could return
a magic negative length in those cases.)

If it matters, Bob Jenkins wrote a "one-at-a-time" hash function
that is actually sligthy less work per character than the current
partial_name_hash.  (Two shifts, two adds and a *11 gets turned into
two shifts, two adds, and and an XOR)

static inline u32 __attribute__((const))
partial_name_hash(unsigned char c, u32 hash)
{
	hash += c;
	hash += hash << 10;
	hash ^= hash >> 6;
	return hash;
}

static inline u32 end_name_hash(u32 hash)
{
	hash += hash << 3;
	hash ^= hash >> 11;
	hash += hash << 15;
	return hash;
}

(I note that there's zero reason for the current partial_name_hash to
use an unsigned long intermediate value, since the high 32 bits have no
effect on the final result.  It just forces unnecessary REX prefixes.)

The main problem with initializing all the qstr structures early is that
it can lead to an oddly dynamic PATH_MAX and/or a type of kernel
DOS by passing in paths with many short directory names that would need
maximal padding.

> On my Westmere system, time to do a stat is 640 cycles plus 10
> cycles for every byte in the string (this cost holds perfectly
> from 1 byte name up to 32 byte names in my test range).
> `git diff` average path name strings are 31 bytes, although this
> is much less cache friendly, and over several components (my
> test is just a single component).

Thank you for this useful real-world number!
I hadn't realized the base cost was so low.

  reply	other threads:[~2010-12-19 17:06 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-12-18 22:54 Big git diff speedup by avoiding x86 "fast string" memcmp George Spelvin
2010-12-19 14:28 ` Boaz Harrosh
2010-12-19 15:46 ` Nick Piggin
2010-12-19 17:06   ` George Spelvin [this message]
2010-12-21  9:26     ` Nick Piggin
  -- strict thread matches above, loose matches on Subject: below --
2010-12-09  7:09 Nick Piggin
2010-12-09 13:37 ` Borislav Petkov
2010-12-10  2:38   ` Nick Piggin
2010-12-10  4:27 ` Nick Piggin
2010-12-10 14:23 ` J. R. Okajima
2010-12-13  1:45   ` Nick Piggin
2010-12-13  7:29     ` J. R. Okajima
2010-12-13  8:25       ` Nick Piggin
2010-12-14 19:01         ` J. R. Okajima
2010-12-15  4:06           ` Nick Piggin
2010-12-15  5:57             ` J. R. Okajima
2010-12-15 13:15             ` Boaz Harrosh
2010-12-15 18:00               ` David Miller
2010-12-16  9:53                 ` Boaz Harrosh
2010-12-16 13:13                   ` Nick Piggin
2010-12-16 14:03                     ` Boaz Harrosh
2010-12-16 14:15                       ` Nick Piggin
2010-12-16 16:51                   ` Linus Torvalds
2010-12-16 17:57                   ` David Miller
2010-12-15  4:38         ` Américo Wang
2010-12-15  5:54           ` Nick Piggin
2010-12-15  7:12             ` Linus Torvalds
2010-12-15 23:09 ` Tony Luck
2010-12-16  2:34   ` Nick Piggin

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=20101219170646.482.qmail@science.horizon.com \
    --to=linux@horizon.com \
    --cc=bharrosh@panasas.com \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=npiggin@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).