LinuxLists.cc - [PATCH 03/10] <linux/sunrpc/svcauth.h>: Define hash_str() in terms of hash

2016-05-25 07:26:32

Subject: [PATCH 03/10] <linux/sunrpc/svcauth.h>: Define hash_str() in terms of hash_string()

2016-05-26 18:45:43

Subject: Re: [PATCH RESEND 03/10] <linux/sunrpc/svcauth.h>: Define hash_str() in terms of hash_string()

On Thu, May 26, 2016 at 02:39:39PM -0400, George Spelvin wrote:
> Finally, the first use of previous two patches: Eliminate the
> separate ad-hoc string hash functions in the sunrpc code.
>
> This also eliminates the only caller of hash_long which asks for
> more than 32 bits of output.
>
> sunrpc guys: Is it okay if I send this to Linus directly?

Sorry for ignoring this.... Looks straightforward to me; for what it's
worth:

Acked-by: J. Bruce Fields <[email protected]>

If you have a git branch I could fetch, I could also run my usual tests
on it. (Which would only help in the unlikely case this breaks nfs
completely somehow--they certainly wouldn't catch some hash performance
regressoin.)

--b.

>
> Signed-off-by: George Spelvin <[email protected]>
> Cc: "J. Bruce Fields" <[email protected]>
> Cc: Jeff Layton <[email protected]>
> Cc: [email protected]
> ---
> I have't heard anything from the linix-NFS crowd, so this is a re-ping.
>
> I've been having some e-mail troubles leading to increased spam scores
> due to a recent domain name change, so this is a resend from a different
> address in case the previous copy got binned.
>
> I'll happily send along the entire patch series, but the tl;dr is this
> is part of a patch series that improves the <linux/hash.h> and fs/namei.c
> hash functions, implementing Linus's suggestion:
>
> On Mon, 02 May 2016 at 09:24:21, Linus Torvalds wrote:
> > That said, I actually think hash_str() should be killed entirely.
> > Better just use what we use for pathnames: full_name_hash() (which
> > gets a pointer and length) and hash_name (which gets the string).
> >
> > Those functions do the "word-at-a-time" optimization, and they should
> > do a good enough job. If they aren't, we should fix them, because they
> > are a hell of a lot more important than anything that the svcauth code
> > does.
>
> He had a typically fulminating comment about the hard-to-predict if()
> branch in the inner loop.
>
> include/linux/sunrpc/svcauth.h | 36 +++++-------------------------------
> 1 file changed, 5 insertions(+), 31 deletions(-)
>
> diff --git a/include/linux/sunrpc/svcauth.h b/include/linux/sunrpc/svcauth.h
> index c00f53a4..ef2b2552 100644
> --- a/include/linux/sunrpc/svcauth.h
> +++ b/include/linux/sunrpc/svcauth.h
> @@ -16,6 +16,7 @@
> #include <linux/sunrpc/cache.h>
> #include <linux/sunrpc/gss_api.h>
> #include <linux/hash.h>
> +#include <linux/stringhash.h>
> #include <linux/cred.h>
>
> struct svc_cred {
> @@ -165,41 +166,14 @@ extern int svcauth_unix_set_client(struct svc_rqst *rqstp);
> extern int unix_gid_cache_create(struct net *net);
> extern void unix_gid_cache_destroy(struct net *net);
>
> -static inline unsigned long hash_str(char *name, int bits)
> +static inline unsigned long hash_str(char const *name, int bits)
> {
> - unsigned long hash = 0;
> - unsigned long l = 0;
> - int len = 0;
> - unsigned char c;
> - do {
> - if (unlikely(!(c = *name++))) {
> - c = (char)len; len = -1;
> - }
> - l = (l << 8) | c;
> - len++;
> - if ((len & (BITS_PER_LONG/8-1))==0)
> - hash = hash_long(hash^l, BITS_PER_LONG);
> - } while (len);
> - return hash >> (BITS_PER_LONG - bits);
> + return hash_32(hashlen_hash(hash_string(name)), bits);
> }
>
> -static inline unsigned long hash_mem(char *buf, int length, int bits)
> +static inline unsigned long hash_mem(char const *buf, int length, int bits)
> {
> - unsigned long hash = 0;
> - unsigned long l = 0;
> - int len = 0;
> - unsigned char c;
> - do {
> - if (len == length) {
> - c = (char)len; len = -1;
> - } else
> - c = *buf++;
> - l = (l << 8) | c;
> - len++;
> - if ((len & (BITS_PER_LONG/8-1))==0)
> - hash = hash_long(hash^l, BITS_PER_LONG);
> - } while (len);
> - return hash >> (BITS_PER_LONG - bits);
> + return hash_32(full_name_hash(buf, length), bits);
> }
>
> #endif /* __KERNEL__ */
> --
> 2.8.1
>

2016-05-27 16:26:21

by George Spelvin

[permalink] [raw]

Subject: [PATCH RESEND 03/10] <linux/sunrpc/svcauth.h>: Define hash_str() in terms of hash_string()

Finally, the first use of previous two patches: Eliminate the
separate ad-hoc string hash functions in the sunrpc code.

This also eliminates the only caller of hash_long which asks for
more than 32 bits of output.

sunrpc guys: Is it okay if I send this to Linus directly?

Signed-off-by: George Spelvin <[email protected]>
Cc: "J. Bruce Fields" <[email protected]>
Cc: Jeff Layton <[email protected]>
Cc: [email protected]
---
I have't heard anything from the linix-NFS crowd, so this is a re-ping.

I've been having some e-mail troubles leading to increased spam scores
due to a recent domain name change, so this is a resend from a different
address in case the previous copy got binned.

I'll happily send along the entire patch series, but the tl;dr is this
is part of a patch series that improves the <linux/hash.h> and fs/namei.c
hash functions, implementing Linus's suggestion:

On Mon, 02 May 2016 at 09:24:21, Linus Torvalds wrote:
> That said, I actually think hash_str() should be killed entirely.
> Better just use what we use for pathnames: full_name_hash() (which
> gets a pointer and length) and hash_name (which gets the string).
>
> Those functions do the "word-at-a-time" optimization, and they should
> do a good enough job. If they aren't, we should fix them, because they
> are a hell of a lot more important than anything that the svcauth code
> does.

He had a typically fulminating comment about the hard-to-predict if()
branch in the inner loop.

include/linux/sunrpc/svcauth.h | 36 +++++-------------------------------
1 file changed, 5 insertions(+), 31 deletions(-)

diff --git a/include/linux/sunrpc/svcauth.h b/include/linux/sunrpc/svcauth.h
index c00f53a4..ef2b2552 100644
--- a/include/linux/sunrpc/svcauth.h
+++ b/include/linux/sunrpc/svcauth.h
@@ -16,6 +16,7 @@
#include <linux/sunrpc/cache.h>
#include <linux/sunrpc/gss_api.h>
#include <linux/hash.h>
+#include <linux/stringhash.h>
#include <linux/cred.h>

struct svc_cred {
@@ -165,41 +166,14 @@ extern int svcauth_unix_set_client(struct svc_rqst *rqstp);
extern int unix_gid_cache_create(struct net *net);
extern void unix_gid_cache_destroy(struct net *net);

-static inline unsigned long hash_str(char *name, int bits)
+static inline unsigned long hash_str(char const *name, int bits)
{
- unsigned long hash = 0;
- unsigned long l = 0;
- int len = 0;
- unsigned char c;
- do {
- if (unlikely(!(c = *name++))) {
- c = (char)len; len = -1;
- }
- l = (l << 8) | c;
- len++;
- if ((len & (BITS_PER_LONG/8-1))==0)
- hash = hash_long(hash^l, BITS_PER_LONG);
- } while (len);
- return hash >> (BITS_PER_LONG - bits);
+ return hash_32(hashlen_hash(hash_string(name)), bits);
}

-static inline unsigned long hash_mem(char *buf, int length, int bits)
+static inline unsigned long hash_mem(char const *buf, int length, int bits)
{
- unsigned long hash = 0;
- unsigned long l = 0;
- int len = 0;
- unsigned char c;
- do {
- if (len == length) {
- c = (char)len; len = -1;
- } else
- c = *buf++;
- l = (l << 8) | c;
- len++;
- if ((len & (BITS_PER_LONG/8-1))==0)
- hash = hash_long(hash^l, BITS_PER_LONG);
- } while (len);
- return hash >> (BITS_PER_LONG - bits);
+ return hash_32(full_name_hash(buf, length), bits);
}

#endif /* __KERNEL__ */
--
2.8.1