Very Computer

by **Christoph Hellwi** » Mon, 05 May 2003 19:20:12

- 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

wait_queue_head_t *assassin = (wait_queue_head_t*) ptr;
int rounds = 0;

- MOD_INC_USE_COUNT;
lock_kernel();
/*

dprintk("RPC: rpciod exiting\n");
unlock_kernel();
- MOD_DEC_USE_COUNT;
return 0;
}

{
int error = 0;

- MOD_INC_USE_COUNT;
down(&rpciod_sema);
dprintk("rpciod_up: pid %d, users %d\n", rpciod_pid, rpciod_users);

error = 0;
out:
up(&rpciod_sema);
- MOD_DEC_USE_COUNT;
return error;
}

{
unsigned long flags;

- MOD_INC_USE_COUNT;
down(&rpciod_sema);
dprintk("rpciod_down pid %d sema %d\n", rpciod_pid, rpciod_users);

spin_unlock_irqrestore(&current->sighand->siglock, flags);
out:
up(&rpciod_sema);
- MOD_DEC_USE_COUNT;
}

#ifdef RPC_DEBUG
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

by **Trond Myklebus** » Mon, 05 May 2003 19:50:05

> - 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

by **Christoph Hellwi** » Mon, 05 May 2003 20:40:13

> 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

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

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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

by **Trond Myklebus** » Mon, 05 May 2003 20:50:15

> On Sun, May 04, 2003 at 07:37:18PM +0200, Trond Myklebust

>> 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

by **Christoph Hellwi** » Mon, 05 May 2003 21:00:13

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

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

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

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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

by **Trond Myklebus** » Mon, 05 May 2003 21:10:08

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

Fair enough...

Cheers,
Trond
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

by **Trond Myklebus** » Mon, 05 May 2003 22:00:18

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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

by **Trond Myklebus** » Mon, 05 May 2003 22:20:09

> 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

by **David S. Mille** » Mon, 05 May 2003 23:10:11

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

--

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

by **Christoph Hellwi** » Mon, 05 May 2003 23:10:12

> 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

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

dprintk("RPC: rpciod exiting\n");
unlock_kernel();
+ module_put(THIS_MODULE);
return 0;
}

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);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

by **David S. Mille** » Mon, 05 May 2003 23:30:13

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...
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

by **Christoph Hellwi** » Tue, 06 May 2003 10:40:10

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

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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Very Computer

remove useless MOD_{INC,DEC}_USE_COUNT from sunrpc

remove useless MOD_{INC,DEC}_USE_COUNT from sunrpc

remove useless MOD_{INC,DEC}_USE_COUNT from sunrpc

remove useless MOD_{INC,DEC}_USE_COUNT from sunrpc

remove useless MOD_{INC,DEC}_USE_COUNT from sunrpc

remove useless MOD_{INC,DEC}_USE_COUNT from sunrpc

remove useless MOD_{INC,DEC}_USE_COUNT from sunrpc

remove useless MOD_{INC,DEC}_USE_COUNT from sunrpc

remove useless MOD_{INC,DEC}_USE_COUNT from sunrpc

remove useless MOD_{INC,DEC}_USE_COUNT from sunrpc

remove useless MOD_{INC,DEC}_USE_COUNT from sunrpc

remove useless MOD_{INC,DEC}_USE_COUNT from sunrpc

remove useless MOD_{INC,DEC}_USE_COUNT from sunrpc