[PATCH v4 0/8] SUNRPC: make rpcbind clients allocated and destroyed dynamically

[PATCH v4 0/8] SUNRPC: make rpcbind clients allocated and destroyed dynamically

[Stanislav Kinsbursky]

v4:
1) creation and destruction on rpcbind clients now depends on service program
versions "vs_hidden" flag.

This patch is required for further RPC layer virtualization, because rpcbind
clients have to be per network namespace.
To achive this, we have to untie network namespace from rpcbind clients sockets.
The idea of this patch set is to make rpcbind clients non-static. I.e. rpcbind
clients will be created during first RPC service creation, and destroyed when
last RPC service is stopped.
With this patch set rpcbind clients can be virtualized easely.


The following series consists of:

---

Stanislav Kinsbursky (8):
      SUNRPC: introduce helpers for reference counted rpcbind clients
      SUNRPC: use rpcbind reference counting helpers
      SUNRPC: introduce svc helpers for prepairing rpcbind infrastructure
      SUNRPC: setup rpcbind clients if service requires it
      SUNRPC: cleanup service destruction
      NFSd: call svc rpcbind cleanup explicitly
      SUNRPC: remove rpcbind clients creation during service registering
      SUNRPC: remove rpcbind clients destruction on module cleanup


 fs/nfsd/nfssvc.c            |    2 +
 include/linux/sunrpc/clnt.h |    2 +
 include/linux/sunrpc/svc.h  |    1 +
 net/sunrpc/rpcb_clnt.c      |   85 ++++++++++++++++++++++++++++---------------
 net/sunrpc/sunrpc_syms.c    |    3 --
 net/sunrpc/svc.c            |   48 +++++++++++++++++++++++-
 6 files changed, 105 insertions(+), 36 deletions(-)

-- 
Signature

[PATCH v4 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients

[Stanislav Kinsbursky]

This helpers will be used for dynamical creation and destruction of rpcbind
clients.
Variable rpcb_users is actually a counter of lauched RPC services. If rpcbind
clients has been created already, then we just increase rpcb_users.

Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>

---
 net/sunrpc/rpcb_clnt.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 50 insertions(+), 0 deletions(-)

diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
index e45d2fb..8724780 100644
--- a/net/sunrpc/rpcb_clnt.c
+++ b/net/sunrpc/rpcb_clnt.c
@@ -114,6 +114,9 @@ static struct rpc_program	rpcb_program;
 static struct rpc_clnt *	rpcb_local_clnt;
 static struct rpc_clnt *	rpcb_local_clnt4;
 
+DEFINE_SPINLOCK(rpcb_clnt_lock);
+unsigned int			rpcb_users;
+
 struct rpcbind_args {
 	struct rpc_xprt *	r_xprt;
 
@@ -161,6 +164,53 @@ static void rpcb_map_release(void *data)
 	kfree(map);
 }
 
+static int rpcb_get_local(void)
+{
+	spin_lock(&rpcb_clnt_lock);
+	if (rpcb_users)
+		rpcb_users++;
+	spin_unlock(&rpcb_clnt_lock);
+
+	return rpcb_users;
+}
+
+void rpcb_put_local(void)
+{
+	struct rpc_clnt *clnt = rpcb_local_clnt;
+	struct rpc_clnt *clnt4 = rpcb_local_clnt4;
+	int shutdown;
+
+	spin_lock(&rpcb_clnt_lock);
+	if (--rpcb_users == 0) {
+		rpcb_local_clnt = NULL;
+		rpcb_local_clnt4 = NULL;
+	}
+	shutdown = !rpcb_users;
+	spin_unlock(&rpcb_clnt_lock);
+
+	if (shutdown) {
+		/*
+		 * cleanup_rpcb_clnt - remove xprtsock's sysctls, unregister
+		 */
+		if (clnt4)
+			rpc_shutdown_client(clnt4);
+		if (clnt)
+			rpc_shutdown_client(clnt);
+	}
+	return;
+}
+
+static void rpcb_set_local(struct rpc_clnt *clnt, struct rpc_clnt *clnt4)
+{
+	/* Protected by rpcb_create_local_mutex */
+	rpcb_local_clnt = clnt;
+	rpcb_local_clnt4 = clnt4;
+	rpcb_users++;
+	dprintk("RPC:       created new rpcb local clients (rpcb_local_clnt: "
+			"%p, rpcb_local_clnt4: %p)\n", rpcb_local_clnt,
+			rpcb_local_clnt4);
+}
+
 /*
  * Returns zero on success, otherwise a negative errno value
  * is returned.

[PATCH v4 2/8] SUNRPC: use rpcbind reference counting helpers

[Stanislav Kinsbursky]

All is simple: we just increase users counter if rpcbind clients has been
created already. Otherwise we create them and set users counter to 1.

Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>

---
 net/sunrpc/rpcb_clnt.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
index 8724780..83634e0 100644
--- a/net/sunrpc/rpcb_clnt.c
+++ b/net/sunrpc/rpcb_clnt.c
@@ -255,9 +255,7 @@ static int rpcb_create_local_unix(void)
 		clnt4 = NULL;
 	}
 
-	/* Protected by rpcb_create_local_mutex */
-	rpcb_local_clnt = clnt;
-	rpcb_local_clnt4 = clnt4;
+	rpcb_set_local(clnt, clnt4);
 
 out:
 	return result;
@@ -309,9 +307,7 @@ static int rpcb_create_local_net(void)
 		clnt4 = NULL;
 	}
 
-	/* Protected by rpcb_create_local_mutex */
-	rpcb_local_clnt = clnt;
-	rpcb_local_clnt4 = clnt4;
+	rpcb_set_local(clnt, clnt4);
 
 out:
 	return result;
@@ -326,11 +322,11 @@ static int rpcb_create_local(void)
 	static DEFINE_MUTEX(rpcb_create_local_mutex);
 	int result = 0;
 
-	if (rpcb_local_clnt)
+	if (rpcb_get_local())
 		return result;
 
 	mutex_lock(&rpcb_create_local_mutex);
-	if (rpcb_local_clnt)
+	if (rpcb_get_local())
 		goto out;
 
 	if (rpcb_create_local_unix() != 0)

[PATCH v4 3/8] SUNRPC: introduce svc helpers for prepairing rpcbind infrastructure

[Stanislav Kinsbursky]

This helpers will be used only for those services, that will send portmapper
registration calls.

Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>

---
 include/linux/sunrpc/clnt.h |    2 ++
 net/sunrpc/rpcb_clnt.c      |    2 +-
 net/sunrpc/svc.c            |   35 +++++++++++++++++++++++++++++++++++
 3 files changed, 38 insertions(+), 1 deletions(-)

diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
index db7bcaf..1eb437d 100644
--- a/include/linux/sunrpc/clnt.h
+++ b/include/linux/sunrpc/clnt.h
@@ -135,6 +135,8 @@ void		rpc_shutdown_client(struct rpc_clnt *);
 void		rpc_release_client(struct rpc_clnt *);
 void		rpc_task_release_client(struct rpc_task *);
 
+int		rpcb_create_local(void);
+void		rpcb_put_local(void);
 int		rpcb_register(u32, u32, int, unsigned short);
 int		rpcb_v4_register(const u32 program, const u32 version,
 				 const struct sockaddr *address,
diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
index 83634e0..64e15d1 100644
--- a/net/sunrpc/rpcb_clnt.c
+++ b/net/sunrpc/rpcb_clnt.c
@@ -317,7 +317,7 @@ out:
  * Returns zero on success, otherwise a negative errno value
  * is returned.
  */
-static int rpcb_create_local(void)
+int rpcb_create_local(void)
 {
 	static DEFINE_MUTEX(rpcb_create_local_mutex);
 	int result = 0;
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 6a69a11..d2d61bf 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -354,6 +354,41 @@ svc_pool_for_cpu(struct svc_serv *serv, int cpu)
 	return &serv->sv_pools[pidx % serv->sv_nrpools];
 }
 
+static int svc_rpcb_setup(struct svc_serv *serv)
+{
+	int err;
+
+	err = rpcb_create_local();
+	if (err)
+		return err;
+
+	/* Remove any stale portmap registrations */
+	svc_unregister(serv);
+	return 0;
+}
+
+static void svc_rpcb_cleanup(struct svc_serv *serv)
+{
+	svc_unregister(serv);
+	rpcb_put_local();
+}
+
+static int svc_uses_rpcbind(struct svc_serv *serv)
+{
+	struct svc_program	*progp;
+	unsigned int		i;
+
+	for (progp = serv->sv_program; progp; progp = progp->pg_next) {
+		for (i = 0; i < progp->pg_nvers; i++) {
+			if (progp->pg_vers[i] == NULL)
+				continue;
+			if (progp->pg_vers[i]->vs_hidden == 0)
+				return 1;
+		}
+	}
+
+	return 0;
+}
 
 /*
  * Create an RPC service

[PATCH v4 4/8] SUNRPC: setup rpcbind clients if service requires it

[Stanislav Kinsbursky]

New function ("svc_uses_rpcbind") will be used to detect, that new service will
send portmapper register calls. For such services we will create rpcbind
clients and remove all stale portmap registrations.
Also, svc_rpcb_cleanup() will be set as sv_shutdown callback for such services
in case of this field wasn't initialized earlier. This will allow to destroy
rpcbind clients when no other users of them left.

Note: Currently, any creating service will be detected as portmap user.
Probably, this is wrong. But now it depends on program versions "vs_hidden"
flag.

Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>

---
 net/sunrpc/svc.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index d2d61bf..918edc3 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -454,8 +454,15 @@ __svc_create(struct svc_program *prog, unsigned int bufsize, int npools,
 		spin_lock_init(&pool->sp_lock);
 	}
 
-	/* Remove any stale portmap registrations */
-	svc_unregister(serv);
+	if (svc_uses_rpcbind(serv)) {
+	       	if (svc_rpcb_setup(serv) < 0) {
+			kfree(serv->sv_pools);
+			kfree(serv);
+			return NULL;
+		}
+		if (!serv->sv_shutdown)
+			serv->sv_shutdown = svc_rpcb_cleanup;
+	}
 
 	return serv;
 }

[PATCH v4 5/8] SUNRPC: cleanup service destruction

[Stanislav Kinsbursky]

svc_unregister() call have to be removed from svc_destroy() since it will be
called in sv_shutdown callback.

Signed-off-by: Stanislav Kinsbursky <skinsbursky-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>

---
 net/sunrpc/svc.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 918edc3..407462f 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -530,7 +530,6 @@ svc_destroy(struct svc_serv *serv)
 	if (svc_serv_is_pooled(serv))
 		svc_pool_map_put();
 
-	svc_unregister(serv);
 	kfree(serv->sv_pools);
 	kfree(serv);
 }

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 6/8] NFSd: call svc rpcbind cleanup explicitly

[Stanislav Kinsbursky]

We have to call svc_rpcb_cleanup() explicitly from nfsd_last_thread() since
this function is registered as service shutdown callback and thus nobody else
will done it for us.

Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>

---
 fs/nfsd/nfssvc.c           |    2 ++
 include/linux/sunrpc/svc.h |    1 +
 net/sunrpc/svc.c           |    3 ++-
 3 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index dc5a1bf..52cd976 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -256,6 +256,8 @@ static void nfsd_last_thread(struct svc_serv *serv)
 	nfsd_serv = NULL;
 	nfsd_shutdown();
 
