On Wed, 2012-06-27 at 11:03 -0700, Andi Kleen wrote:
> > > Can you try this patch?
> >
> > Thank you! This patch brings %sys down to 24%, a pretty large improvement!
>
> I redid the patch now and fixed the race Trond pointed out.
>
> Fengguang, can you retest please? Trond, does it look good now?
Hi Andi,
There is still a race there: if your thread does atomic_add_return() ==
1, then a second thread can scan, do the atomic_add_return() and get a
value of 2, in which case it won't check for HASHED...
Cheers
Trond
> ---
>
> From: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> Date: Sun, 24 Jun 2012 14:31:06 -0700
> Subject: [PATCH] sunrpc: Improve spinlocks in credential lookup path v2
>
> Fengguang noticed that rpcauth_lookup_credcache has high lock contention
> on the nfs server when doing kernel builds on nfsroot.
>
> The only reason the spinlock is needed in the lockup path is to
> synchronize with the garbage collector. First the object itself
> is stable because it's RCU'ed.
>
> So now we use an atomic_add_return and check for the 0 reference
> count case. When the count was 0 assume the garbage collector
> is active on this entry and take the lock and recheck.
>
> Otherwise the path is lockless.
>
> v2: Add GC race check based on Trond's feedback
> Cc: Trond.Myklebust@xxxxxxxxxx
> Reported-by: Fengguang Wu
> Signed-off-by: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> ---
> net/sunrpc/auth.c | 19 +++++++++++++++----
> 1 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
> index 727e506..645769e 100644
> --- a/net/sunrpc/auth.c
> +++ b/net/sunrpc/auth.c
> @@ -364,13 +364,24 @@ rpcauth_lookup_credcache(struct rpc_auth *auth, struct auth_cred * acred,
> hlist_for_each_entry_rcu(entry, pos, &cache->hashtable[nr], cr_hash) {
> if (!entry->cr_ops->crmatch(acred, entry, flags))
> continue;
> - spin_lock(&cache->lock);
> if (test_bit(RPCAUTH_CRED_HASHED, &entry->cr_flags) == 0) {
> - spin_unlock(&cache->lock);
> continue;
> }
> - cred = get_rpccred(entry);
> - spin_unlock(&cache->lock);
> + cred = entry;
> + if (atomic_add_return(1, &cred->cr_count) == 1) {
> + /*
> + * When the count was 0 we could race with the GC.
> + * Take the lock and recheck.
> + */
> + spin_lock(&cache->lock);
> + if (!test_bit(RPCAUTH_CRED_HASHED, &entry->cr_flags)) {
> + /* Lost the race. */
> + atomic_dec(&cred->cr_count);
> + spin_unlock(&cache->lock);
> + continue;
> + }
> + spin_unlock(&cache->lock);
> + }
> break;
> }
> rcu_read_unlock();
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust@xxxxxxxxxx
www.netapp.com
��.n��������+%������w��{.n�����{��w��� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
