remove useless MOD_{INC,DEC}_USE_COUNT from sunrpc

- both rpciod_up and rpciod_down do a gratious inc/dec of the use count - but we can't ever be inside those function unless it's called from an other module -> totally useless
- rpciod() (the ernel thread) also bumps the refcount when starting and decrements it when exiting. but as a different module must initiate this using rpciod_up/rpciod_down this is again not needed. (except when a module does rpciod_up without a matching rpciod_down - but that a big bug anyway and we don't need to partially handle that using module refcounts).

@@ -952,7 +952,6 @@
 	wait_queue_head_t *assassin = (wait_queue_head_t*) ptr;
 	int		rounds = 0;
 
-	MOD_INC_USE_COUNT;
 	lock_kernel();
 	/*
 	 * Let our maker know we're running ...

@@ -995,7 +994,6 @@
 
 	dprintk("RPC: rpciod exiting\n");
 	unlock_kernel();
-	MOD_DEC_USE_COUNT;
 	return 0;
 }
 

@@ -1027,7 +1025,6 @@
 {
 	int error = 0;
 
-	MOD_INC_USE_COUNT;
 	down(&rpciod_sema);
 	dprintk("rpciod_up: pid %d, users %d\n", rpciod_pid, rpciod_users);
 	rpciod_users++;

@@ -1051,7 +1048,6 @@
 	error = 0;
 out:
 	up(&rpciod_sema);
-	MOD_DEC_USE_COUNT;
 	return error;
 }
 

@@ -1060,7 +1056,6 @@
 {
 	unsigned long flags;
 
-	MOD_INC_USE_COUNT;
 	down(&rpciod_sema);
 	dprintk("rpciod_down pid %d sema %d\n", rpciod_pid, rpciod_users);
 	if (rpciod_users) {

@@ -1097,7 +1092,6 @@
 	spin_unlock_irqrestore(&current->sighand->siglock, flags);
 out:
 	up(&rpciod_sema);
-	MOD_DEC_USE_COUNT;
 }
 
 #ifdef RPC_DEBUG

> - rpciod() (the ernel thread) also bumps the refcount when starting
> and decrements it when exiting. but as a different module
> must initiate this using rpciod_up/rpciod_down this is again
> not needed. (except when a module does rpciod_up without a
> matching rpciod_down - but that a big bug anyway and we
> don't need to partially handle that using module refcounts).

There's another case which you appear to be ignoring: rpciod_down() is interruptible and does not have to wait on the rpciod() thread to complete.

Cheers,
Trond

What do you thing about something like the following to wait on the thread in module_exit()?

@@ -188,6 +188,7 @@
 int		rpciod_up(void);
 void		rpciod_down(void);
 void		rpciod_wake_up(void);
+void		wait_on_rpciod(void);
 #ifdef RPC_DEBUG
 void		rpc_show_tasks(void);
 #endif

@@ -1097,6 +1092,12 @@
 	spin_unlock_irqrestore(&current->sighand->siglock, flags);
 out:
 	up(&rpciod_sema);
+}
+
+void
+wait_on_rpciod(void)
+{
+	wait_event_interruptible(rpciod_killer, !rpciod_pid);
 }
 
 #ifdef RPC_DEBUG

> On Sun, May 04, 2003 at 07:37:18PM +0200, Trond Myklebust > wrote:
>> There's another case which you appear to be ignoring:
>> rpciod_down() is interruptible and does not have to wait on the >> rpciod() thread to complete.

> What do you thing about something like the following to wait on > the thread in module_exit()?

I don't understand. That is still an interruptible wait, so how would that help?

What is wrong with just assuming that the rpciod() thread might need to run independently of the calling module for a short period of time in order to kill/clean up the pending tasks?

Cheers,
Trond

It doesn't :) Sorry, the code should use wait_even(), the _interruptible was a pasto..

That's fine and the patch doesn't change anything about the assumption. If just changes how to make sure the sunrpc module can't be unloaded during that period. Previously you incremented the usecount and now we're waiting for the thread to finish in module_exit().

@@ -188,6 +188,7 @@
 int		rpciod_up(void);
 void		rpciod_down(void);
 void		rpciod_wake_up(void);
+void		wait_on_rpciod(void);
 #ifdef RPC_DEBUG
 void		rpc_show_tasks(void);
 #endif

@@ -1097,6 +1092,12 @@
 	spin_unlock_irqrestore(&current->sighand->siglock, flags);
 out:
 	up(&rpciod_sema);
+}
+
+void
+wait_on_rpciod(void)
+{
+	wait_event(&rpciod_killer, !rpciod_pid);
 }
 
 #ifdef RPC_DEBUG

> Previously you incremented the usecount and now we're waiting > for the thread to finish in module_exit().

Fair enough...

Cheers,
Trond

Well, what if this hangs? Unless the user specifies "--wait" to 2.5.x's rmmod, the user absolutely does not expect this behavior.

Rather, so that the "--wait" is respected, something one level up ought to be doing try_module_get().

If you want to change the behavior, that's definitely to be considered, but on a global scale not locally for sunrpc.

David S. Miller <[email protected]>

Actually. Looking around at the server side, there appear to be a couple of potential problems too...

There don't seem to be any checks and balances to prevent the user from removing the sunrpc module immediately after svc_create_thread() gets called.
I presume that the call to lockd_up() in nfsd() will help (since that calls rpciod_up() and hence MOD_INC_COUNT) but AFAICS there appears to be plenty of possible races before we get to that point.

Cheers,
Trond

> There don't seem to be any checks and balances to prevent the > user from removing the sunrpc module immediately after > svc_create_thread() gets called. I presume that the call to > lockd_up() in nfsd() will help (since that calls rpciod_up() > and hence MOD_INC_COUNT) but AFAICS there appears to be plenty > of possible races before we get to that point.

Sorry. I misread that code. The only nfsd problem goes the other way round. nfsd() is not exported, and nfsd_svc() does not wait on the threads to start, hence you have a race between nfsd_svc(), and the MOD_INC_USE_COUNT inside the nfsd() threads.

Cheers,
Trond

Oh well, what about something like the following?

@@ -952,6 +952,14 @@
 	wait_queue_head_t *assassin = (wait_queue_head_t*) ptr;
 	int		rounds = 0;
 
+	/*
+	 * We are locked into memory by beeing used by another module,
+	 * but the refcount might be 0 neverless so we can't use
+	 * __module_get().
+	 */
+	if (!try_module_get(THIS_MODULE))
+		return -EBUSY;
+
 	lock_kernel();
 	/*
 	 * Let our maker know we're running ...

@@ -994,6 +1002,7 @@
 
 	dprintk("RPC: rpciod exiting\n");
 	unlock_kernel();
+	module_put(THIS_MODULE);
 	return 0;
 }
 

@@ -1042,6 +1051,11 @@
 	if (error < 0) {
 		printk(KERN_WARNING "rpciod_up: create thread failed, error=%d\n", error);
 		rpciod_users--;
+		goto out;
+	}
+	if (!rpciod_pid) {
+		rpciod_users--;
+		error = -EBUSY;
 		goto out;
 	}
 	down(&rpciod_running);

From: Christoph Hellwig <[email protected]>
Date: Sun, 4 May 2003 23:00:11 +0200

Oh well, what about something like the following?
...
+ */
+ if (!try_module_get(THIS_MODULE))
+ return -EBUSY;

Ahem... Why don't we just do this right? :-)

By this I mean provide some real registry thing in the main kernel image that we can use to do try_module_get() outside of the sunrpc module?

The other option is the make more progress in the area of two-stage module unload, and allowing cleanup() to return whether the module is unloadable or not. This is being discussed on netdev so that we have some way to make ipv6 modules work sanely (instead of putting try_module_get() in every other line, that simply isn't acceptable).

The situation in rxrpc looks worse btw...

Please read the comment above that piece of code. We always enter this through an exported function so we know we currently aren't unloadable.

That's fine with me, but I won't sign up to do that work :)