Re: [PATCH v2 5/6] signal: sys_sigprocmask() needs retarget_shared_pending()

Re: [PATCH v2 5/6] signal: sys_sigprocmask() needs retarget_shared_pending()

[George Spelvin]

> and notice how you now can do that helper function *WITHOUT* any
> conditionals, and just make it do
> 
>     sigprocmask(&clear, &set, NULL);
> 
> which handles all cases correctly (just "andn clear" + "or set") with
> no if's or switch'es.

Gripe: Arrgh, apostrophe disease!  Linus, youre picking up nasty habits.

Suggestion: Actually, the usual, more flexible implementation, is
"andn mask" + "xor set".  That gives all 4 bit operations (nop, set,
clear, and toggle) unique bit combinations.

Not that it's of any use in many cases, but it has better hack value.

That would convert to:
  switch (how) {
  case SIG_BLOCK:
      mask_bits = new_set;
      set_bits = new_set;
      break;
  case SIG_UNBLOCK:
      mask_bits = new_set;
      set_bits = 0;
      break
  case SIG_SET:
     mask_bits = low_bits | new_set;
     set_bits = new_set;
     break;
   default:
     return -EINVAL;
  }

If you prefer separate set & clear fields, with both set meaning "toggle",
which admittedly is a more elegant representation, then it's
"andn (set|clear)" + "xor set".

Re: [PATCH v2 5/6] signal: sys_sigprocmask() needs retarget_shared_pending()

[Linus Torvalds]

On Tue, Apr 26, 2011 at 6:35 PM, George Spelvin <linux@horizon.com> wrote:
>>
>> which handles all cases correctly (just "andn clear" + "or set") with
>> no if's or switch'es.
>
> Gripe: Arrgh, apostrophe disease!  Linus, youre picking up nasty habits.

I didn't want to make C keywords into English words. So if it's about
English words it's "no ifs or buts", but when talking about C keywords
it's "if's or switch'es".

That's my stand, and I'll stick to it. At least until next time, when
I might decide on yet another arbitrary rule ;)

> Suggestion: Actually, the usual, more flexible implementation, is
> "andn mask" + "xor set".  That gives all 4 bit operations (nop, set,
> clear, and toggle) unique bit combinations.

Yeah, we can do that too. I agree it is more flexible, although I
don't think we'd ever need it.

And I'm not sure it's more "usual" - most drivers I've seen (where
this kind of "clear/set mask" issue comes up pretty often) tend to use
the simpler clear/set model, because it tends to make the usage more
sensible. Most users simply don't tend to need the toggle operation.

But my grep-fu to actually show examples is weak. One example I found would be

 - drivers/media/video/tw9910.c: tw9910_mask_set()

which uses that "andnot + or" model I suggested.

But I'm sure there are "andnot + xor" uses out there too.

And I don't actually mind either way. I'd certainly be perfectly ok
with the andn/xor model. It's just the "how" model I detest.

                            Linus

[RFC PATCH v2 0/7] signal: sigprocmask fixes

[Oleg Nesterov]

Andrew, please drop V1:

	signal-introduce-retarget_shared_pending.patch
	signal-retarget_shared_pending-consider-shared-unblocked-signals-only.patch
	signal-sigprocmask-narrow-the-scope-of-sigloc.patch
	signal-sigprocmask-should-do-retarget_shared_pending.patch
	x86-signal-handle_signal-should-use-sigprocmask.patch
	x86-signal-sys_rt_sigreturn-should-use-sigprocmask.patch

Changes in V2:

	- 2/7 change retarget_shared_pending() to accept mask, not ~mask

	- 3/7 is new, it adds the optimization promised in 2/7

	- 4/7 add the small comment about current->blocked as Matt
	      suggested

	- 5/7 add the new helper, set_current_blocked(), suggested
	      by Linus

	- 8/7 is the new and a bit off-topic cleanup, but sys_rt_sigprocmask()
	      looks so annoying

Matt, I didn't dare to keep your reviewed-by tags because of the
changes above, hopefully you can re-ack.

