[PATCH] sunrpc: include gid in the rpc_cred_cache hash

[PATCH] sunrpc: include gid in the rpc_cred_cache hash

[Frank Sorenson]

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 <sorenson@redhat.com>

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) {

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

[Jeff Layton]

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 <sorenson@redhat.com>
> 
> 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 <jlayton@redhat.com>

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2328 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 12324 bytes --]

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

[Trond Myklebust]

> On Sep 16, 2016, at 16:12, Frank Sorenson <sorenson@redhat.com> wrote:
>=20
> 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.
>=20
> 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.
>=20
>    for (i =3D 0 ; i < 100000 ; i++) {
>        setregid(-1, i);
>        stat(path, &st);
>    }
>=20
> Add the gid to the hash algorithm to distribute the rpc_creds
> throughout the cache to avoid long individual hash chains.
>=20
> Signed-off-by: Frank Sorenson <sorenson@redhat.com>
>=20
> 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)
> =09rpcauth_cache_do_shrink(nr_to_scan);
> }
>=20
> +static unsigned int
> +rpcauth_hash_acred(struct auth_cred *acred, unsigned int hashbits)
> +{
> +=09return hash_64(from_kgid(&init_user_ns, acred->gid) |
> +=09=09(from_kuid(&init_user_ns, acred->uid) << (sizeof(gid_t) * 8)),
> +=09=09hashbits);
> +}
> +

NACK. The choice of only using the uid when hashing was deliberate; RPCSEC_=
GSS is keyed only on the uid=85=20
If you want to do this in order to accelerate AUTH_SYS lookups, then you ne=
ed to push the hashing down to the auth flavour ops.

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

[Frank Sorenson]

----- Original Message -----
> From: "Trond Myklebust" <trondmy@primarydata.com>
> To: "Frank Sorenson" <sorenson@redhat.com>
> Cc: "List Linux NFS Mailing" <linux-nfs@vger.kernel.org>
> 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
sorenson@redhat.com
Senior Software Maintenance Engineer
Global Support Services - filesystems
Red Hat

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

[Trond Myklebust]