+	svc_rpcb_cleanup(serv);
+
 	printk(KERN_WARNING "nfsd: last server has exited, flushing export "
 			    "cache\n");
 	nfsd_export_flush();
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 223588a..5e71a30 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -401,6 +401,7 @@ struct svc_procedure {
 /*
  * Function prototypes.
  */
+void svc_rpcb_cleanup(struct svc_serv *serv);
 struct svc_serv *svc_create(struct svc_program *, unsigned int,
 			    void (*shutdown)(struct svc_serv *));
 struct svc_rqst *svc_prepare_thread(struct svc_serv *serv,
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 407462f..252552a 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -367,11 +367,12 @@ static int svc_rpcb_setup(struct svc_serv *serv)
 	return 0;
 }
 
-static void svc_rpcb_cleanup(struct svc_serv *serv)
+void svc_rpcb_cleanup(struct svc_serv *serv)
 {
 	svc_unregister(serv);
 	rpcb_put_local();
 }
+EXPORT_SYMBOL_GPL(svc_rpcb_cleanup);
 
 static int svc_uses_rpcbind(struct svc_serv *serv)
 {

[PATCH v4 7/8] SUNRPC: remove rpcbind clients creation during service registering

[Stanislav Kinsbursky]

We don't need this code since rpcbind clients are creating during RPC service
creation.

Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>

---
 net/sunrpc/rpcb_clnt.c |    9 ---------
 1 files changed, 0 insertions(+), 9 deletions(-)

diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
index 64e15d1..9327305 100644
--- a/net/sunrpc/rpcb_clnt.c
+++ b/net/sunrpc/rpcb_clnt.c
@@ -428,11 +428,6 @@ int rpcb_register(u32 prog, u32 vers, int prot, unsigned short port)
 	struct rpc_message msg = {
 		.rpc_argp	= &map,
 	};
-	int error;
-
-	error = rpcb_create_local();
-	if (error)
-		return error;
 
 	dprintk("RPC:       %sregistering (%u, %u, %d, %u) with local "
 			"rpcbind\n", (port ? "" : "un"),
@@ -568,11 +563,7 @@ int rpcb_v4_register(const u32 program, const u32 version,
 	struct rpc_message msg = {
 		.rpc_argp	= &map,
 	};
-	int error;
 
-	error = rpcb_create_local();
-	if (error)
-		return error;
 	if (rpcb_local_clnt4 == NULL)
 		return -EPROTONOSUPPORT;
 

[PATCH v4 8/8] SUNRPC: remove rpcbind clients destruction on module cleanup

[Stanislav Kinsbursky]

Rpcbind clients destruction during SUNRPC module removing is obsolete since now
those clients are destroying during last RPC service shutdown.

Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>

---
 net/sunrpc/rpcb_clnt.c   |   12 ------------
 net/sunrpc/sunrpc_syms.c |    3 ---
 2 files changed, 0 insertions(+), 15 deletions(-)

diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
index 9327305..80ddf55 100644
--- a/net/sunrpc/rpcb_clnt.c
+++ b/net/sunrpc/rpcb_clnt.c
@@ -1097,15 +1097,3 @@ static struct rpc_program rpcb_program = {
 	.version	= rpcb_version,
 	.stats		= &rpcb_stats,
 };
-
-/**
- * cleanup_rpcb_clnt - remove xprtsock's sysctls, unregister
- *
- */
-void cleanup_rpcb_clnt(void)
-{
-	if (rpcb_local_clnt4)
-		rpc_shutdown_client(rpcb_local_clnt4);
-	if (rpcb_local_clnt)
-		rpc_shutdown_client(rpcb_local_clnt);
-}
diff --git a/net/sunrpc/sunrpc_syms.c b/net/sunrpc/sunrpc_syms.c
index 9d08091..8ec9778 100644
--- a/net/sunrpc/sunrpc_syms.c
+++ b/net/sunrpc/sunrpc_syms.c
@@ -61,8 +61,6 @@ static struct pernet_operations sunrpc_net_ops = {
 
 extern struct cache_detail unix_gid_cache;
 
-extern void cleanup_rpcb_clnt(void);
-
 static int __init
 init_sunrpc(void)
 {
@@ -102,7 +100,6 @@ out:
 static void __exit
 cleanup_sunrpc(void)
 {
-	cleanup_rpcb_clnt();
 	rpcauth_remove_module();
 	cleanup_socket_xprt();
 	svc_cleanup_xprt_sock();

Re: [PATCH v4 4/8] SUNRPC: setup rpcbind clients if service requires it

[Jeff Layton]

On Tue, 20 Sep 2011 14:14:04 +0400
Stanislav Kinsbursky <skinsbursky-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> wrote:

> New function ("svc_uses_rpcbind") will be used to detect, that new service will
> send portmapper register calls. For such services we will create rpcbind
> clients and remove all stale portmap registrations.
> Also, svc_rpcb_cleanup() will be set as sv_shutdown callback for such services
> in case of this field wasn't initialized earlier. This will allow to destroy
> rpcbind clients when no other users of them left.
> 
> Note: Currently, any creating service will be detected as portmap user.
> Probably, this is wrong. But now it depends on program versions "vs_hidden"
> flag.
> 

Yes, I think that nfs4_callback_version4 should also have vs_hidden
set. Currently, it's trying to unregister the service from the
portmapper on shutdown even though it's not registering it. Basically,
any service that sets up its sockets with SVC_SOCK_ANONYMOUS should
also have vs_hidden set on all versions.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 0/8] SUNRPC: make rpcbind clients allocated and destroyed dynamically

[Jeff Layton]

On Tue, 20 Sep 2011 14:13:32 +0400
Stanislav Kinsbursky <skinsbursky-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org> wrote:

> v4:
> 1) creation and destruction on rpcbind clients now depends on service program
> versions "vs_hidden" flag.
> 
> This patch is required for further RPC layer virtualization, because rpcbind
> clients have to be per network namespace.
> To achive this, we have to untie network namespace from rpcbind clients sockets.
> The idea of this patch set is to make rpcbind clients non-static. I.e. rpcbind
> clients will be created during first RPC service creation, and destroyed when
> last RPC service is stopped.
> With this patch set rpcbind clients can be virtualized easely.
> 
> 
> The following series consists of:
> 
> ---
> 
> Stanislav Kinsbursky (8):
>       SUNRPC: introduce helpers for reference counted rpcbind clients
>       SUNRPC: use rpcbind reference counting helpers
>       SUNRPC: introduce svc helpers for prepairing rpcbind infrastructure
>       SUNRPC: setup rpcbind clients if service requires it
>       SUNRPC: cleanup service destruction
>       NFSd: call svc rpcbind cleanup explicitly
>       SUNRPC: remove rpcbind clients creation during service registering
>       SUNRPC: remove rpcbind clients destruction on module cleanup
> 
> 
>  fs/nfsd/nfssvc.c            |    2 +
>  include/linux/sunrpc/clnt.h |    2 +
>  include/linux/sunrpc/svc.h  |    1 +
>  net/sunrpc/rpcb_clnt.c      |   85 ++++++++++++++++++++++++++++---------------
>  net/sunrpc/sunrpc_syms.c    |    3 --
>  net/sunrpc/svc.c            |   48 +++++++++++++++++++++++-
>  6 files changed, 105 insertions(+), 36 deletions(-)
> 

Patchset looks good to me. The only remaining thing I think is to set
vs_hidden on nfs4_callback_version4, but that patch is orthogonal to
this set.

Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients

[Bryan Schumaker]

On 09/20/2011 06:13 AM, Stanislav Kinsbursky wrote:
> This helpers will be used for dynamical creation and destruction of rpcbind
> clients.
> Variable rpcb_users is actually a counter of lauched RPC services. If rpcbind
> clients has been created already, then we just increase rpcb_users.
> 
> Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
> 
> ---
>  net/sunrpc/rpcb_clnt.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 50 insertions(+), 0 deletions(-)
> 
> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
> index e45d2fb..8724780 100644
> --- a/net/sunrpc/rpcb_clnt.c
> +++ b/net/sunrpc/rpcb_clnt.c
> @@ -114,6 +114,9 @@ static struct rpc_program	rpcb_program;
>  static struct rpc_clnt *	rpcb_local_clnt;
>  static struct rpc_clnt *	rpcb_local_clnt4;
>  
> +DEFINE_SPINLOCK(rpcb_clnt_lock);
> +unsigned int			rpcb_users;
> +
>  struct rpcbind_args {
>  	struct rpc_xprt *	r_xprt;
>  
> @@ -161,6 +164,53 @@ static void rpcb_map_release(void *data)
>  	kfree(map);
>  }
>  
> +static int rpcb_get_local(void)
> +{
> +	spin_lock(&rpcb_clnt_lock);
> +	if (rpcb_users)
> +		rpcb_users++;
> +	spin_unlock(&rpcb_clnt_lock);
> +
> +	return rpcb_users;
        ^^^^^^^^^^^^^^^^^^
Is it safe to use this variable outside of the rpcb_clnt_lock?

> +}
> +
> +void rpcb_put_local(void)
> +{
> +	struct rpc_clnt *clnt = rpcb_local_clnt;
> +	struct rpc_clnt *clnt4 = rpcb_local_clnt4;
> +	int shutdown;
> +
> +	spin_lock(&rpcb_clnt_lock);
> +	if (--rpcb_users == 0) {
> +		rpcb_local_clnt = NULL;
> +		rpcb_local_clnt4 = NULL;
> +	}
> +	shutdown = !rpcb_users;
> +	spin_unlock(&rpcb_clnt_lock);
> +
> +	if (shutdown) {
> +		/*
> +		 * cleanup_rpcb_clnt - remove xprtsock's sysctls, unregister
> +		 */
> +		if (clnt4)
> +			rpc_shutdown_client(clnt4);
> +		if (clnt)
> +			rpc_shutdown_client(clnt);
> +	}
> +	return;
> +}
> +
> +static void rpcb_set_local(struct rpc_clnt *clnt, struct rpc_clnt *clnt4)
> +{
> +	/* Protected by rpcb_create_local_mutex */
> +	rpcb_local_clnt = clnt;
> +	rpcb_local_clnt4 = clnt4;
> +	rpcb_users++;
> +	dprintk("RPC:       created new rpcb local clients (rpcb_local_clnt: "
> +			"%p, rpcb_local_clnt4: %p)\n", rpcb_local_clnt,
> +			rpcb_local_clnt4);
> +}
> +
>  /*
>   * Returns zero on success, otherwise a negative errno value
>   * is returned.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v4 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients

[Myklebust, Trond]

> -----Original Message-----
> From: Schumaker, Bryan
> Sent: Tuesday, September 20, 2011 9:05 AM
> To: Stanislav Kinsbursky
> Cc: Myklebust, Trond; linux-nfs@vger.kernel.org; xemul@parallels.com;
> neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> bfields@fieldses.org; davem@davemloft.net
> Subject: Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference
> counted rpcbind clients
> 
> On 09/20/2011 06:13 AM, Stanislav Kinsbursky wrote:
> > This helpers will be used for dynamical creation and destruction of
> > rpcbind clients.
> > Variable rpcb_users is actually a counter of lauched RPC services. If
> > rpcbind clients has been created already, then we just increase rpcb_users.
> >
> > Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
> >
> > ---
> >  net/sunrpc/rpcb_clnt.c |   50
> ++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 50 insertions(+), 0 deletions(-)
> >
> > diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c index
> > e45d2fb..8724780 100644
> > --- a/net/sunrpc/rpcb_clnt.c
> > +++ b/net/sunrpc/rpcb_clnt.c
> > @@ -114,6 +114,9 @@ static struct rpc_program	rpcb_program;
> >  static struct rpc_clnt *	rpcb_local_clnt;
> >  static struct rpc_clnt *	rpcb_local_clnt4;
> >
> > +DEFINE_SPINLOCK(rpcb_clnt_lock);
> > +unsigned int			rpcb_users;
> > +
> >  struct rpcbind_args {
> >  	struct rpc_xprt *	r_xprt;
> >
> > @@ -161,6 +164,53 @@ static void rpcb_map_release(void *data)
> >  	kfree(map);
> >  }
> >
> > +static int rpcb_get_local(void)
> > +{
> > +	spin_lock(&rpcb_clnt_lock);
> > +	if (rpcb_users)
> > +		rpcb_users++;
> > +	spin_unlock(&rpcb_clnt_lock);
> > +
> > +	return rpcb_users;
>         ^^^^^^^^^^^^^^^^^^
> Is it safe to use this variable outside of the rpcb_clnt_lock?
>
Nope. If rpcb_users was zero in the protected section above, nothing guarantees that it will still be zero here, and so the caller may get the (wrong) impression that the counter was incremented.

> > +}
> > +
> > +void rpcb_put_local(void)
> > +{
> > +	struct rpc_clnt *clnt = rpcb_local_clnt;
> > +	struct rpc_clnt *clnt4 = rpcb_local_clnt4;
> > +	int shutdown;
> > +
> > +	spin_lock(&rpcb_clnt_lock);
> > +	if (--rpcb_users == 0) {
> > +		rpcb_local_clnt = NULL;
> > +		rpcb_local_clnt4 = NULL;
> > +	}
> > +	shutdown = !rpcb_users;
> > +	spin_unlock(&rpcb_clnt_lock);
> > +
> > +	if (shutdown) {
> > +		/*
> > +		 * cleanup_rpcb_clnt - remove xprtsock's sysctls, unregister
> > +		 */
> > +		if (clnt4)
> > +			rpc_shutdown_client(clnt4);
> > +		if (clnt)
> > +			rpc_shutdown_client(clnt);
> > +	}
> > +	return;
> > +}
> > +
> > +static void rpcb_set_local(struct rpc_clnt *clnt, struct rpc_clnt
> > +*clnt4) {
> > +	/* Protected by rpcb_create_local_mutex */

Doesn't it  need to be protected by rpcb_clnt_lock too?

> > +	rpcb_local_clnt = clnt;
> > +	rpcb_local_clnt4 = clnt4;
> > +	rpcb_users++;
^^^^^^^^^^^^^^^^^^^

> > +	dprintk("RPC:       created new rpcb local clients (rpcb_local_clnt: "
> > +			"%p, rpcb_local_clnt4: %p)\n", rpcb_local_clnt,
> > +			rpcb_local_clnt4);
> > +}
> > +
> >  /*
> >   * Returns zero on success, otherwise a negative errno value
> >   * is returned.
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs"
> > in the body of a message to majordomo@vger.kernel.org More
> majordomo
> > info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients

[Stanislav Kinsbursky]

20.09.2011 17:15, Myklebust, Trond пишет:
>> -----Original Message-----
>> From: Schumaker, Bryan
>> Sent: Tuesday, September 20, 2011 9:05 AM
>> To: Stanislav Kinsbursky
>> Cc: Myklebust, Trond; linux-nfs@vger.kernel.org; xemul@parallels.com;
>> neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
>> bfields@fieldses.org; davem@davemloft.net
>> Subject: Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference
>> counted rpcbind clients
>>
>> On 09/20/2011 06:13 AM, Stanislav Kinsbursky wrote:
>>> This helpers will be used for dynamical creation and destruction of
>>> rpcbind clients.
>>> Variable rpcb_users is actually a counter of lauched RPC services. If
>>> rpcbind clients has been created already, then we just increase rpcb_users.
>>>
>>> Signed-off-by: Stanislav Kinsbursky<skinsbursky@parallels.com>
>>>
>>> ---
>>>   net/sunrpc/rpcb_clnt.c |   50
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>   1 files changed, 50 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c index
>>> e45d2fb..8724780 100644
>>> --- a/net/sunrpc/rpcb_clnt.c
>>> +++ b/net/sunrpc/rpcb_clnt.c
>>> @@ -114,6 +114,9 @@ static struct rpc_program	rpcb_program;
>>>   static struct rpc_clnt *	rpcb_local_clnt;
>>>   static struct rpc_clnt *	rpcb_local_clnt4;
>>>
>>> +DEFINE_SPINLOCK(rpcb_clnt_lock);
>>> +unsigned int			rpcb_users;
>>> +
>>>   struct rpcbind_args {
>>>   	struct rpc_xprt *	r_xprt;
>>>
>>> @@ -161,6 +164,53 @@ static void rpcb_map_release(void *data)
>>>   	kfree(map);
>>>   }
>>>
>>> +static int rpcb_get_local(void)
>>> +{
>>> +	spin_lock(&rpcb_clnt_lock);
>>> +	if (rpcb_users)
>>> +		rpcb_users++;
>>> +	spin_unlock(&rpcb_clnt_lock);
>>> +
>>> +	return rpcb_users;
>>          ^^^^^^^^^^^^^^^^^^
>> Is it safe to use this variable outside of the rpcb_clnt_lock?
>>
> Nope. If rpcb_users was zero in the protected section above, nothing guarantees that it will still be zero here, and so the caller may get the (wrong) impression that the counter was incremented.
>

Yep, you right. Will fix this races.

>>> +}
>>> +
>>> +void rpcb_put_local(void)
>>> +{
>>> +	struct rpc_clnt *clnt = rpcb_local_clnt;
>>> +	struct rpc_clnt *clnt4 = rpcb_local_clnt4;
>>> +	int shutdown;
>>> +
>>> +	spin_lock(&rpcb_clnt_lock);
>>> +	if (--rpcb_users == 0) {
>>> +		rpcb_local_clnt = NULL;
>>> +		rpcb_local_clnt4 = NULL;
>>> +	}
>>> +	shutdown = !rpcb_users;
>>> +	spin_unlock(&rpcb_clnt_lock);
>>> +
>>> +	if (shutdown) {
>>> +		/*
>>> +		 * cleanup_rpcb_clnt - remove xprtsock's sysctls, unregister
>>> +		 */
>>> +		if (clnt4)
>>> +			rpc_shutdown_client(clnt4);
>>> +		if (clnt)
>>> +			rpc_shutdown_client(clnt);
>>> +	}
>>> +	return;
>>> +}
>>> +
>>> +static void rpcb_set_local(struct rpc_clnt *clnt, struct rpc_clnt
>>> +*clnt4) {
>>> +	/* Protected by rpcb_create_local_mutex */
>
> Doesn't it  need to be protected by rpcb_clnt_lock too?
>

Nope from my pow. It's protected by rpcb_create_local_mutex. I.e. no one will 
change rpcb_users since it's zero. If it's non zero - we willn't get to 
rpcb_set_local().

>>> +	rpcb_local_clnt = clnt;
>>> +	rpcb_local_clnt4 = clnt4;
>>> +	rpcb_users++;
> ^^^^^^^^^^^^^^^^^^^
>
>>> +	dprintk("RPC:       created new rpcb local clients (rpcb_local_clnt: "
>>> +			"%p, rpcb_local_clnt4: %p)\n", rpcb_local_clnt,
>>> +			rpcb_local_clnt4);
>>> +}
>>> +
>>>   /*
>>>    * Returns zero on success, otherwise a negative errno value
>>>    * is returned.
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs"
>>> in the body of a message to majordomo@vger.kernel.org More
>> majordomo
>>> info at  http://vger.kernel.org/majordomo-info.html
>
> \x04�{.n�+�������+%��lzwm��b�맲��r��zX��\x19߲)���w*\x1fjg���\x1e�����ݢj/���z�ޖ��2�ޙ���&�)ߡ�a��\x7f��\x1e�G���h�\x0f�j:+v���w�٥


-- 
Best regards,
Stanislav Kinsbursky

Re: [PATCH v5 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients

[Stanislav Kinsbursky]

v5: fixed races with rpcb_users in rpcb_get_local()

This helpers will be used for dynamical creation and destruction of rpcbind
clients.
Variable rpcb_users is actually a counter of lauched RPC services. If rpcbind
clients has been created already, then we just increase rpcb_users.

Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>

---
  net/sunrpc/rpcb_clnt.c |   53 ++++++++++++++++++++++++++++++++++++++++++++++++
  1 files changed, 53 insertions(+), 0 deletions(-)

diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
index e45d2fb..5f4a406 100644
--- a/net/sunrpc/rpcb_clnt.c
+++ b/net/sunrpc/rpcb_clnt.c
@@ -114,6 +114,9 @@ static struct rpc_program	rpcb_program;
  static struct rpc_clnt *	rpcb_local_clnt;
  static struct rpc_clnt *	rpcb_local_clnt4;
  +DEFINE_SPINLOCK(rpcb_clnt_lock);
+unsigned int			rpcb_users;
+
  struct rpcbind_args {
  	struct rpc_xprt *	r_xprt;
  @@ -161,6 +164,56 @@ static void rpcb_map_release(void *data)
  	kfree(map);
  }
  +static int rpcb_get_local(void)
+{
+	int cnt;
+
+	spin_lock(&rpcb_clnt_lock);
+	if (rpcb_users)
+		rpcb_users++;
+	cnt = rpcb_users;
+	spin_unlock(&rpcb_clnt_lock);
+
+	return cnt;
+}
+
+void rpcb_put_local(void)
+{
+	struct rpc_clnt *clnt = rpcb_local_clnt;
+	struct rpc_clnt *clnt4 = rpcb_local_clnt4;
+	int shutdown;
+
+	spin_lock(&rpcb_clnt_lock);
+	if (--rpcb_users == 0) {
+		rpcb_local_clnt = NULL;
+		rpcb_local_clnt4 = NULL;
+	}
+	shutdown = !rpcb_users;
+	spin_unlock(&rpcb_clnt_lock);
+
+	if (shutdown) {
+		/*
+		 * cleanup_rpcb_clnt - remove xprtsock's sysctls, unregister
+		 */
+		if (clnt4)
+			rpc_shutdown_client(clnt4);
+		if (clnt)
+			rpc_shutdown_client(clnt);
+	}
+	return;
+}
+
+static void rpcb_set_local(struct rpc_clnt *clnt, struct rpc_clnt *clnt4)
+{
+	/* Protected by rpcb_create_local_mutex */
+	rpcb_local_clnt = clnt;
+	rpcb_local_clnt4 = clnt4;
+	rpcb_users++;
+	dprintk("RPC:       created new rpcb local clients (rpcb_local_clnt: "
+			"%p, rpcb_local_clnt4: %p)\n", rpcb_local_clnt,
+			rpcb_local_clnt4);
+}
+
  /*
   * Returns zero on success, otherwise a negative errno value
   * is returned.

RE: [PATCH v4 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients

[Myklebust, Trond]

> -----Original Message-----
> From: Stanislav Kinsbursky [mailto:skinsbursky@parallels.com]
> Sent: Tuesday, September 20, 2011 9:35 AM
> To: Myklebust, Trond
> Cc: Schumaker, Bryan; linux-nfs@vger.kernel.org; Pavel Emelianov;
> neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> bfields@fieldses.org; davem@davemloft.net
> Subject: Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference
> counted rpcbind clients
> 
> 20.09.2011 17:15, Myklebust, Trond пишет:
> >> -----Original Message-----
> >> From: Schumaker, Bryan
> >> Sent: Tuesday, September 20, 2011 9:05 AM
> >> To: Stanislav Kinsbursky
> >> Cc: Myklebust, Trond; linux-nfs@vger.kernel.org; xemul@parallels.com;
> >> neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> >> bfields@fieldses.org; davem@davemloft.net
> >> Subject: Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference
> >> counted rpcbind clients
> >>
> >> On 09/20/2011 06:13 AM, Stanislav Kinsbursky wrote:
> >>> This helpers will be used for dynamical creation and destruction of
> >>> rpcbind clients.
> >>> Variable rpcb_users is actually a counter of lauched RPC services.
> >>> If rpcbind clients has been created already, then we just increase
> rpcb_users.
> >>>
> >>> Signed-off-by: Stanislav Kinsbursky<skinsbursky@parallels.com>
> >>>
> >>> ---
> >>>   net/sunrpc/rpcb_clnt.c |   50
> >> ++++++++++++++++++++++++++++++++++++++++++++++++
> >>>   1 files changed, 50 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c index
> >>> e45d2fb..8724780 100644
> >>> --- a/net/sunrpc/rpcb_clnt.c
> >>> +++ b/net/sunrpc/rpcb_clnt.c
> >>> @@ -114,6 +114,9 @@ static struct rpc_program	rpcb_program;
> >>>   static struct rpc_clnt *	rpcb_local_clnt;
> >>>   static struct rpc_clnt *	rpcb_local_clnt4;
> >>>
> >>> +DEFINE_SPINLOCK(rpcb_clnt_lock);
> >>> +unsigned int			rpcb_users;
> >>> +
> >>>   struct rpcbind_args {
> >>>   	struct rpc_xprt *	r_xprt;
> >>>
> >>> @@ -161,6 +164,53 @@ static void rpcb_map_release(void *data)
> >>>   	kfree(map);
> >>>   }
> >>>
> >>> +static int rpcb_get_local(void)
> >>> +{
> >>> +	spin_lock(&rpcb_clnt_lock);
> >>> +	if (rpcb_users)
> >>> +		rpcb_users++;
> >>> +	spin_unlock(&rpcb_clnt_lock);
> >>> +
> >>> +	return rpcb_users;
> >>          ^^^^^^^^^^^^^^^^^^
> >> Is it safe to use this variable outside of the rpcb_clnt_lock?
> >>
> > Nope. If rpcb_users was zero in the protected section above, nothing
> guarantees that it will still be zero here, and so the caller may get the (wrong)
> impression that the counter was incremented.
> >
> 
> Yep, you right. Will fix this races.
> 
> >>> +}
> >>> +
> >>> +void rpcb_put_local(void)
> >>> +{
> >>> +	struct rpc_clnt *clnt = rpcb_local_clnt;
> >>> +	struct rpc_clnt *clnt4 = rpcb_local_clnt4;
> >>> +	int shutdown;
> >>> +
> >>> +	spin_lock(&rpcb_clnt_lock);
> >>> +	if (--rpcb_users == 0) {
> >>> +		rpcb_local_clnt = NULL;
> >>> +		rpcb_local_clnt4 = NULL;
> >>> +	}
> >>> +	shutdown = !rpcb_users;
> >>> +	spin_unlock(&rpcb_clnt_lock);
> >>> +
> >>> +	if (shutdown) {
> >>> +		/*
> >>> +		 * cleanup_rpcb_clnt - remove xprtsock's sysctls, unregister
> >>> +		 */
> >>> +		if (clnt4)
> >>> +			rpc_shutdown_client(clnt4);
> >>> +		if (clnt)
> >>> +			rpc_shutdown_client(clnt);
> >>> +	}
> >>> +	return;
> >>> +}
> >>> +
> >>> +static void rpcb_set_local(struct rpc_clnt *clnt, struct rpc_clnt
> >>> +*clnt4) {
> >>> +	/* Protected by rpcb_create_local_mutex */
> >
> > Doesn't it  need to be protected by rpcb_clnt_lock too?
> >
> 
> Nope from my pow. It's protected by rpcb_create_local_mutex. I.e. no one
> will change rpcb_users since it's zero. If it's non zero - we willn't get to
> rpcb_set_local().

OK, so you are saying that the rpcb_users++ below could be replaced by rpcb_users=1?

In that case, don't you need a smp_wmb() between the setting of rpcb_local_clnt/4 and the setting of rpcb_users? Otherwise, how do you guarantee that rpcb_users != 0 implies rpbc_local_clnt/4 != NULL?

> >>> +	rpcb_local_clnt = clnt;
> >>> +	rpcb_local_clnt4 = clnt4;
> >>> +	rpcb_users++;
> > ^^^^^^^^^^^^^^^^^^^
> >
> >>> +	dprintk("RPC:       created new rpcb local clients (rpcb_local_clnt: "
> >>> +			"%p, rpcb_local_clnt4: %p)\n", rpcb_local_clnt,
> >>> +			rpcb_local_clnt4);
> >>> +}
> >>> +

