sunrpc: include gid in the rpc_cred_cache hash

Commit Message

Frank Sorenson Sept. 16, 2016, 8:12 p.m. UTC

The current rpc_cred_cache hashtable uses only the uid in the hash
computation.  rpc_creds created with the same uid but different
gids will all go on the same hash chain.

In certain usage patterns, such as the following, this can lead to
extremely long hash chains for these uids, while the rest of the
hashtable remains nearly empty. This causes very high cpu usage
in rpcauth_lookup_credcache, and slow performance for that uid.

    for (i = 0 ; i < 100000 ; i++) {
        setregid(-1, i);
        stat(path, &st);
    }

Add the gid to the hash algorithm to distribute the rpc_creds
throughout the cache to avoid long individual hash chains.

Signed-off-by: Frank Sorenson <[e-mail]>

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Jeff Layton Sept. 16, 2016, 8:42 p.m. UTC | #1

On Fri, 2016-09-16 at 15:12 -0500, Frank Sorenson wrote:
> The current rpc_cred_cache hashtable uses only the uid in the hash
> computation.  rpc_creds created with the same uid but different
> gids will all go on the same hash chain.
> 
> In certain usage patterns, such as the following, this can lead to
> extremely long hash chains for these uids, while the rest of the
> hashtable remains nearly empty. This causes very high cpu usage
> in rpcauth_lookup_credcache, and slow performance for that uid.
> 
>     for (i = 0 ; i < 100000 ; i++) {
>         setregid(-1, i);
>         stat(path, &st);
>     }
> 
> Add the gid to the hash algorithm to distribute the rpc_creds
> throughout the cache to avoid long individual hash chains.
> 
> > Signed-off-by: Frank Sorenson <[e-mail]>
> 
> diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
> index a7e42f9..2260e58 100644
> --- a/net/sunrpc/auth.c
> +++ b/net/sunrpc/auth.c
> @@ -538,6 +538,14 @@ rpcauth_cache_enforce_limit(void)
> >  	rpcauth_cache_do_shrink(nr_to_scan);
>  }
>  
> +static unsigned int
> +rpcauth_hash_acred(struct auth_cred *acred, unsigned int hashbits)
> +{
> > +	return hash_64(from_kgid(&init_user_ns, acred->gid) |
> > +		(from_kuid(&init_user_ns, acred->uid) << (sizeof(gid_t) * 8)),
> > +		hashbits);
> +}
> +
>  /*
>   * Look up a process' credentials in the authentication cache
>   */
> @@ -551,7 +559,7 @@ rpcauth_lookup_credcache(struct rpc_auth *auth, struct auth_cred * acred,
> >  			*entry, *new;
> >  	unsigned int nr;
>  
> > -	nr = hash_long(from_kuid(&init_user_ns, acred->uid), cache->hashbits);
> > +	nr = rpcauth_hash_acred(acred, cache->hashbits);
>  
> >  	rcu_read_lock();
> >  	hlist_for_each_entry_rcu(entry, &cache->hashtable[nr], cr_hash) {
> 

Nice work.

Reviewed-by: Jeff Layton <[e-mail]>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

kernel test robot Sept. 16, 2016, 9:10 p.m. UTC | #2

Hi Frank,

[auto build test WARNING on nfs/linux-next]
[also build test WARNING on v4.8-rc6 next-20160916]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/Frank-Sorenson/sunrpc-include-gid-in-the-rpc_cred_cache-hash/20160917-042716
base:   git://git.linux-nfs.org/projects/trondmy/linux-nfs.git linux-next
config: microblaze-mmu_defconfig (attached as .config)
compiler: microblaze-linux-gcc (GCC) 6.2.0
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=microblaze 

All warnings (new ones prefixed by >>):

   net/sunrpc/auth.c: In function 'rpcauth_hash_acred':
>> net/sunrpc/auth.c:545:41: warning: left shift count >= width of type [-Wshift-count-overflow]
      (from_kuid(&init_user_ns, acred->uid) << (sizeof(gid_t) * 8)),
                                            ^~

vim +545 net/sunrpc/auth.c

   529		unsigned long diff;
   530		unsigned int nr_to_scan;
   531	
   532		if (number_cred_unused <= auth_max_cred_cachesize)
   533			return;
   534		diff = number_cred_unused - auth_max_cred_cachesize;
   535		nr_to_scan = 100;
   536		if (diff < nr_to_scan)
   537			nr_to_scan = diff;
   538		rpcauth_cache_do_shrink(nr_to_scan);
   539	}
   540	
   541	static unsigned int
   542	rpcauth_hash_acred(struct auth_cred *acred, unsigned int hashbits)
   543	{
   544		return hash_64(from_kgid(&init_user_ns, acred->gid) |
 > 545			(from_kuid(&init_user_ns, acred->uid) << (sizeof(gid_t) * 8)),
   546			hashbits);
   547	}
   548	
   549	/*
   550	 * Look up a process' credentials in the authentication cache
   551	 */
   552	struct rpc_cred *
   553	rpcauth_lookup_credcache(struct rpc_auth *auth, struct auth_cred * acred,

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Trond Myklebust Sept. 16, 2016, 9:37 p.m. UTC | #3

> On Sep 16, 2016, at 16:12, Frank Sorenson <[e-mail]> wrote:
> 
> The current rpc_cred_cache hashtable uses only the uid in the hash
> computation.  rpc_creds created with the same uid but different
> gids will all go on the same hash chain.
> 
> In certain usage patterns, such as the following, this can lead to
> extremely long hash chains for these uids, while the rest of the
> hashtable remains nearly empty. This causes very high cpu usage
> in rpcauth_lookup_credcache, and slow performance for that uid.
> 
>    for (i = 0 ; i < 100000 ; i++) {
>        setregid(-1, i);
>        stat(path, &st);
>    }
> 
> Add the gid to the hash algorithm to distribute the rpc_creds
> throughout the cache to avoid long individual hash chains.
> 
> Signed-off-by: Frank Sorenson <[e-mail]>
> 
> diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
> index a7e42f9..2260e58 100644
> --- a/net/sunrpc/auth.c
> +++ b/net/sunrpc/auth.c
> @@ -538,6 +538,14 @@ rpcauth_cache_enforce_limit(void)
> 	rpcauth_cache_do_shrink(nr_to_scan);
> }
> 
> +static unsigned int
> +rpcauth_hash_acred(struct auth_cred *acred, unsigned int hashbits)
> +{
> +	return hash_64(from_kgid(&init_user_ns, acred->gid) |
> +		(from_kuid(&init_user_ns, acred->uid) << (sizeof(gid_t) * 8)),
> +		hashbits);
> +}
> +

NACK. The choice of only using the uid when hashing was deliberate; RPCSEC_GSS is keyed only on the uid… 
If you want to do this in order to accelerate AUTH_SYS lookups, then you need to push the hashing down to the auth flavour ops.

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Frank Sorenson Sept. 19, 2016, 1:10 p.m. UTC | #4

----- Original Message -----
> From: "Trond Myklebust" <[e-mail]>
> To: "Frank Sorenson" <[e-mail]>
> Cc: "List Linux NFS Mailing" <[e-mail]>
> Sent: Friday, September 16, 2016 4:37:39 PM
> Subject: Re: [PATCH] sunrpc: include gid in the rpc_cred_cache hash

> > +rpcauth_hash_acred(struct auth_cred *acred, unsigned int hashbits)
> > +{
> > +	return hash_64(from_kgid(&init_user_ns, acred->gid) |
> > +		(from_kuid(&init_user_ns, acred->uid) << (sizeof(gid_t) * 8)),
> > +		hashbits);
> > +}
> > +

> NACK. The choice of only using the uid when hashing was deliberate;
> RPCSEC_GSS is keyed only on the uid…
> If you want to do this in order to accelerate AUTH_SYS lookups, then you need
> to push the hashing down to the auth flavour ops.

I recognize that RPCSEC_GSS only uses the uid as a key.  However, RPCSEC_GSS
calls rpcauth_lookup_credcache with an auth_cred, just like AUTH_SYS, only with
the gid set to 0.  Including the gid in the hash has no effect on RPCSEC_GSS;
if the function is flipped to shift the gid instead of the uid, it even hashes
to the same result as it did previously.

Adding a shift and bitwise OR to the hash is more straightforward and
efficient than adding the logic to provide a per-auth flavour hash op that
differs only in that it doesn't shift and OR a 0 value.

Or are there additional benefits to be gained from each having its own hash
function?


Thanks,

Frank
--
Frank Sorenson
[e-mail]
Senior Software Maintenance Engineer
Global Support Services - filesystems
Red Hat
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Trond Myklebust Sept. 19, 2016, 2:49 p.m. UTC | #5

> On Sep 19, 2016, at 09:10, Frank Sorenson <[e-mail]> wrote:

> 

> 

> ----- Original Message -----

>> From: "Trond Myklebust" <[e-mail]>

>> To: "Frank Sorenson" <[e-mail]>

>> Cc: "List Linux NFS Mailing" <[e-mail]>

>> Sent: Friday, September 16, 2016 4:37:39 PM

>> Subject: Re: [PATCH] sunrpc: include gid in the rpc_cred_cache hash

> 

>>> +rpcauth_hash_acred(struct auth_cred *acred, unsigned int hashbits)

>>> +{

>>> +	return hash_64(from_kgid(&init_user_ns, acred->gid) |

>>> +		(from_kuid(&init_user_ns, acred->uid) << (sizeof(gid_t) * 8)),

>>> +		hashbits);

>>> +}

>>> +

> 

>> NACK. The choice of only using the uid when hashing was deliberate;

>> RPCSEC_GSS is keyed only on the uid…

>> If you want to do this in order to accelerate AUTH_SYS lookups, then you need

>> to push the hashing down to the auth flavour ops.

> 

> I recognize that RPCSEC_GSS only uses the uid as a key.  However, RPCSEC_GSS

> calls rpcauth_lookup_credcache with an auth_cred, just like AUTH_SYS, only with

> the gid set to 0.  Including the gid in the hash has no effect on RPCSEC_GSS;

> if the function is flipped to shift the gid instead of the uid, it even hashes

> to the same result as it did previously.

> 


AFAIK, both generic_bind_cred() and rpcauth_lookupcred() can make indirect calls to gss_lookup_cred() with a bog standard credential (acred->gid == current_cred()->fsgid). Am I missing something?

> Adding a shift and bitwise OR to the hash is more straightforward and

> efficient than adding the logic to provide a per-auth flavour hash op that

> differs only in that it doesn't shift and OR a 0 value.

> 

> Or are there additional benefits to be gained from each having its own hash

> function?

> 

> 

> Thanks,

> 

> Frank

> --

> Frank Sorenson

> [e-mail]

> Senior Software Maintenance Engineer

> Global Support Services - filesystems

> Red Hat

> --

> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in

> the body of a message to [e-mail]

> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Frank Sorenson Sept. 19, 2016, 3:37 p.m. UTC | #6

----- Original Message -----
> From: "Trond Myklebust" <[e-mail]>
> To: "Frank Sorenson" <[e-mail]>
> Cc: "List Linux NFS Mailing" <[e-mail]>
> Sent: Monday, September 19, 2016 9:49:02 AM
> Subject: Re: [PATCH] sunrpc: include gid in the rpc_cred_cache hash

> AFAIK, both generic_bind_cred() and rpcauth_lookupcred() can make indirect
> calls to gss_lookup_cred() with a bog standard credential (acred->gid ==
> current_cred()->fsgid). Am I missing something?

Nope.  I was.  Thanks.

I will change direction and go down the route of hashing in the auth flavour ops.


Thanks,

Frank
--
Frank Sorenson
[e-mail]
Senior Software Maintenance Engineer
Global Support Services - filesystems
Red Hat
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
index a7e42f9..2260e58 100644
--- a/net/sunrpc/auth.c
+++ b/net/sunrpc/auth.c
@@ -538,6 +538,14 @@  rpcauth_cache_enforce_limit(void)
 	rpcauth_cache_do_shrink(nr_to_scan);
 }
 
+static unsigned int
+rpcauth_hash_acred(struct auth_cred *acred, unsigned int hashbits)
+{
+	return hash_64(from_kgid(&init_user_ns, acred->gid) |
+		(from_kuid(&init_user_ns, acred->uid) << (sizeof(gid_t) * 8)),
+		hashbits);
+}
+
 /*
  * Look up a process' credentials in the authentication cache
  */
@@ -551,7 +559,7 @@  rpcauth_lookup_credcache(struct rpc_auth *auth, struct auth_cred * acred,
 			*entry, *new;
 	unsigned int nr;
 
-	nr = hash_long(from_kuid(&init_user_ns, acred->uid), cache->hashbits);
+	nr = rpcauth_hash_acred(acred, cache->hashbits);
 
 	rcu_read_lock();
 	hlist_for_each_entry_rcu(entry, &cache->hashtable[nr], cr_hash) {