[PATCH] remove useless MOD_{INC,DEC}_USE_COUNT from sunrpc

[PATCH] remove useless MOD_{INC,DEC}_USE_COUNT from sunrpc

[Christoph Hellwig]

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


--- 1.24/net/sunrpc/sched.c	Thu Mar 27 12:42:11 2003
+++ edited/net/sunrpc/sched.c	Thu May  1 16:52:23 2003
@@ -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

[PATCH] remove useless MOD_{INC,DEC}_USE_COUNT from sunrpc

[Trond Myklebust]

>>>>> " " == Christoph Hellwig <hch@lst.de> writes:

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

Re: [PATCH] remove useless MOD_{INC,DEC}_USE_COUNT from sunrpc

[Christoph Hellwig]

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


--- 1.10/include/linux/sunrpc/sched.h	Sun Jan 12 16:40:13 2003
+++ edited/include/linux/sunrpc/sched.h	Sun May  4 19:08:09 2003
@@ -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
--- 1.24/net/sunrpc/sched.c	Thu Mar 27 12:42:11 2003
+++ edited/net/sunrpc/sched.c	Sun May  4 19:07:42 2003
@@ -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

Re: [PATCH] remove useless MOD_{INC,DEC}_USE_COUNT from sunrpc

[Trond Myklebust]

>>>>> " " == Christoph Hellwig <hch@lst.de> writes:

     > 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

Re: [PATCH] remove useless MOD_{INC,DEC}_USE_COUNT from sunrpc

[Christoph Hellwig]

On Sun, May 04, 2003 at 08:46:47PM +0200, Trond Myklebust wrote:
>      > 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?

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

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

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


--- 1.10/include/linux/sunrpc/sched.h	Sun Jan 12 16:40:13 2003
+++ edited/include/linux/sunrpc/sched.h	Sun May  4 19:08:09 2003
@@ -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
--- 1.24/net/sunrpc/sched.c	Thu Mar 27 12:42:11 2003
+++ edited/net/sunrpc/sched.c	Sun May  4 19:07:42 2003
@@ -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

Re: [PATCH] remove useless MOD_{INC,DEC}_USE_COUNT from sunrpc

[Trond Myklebust]

>>>>> " " == Christoph Hellwig <hch@lst.de> writes:

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

Fair enough...

Cheers,
  Trond

Re: [PATCH] remove useless MOD_{INC,DEC}_USE_COUNT from sunrpc

[David S. Miller]

On Sun, 2003-05-04 at 12:00, Trond Myklebust wrote:
> >>>>> " " == Christoph Hellwig <hch@lst.de> writes:
> 
>      > Previously you incremented the usecount and now we're waiting
>      > for the thread to finish in module_exit().
> 
> Fair enough...

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

Re: [PATCH] remove useless MOD_{INC,DEC}_USE_COUNT from sunrpc

[Trond Myklebust]

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

Re: [PATCH] remove useless MOD_{INC,DEC}_USE_COUNT from sunrpc

[Trond Myklebust]

>>>>> " " == Trond Myklebust <trond.myklebust@fys.uio.no> writes:

     > 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

Re: [PATCH] remove useless MOD_{INC,DEC}_USE_COUNT from sunrpc

[David S. Miller]

   From: Christoph Hellwig <hch@lst.de>
   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...

Re: [PATCH] remove useless MOD_{INC,DEC}_USE_COUNT from sunrpc

[Christoph Hellwig]

On Sun, May 04, 2003 at 12:06:06PM -0700, David S. Miller wrote:
> 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().

Oh well, what about something like the following?


--- 1.25/net/sunrpc/sched.c	Thu May  1 12:52:23 2003
+++ edited/net/sunrpc/sched.c	Sun May  4 21:25:21 2003
@@ -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);

Re: [PATCH] remove useless MOD_{INC,DEC}_USE_COUNT from sunrpc

[Christoph Hellwig]

On Sun, May 04, 2003 at 01:15:58PM -0700, David S. Miller wrote:
>    From: Christoph Hellwig <hch@lst.de>
>    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?

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.

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

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