Re: [PATCH v5 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients

[Jeff Layton]

On Tue, 20 Sep 2011 17:49:27 +0400
Stanislav Kinsbursky <skinsbursky@parallels.com> wrote:

> v5: fixed races with rpcb_users in rpcb_get_local()
> 
> This helpers will be used for dynamical creation and destruction of rpcbind
> clients.
> Variable rpcb_users is actually a counter of lauched RPC services. If rpcbind
> clients has been created already, then we just increase rpcb_users.
> 
> Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
> 
> ---
>   net/sunrpc/rpcb_clnt.c |   53 ++++++++++++++++++++++++++++++++++++++++++++++++
>   1 files changed, 53 insertions(+), 0 deletions(-)
> 
> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
> index e45d2fb..5f4a406 100644
> --- a/net/sunrpc/rpcb_clnt.c
> +++ b/net/sunrpc/rpcb_clnt.c
> @@ -114,6 +114,9 @@ static struct rpc_program	rpcb_program;
>   static struct rpc_clnt *	rpcb_local_clnt;
>   static struct rpc_clnt *	rpcb_local_clnt4;
>   +DEFINE_SPINLOCK(rpcb_clnt_lock);
> +unsigned int			rpcb_users;
> +
>   struct rpcbind_args {
>   	struct rpc_xprt *	r_xprt;
>   @@ -161,6 +164,56 @@ static void rpcb_map_release(void *data)
>   	kfree(map);
>   }
>   +static int rpcb_get_local(void)
> +{
> +	int cnt;
> +
> +	spin_lock(&rpcb_clnt_lock);
> +	if (rpcb_users)
> +		rpcb_users++;
> +	cnt = rpcb_users;
> +	spin_unlock(&rpcb_clnt_lock);
> +
> +	return cnt;
> +}
> +
> +void rpcb_put_local(void)
> +{
> +	struct rpc_clnt *clnt = rpcb_local_clnt;
> +	struct rpc_clnt *clnt4 = rpcb_local_clnt4;
> +	int shutdown;
> +
> +	spin_lock(&rpcb_clnt_lock);
> +	if (--rpcb_users == 0) {
> +		rpcb_local_clnt = NULL;
> +		rpcb_local_clnt4 = NULL;
> +	}

In the function below, you mention that the above pointers are
protected by rpcb_create_local_mutex, but it looks like they get reset
here without that being held?

Might it be simpler to just protect rpcb_users with the
rpcb_create_local_mutex and ensure that it's held whenever you call one
of these routines? None of these are codepaths are particularly hot.

> +	shutdown = !rpcb_users;
> +	spin_unlock(&rpcb_clnt_lock);
> +
> +	if (shutdown) {
> +		/*
> +		 * cleanup_rpcb_clnt - remove xprtsock's sysctls, unregister
> +		 */
> +		if (clnt4)
> +			rpc_shutdown_client(clnt4);
> +		if (clnt)
> +			rpc_shutdown_client(clnt);
> +	}
> +	return;
> +}
> +
> +static void rpcb_set_local(struct rpc_clnt *clnt, struct rpc_clnt *clnt4)
> +{
> +	/* Protected by rpcb_create_local_mutex */
> +	rpcb_local_clnt = clnt;
> +	rpcb_local_clnt4 = clnt4;
> +	rpcb_users++;
> +	dprintk("RPC:       created new rpcb local clients (rpcb_local_clnt: "
> +			"%p, rpcb_local_clnt4: %p)\n", rpcb_local_clnt,
> +			rpcb_local_clnt4);
> +}
> +
>   /*
>    * Returns zero on success, otherwise a negative errno value
>    * is returned.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients

[Stanislav Kinsbursky]

20.09.2011 18:14, Myklebust, Trond пишет:

>>>
>>> Doesn't it  need to be protected by rpcb_clnt_lock too?
>>>
>>
>> Nope from my pow. It's protected by rpcb_create_local_mutex. I.e. no one
>> will change rpcb_users since it's zero. If it's non zero - we willn't get to
>> rpcb_set_local().
>
> OK, so you are saying that the rpcb_users++ below could be replaced by rpcb_users=1?
>

Yes, you right.

> In that case, don't you need a smp_wmb() between the setting of rpcb_local_clnt/4 and the setting of rpcb_users? Otherwise, how do you guarantee that rpcb_users != 0 implies rpbc_local_clnt/4 != NULL?
>

We check rpcb_users under spinlock. Gain spinlock forces memory barrier, doesn't it?

-- 
Best regards,
Stanislav Kinsbursky

RE: [PATCH v4 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients

[Myklebust, Trond]

> -----Original Message-----
> From: Stanislav Kinsbursky [mailto:skinsbursky@parallels.com]
> Sent: Tuesday, September 20, 2011 10:35 AM
> To: Myklebust, Trond
> Cc: Schumaker, Bryan; linux-nfs@vger.kernel.org; Pavel Emelianov;
> neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> bfields@fieldses.org; davem@davemloft.net
> Subject: Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference
> counted rpcbind clients
> 
> 20.09.2011 18:14, Myklebust, Trond пишет:
> 
> >>>
> >>> Doesn't it  need to be protected by rpcb_clnt_lock too?
> >>>
> >>
> >> Nope from my pow. It's protected by rpcb_create_local_mutex. I.e. no
> >> one will change rpcb_users since it's zero. If it's non zero - we
> >> willn't get to rpcb_set_local().
> >
> > OK, so you are saying that the rpcb_users++ below could be replaced by
> rpcb_users=1?
> >
> 
> Yes, you right.
> 
> > In that case, don't you need a smp_wmb() between the setting of
> rpcb_local_clnt/4 and the setting of rpcb_users? Otherwise, how do you
> guarantee that rpcb_users != 0 implies rpbc_local_clnt/4 != NULL?
> >
> 
> We check rpcb_users under spinlock. Gain spinlock forces memory barrier,
> doesn't it?

Yes, and so you don't need an smp_rmb() on the reader side. However, you still need to ensure that the processor which _sets_ the rpcb_users and rpcb_local_clnt/4 actually writes them in the correct order.

Cheers
  Trond

RE: [PATCH v5 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients

[Myklebust, Trond]

> -----Original Message-----
> From: Jeff Layton [mailto:jlayton@redhat.com]
> Sent: Tuesday, September 20, 2011 10:25 AM
> To: Stanislav Kinsbursky
> Cc: Myklebust, Trond; linux-nfs@vger.kernel.org; Pavel Emelianov;
> neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> bfields@fieldses.org; davem@davemloft.net
> Subject: Re: [PATCH v5 1/8] SUNRPC: introduce helpers for reference
> counted rpcbind clients
> 
> On Tue, 20 Sep 2011 17:49:27 +0400
> Stanislav Kinsbursky <skinsbursky@parallels.com> wrote:
> 
> > v5: fixed races with rpcb_users in rpcb_get_local()
> >
> > This helpers will be used for dynamical creation and destruction of
> > rpcbind clients.
> > Variable rpcb_users is actually a counter of lauched RPC services.
If
> > rpcbind clients has been created already, then we just increase
rpcb_users.
> >
> > Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
> >
> > ---
> >   net/sunrpc/rpcb_clnt.c |   53
> ++++++++++++++++++++++++++++++++++++++++++++++++
> >   1 files changed, 53 insertions(+), 0 deletions(-)
> >
> > diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c index
> > e45d2fb..5f4a406 100644
> > --- a/net/sunrpc/rpcb_clnt.c
> > +++ b/net/sunrpc/rpcb_clnt.c
> > @@ -114,6 +114,9 @@ static struct rpc_program	rpcb_program;
> >   static struct rpc_clnt *	rpcb_local_clnt;
> >   static struct rpc_clnt *	rpcb_local_clnt4;
> >   +DEFINE_SPINLOCK(rpcb_clnt_lock);
> > +unsigned int			rpcb_users;
> > +
> >   struct rpcbind_args {
> >   	struct rpc_xprt *	r_xprt;
> >   @@ -161,6 +164,56 @@ static void rpcb_map_release(void *data)
> >   	kfree(map);
> >   }
> >   +static int rpcb_get_local(void)
> > +{
> > +	int cnt;
> > +
> > +	spin_lock(&rpcb_clnt_lock);
> > +	if (rpcb_users)
> > +		rpcb_users++;
> > +	cnt = rpcb_users;
> > +	spin_unlock(&rpcb_clnt_lock);
> > +
> > +	return cnt;
> > +}
> > +
> > +void rpcb_put_local(void)
> > +{
> > +	struct rpc_clnt *clnt = rpcb_local_clnt;
> > +	struct rpc_clnt *clnt4 = rpcb_local_clnt4;
> > +	int shutdown;
> > +
> > +	spin_lock(&rpcb_clnt_lock);
> > +	if (--rpcb_users == 0) {
> > +		rpcb_local_clnt = NULL;
> > +		rpcb_local_clnt4 = NULL;
> > +	}
> 
> In the function below, you mention that the above pointers are
protected by
> rpcb_create_local_mutex, but it looks like they get reset here without
that
> being held?
> 
> Might it be simpler to just protect rpcb_users with the
> rpcb_create_local_mutex and ensure that it's held whenever you call
one of
> these routines? None of these are codepaths are particularly hot.

Alternatively, if you do

	if (rpcb_users == 1) {
		rpcb_local_clnt = NULL;
		rpcb_local_clnt4 = NULL;
		smp_wmb();
		rpcb_users = 0;
	} else
		rpcb_users--;

then the spinlock protection in rpbc_get_local() is still good enough to
guarantee correctness.

Re: [PATCH v5 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients

[Stanislav Kinsbursky]

20.09.2011 18:24, Jeff Layton пишет:
> On Tue, 20 Sep 2011 17:49:27 +0400
> Stanislav Kinsbursky<skinsbursky-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>  wrote:
>
>> v5: fixed races with rpcb_users in rpcb_get_local()
>>
>> This helpers will be used for dynamical creation and destruction of rpcbind
>> clients.
>> Variable rpcb_users is actually a counter of lauched RPC services. If rpcbind
>> clients has been created already, then we just increase rpcb_users.
>>
>> Signed-off-by: Stanislav Kinsbursky<skinsbursky-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
>>
>> ---
>>    net/sunrpc/rpcb_clnt.c |   53 ++++++++++++++++++++++++++++++++++++++++++++++++
>>    1 files changed, 53 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
>> index e45d2fb..5f4a406 100644
>> --- a/net/sunrpc/rpcb_clnt.c
>> +++ b/net/sunrpc/rpcb_clnt.c
>> @@ -114,6 +114,9 @@ static struct rpc_program	rpcb_program;
>>    static struct rpc_clnt *	rpcb_local_clnt;
>>    static struct rpc_clnt *	rpcb_local_clnt4;
>>    +DEFINE_SPINLOCK(rpcb_clnt_lock);
>> +unsigned int			rpcb_users;
>> +
>>    struct rpcbind_args {
>>    	struct rpc_xprt *	r_xprt;
>>    @@ -161,6 +164,56 @@ static void rpcb_map_release(void *data)
>>    	kfree(map);
>>    }
>>    +static int rpcb_get_local(void)
>> +{
>> +	int cnt;
>> +
>> +	spin_lock(&rpcb_clnt_lock);
>> +	if (rpcb_users)
>> +		rpcb_users++;
>> +	cnt = rpcb_users;
>> +	spin_unlock(&rpcb_clnt_lock);
>> +
>> +	return cnt;
>> +}
>> +
>> +void rpcb_put_local(void)
>> +{
>> +	struct rpc_clnt *clnt = rpcb_local_clnt;
>> +	struct rpc_clnt *clnt4 = rpcb_local_clnt4;
>> +	int shutdown;
>> +
>> +	spin_lock(&rpcb_clnt_lock);
>> +	if (--rpcb_users == 0) {
>> +		rpcb_local_clnt = NULL;
>> +		rpcb_local_clnt4 = NULL;
>> +	}
>
> In the function below, you mention that the above pointers are
> protected by rpcb_create_local_mutex, but it looks like they get reset
> here without that being held?
>

Assigning of them is protected by rpcb_create_local_mutex.
Dereferencing of them is protected by rpcb_clnt_lock.

> Might it be simpler to just protect rpcb_users with the
> rpcb_create_local_mutex and ensure that it's held whenever you call one
> of these routines? None of these are codepaths are particularly hot.
>

I just inherited this lock-mutex logic.
Actually, you right. This codepaths are used rarely.
But are use sure, that we need to remove this "speed-up" logic if we take into 
account that it was here already?

>> +	shutdown = !rpcb_users;
>> +	spin_unlock(&rpcb_clnt_lock);
>> +
>> +	if (shutdown) {
>> +		/*
>> +		 * cleanup_rpcb_clnt - remove xprtsock's sysctls, unregister
>> +		 */
>> +		if (clnt4)
>> +			rpc_shutdown_client(clnt4);
>> +		if (clnt)
>> +			rpc_shutdown_client(clnt);
>> +	}
>> +	return;
>> +}
>> +
>> +static void rpcb_set_local(struct rpc_clnt *clnt, struct rpc_clnt *clnt4)
>> +{
>> +	/* Protected by rpcb_create_local_mutex */
>> +	rpcb_local_clnt = clnt;
>> +	rpcb_local_clnt4 = clnt4;
>> +	rpcb_users++;
>> +	dprintk("RPC:       created new rpcb local clients (rpcb_local_clnt: "
>> +			"%p, rpcb_local_clnt4: %p)\n", rpcb_local_clnt,
>> +			rpcb_local_clnt4);
>> +}
>> +
>>    /*
>>     * Returns zero on success, otherwise a negative errno value
>>     * is returned.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>


-- 
Best regards,
Stanislav Kinsbursky
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients

[Bryan Schumaker]

On 09/20/2011 10:43 AM, Stanislav Kinsbursky wrote:
> 20.09.2011 18:24, Jeff Layton пишет:
>> On Tue, 20 Sep 2011 17:49:27 +0400
>> Stanislav Kinsbursky<skinsbursky@parallels.com>  wrote:
>>
>>> v5: fixed races with rpcb_users in rpcb_get_local()
>>>
>>> This helpers will be used for dynamical creation and destruction of rpcbind
>>> clients.
>>> Variable rpcb_users is actually a counter of lauched RPC services. If rpcbind
>>> clients has been created already, then we just increase rpcb_users.
>>>
>>> Signed-off-by: Stanislav Kinsbursky<skinsbursky@parallels.com>
>>>
>>> ---
>>>    net/sunrpc/rpcb_clnt.c |   53 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>    1 files changed, 53 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
>>> index e45d2fb..5f4a406 100644
>>> --- a/net/sunrpc/rpcb_clnt.c
>>> +++ b/net/sunrpc/rpcb_clnt.c
>>> @@ -114,6 +114,9 @@ static struct rpc_program    rpcb_program;
>>>    static struct rpc_clnt *    rpcb_local_clnt;
>>>    static struct rpc_clnt *    rpcb_local_clnt4;
>>>    +DEFINE_SPINLOCK(rpcb_clnt_lock);
>>> +unsigned int            rpcb_users;
>>> +
>>>    struct rpcbind_args {
>>>        struct rpc_xprt *    r_xprt;
>>>    @@ -161,6 +164,56 @@ static void rpcb_map_release(void *data)
>>>        kfree(map);
>>>    }
>>>    +static int rpcb_get_local(void)
>>> +{
>>> +    int cnt;
>>> +
>>> +    spin_lock(&rpcb_clnt_lock);
>>> +    if (rpcb_users)
>>> +        rpcb_users++;
>>> +    cnt = rpcb_users;
>>> +    spin_unlock(&rpcb_clnt_lock);
>>> +
>>> +    return cnt;
>>> +}
>>> +
>>> +void rpcb_put_local(void)
>>> +{
>>> +    struct rpc_clnt *clnt = rpcb_local_clnt;
>>> +    struct rpc_clnt *clnt4 = rpcb_local_clnt4;
>>> +    int shutdown;
>>> +
>>> +    spin_lock(&rpcb_clnt_lock);
>>> +    if (--rpcb_users == 0) {
>>> +        rpcb_local_clnt = NULL;
>>> +        rpcb_local_clnt4 = NULL;
>>> +    }
>>
>> In the function below, you mention that the above pointers are
>> protected by rpcb_create_local_mutex, but it looks like they get reset
>> here without that being held?
>>
> 
> Assigning of them is protected by rpcb_create_local_mutex.
> Dereferencing of them is protected by rpcb_clnt_lock.

Shouldn't you be using the same lock for assigning and dereferencing?  Otherwise one thread could change these variables while another is using them.
> 
>> Might it be simpler to just protect rpcb_users with the
>> rpcb_create_local_mutex and ensure that it's held whenever you call one
>> of these routines? None of these are codepaths are particularly hot.
>>
> 
> I just inherited this lock-mutex logic.
> Actually, you right. This codepaths are used rarely.
> But are use sure, that we need to remove this "speed-up" logic if we take into account that it was here already?
> 
>>> +    shutdown = !rpcb_users;
>>> +    spin_unlock(&rpcb_clnt_lock);
>>> +
>>> +    if (shutdown) {
>>> +        /*
>>> +         * cleanup_rpcb_clnt - remove xprtsock's sysctls, unregister
>>> +         */
>>> +        if (clnt4)
>>> +            rpc_shutdown_client(clnt4);
>>> +        if (clnt)
>>> +            rpc_shutdown_client(clnt);
>>> +    }
>>> +    return;
>>> +}
>>> +
>>> +static void rpcb_set_local(struct rpc_clnt *clnt, struct rpc_clnt *clnt4)
>>> +{
>>> +    /* Protected by rpcb_create_local_mutex */
>>> +    rpcb_local_clnt = clnt;
>>> +    rpcb_local_clnt4 = clnt4;
>>> +    rpcb_users++;
>>> +    dprintk("RPC:       created new rpcb local clients (rpcb_local_clnt: "
>>> +            "%p, rpcb_local_clnt4: %p)\n", rpcb_local_clnt,
>>> +            rpcb_local_clnt4);
>>> +}
>>> +
>>>    /*
>>>     * Returns zero on success, otherwise a negative errno value
>>>     * is returned.
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
> 
> 

Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients

[Stanislav Kinsbursky]

20.09.2011 18:38, Myklebust, Trond пишет:
>> -----Original Message-----
>> From: Stanislav Kinsbursky [mailto:skinsbursky@parallels.com]
>> Sent: Tuesday, September 20, 2011 10:35 AM
>> To: Myklebust, Trond
>> Cc: Schumaker, Bryan; linux-nfs@vger.kernel.org; Pavel Emelianov;
>> neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
>> bfields@fieldses.org; davem@davemloft.net
>> Subject: Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference
>> counted rpcbind clients
>>
>> 20.09.2011 18:14, Myklebust, Trond пишет:
>>
>>>>>
>>>>> Doesn't it  need to be protected by rpcb_clnt_lock too?
>>>>>
>>>>
>>>> Nope from my pow. It's protected by rpcb_create_local_mutex. I.e. no
>>>> one will change rpcb_users since it's zero. If it's non zero - we
>>>> willn't get to rpcb_set_local().
>>>
>>> OK, so you are saying that the rpcb_users++ below could be replaced by
>> rpcb_users=1?
>>>
>>
>> Yes, you right.
>>
>>> In that case, don't you need a smp_wmb() between the setting of
>> rpcb_local_clnt/4 and the setting of rpcb_users? Otherwise, how do you
>> guarantee that rpcb_users != 0 implies rpbc_local_clnt/4 != NULL?
>>>
>>
>> We check rpcb_users under spinlock. Gain spinlock forces memory barrier,
>> doesn't it?
>
> Yes, and so you don't need an smp_rmb() on the reader side. However, you still need to ensure that the processor which _sets_ the rpcb_users and rpcb_local_clnt/4 actually writes them in the correct order.
>

Yep, now I understand what are you talking about.
Will fix both places (set and put).

-- 
Best regards,
Stanislav Kinsbursky

Re: [PATCH v5 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients

[Jeff Layton]

On Tue, 20 Sep 2011 18:43:45 +0400
Stanislav Kinsbursky <skinsbursky@parallels.com> wrote:

> 20.09.2011 18:24, Jeff Layton пишет:
> > On Tue, 20 Sep 2011 17:49:27 +0400
> > Stanislav Kinsbursky<skinsbursky@parallels.com>  wrote:
> >
> >> v5: fixed races with rpcb_users in rpcb_get_local()
> >>
> >> This helpers will be used for dynamical creation and destruction of rpcbind
> >> clients.
> >> Variable rpcb_users is actually a counter of lauched RPC services. If rpcbind
> >> clients has been created already, then we just increase rpcb_users.
> >>
> >> Signed-off-by: Stanislav Kinsbursky<skinsbursky@parallels.com>
> >>
> >> ---
> >>    net/sunrpc/rpcb_clnt.c |   53 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>    1 files changed, 53 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
> >> index e45d2fb..5f4a406 100644
> >> --- a/net/sunrpc/rpcb_clnt.c
> >> +++ b/net/sunrpc/rpcb_clnt.c
> >> @@ -114,6 +114,9 @@ static struct rpc_program	rpcb_program;
> >>    static struct rpc_clnt *	rpcb_local_clnt;
> >>    static struct rpc_clnt *	rpcb_local_clnt4;
> >>    +DEFINE_SPINLOCK(rpcb_clnt_lock);
> >> +unsigned int			rpcb_users;
> >> +
> >>    struct rpcbind_args {
> >>    	struct rpc_xprt *	r_xprt;
> >>    @@ -161,6 +164,56 @@ static void rpcb_map_release(void *data)
> >>    	kfree(map);
> >>    }
> >>    +static int rpcb_get_local(void)
> >> +{
> >> +	int cnt;
> >> +
> >> +	spin_lock(&rpcb_clnt_lock);
> >> +	if (rpcb_users)
> >> +		rpcb_users++;
> >> +	cnt = rpcb_users;
> >> +	spin_unlock(&rpcb_clnt_lock);
> >> +
> >> +	return cnt;
> >> +}
> >> +
> >> +void rpcb_put_local(void)
> >> +{
> >> +	struct rpc_clnt *clnt = rpcb_local_clnt;
> >> +	struct rpc_clnt *clnt4 = rpcb_local_clnt4;
> >> +	int shutdown;
> >> +
> >> +	spin_lock(&rpcb_clnt_lock);
> >> +	if (--rpcb_users == 0) {
> >> +		rpcb_local_clnt = NULL;
> >> +		rpcb_local_clnt4 = NULL;
> >> +	}
> >
> > In the function below, you mention that the above pointers are
> > protected by rpcb_create_local_mutex, but it looks like they get reset
> > here without that being held?
> >
> 
> Assigning of them is protected by rpcb_create_local_mutex.
> Dereferencing of them is protected by rpcb_clnt_lock.
> 

That's probably a bug, but I haven't sat down to work through the logic.

> > Might it be simpler to just protect rpcb_users with the
> > rpcb_create_local_mutex and ensure that it's held whenever you call one
> > of these routines? None of these are codepaths are particularly hot.
> >
> 
> I just inherited this lock-mutex logic.
> Actually, you right. This codepaths are used rarely.
> But are use sure, that we need to remove this "speed-up" logic if we take into 
> account that it was here already?
> 

There are many ways to do this...

In general, it's difficult to get locking right, especially when you
start mixing multiple locks on related resources. Personally, I'd go
with a simpler scheme here. There's not much value in protecting this
counter with a spinlock when the other parts need to be protected by a
mutex. If you do decide to do it with multiple locks, then please do
document in comments how the locking is expected to work. 

An alternate scheme might be to consider doing this with krefs, but I
haven't really considered whether that idiom makes sense here.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH v5 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients

[Stanislav Kinsbursky]

20.09.2011 18:58, Bryan Schumaker пишет:
> On 09/20/2011 10:43 AM, Stanislav Kinsbursky wrote:
>> 20.09.2011 18:24, Jeff Layton пишет:
>>> On Tue, 20 Sep 2011 17:49:27 +0400
>>> Stanislav Kinsbursky<skinsbursky@parallels.com>   wrote:
>>>
>>>> v5: fixed races with rpcb_users in rpcb_get_local()
>>>>
>>>> This helpers will be used for dynamical creation and destruction of rpcbind
>>>> clients.
>>>> Variable rpcb_users is actually a counter of lauched RPC services. If rpcbind
>>>> clients has been created already, then we just increase rpcb_users.
>>>>
>>>> Signed-off-by: Stanislav Kinsbursky<skinsbursky@parallels.com>
>>>>
>>>> ---
>>>>     net/sunrpc/rpcb_clnt.c |   53 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>     1 files changed, 53 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
>>>> index e45d2fb..5f4a406 100644
>>>> --- a/net/sunrpc/rpcb_clnt.c
>>>> +++ b/net/sunrpc/rpcb_clnt.c
>>>> @@ -114,6 +114,9 @@ static struct rpc_program    rpcb_program;
>>>>     static struct rpc_clnt *    rpcb_local_clnt;
>>>>     static struct rpc_clnt *    rpcb_local_clnt4;
>>>>     +DEFINE_SPINLOCK(rpcb_clnt_lock);
>>>> +unsigned int            rpcb_users;
>>>> +
>>>>     struct rpcbind_args {
>>>>         struct rpc_xprt *    r_xprt;
>>>>     @@ -161,6 +164,56 @@ static void rpcb_map_release(void *data)
>>>>         kfree(map);
>>>>     }
>>>>     +static int rpcb_get_local(void)
>>>> +{
>>>> +    int cnt;
>>>> +
>>>> +    spin_lock(&rpcb_clnt_lock);
>>>> +    if (rpcb_users)
>>>> +        rpcb_users++;
>>>> +    cnt = rpcb_users;
>>>> +    spin_unlock(&rpcb_clnt_lock);
>>>> +
>>>> +    return cnt;
>>>> +}
>>>> +
>>>> +void rpcb_put_local(void)
>>>> +{
>>>> +    struct rpc_clnt *clnt = rpcb_local_clnt;
>>>> +    struct rpc_clnt *clnt4 = rpcb_local_clnt4;
>>>> +    int shutdown;
>>>> +
>>>> +    spin_lock(&rpcb_clnt_lock);
>>>> +    if (--rpcb_users == 0) {
>>>> +        rpcb_local_clnt = NULL;
>>>> +        rpcb_local_clnt4 = NULL;
>>>> +    }
>>>
>>> In the function below, you mention that the above pointers are
>>> protected by rpcb_create_local_mutex, but it looks like they get reset
>>> here without that being held?
>>>
>>
>> Assigning of them is protected by rpcb_create_local_mutex.
>> Dereferencing of them is protected by rpcb_clnt_lock.
>
> Shouldn't you be using the same lock for assigning and dereferencing?  Otherwise one thread could change these variables while another is using them.

Probably I wasn't clear with my previous explanation.
Actually, we use only spinlock to make sure, that the pointers are valid when we 
dereferencing them. Synchronization point here is rpcb_users counter.
IOW, we use these pointers only from svc code and only after service already 
started. And with this patch-set we can be sure, that this pointers has been 
created already to the point, when this service starts.

But when this counter is zero, we can experience races with assigning those 
pointers. It takes a lot of time, so we use local mutex here instead of spinlock.

Have I answered your question?

-- 
Best regards,
Stanislav Kinsbursky

Re: [PATCH v5 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients

[Stanislav Kinsbursky]

20.09.2011 18:41, Myklebust, Trond пишет:
>> -----Original Message-----
>> From: Jeff Layton [mailto:jlayton@redhat.com]
>> Sent: Tuesday, September 20, 2011 10:25 AM
>> To: Stanislav Kinsbursky
>> Cc: Myklebust, Trond; linux-nfs@vger.kernel.org; Pavel Emelianov;
>> neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
>> bfields@fieldses.org; davem@davemloft.net
>> Subject: Re: [PATCH v5 1/8] SUNRPC: introduce helpers for reference
>> counted rpcbind clients
>>
>> On Tue, 20 Sep 2011 17:49:27 +0400
>> Stanislav Kinsbursky<skinsbursky@parallels.com>  wrote:
>>
>>> v5: fixed races with rpcb_users in rpcb_get_local()
>>>
>>> This helpers will be used for dynamical creation and destruction of
>>> rpcbind clients.
>>> Variable rpcb_users is actually a counter of lauched RPC services.
> If
>>> rpcbind clients has been created already, then we just increase
> rpcb_users.
>>>
>>> Signed-off-by: Stanislav Kinsbursky<skinsbursky@parallels.com>
>>>
>>> ---
>>>    net/sunrpc/rpcb_clnt.c |   53
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>    1 files changed, 53 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c index
>>> e45d2fb..5f4a406 100644
>>> --- a/net/sunrpc/rpcb_clnt.c
>>> +++ b/net/sunrpc/rpcb_clnt.c
>>> @@ -114,6 +114,9 @@ static struct rpc_program	rpcb_program;
>>>    static struct rpc_clnt *	rpcb_local_clnt;
>>>    static struct rpc_clnt *	rpcb_local_clnt4;
>>>    +DEFINE_SPINLOCK(rpcb_clnt_lock);
>>> +unsigned int			rpcb_users;
>>> +
>>>    struct rpcbind_args {
>>>    	struct rpc_xprt *	r_xprt;
>>>    @@ -161,6 +164,56 @@ static void rpcb_map_release(void *data)
>>>    	kfree(map);
>>>    }
>>>    +static int rpcb_get_local(void)
>>> +{
>>> +	int cnt;
>>> +
>>> +	spin_lock(&rpcb_clnt_lock);
>>> +	if (rpcb_users)
>>> +		rpcb_users++;
>>> +	cnt = rpcb_users;
>>> +	spin_unlock(&rpcb_clnt_lock);
>>> +
>>> +	return cnt;
>>> +}
>>> +
>>> +void rpcb_put_local(void)
>>> +{
>>> +	struct rpc_clnt *clnt = rpcb_local_clnt;
>>> +	struct rpc_clnt *clnt4 = rpcb_local_clnt4;
>>> +	int shutdown;
>>> +
>>> +	spin_lock(&rpcb_clnt_lock);
>>> +	if (--rpcb_users == 0) {
>>> +		rpcb_local_clnt = NULL;
>>> +		rpcb_local_clnt4 = NULL;
>>> +	}
>>
>> In the function below, you mention that the above pointers are
> protected by
>> rpcb_create_local_mutex, but it looks like they get reset here without
> that
>> being held?
>>
>> Might it be simpler to just protect rpcb_users with the
>> rpcb_create_local_mutex and ensure that it's held whenever you call
> one of
>> these routines? None of these are codepaths are particularly hot.
>
> Alternatively, if you do
>
> 	if (rpcb_users == 1) {
> 		rpcb_local_clnt = NULL;
> 		rpcb_local_clnt4 = NULL;
> 		smp_wmb();
> 		rpcb_users = 0;
> 	} else
> 		rpcb_users--;
>
> then the spinlock protection in rpbc_get_local() is still good enough to
> guarantee correctness.

I don't understand the idea of this code. It guarantees, that if rpcb_users == 
0, then rpcb_local_clnt == NULL and rpcb_local_clnt4 == NULL.
But we don't need such guarantee from my pow.
I.e. if rpcb_users == 0, then it means, that no services running right now.

For example, processes, destroying those clients, is running on CPU#0.
On CPU#1, for example, we have another process trying to get those clients and 
waiting on spinlock. When this process will gain the spinlock, it will see 0 
users, gain mutex and then try to create new clients. We still have no users on 
this clients yet. And this process will just reassign whose rpcbind clients 
pointers (and here we need memmory barrier for sure).

-- 
Best regards,
Stanislav Kinsbursky

Re: [PATCH v5 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients

[Bryan Schumaker]

On 09/20/2011 11:38 AM, Stanislav Kinsbursky wrote:
> 20.09.2011 18:58, Bryan Schumaker пишет:
>> On 09/20/2011 10:43 AM, Stanislav Kinsbursky wrote:
>>> 20.09.2011 18:24, Jeff Layton пишет:
>>>> On Tue, 20 Sep 2011 17:49:27 +0400
>>>> Stanislav Kinsbursky<skinsbursky@parallels.com>   wrote:
>>>>
>>>>> v5: fixed races with rpcb_users in rpcb_get_local()
>>>>>
>>>>> This helpers will be used for dynamical creation and destruction of rpcbind
>>>>> clients.
>>>>> Variable rpcb_users is actually a counter of lauched RPC services. If rpcbind
>>>>> clients has been created already, then we just increase rpcb_users.
>>>>>
>>>>> Signed-off-by: Stanislav Kinsbursky<skinsbursky@parallels.com>
>>>>>
>>>>> ---
>>>>>     net/sunrpc/rpcb_clnt.c |   53 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>     1 files changed, 53 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
>>>>> index e45d2fb..5f4a406 100644
>>>>> --- a/net/sunrpc/rpcb_clnt.c
>>>>> +++ b/net/sunrpc/rpcb_clnt.c
>>>>> @@ -114,6 +114,9 @@ static struct rpc_program    rpcb_program;
>>>>>     static struct rpc_clnt *    rpcb_local_clnt;
>>>>>     static struct rpc_clnt *    rpcb_local_clnt4;
>>>>>     +DEFINE_SPINLOCK(rpcb_clnt_lock);
>>>>> +unsigned int            rpcb_users;
>>>>> +
>>>>>     struct rpcbind_args {
>>>>>         struct rpc_xprt *    r_xprt;
>>>>>     @@ -161,6 +164,56 @@ static void rpcb_map_release(void *data)
>>>>>         kfree(map);
>>>>>     }
>>>>>     +static int rpcb_get_local(void)
>>>>> +{
>>>>> +    int cnt;
>>>>> +
>>>>> +    spin_lock(&rpcb_clnt_lock);
>>>>> +    if (rpcb_users)
>>>>> +        rpcb_users++;
>>>>> +    cnt = rpcb_users;
>>>>> +    spin_unlock(&rpcb_clnt_lock);
>>>>> +
>>>>> +    return cnt;
>>>>> +}
>>>>> +
>>>>> +void rpcb_put_local(void)
>>>>> +{
>>>>> +    struct rpc_clnt *clnt = rpcb_local_clnt;
>>>>> +    struct rpc_clnt *clnt4 = rpcb_local_clnt4;
>>>>> +    int shutdown;
>>>>> +
>>>>> +    spin_lock(&rpcb_clnt_lock);
>>>>> +    if (--rpcb_users == 0) {
>>>>> +        rpcb_local_clnt = NULL;
>>>>> +        rpcb_local_clnt4 = NULL;
>>>>> +    }
>>>>
>>>> In the function below, you mention that the above pointers are
>>>> protected by rpcb_create_local_mutex, but it looks like they get reset
>>>> here without that being held?
>>>>
>>>
>>> Assigning of them is protected by rpcb_create_local_mutex.
>>> Dereferencing of them is protected by rpcb_clnt_lock.
>>
>> Shouldn't you be using the same lock for assigning and dereferencing?  Otherwise one thread could change these variables while another is using them.
> 
> Probably I wasn't clear with my previous explanation.
> Actually, we use only spinlock to make sure, that the pointers are valid when we dereferencing them. Synchronization point here is rpcb_users counter.
> IOW, we use these pointers only from svc code and only after service already started. And with this patch-set we can be sure, that this pointers has been created already to the point, when this service starts.
> 
> But when this counter is zero, we can experience races with assigning those pointers. It takes a lot of time, so we use local mutex here instead of spinlock.
> 
> Have I answered your question?

I think I understand now.  Thanks!
> 

Re: [PATCH v5 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients

[Stanislav Kinsbursky]

20.09.2011 19:11, Jeff Layton пишет:
>
> In general, it's difficult to get locking right, especially when you
> start mixing multiple locks on related resources. Personally, I'd go
> with a simpler scheme here. There's not much value in protecting this
> counter with a spinlock when the other parts need to be protected by a
> mutex. If you do decide to do it with multiple locks, then please do
> document in comments how the locking is expected to work.
>
> An alternate scheme might be to consider doing this with krefs, but I
> haven't really considered whether that idiom makes sense here.
>

Jeff, please, have a look at my answer to Bryan Schumaker.
Does it allay your apprehensions?

-- 
Best regards,
Stanislav Kinsbursky

Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients

[Stanislav Kinsbursky]

20.09.2011 18:38, Myklebust, Trond пишет:
>> -----Original Message-----
>> From: Stanislav Kinsbursky [mailto:skinsbursky@parallels.com]
>> Sent: Tuesday, September 20, 2011 10:35 AM
>> To: Myklebust, Trond
>> Cc: Schumaker, Bryan; linux-nfs@vger.kernel.org; Pavel Emelianov;
>> neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
>> bfields@fieldses.org; davem@davemloft.net
>> Subject: Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference
>> counted rpcbind clients
>>
>> 20.09.2011 18:14, Myklebust, Trond пишет:
>>
>>>>>
>>>>> Doesn't it  need to be protected by rpcb_clnt_lock too?
>>>>>
>>>>
>>>> Nope from my pow. It's protected by rpcb_create_local_mutex. I.e. no
>>>> one will change rpcb_users since it's zero. If it's non zero - we
>>>> willn't get to rpcb_set_local().
>>>
>>> OK, so you are saying that the rpcb_users++ below could be replaced by
>> rpcb_users=1?
>>>
>>
>> Yes, you right.
>>
>>> In that case, don't you need a smp_wmb() between the setting of
>> rpcb_local_clnt/4 and the setting of rpcb_users? Otherwise, how do you
>> guarantee that rpcb_users != 0 implies rpbc_local_clnt/4 != NULL?
>>>
>>
>> We check rpcb_users under spinlock. Gain spinlock forces memory barrier,
>> doesn't it?
>
> Yes, and so you don't need an smp_rmb() on the reader side. However, you still need to ensure that the processor which _sets_ the rpcb_users and rpcb_local_clnt/4 actually writes them in the correct order.
>

Trond, I've thought again and realized, that even if these writes (rpcb_users 
and rpbc_local_clnt/4) will be done in reversed order, then no barrier required 
here.
Because if we have another process trying to create those clients (it can't use 
them since it's not started yet) on other CPU, than it's waiting on creation 
mutex. When it will gain the mutex, we will have required memory barrier for us.

Or I missed something in your explanation?

-- 
Best regards,
Stanislav Kinsbursky

RE: [PATCH v4 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients

[Myklebust, Trond]

> -----Original Message-----
> From: Stanislav Kinsbursky [mailto:skinsbursky@parallels.com]
> Sent: Tuesday, September 20, 2011 12:21 PM
> To: Myklebust, Trond
> Cc: Schumaker, Bryan; linux-nfs@vger.kernel.org; Pavel Emelianov;
> neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> bfields@fieldses.org; davem@davemloft.net
> Subject: Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference
> counted rpcbind clients
> 
> 20.09.2011 18:38, Myklebust, Trond пишет:
> >> -----Original Message-----
> >> From: Stanislav Kinsbursky [mailto:skinsbursky@parallels.com]
> >> Sent: Tuesday, September 20, 2011 10:35 AM
> >> To: Myklebust, Trond
> >> Cc: Schumaker, Bryan; linux-nfs@vger.kernel.org; Pavel Emelianov;
> >> neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> >> bfields@fieldses.org; davem@davemloft.net
> >> Subject: Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference
> >> counted rpcbind clients
> >>
> >> 20.09.2011 18:14, Myklebust, Trond пишет:
> >>
> >>>>>
> >>>>> Doesn't it  need to be protected by rpcb_clnt_lock too?
> >>>>>
> >>>>
> >>>> Nope from my pow. It's protected by rpcb_create_local_mutex. I.e.
> >>>> no one will change rpcb_users since it's zero. If it's non zero -
> >>>> we willn't get to rpcb_set_local().
> >>>
> >>> OK, so you are saying that the rpcb_users++ below could be replaced
> >>> by
> >> rpcb_users=1?
> >>>
> >>
> >> Yes, you right.
> >>
> >>> In that case, don't you need a smp_wmb() between the setting of
> >> rpcb_local_clnt/4 and the setting of rpcb_users? Otherwise, how do
> >> you guarantee that rpcb_users != 0 implies rpbc_local_clnt/4 != NULL?
> >>>
> >>
> >> We check rpcb_users under spinlock. Gain spinlock forces memory
> >> barrier, doesn't it?
> >
> > Yes, and so you don't need an smp_rmb() on the reader side. However,
> you still need to ensure that the processor which _sets_ the rpcb_users and
> rpcb_local_clnt/4 actually writes them in the correct order.
> >
> 
> Trond, I've thought again and realized, that even if these writes (rpcb_users
> and rpbc_local_clnt/4) will be done in reversed order, then no barrier
> required here.
> Because if we have another process trying to create those clients (it can't use
> them since it's not started yet) on other CPU, than it's waiting on creation
> mutex. When it will gain the mutex, we will have required memory barrier
> for us.
> 
> Or I missed something in your explanation?

You need to ensure that if someone calls rpcb_get_local() and gets a positive result, then they can rely on rpcb_local_clnt/4 being non-zero. Without the write barrier, that is not the case.

Without that guarantee, you can't really ensure that rpcb_put_local() will work as expected either, since it will be possible to trigger the --rpcb_users == 0 case without shutting down rpcb_local_clnt/4 (because clnt/clnt4 == 0).

Cheers
  Trond

Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients

[Stanislav Kinsbursky]

20.09.2011 21:13, Myklebust, Trond пишет:
>> -----Original Message-----
>> From: Stanislav Kinsbursky [mailto:skinsbursky@parallels.com]
>> Sent: Tuesday, September 20, 2011 12:21 PM
>> To: Myklebust, Trond
>> Cc: Schumaker, Bryan; linux-nfs@vger.kernel.org; Pavel Emelianov;
>> neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
>> bfields@fieldses.org; davem@davemloft.net
>> Subject: Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference
>> counted rpcbind clients
>>
>> 20.09.2011 18:38, Myklebust, Trond пишет:
>>>> -----Original Message-----
>>>> From: Stanislav Kinsbursky [mailto:skinsbursky@parallels.com]
>>>> Sent: Tuesday, September 20, 2011 10:35 AM
>>>> To: Myklebust, Trond
>>>> Cc: Schumaker, Bryan; linux-nfs@vger.kernel.org; Pavel Emelianov;
>>>> neilb@suse.de; netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
>>>> bfields@fieldses.org; davem@davemloft.net
>>>> Subject: Re: [PATCH v4 1/8] SUNRPC: introduce helpers for reference
>>>> counted rpcbind clients
>>>>
>>>> 20.09.2011 18:14, Myklebust, Trond пишет:
>>>>
>>>>>>>
>>>>>>> Doesn't it  need to be protected by rpcb_clnt_lock too?
>>>>>>>
>>>>>>
>>>>>> Nope from my pow. It's protected by rpcb_create_local_mutex. I.e.
>>>>>> no one will change rpcb_users since it's zero. If it's non zero -
>>>>>> we willn't get to rpcb_set_local().
>>>>>
>>>>> OK, so you are saying that the rpcb_users++ below could be replaced
>>>>> by
>>>> rpcb_users=1?
>>>>>
>>>>
>>>> Yes, you right.
>>>>
>>>>> In that case, don't you need a smp_wmb() between the setting of
>>>> rpcb_local_clnt/4 and the setting of rpcb_users? Otherwise, how do
>>>> you guarantee that rpcb_users != 0 implies rpbc_local_clnt/4 != NULL?
>>>>>
>>>>
>>>> We check rpcb_users under spinlock. Gain spinlock forces memory
>>>> barrier, doesn't it?
>>>
>>> Yes, and so you don't need an smp_rmb() on the reader side. However,
>> you still need to ensure that the processor which _sets_ the rpcb_users and
>> rpcb_local_clnt/4 actually writes them in the correct order.
>>>
>>
>> Trond, I've thought again and realized, that even if these writes (rpcb_users
>> and rpbc_local_clnt/4) will be done in reversed order, then no barrier
>> required here.
>> Because if we have another process trying to create those clients (it can't use
>> them since it's not started yet) on other CPU, than it's waiting on creation
>> mutex. When it will gain the mutex, we will have required memory barrier
>> for us.
>>
>> Or I missed something in your explanation?
>
> You need to ensure that if someone calls rpcb_get_local() and gets a positive result, then they can rely on rpcb_local_clnt/4 being non-zero. Without the write barrier, that is not the case.
>

In current context we can be sure, that between rpcb_get_local() and first 
dereference of rpcb_local_clnt/4 we have at least one spinlock 
(svc_xprt_class_lock in svc_create_xprt).
But I understand, that we can't relay on it since this code can be changed in 
future.
So I'll add barrier there.

> Without that guarantee, you can't really ensure that rpcb_put_local() will work as expected either, since it will be possible to trigger the --rpcb_users == 0 case without shutting down rpcb_local_clnt/4 (because clnt/clnt4 == 0).
>

Yes, you right. But it doesn't mean, that we require barrier here, because we 
don't need this garantee you are talking about.
We can be sure, that we always see right rpcb_users value. It means that, if we 
set this value to zero, then no other running services left and no references to 
those clients can occur.
And even if we have another process which is going to create new service right 
now on another CPU, then this process will see that no rpcbind users present and 
will create new clients and assign them to global variables prior to any use of 
this clients can occur.
And this assign will be done with barrier as we agreed earlier.

> Cheers
>    Trond
>


-- 
Best regards,
Stanislav Kinsbursky

Re: [PATCH v6 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients

[Stanislav Kinsbursky]

v6:
1) added write memory barrier to rpcb_set_local to make sure, that rpcbind 
clients become valid before rpcb_users assignment
2) explicitly set rpcb_users to 1 instead of incrementing it (looks clearer from 
my pow).

Notice: write memory barrier after zeroing rpcbind clients in rpcb_put_local() 
is not required, since to users of them left. New user (service) will create new 
clients before dereferencing them.

This helpers will be used for dynamical creation and destruction of rpcbind
clients.
Variable rpcb_users is actually a counter of lauched RPC services. If rpcbind
clients has been created already, then we just increase rpcb_users.

Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>

---
  net/sunrpc/rpcb_clnt.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++++
  1 files changed, 54 insertions(+), 0 deletions(-)

diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
index e45d2fb..9fcdb42 100644
--- a/net/sunrpc/rpcb_clnt.c
+++ b/net/sunrpc/rpcb_clnt.c
@@ -114,6 +114,9 @@ static struct rpc_program	rpcb_program;
  static struct rpc_clnt *	rpcb_local_clnt;
  static struct rpc_clnt *	rpcb_local_clnt4;

+DEFINE_SPINLOCK(rpcb_clnt_lock);
+unsigned int			rpcb_users;
+
  struct rpcbind_args {
  	struct rpc_xprt *	r_xprt;

@@ -161,6 +164,57 @@ static void rpcb_map_release(void *data)
  	kfree(map);
  }

+static int rpcb_get_local(void)
+{
+	int cnt;
+
+	spin_lock(&rpcb_clnt_lock);
+	if (rpcb_users)
+		rpcb_users++;
+	cnt = rpcb_users;
+	spin_unlock(&rpcb_clnt_lock);
+
+	return cnt;
+}
+
+void rpcb_put_local(void)
+{
+	struct rpc_clnt *clnt = rpcb_local_clnt;
+	struct rpc_clnt *clnt4 = rpcb_local_clnt4;
+	int shutdown;
+
+	spin_lock(&rpcb_clnt_lock);
+	if (--rpcb_users == 0) {
+		rpcb_local_clnt = NULL;
+		rpcb_local_clnt4 = NULL;
+	}
+	shutdown = !rpcb_users;
+	spin_unlock(&rpcb_clnt_lock);
+
+	if (shutdown) {
+		/*
+		 * cleanup_rpcb_clnt - remove xprtsock's sysctls, unregister
+		 */
+		if (clnt4)
+			rpc_shutdown_client(clnt4);
+		if (clnt)
+			rpc_shutdown_client(clnt);
+	}
+	return;
+}
+
+static void rpcb_set_local(struct rpc_clnt *clnt, struct rpc_clnt *clnt4)
+{
+	/* Protected by rpcb_create_local_mutex */
+	rpcb_local_clnt = clnt;
+	rpcb_local_clnt4 = clnt4;
+	smp_wmb();
+	rpcb_users = 1;
+	dprintk("RPC:       created new rpcb local clients (rpcb_local_clnt: "
+			"%p, rpcb_local_clnt4: %p)\n", rpcb_local_clnt,
+			rpcb_local_clnt4);
+}
+
  /*
   * Returns zero on success, otherwise a negative errno value
   * is returned.

Re: [PATCH v6 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients

[Stanislav Kinsbursky]

Trond, is this patch version suits you now? Or not?
Please, comment somehow to let me know, may I proceed with further development 
or not.

Sorry for disturbing (if so).

21.09.2011 13:07, Stanislav Kinsbursky пишет:
> v6:
> 1) added write memory barrier to rpcb_set_local to make sure, that rpcbind
> clients become valid before rpcb_users assignment
> 2) explicitly set rpcb_users to 1 instead of incrementing it (looks clearer from
> my pow).
>
> Notice: write memory barrier after zeroing rpcbind clients in rpcb_put_local()
> is not required, since to users of them left. New user (service) will create new
> clients before dereferencing them.
>
> This helpers will be used for dynamical creation and destruction of rpcbind
> clients.
> Variable rpcb_users is actually a counter of lauched RPC services. If rpcbind
> clients has been created already, then we just increase rpcb_users.
>
> Signed-off-by: Stanislav Kinsbursky<skinsbursky@parallels.com>
>
> ---
>    net/sunrpc/rpcb_clnt.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++++
>    1 files changed, 54 insertions(+), 0 deletions(-)
>
> diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
> index e45d2fb..9fcdb42 100644
> --- a/net/sunrpc/rpcb_clnt.c
> +++ b/net/sunrpc/rpcb_clnt.c
> @@ -114,6 +114,9 @@ static struct rpc_program	rpcb_program;
>    static struct rpc_clnt *	rpcb_local_clnt;
>    static struct rpc_clnt *	rpcb_local_clnt4;
>
> +DEFINE_SPINLOCK(rpcb_clnt_lock);
> +unsigned int			rpcb_users;
> +
>    struct rpcbind_args {
>    	struct rpc_xprt *	r_xprt;
>
> @@ -161,6 +164,57 @@ static void rpcb_map_release(void *data)
>    	kfree(map);
>    }
>
> +static int rpcb_get_local(void)
> +{
> +	int cnt;
> +
> +	spin_lock(&rpcb_clnt_lock);
> +	if (rpcb_users)
> +		rpcb_users++;
> +	cnt = rpcb_users;
> +	spin_unlock(&rpcb_clnt_lock);
> +
> +	return cnt;
> +}
> +
> +void rpcb_put_local(void)
> +{
> +	struct rpc_clnt *clnt = rpcb_local_clnt;
> +	struct rpc_clnt *clnt4 = rpcb_local_clnt4;
> +	int shutdown;
> +
> +	spin_lock(&rpcb_clnt_lock);
> +	if (--rpcb_users == 0) {
> +		rpcb_local_clnt = NULL;
> +		rpcb_local_clnt4 = NULL;
> +	}
> +	shutdown = !rpcb_users;
> +	spin_unlock(&rpcb_clnt_lock);
> +
> +	if (shutdown) {
> +		/*
> +		 * cleanup_rpcb_clnt - remove xprtsock's sysctls, unregister
> +		 */
> +		if (clnt4)
> +			rpc_shutdown_client(clnt4);
> +		if (clnt)
> +			rpc_shutdown_client(clnt);
> +	}
> +	return;
> +}
> +
> +static void rpcb_set_local(struct rpc_clnt *clnt, struct rpc_clnt *clnt4)
> +{
> +	/* Protected by rpcb_create_local_mutex */
> +	rpcb_local_clnt = clnt;
> +	rpcb_local_clnt4 = clnt4;
> +	smp_wmb();
> +	rpcb_users = 1;
> +	dprintk("RPC:       created new rpcb local clients (rpcb_local_clnt: "
> +			"%p, rpcb_local_clnt4: %p)\n", rpcb_local_clnt,
> +			rpcb_local_clnt4);
> +}
> +
>    /*
>     * Returns zero on success, otherwise a negative errno value
>     * is returned.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Best regards,
Stanislav Kinsbursky

RE: [PATCH v6 1/8] SUNRPC: introduce helpers for reference counted rpcbind clients

[Myklebust, Trond]

> -----Original Message-----
> From: Stanislav Kinsbursky [mailto:skinsbursky@parallels.com]
> Sent: Friday, September 23, 2011 10:41 AM
> To: Myklebust, Trond
> Cc: linux-nfs@vger.kernel.org; Pavel Emelianov; neilb@suse.de;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> bfields@fieldses.org; davem@davemloft.net
> Subject: Re: [PATCH v6 1/8] SUNRPC: introduce helpers for reference
> counted rpcbind clients
> 
> Trond, is this patch version suits you now? Or not?
> Please, comment somehow to let me know, may I proceed with further
> development or not.

Hi Stanislav,

Yes, this version of the patch looks safe to me.

Cheers
  Trond