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

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

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 <redacted>

---
 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.

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 <redacted>

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

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

Signed-off-by: Stanislav Kinsbursky <redacted>

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

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 <redacted>

---
 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;
 }

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 <redacted>

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

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

Signed-off-by: Stanislav Kinsbursky <redacted>

---
 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;
 

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 <redacted>

---
 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();

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

Signed-off-by: Stanislav Kinsbursky <redacted>

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

On Tue, 20 Sep 2011 14:14:04 +0400
Stanislav Kinsbursky [off-list ref] 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 [off-list ref]
--
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

On Tue, 20 Sep 2011 14:13:32 +0400
Stanislav Kinsbursky [off-list ref] 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 <redacted>
--
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

On 09/20/2011 06:13 AM, Stanislav Kinsbursky wrote:

        ^^^^^^^^^^^^^^^^^^
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

-----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:

quoted

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 <redacted>

---
 net/sunrpc/rpcb_clnt.c |   50

++++++++++++++++++++++++++++++++++++++++++++++++

quoted

 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.

quoted

+}
+
+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?

quoted

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

^^^^^^^^^^^^^^^^^^^

quoted

+	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

quoted

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

20.09.2011 17:15, Myklebust, Trond пишет:

quoted

-----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:

quoted

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<redacted>

---
  net/sunrpc/rpcb_clnt.c |   50

++++++++++++++++++++++++++++++++++++++++++++++++

quoted

  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.

quoted

quoted

+}
+
+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().

quoted

quoted

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

^^^^^^^^^^^^^^^^^^^

quoted

quoted

+	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

quoted

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

�{.n�+�������+%��lzwm��b�맲��r��zX��߲)���w*jg���

�����ݢj/���z�ޖ��2�ޙ���&�)ߡ�a����
�G���h��j:+v���w�٥


-- 
Best regards,
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 <redacted>

---
 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.

-----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 пишет:

quoted

quoted

-----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:

quoted

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.

quoted

quoted

quoted

Signed-off-by: Stanislav Kinsbursky<redacted>

---
  net/sunrpc/rpcb_clnt.c |   50

++++++++++++++++++++++++++++++++++++++++++++++++

quoted

  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.

quoted

Yep, you right. Will fix this races.

quoted

quoted

quoted

+}
+
+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?

quoted

quoted

quoted

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

^^^^^^^^^^^^^^^^^^^

quoted

quoted

+	dprintk("RPC:       created new rpcb local clients (rpcb_local_clnt: "
+			"%p, rpcb_local_clnt4: %p)\n", rpcb_local_clnt,
+			rpcb_local_clnt4);
+}
+

��칻
�&�~�&���+-��ݶ��w��˛���m�b��g~ȧ���ܨ}���Ơz�&j:+v����n�r��6;靫3��\
nnX��f�z��2�ޙ���&�)ߡ�a����
�G���h��j:+v���w�٥

On Tue, 20 Sep 2011 17:49:27 +0400
Stanislav Kinsbursky [off-list ref] wrote:

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 [off-list ref]

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

quoted

quoted

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

-----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 пишет:

quoted

quoted

quoted

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?

quoted

Yes, you right.

quoted

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?

quoted

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

-----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 [off-list ref] wrote:

quoted

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

quoted

rpcbind clients has been created already, then we just increase

rpcb_users.

quoted

Signed-off-by: Stanislav Kinsbursky <redacted>

---
  net/sunrpc/rpcb_clnt.c |   53

++++++++++++++++++++++++++++++++++++++++++++++++

quoted

  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.

20.09.2011 18:24, Jeff Layton пишет:

On Tue, 20 Sep 2011 17:49:27 +0400
Stanislav Kinsbursky[off-list ref]  wrote:

quoted

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<redacted>

---
   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?

quoted

+	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

On 09/20/2011 10:43 AM, Stanislav Kinsbursky wrote:

20.09.2011 18:24, Jeff Layton пишет:

quoted

On Tue, 20 Sep 2011 17:49:27 +0400
Stanislav Kinsbursky[off-list ref]  wrote:

quoted

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<redacted>

---
   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.

quoted

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?

quoted

quoted

+    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

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

quoted

-----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 пишет:

quoted

quoted

quoted

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?

quoted

Yes, you right.

quoted

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?

quoted

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

On Tue, 20 Sep 2011 18:43:45 +0400
Stanislav Kinsbursky [off-list ref] wrote:

20.09.2011 18:24, Jeff Layton пишет:

quoted

On Tue, 20 Sep 2011 17:49:27 +0400
Stanislav Kinsbursky[off-list ref]  wrote:

quoted

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<redacted>

---
   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.

quoted

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 [off-list ref]

20.09.2011 18:58, Bryan Schumaker пишет:

On 09/20/2011 10:43 AM, Stanislav Kinsbursky wrote:

quoted

20.09.2011 18:24, Jeff Layton пишет:

quoted

On Tue, 20 Sep 2011 17:49:27 +0400
Stanislav Kinsbursky[off-list ref]   wrote:

quoted

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<redacted>

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

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

quoted

-----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[off-list ref]  wrote:

quoted

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

quoted

quoted

rpcbind clients has been created already, then we just increase

rpcb_users.

quoted

quoted

Signed-off-by: Stanislav Kinsbursky<redacted>

---
   net/sunrpc/rpcb_clnt.c |   53

++++++++++++++++++++++++++++++++++++++++++++++++

quoted

   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

quoted

rpcb_create_local_mutex, but it looks like they get reset here without

that

quoted

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

quoted

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

On 09/20/2011 11:38 AM, Stanislav Kinsbursky wrote:

20.09.2011 18:58, Bryan Schumaker пишет:

quoted

On 09/20/2011 10:43 AM, Stanislav Kinsbursky wrote:

quoted

20.09.2011 18:24, Jeff Layton пишет:

quoted

On Tue, 20 Sep 2011 17:49:27 +0400
Stanislav Kinsbursky[off-list ref]   wrote:

quoted

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<redacted>

---
    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!

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

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

quoted

-----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 пишет:

quoted

quoted

quoted

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?

quoted

Yes, you right.

quoted

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?

quoted

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

-----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 пишет:

quoted

quoted

-----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 пишет:

quoted

quoted

quoted

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?

quoted

Yes, you right.

quoted

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?

quoted

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.

quoted

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

20.09.2011 21:13, Myklebust, Trond пишет:

quoted

-----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 пишет:

quoted

quoted

-----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 пишет:

quoted

quoted

quoted

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?

quoted

Yes, you right.

quoted

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?

quoted

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.

quoted

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

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 <redacted>

---
 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.

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 пишет:

-- 
Best regards,
Stanislav Kinsbursky

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