Once again: if we need this, then we need a lot more (trivial) changes
like 6/7 and 7/7. Basically every change of ->blocked should be converted
to use set_current_blocked(). OTOH, perhaps this makes sense by itself.

Oleg.

Re: [RFC PATCH v2 0/7] signal: sigprocmask fixes

[Linus Torvalds]

On Mon, Apr 18, 2011 at 6:44 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>
> Once again: if we need this, then we need a lot more (trivial) changes
> like 6/7 and 7/7. Basically every change of ->blocked should be converted
> to use set_current_blocked(). OTOH, perhaps this makes sense by itself.

Hmm. The more I think about this, the less I like it.

What if the pending thread signal was thread-specific to begin with?

For example, if we have a SIGFPE and a SIGKILL that happen at the same
time, a dying task may have a SIGFPE pending when it dies, and that
SIGFPE should _not_ be just distributed out to the other threads in
the thread group.

Am I missing something that protects against this?

                  Linus

Re: [RFC PATCH v2 0/7] signal: sigprocmask fixes

[Oleg Nesterov]

On 04/18, Linus Torvalds wrote:
>
> On Mon, Apr 18, 2011 at 6:44 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > Once again: if we need this, then we need a lot more (trivial) changes
> > like 6/7 and 7/7. Basically every change of ->blocked should be converted
> > to use set_current_blocked(). OTOH, perhaps this makes sense by itself.
>
> Hmm. The more I think about this, the less I like it.
>
> What if the pending thread signal was thread-specific to begin with?

These patches should not change the current behaviour in this case.
We never try to re-target the thread-specific signals. Note that
retarget_shared_pending() checks ->signal->shared_pending only.

> For example, if we have a SIGFPE and a SIGKILL that happen at the same
> time, a dying task may have a SIGFPE pending when it dies, and that
> SIGFPE should _not_ be just distributed out to the other threads in
> the thread group.

Yes, and it won't be.

Btw, we do not need to distribute SIGKILL too, we can change
retarget_shared_pending() to remove SIGKILL from shared_pending.
But this only matters when the caller is exit_signals(), and in
this case it should likely notice signal_group_exit() unless
SIGKILL (in unlikely case) it comes in between.

Or I misunderstood?

Oleg.

Re: [RFC PATCH v2 0/7] signal: sigprocmask fixes

[Linus Torvalds]

On Mon, Apr 18, 2011 at 10:32 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>
> These patches should not change the current behaviour in this case.
> We never try to re-target the thread-specific signals. Note that
> retarget_shared_pending() checks ->signal->shared_pending only.

Ok, I clearly didn't follow the details closely enough.

In that case, this series looks fairly ok to me. I'm still unsure
whether it's _required_, but it looks sane and reasonable.

                   Linus

[PATCH 0/3] do_sigtimedwait() needs retarget_shared_pending()

[Oleg Nesterov]

On 04/18, Linus Torvalds wrote:
>
> In that case, this series looks fairly ok to me. I'm still unsure
> whether it's _required_,

Indeed! It is a bit annoying, I am not sure too. This "problem" is
very, very, old. And nobody complained so far. Except, this _might_
explain the hang reported by Nikita.

OTOH. Probably set_current_blocked() makes sense anyway, it would be
nice to require that nobody can play with ->blocked directly. We can
reconsider retarget_shared_pendind() or simply kill it if it adds the
noticeable overhead in practice.

So, I'll assume we need this changes. Correctness is always good. All
further changes should be simple and straightforward, I am going to
trim CC. Except sys_rt_sigtimedwait() is nasty, we need another helper
for simplicity.

If nobody objects, I'll send at lot of other "needs set_current_blocked"
simple patches to Andrew.

Oleg.

[PATCH v2 0/6] sigtimedwait/sigprocmask need retarget_shared_pending()

[Oleg Nesterov]