DQo+IE9uIFNlcCAxOSwgMjAxNiwgYXQgMDk6MTAsIEZyYW5rIFNvcmVuc29uIDxzb3JlbnNvbkBy
ZWRoYXQuY29tPiB3cm90ZToNCj4gDQo+IA0KPiAtLS0tLSBPcmlnaW5hbCBNZXNzYWdlIC0tLS0t
DQo+PiBGcm9tOiAiVHJvbmQgTXlrbGVidXN0IiA8dHJvbmRteUBwcmltYXJ5ZGF0YS5jb20+DQo+
PiBUbzogIkZyYW5rIFNvcmVuc29uIiA8c29yZW5zb25AcmVkaGF0LmNvbT4NCj4+IENjOiAiTGlz
dCBMaW51eCBORlMgTWFpbGluZyIgPGxpbnV4LW5mc0B2Z2VyLmtlcm5lbC5vcmc+DQo+PiBTZW50
OiBGcmlkYXksIFNlcHRlbWJlciAxNiwgMjAxNiA0OjM3OjM5IFBNDQo+PiBTdWJqZWN0OiBSZTog
W1BBVENIXSBzdW5ycGM6IGluY2x1ZGUgZ2lkIGluIHRoZSBycGNfY3JlZF9jYWNoZSBoYXNoDQo+
IA0KPj4+ICtycGNhdXRoX2hhc2hfYWNyZWQoc3RydWN0IGF1dGhfY3JlZCAqYWNyZWQsIHVuc2ln
bmVkIGludCBoYXNoYml0cykNCj4+PiArew0KPj4+ICsJcmV0dXJuIGhhc2hfNjQoZnJvbV9rZ2lk
KCZpbml0X3VzZXJfbnMsIGFjcmVkLT5naWQpIHwNCj4+PiArCQkoZnJvbV9rdWlkKCZpbml0X3Vz
ZXJfbnMsIGFjcmVkLT51aWQpIDw8IChzaXplb2YoZ2lkX3QpICogOCkpLA0KPj4+ICsJCWhhc2hi
aXRzKTsNCj4+PiArfQ0KPj4+ICsNCj4gDQo+PiBOQUNLLiBUaGUgY2hvaWNlIG9mIG9ubHkgdXNp
bmcgdGhlIHVpZCB3aGVuIGhhc2hpbmcgd2FzIGRlbGliZXJhdGU7DQo+PiBSUENTRUNfR1NTIGlz
IGtleWVkIG9ubHkgb24gdGhlIHVpZOKApg0KPj4gSWYgeW91IHdhbnQgdG8gZG8gdGhpcyBpbiBv
cmRlciB0byBhY2NlbGVyYXRlIEFVVEhfU1lTIGxvb2t1cHMsIHRoZW4geW91IG5lZWQNCj4+IHRv
IHB1c2ggdGhlIGhhc2hpbmcgZG93biB0byB0aGUgYXV0aCBmbGF2b3VyIG9wcy4NCj4gDQo+IEkg
cmVjb2duaXplIHRoYXQgUlBDU0VDX0dTUyBvbmx5IHVzZXMgdGhlIHVpZCBhcyBhIGtleS4gIEhv
d2V2ZXIsIFJQQ1NFQ19HU1MNCj4gY2FsbHMgcnBjYXV0aF9sb29rdXBfY3JlZGNhY2hlIHdpdGgg
YW4gYXV0aF9jcmVkLCBqdXN0IGxpa2UgQVVUSF9TWVMsIG9ubHkgd2l0aA0KPiB0aGUgZ2lkIHNl
dCB0byAwLiAgSW5jbHVkaW5nIHRoZSBnaWQgaW4gdGhlIGhhc2ggaGFzIG5vIGVmZmVjdCBvbiBS
UENTRUNfR1NTOw0KPiBpZiB0aGUgZnVuY3Rpb24gaXMgZmxpcHBlZCB0byBzaGlmdCB0aGUgZ2lk
IGluc3RlYWQgb2YgdGhlIHVpZCwgaXQgZXZlbiBoYXNoZXMNCj4gdG8gdGhlIHNhbWUgcmVzdWx0
IGFzIGl0IGRpZCBwcmV2aW91c2x5Lg0KPiANCg0KQUZBSUssIGJvdGggZ2VuZXJpY19iaW5kX2Ny
ZWQoKSBhbmQgcnBjYXV0aF9sb29rdXBjcmVkKCkgY2FuIG1ha2UgaW5kaXJlY3QgY2FsbHMgdG8g
Z3NzX2xvb2t1cF9jcmVkKCkgd2l0aCBhIGJvZyBzdGFuZGFyZCBjcmVkZW50aWFsIChhY3JlZC0+
Z2lkID09IGN1cnJlbnRfY3JlZCgpLT5mc2dpZCkuIEFtIEkgbWlzc2luZyBzb21ldGhpbmc/DQoN
Cj4gQWRkaW5nIGEgc2hpZnQgYW5kIGJpdHdpc2UgT1IgdG8gdGhlIGhhc2ggaXMgbW9yZSBzdHJh
aWdodGZvcndhcmQgYW5kDQo+IGVmZmljaWVudCB0aGFuIGFkZGluZyB0aGUgbG9naWMgdG8gcHJv
dmlkZSBhIHBlci1hdXRoIGZsYXZvdXIgaGFzaCBvcCB0aGF0DQo+IGRpZmZlcnMgb25seSBpbiB0
aGF0IGl0IGRvZXNuJ3Qgc2hpZnQgYW5kIE9SIGEgMCB2YWx1ZS4NCj4gDQo+IE9yIGFyZSB0aGVy
ZSBhZGRpdGlvbmFsIGJlbmVmaXRzIHRvIGJlIGdhaW5lZCBmcm9tIGVhY2ggaGF2aW5nIGl0cyBv
d24gaGFzaA0KPiBmdW5jdGlvbj8NCj4gDQo+IA0KPiBUaGFua3MsDQo+IA0KPiBGcmFuaw0KPiAt
LQ0KPiBGcmFuayBTb3JlbnNvbg0KPiBzb3JlbnNvbkByZWRoYXQuY29tDQo+IFNlbmlvciBTb2Z0
d2FyZSBNYWludGVuYW5jZSBFbmdpbmVlcg0KPiBHbG9iYWwgU3VwcG9ydCBTZXJ2aWNlcyAtIGZp
bGVzeXN0ZW1zDQo+IFJlZCBIYXQNCj4gLS0NCj4gVG8gdW5zdWJzY3JpYmUgZnJvbSB0aGlzIGxp
c3Q6IHNlbmQgdGhlIGxpbmUgInVuc3Vic2NyaWJlIGxpbnV4LW5mcyIgaW4NCj4gdGhlIGJvZHkg
b2YgYSBtZXNzYWdlIHRvIG1ham9yZG9tb0B2Z2VyLmtlcm5lbC5vcmcNCj4gTW9yZSBtYWpvcmRv
bW8gaW5mbyBhdCAgaHR0cDovL3ZnZXIua2VybmVsLm9yZy9tYWpvcmRvbW8taW5mby5odG1sDQoN
Cg==

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

[Frank Sorenson]

----- Original Message -----
> From: "Trond Myklebust" <trondmy@primarydata.com>
> To: "Frank Sorenson" <sorenson@redhat.com>
> Cc: "List Linux NFS Mailing" <linux-nfs@vger.kernel.org>
> 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
sorenson@redhat.com
Senior Software Maintenance Engineer
Global Support Services - filesystems
Red Hat