recalc_sigpending() / recalc_sigpending_tsk() ?

recalc_sigpending() / recalc_sigpending_tsk() ?

[David Woodhouse]

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

Re: recalc_sigpending() / recalc_sigpending_tsk() ?

[Linus Torvalds]

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

Re: recalc_sigpending() / recalc_sigpending_tsk() ?

[David Woodhouse]

torvalds@transmeta.com 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

Re: recalc_sigpending() / recalc_sigpending_tsk() ?

[David Woodhouse]

torvalds@transmeta.com 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

Re: recalc_sigpending() / recalc_sigpending_tsk() ?

[David S. Miller]

   From: David Woodhouse <dwmw2@infradead.org>
   Date: Fri, 01 Mar 2002 15:33:01 +0000

   
   torvalds@transmeta.com 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.

Re: recalc_sigpending() / recalc_sigpending_tsk() ?

[David Woodhouse]

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

Re: recalc_sigpending() / recalc_sigpending_tsk() ?

[David S. Miller]

   From: David Woodhouse <dwmw2@infradead.org>
   Date: Fri, 01 Mar 2002 15:37:41 +0000

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

Re: recalc_sigpending() / recalc_sigpending_tsk() ?

[David Woodhouse]

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

Re: recalc_sigpending() / recalc_sigpending_tsk() ?

[Linus Torvalds]

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

Re: recalc_sigpending() / recalc_sigpending_tsk() ?

[David Woodhouse]

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

Re: recalc_sigpending() / recalc_sigpending_tsk() ?

[David Howells]

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

Maybe you should return the old mask too?

David

Re: recalc_sigpending() / recalc_sigpending_tsk() ?

[David Woodhouse]

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

[PATCH] small proposal.

[Martin Dalecki]

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