Changes:

	1/6: remove the evidence of my ignorance from the changelog,
	     += (tv_sec || tv_nsec) is correct

	2/6: more factorization, rename the argument, add the comment
	     from Linus.

	     please re-add the acks.

	3/6: fix the typos in the changelog, thanks Matt

	4-6: new.	

I really hope that this is the last "nontrivial" changes in this area
which need any discussion, everything else looks simple.

Oleg.

[PATCH v2 5/6] signal: sys_sigprocmask() needs retarget_shared_pending()

[Oleg Nesterov]

sys_sigprocmask() changes current->blocked by hand. Convert this code to
use sigprocmask() which implies the necessary retarget_shared_pending().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---

 kernel/signal.c |   24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

--- sigprocmask/kernel/signal.c~5_old_sigprocmask_retarget	2011-04-26 19:54:09.000000000 +0200
+++ sigprocmask/kernel/signal.c	2011-04-26 19:54:20.000000000 +0200
@@ -2702,6 +2702,7 @@ SYSCALL_DEFINE3(sigprocmask, int, how, o
 		old_sigset_t __user *, oset)
 {
 	old_sigset_t old_set, new_set;
+	sigset_t new_full_set;
 	int error;
 
 	old_set = current->blocked.sig[0];
@@ -2711,25 +2712,12 @@ SYSCALL_DEFINE3(sigprocmask, int, how, o
 			return -EFAULT;
 		new_set &= ~(sigmask(SIGKILL) | sigmask(SIGSTOP));
 
-		error = 0;
-		spin_lock_irq(&current->sighand->siglock);
-		switch (how) {
-		default:
-			error = -EINVAL;
-			break;
-		case SIG_BLOCK:
-			sigaddsetmask(&current->blocked, new_set);
-			break;
-		case SIG_UNBLOCK:
-			sigdelsetmask(&current->blocked, new_set);
-			break;
-		case SIG_SETMASK:
-			current->blocked.sig[0] = new_set;
-			break;
-		}
+		sigemptyset(&new_full_set);
+		if (how == SIG_SETMASK)
+			new_full_set = current->blocked;
+		new_full_set.sig[0] = new_set;
 
-		recalc_sigpending();
-		spin_unlock_irq(&current->sighand->siglock);
+		error = sigprocmask(how, &new_full_set, NULL);
 		if (error)
 			return error;
 	}

Re: [PATCH v2 5/6] signal: sys_sigprocmask() needs retarget_shared_pending()

[Linus Torvalds]

> +               sigemptyset(&new_full_set);
> +               if (how == SIG_SETMASK)
> +                       new_full_set = current->blocked;
> +               new_full_set.sig[0] = new_set;

Ugh. This is just ugly.

Could we not instead turn the whole thing into a "clear these bits"
and "set these bits", and get rid of the "how" entirely for the helper
function?

IOW, we'd have

  switch (how) {
  case SIG_BLOCK:
      clear_bits = 0;
      set_bits = new_set;
      break;
  case SIG_UNBLOCK:
      clear_bits = new_set;
      set_bits = 0;
      break
  case SIG_SET:
     clear_bits = low_bits;
     set_bits = new_set;
     break;
   default:
     return -EINVAL;
  }

and notice how you now can do that helper function *WITHOUT* any
conditionals, and just make it do

    sigprocmask(&clear, &set, NULL);

which handles all cases correctly (just "andn clear" + "or set") with
no if's or switch'es.

This is why I _detest_ that idiotic "sigprocmask()" interface. That
"how" parameter is the invention of somebody who didn't understand
sets. It's stupid. There is no reason to have multiple different set
operations, when in practice all anybody ever wants is the "clear
these bits and set those other bits" - an operation that is not only
more generic than the idiotic "how", but is _faster_ too, because it
involves no conditionals.

So I realize that we cannot get away from the broken user interface,
but I do not believe that that means that our _internal_ helper
functions should use that idiotic and broken interface!

I had basically this same comment earlier when you did something
similarly mindless for another case.

So basic rule should be: if you ever pass "how" to any helper
functions, it's broken.

                                     Linus

Re: [PATCH v2 5/6] signal: sys_sigprocmask() needs retarget_shared_pending()

[Oleg Nesterov]

On 04/26, Linus Torvalds wrote:
>
> > +               sigemptyset(&new_full_set);
> > +               if (how == SIG_SETMASK)
> > +                       new_full_set = current->blocked;
> > +               new_full_set.sig[0] = new_set;
>
> Ugh. This is just ugly.

Agreed.

> Could we not instead turn the whole thing into a "clear these bits"
> and "set these bits", and get rid of the "how" entirely for the helper
> function?
>
> IOW, we'd have
>
>   switch (how) {
>   case SIG_BLOCK:
>       clear_bits = 0;
>       set_bits = new_set;
>       break;
>   case SIG_UNBLOCK:
>       clear_bits = new_set;
>       set_bits = 0;
>       break
>   case SIG_SET:
>      clear_bits = low_bits;
>      set_bits = new_set;
>      break;
>    default:
>      return -EINVAL;
>   }
>
> and notice how you now can do that helper function *WITHOUT* any
> conditionals, and just make it do
>
>     sigprocmask(&clear, &set, NULL);
>
> which handles all cases correctly (just "andn clear" + "or set") with
> no if's or switch'es.
>
> This is why I _detest_ that idiotic "sigprocmask()" interface.

Agreed, but...

Yes, sigprocmask(how) is ugly, but there are sys_rt_sigprocmask() and
sys_sigprocmask() which have to handle these SIG_* operations anyway.
So, I think we should do:

	1. Almost all callers of sigprocmask() use SIG_SETMASK, we can
	   simply change them to use set_current_blocked().

	2. Add the new helper (probably like you suggested) and convert
	   other 9 callers.

	3. Unexport sigprocmask() and remove the last "*oldset" argument,
	   it should be only used by sys_*sigprocmask() syscalls.

But firstly I'd like to finish this "don't change ->blocked directly"
conversion. And this patch changes sys_sigprocmask() so that it looks
similar to sys_rt_sigprocmask().

What do you think?

If you can't accept sys_sigprocmask()->sigprocmask(), will you agree
with

	SYSCALL_DEFINE3(sigprocmask, int, how, old_sigset_t __user *, nset,
			old_sigset_t __user *, oset)
	{
		old_sigset_t old_set, new_set;
		sigset_t new_blocked;

		old_set = current->blocked.sig[0];

		if (nset) {
			if (copy_from_user(&new_set, nset, sizeof(*nset)))
				return -EFAULT;
			new_set &= ~(sigmask(SIGKILL) | sigmask(SIGSTOP));

			new_blocked = current->blocked;

			switch (how) {
			case SIG_BLOCK:
				sigaddsetmask(&new_blocked, new_set);
				break;
			case SIG_UNBLOCK:
				sigdelsetmask(&new_blocked, new_set);
				break;
			case SIG_SETMASK:
				new_blocked.sig[0] = new_set;
				break;
			default:
				return -EINVAL;
			}

			set_current_blocked(&new_blocked);
		}

		if (oset) {
			if (copy_to_user(oset, &old_set, sizeof(*oset)))
				return -EFAULT;
		}

		return 0;
	}
?

Oleg.

Re: [PATCH v2 5/6] signal: sys_sigprocmask() needs retarget_shared_pending()

[Tejun Heo]

Hello,

Just my 5 cents.

On Wed, Apr 27, 2011 at 02:57:10PM +0200, Oleg Nesterov wrote:
> Yes, sigprocmask(how) is ugly, but there are sys_rt_sigprocmask() and
> sys_sigprocmask() which have to handle these SIG_* operations anyway.
> So, I think we should do:
> 
> 	1. Almost all callers of sigprocmask() use SIG_SETMASK, we can
> 	   simply change them to use set_current_blocked().

I agree.  We don't need to worry about atomicity here, so there's no
reason to encode bitops (be it and/or or andn/xor) when the
determination of the new value can be simply done in the caller.

Thanks.

-- 
tejun