I'm looking at merging the changes made to JFFS2 in 2.5, with the intention
of again having a single codebase that compiles in both 2.5 and 2.4.
I'm a little confused by the changes to recalc_sigpending().
It seems that the name of the function was changed to recalc_sigpending_tsk()
and a new function called recalc_sigpending() was added.
Was there a reason for doing this, rather than just introducing the new
function with a different name, such as recalc_sigpending_cur()? It breaks
2.4 source compatibility in a way that seems entirely gratuitous.
Before I have to go and do something evil in my compatmac.h to work round
this, is there any chance of putting the original recalc_sigpending() back?
Linus, would you accept such a patch?
--
dwmw2
#if LINUX_VERSION_CODE < KERNEL_VERSION(2,5,5)
#include <linux/sched.h>
/* Grrr. Gratuitous breakage */
#define recalc_sigpending() recalc_sigpending(current)
/* Unfortunately this one can't work, because of the above
#define recalc_sigpending_tsk(t) recalc_sigpending(t)
*/
#endif
On Thu, 28 Feb 2002, David Woodhouse wrote:
>
> It seems that the name of the function was changed to recalc_sigpending_tsk()
> and a new function called recalc_sigpending() was added.
Yes.
> Was there a reason for doing this, rather than just introducing the new
> function with a different name, such as recalc_sigpending_cur()? It breaks
> 2.4 source compatibility in a way that seems entirely gratuitous.
I don't care. I care 1000% more about clean code than about backwards
source level compatibility.
99% of all users did it for the current task, and for the current task
only. And the non-local users were all in low-level core code (and should
_not_ exist anywhere else - if some driver is playing around with another
tasks signal state, that driver is so incredibly fundamentally broken that
I don't even want to hear about it)
In short, the 2.5.x interface is the correct one.
> Before I have to go and do something evil in my compatmac.h to work round
> this, is there any chance of putting the original recalc_sigpending() back?
Not a chance in hell. The backwards compatibility looks like a trivial
one-liner:
compat-2.4.h:
#define recalc_sigpending() recalc_sigpending(current)
so what are you complaining about?
Linus
[email protected] said:
> Not a chance in hell. The backwards compatibility looks like a
> trivial one-liner:
> compat-2.4.h:
> #define recalc_sigpending() recalc_sigpending(current)
> so what are you complaining about?
Fine. I was trying to define a back-compat version of recalc_sigpending_tsk()
too, before going through all the code and changing recalc_sigpending(current)
to recalc_sigpending and recalc_sigpending(other) to recalc_sigpending_tsk()
- but as you rightly point out there's no justification for using the latter
in any of the driver or fs code that I'm trying to support - and hence it
isn't present, and doesn't need the compat support.
--
dwmw2
[email protected] said:
> Not a chance in hell. The backwards compatibility looks like a
> trivial one-liner:
> compat-2.4.h:
> #define recalc_sigpending() recalc_sigpending(current)
> so what are you complaining about?
It may be possible, but it's not a trivial one-liner. Am I missing
something obvious? Other than the fact that you don't care, of course.
background.c: In function `jffs2_garbage_collect_thread':
background.c:116: warning: implicit declaration of function `recalc_sigpending'
$ grep recalc_sigpending background.i
#define __ver_recalc_sigpending _ver_str(6682695c)
#define recalc_sigpending _set_ver(recalc_sigpending)
static inline void recalc_sigpending_Rsmp_6682695c(struct task_struct *t)
#define recalc_sigpending() recalc_sigpending(current)
recalc_sigpending(get_current());
recalc_sigpending(get_current());
I appreciate that the old recalc_sigpending(task) needed to stop working,
to force people to stop doing recalc_sigpending(current). How about
recalc_sigpending_cur() and recalc_sigpending_tsk() then?
--
dwmw2
From: David Woodhouse <[email protected]>
Date: Fri, 01 Mar 2002 15:33:01 +0000
[email protected] said:
> compat-2.4.h:
> #define recalc_sigpending() recalc_sigpending(current)
> so what are you complaining about?
It may be possible, but it's not a trivial one-liner.
Add a space to the definition, ala "recalc_sigpending (current)"
so that CPP expands the module version properly.
[email protected] said:
> Add a space to the definition, ala "recalc_sigpending (current)" so
> that CPP expands the module version properly.
$ grep recalc_sigpending background.i
#define __ver_recalc_sigpending _ver_str(6682695c)
#define recalc_sigpending _set_ver(recalc_sigpending)
static inline void recalc_sigpending_Rsmp_6682695c(struct task_struct *t)
#define recalc_sigpending() recalc_sigpending (current)
recalc_sigpending (get_current());
recalc_sigpending (get_current());
--
dwmw2
From: David Woodhouse <[email protected]>
Date: Fri, 01 Mar 2002 15:37:41 +0000
[email protected] said:
> Add a space to the definition, ala "recalc_sigpending (current)" so
> that CPP expands the module version properly.
$ grep recalc_sigpending background.i
Sorry, use an inline instead.
static __inline__ void jffs2_recalc_sigpending(void)
{
recalc_sigpending(current);
}
[email protected] said:
> Sorry, use an inline instead.
Thanks - that works, with a slight modification:
#if LINUX_VERSION_CODE < KERNEL_VERSION(2,5,5)
#include <linux/sched.h>
static inline void __recalc_sigpending(void)
{
recalc_sigpending(current);
}
#define recalc_sigpending() __recalc_sigpending ()
#endif
The goal is to have code that just works in 2.5 without compat crap like
jffs2_recalc_sigpending, and to fix it up in the headers only for older
kernels. Most of the changes in 2.5 (and indeed 2.4) have been done in a way
that makes such an approach feasible.
I had been assuming this was intentional good design; perhaps it was just a
fluke.
--
dwmw2
On Fri, 1 Mar 2002, David Woodhouse wrote:
>
> I appreciate that the old recalc_sigpending(task) needed to stop working,
> to force people to stop doing recalc_sigpending(current). How about
> recalc_sigpending_cur() and recalc_sigpending_tsk() then?
The thing is, I refuse to have the "wrong" interface just because of
backwards compatibility.
And it's just _wrong_ to have to write "_cur" to point out that it's
current, when current is the only thing that makes sense from a conceptual
standpoint (except, as mentioned, in the special "internal signal sending
implementation" case).
But the solution might be a higher-level interface altogether: I don't
think it's a good idea in the first place to have drivers and filesystems
playing directly with the blocked signals or whatever it is that you are
doing that makes you want to use recalc_sigpending() in the first place.
So the basic rule should be: drivers etc should not ever have to touch
"sigmask_lock", because they simply should never even _know_ about things
like that. Agreed?
So I would suggest solving this problem by just adding something like
/* Block all signals except for mask */
void sigallow(unsigned long mask)
{
spin_lock_irq(¤t->sigmask_lock);
siginitsetinv(current->blocked, mask);
recalc_sigpending();
spin_unlock_irq(¤t->sigmask_lock);
}
and be done with it. That seems to be what most of the non-signal.c users
actually _want_.
So let's fix this by improving the interfaces, not by making them less
readable.
Linus
[email protected] said:
> So the basic rule should be: drivers etc should not ever have to
> touch "sigmask_lock", because they simply should never even _know_
> about things like that. Agreed?
Absolutely.
> So I would suggest solving this problem by just adding something like
> /* Block all signals except for mask */
> void sigallow(unsigned long mask)
> {
> spin_lock_irq(¤t->sigmask_lock);
> siginitsetinv(current->blocked, mask);
> recalc_sigpending();
> spin_unlock_irq(¤t->sigmask_lock);
> }
Now that I like. Especially if coupled with the equivalent for dequeuing a
signal with implicit locking too, so I really can drop all references to
current->sigmask_lock -- the JFFS2 garbage collection thread dequeues
signals and deals as you might expect with SIGSTOP/SIGKILL:
for (;;) {
spin_lock_irq(¤t->sigmask_lock);
siginitsetinv (¤t->blocked, sigmask(SIGHUP) | sigmask(SIGKILL) | sigmask(SIGSTOP) | sigmask(SIGCONT));
recalc_sigpending();
spin_unlock_irq(¤t->sigmask_lock);
if (!thread_should_wake(c)) {
set_current_state (TASK_INTERRUPTIBLE);
D1(printk(KERN_DEBUG "jffs2_garbage_collect_thread sleeping...\n"));
/* Yes, there's a race here; we checked thread_should_wake() before
setting current->state to TASK_INTERRUPTIBLE. But it doesn't
matter - We don't care if we miss a wakeup, because the GC thread
is only an optimisation anyway. */
schedule();
}
cond_resched();
/* Put_super will send a SIGKILL and then wait on the sem.
*/
while (signal_pending(current)) {
siginfo_t info;
unsigned long signr;
spin_lock_irq(¤t->sigmask_lock);
signr = dequeue_signal(¤t->blocked, &info);
spin_unlock_irq(¤t->sigmask_lock);
switch(signr) {
case SIGSTOP:
D1(printk(KERN_DEBUG "jffs2_garbage_collect_thread(): SIGSTOP received.\n"));
set_current_state(TASK_STOPPED);
schedule();
break;
case SIGKILL:
D1(printk(KERN_DEBUG "jffs2_garbage_collect_thread(): SIGKILL received.\n"));
spin_lock_bh(&c->erase_completion_lock);
c->gc_task = NULL;
spin_unlock_bh(&c->erase_completion_lock);
complete_and_exit(&c->gc_thread_exit, 0);
case SIGHUP:
D1(printk(KERN_DEBUG "jffs2_garbage_collect_thread(): SIGHUP received.\n"));
break;
default:
D1(printk(KERN_DEBUG "jffs2_garbage_collect_thread(): signal %ld received\n", signr));
}
}
/* We don't want SIGHUP to interrupt us. STOP and KILL are OK though. */
spin_lock_irq(¤t->sigmask_lock);
siginitsetinv (¤t->blocked, sigmask(SIGKILL) | sigmask(SIGSTOP) | sigmask(SIGCONT));
recalc_sigpending();
spin_unlock_irq(¤t->sigmask_lock);
D1(printk(KERN_DEBUG "jffs2_garbage_collect_thread(): pass\n"));
jffs2_garbage_collect_pass(c);
}
--
dwmw2
> So I would suggest solving this problem by just adding something like
>
> /* Block all signals except for mask */
> void sigallow(unsigned long mask)
> {
> spin_lock_irq(¤t->sigmask_lock);
> siginitsetinv(current->blocked, mask);
> recalc_sigpending();
> spin_unlock_irq(¤t->sigmask_lock);
> }
>
> and be done with it. That seems to be what most of the non-signal.c users
> actually _want_.
Maybe you should return the old mask too?
David
[email protected] said:
> Maybe you should return the old mask too?
rpc_clnt_sigmask() looks like it could do with that, yes. I note that uses
spin_lock_irqsave() not spin_lock_irq() too - do they really call it with
interrupts already disabled? Should we do the same?
--
dwmw2
I think the following patch would rather serve an obvious
purpose, and wonder why it didn't occur to anybody before.
Please apply it :-).
--- MAINTAINERS.orig Fri Mar 1 19:41:17 2002
+++ MAINTAINERS Fri Mar 1 19:46:53 2002
@@ -54,6 +54,9 @@
so much easier [Ed]
P: Person
+U: The name of the person in UTF-7, if different from the above
+I: contact languages applicable to the person in LOCALE-alike encoding
+ (for example: pl_PL.ISO8859-2 ru_RU.KOI8-R)
M: Mail patches to
L: Mailing list that is relevant to this area
W: Web-page with status/info