recalc_sigpending() / recalc_sigpending_tsk() ?

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

Yes.

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.

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

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

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

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.

$ 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

   $ grep recalc_sigpending background.i 

Sorry, use an inline instead.

static __inline__ void jffs2_recalc_sigpending(void)

{
	recalc_sigpending(current);
}

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

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(&current->sigmask_lock);
		siginitsetinv(current->blocked, mask);
		recalc_sigpending();
		spin_unlock_irq(&current->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

Absolutely.

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(&current->sigmask_lock);
		siginitsetinv (&current->blocked, sigmask(SIGHUP) | sigmask(SIGKILL) | sigmask(SIGSTOP) | sigmask(SIGCONT));
		recalc_sigpending();
		spin_unlock_irq(&current->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(&current->sigmask_lock);
                        signr = dequeue_signal(&current->blocked, &info);
                        spin_unlock_irq(&current->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(&current->sigmask_lock);
		siginitsetinv (&current->blocked, sigmask(SIGKILL) | sigmask(SIGSTOP) | sigmask(SIGCONT));
		recalc_sigpending();
		spin_unlock_irq(&current->sigmask_lock);

		D1(printk(KERN_DEBUG "jffs2_garbage_collect_thread(): pass\n"));
		jffs2_garbage_collect_pass(c);
	}

--
dwmw2

Maybe you should return the old mask too?

David

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

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