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

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

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

[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

[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;
 	}