Message-Id: <1248953485.6391.41.camel@twins>
Date:	Thu, 30 Jul 2009 13:31:25 +0200
From:	Peter Zijlstra <a.p.zijlstra@...llo.nl>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	eranian@...il.com, Ingo Molnar <mingo@...e.hu>,
	LKML <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Robert Richter <robert.richter@....com>,
	Paul Mackerras <paulus@...ba.org>,
	Andi Kleen <andi@...stfloor.org>,
	Maynard Johnson <mpjohn@...ibm.com>,
	Carl Love <cel@...ibm.com>,
	Corey J Ashford <cjashfor@...ibm.com>,
	Philip Mucci <mucci@...s.utk.edu>,
	Dan Terpstra <terpstra@...s.utk.edu>,
	perfmon2-devel <perfmon2-devel@...ts.sourceforge.net>,
	Michael Kerrisk <mtk.manpages@...glemail.com>,
	roland <roland@...hat.com>
Subject: Re: perf_counters issue with self-sampling threads

On Thu, 2009-07-30 at 00:17 +0200, Oleg Nesterov wrote:
> (add Roland)

but you seem to have forgotten to actually edit the CC line, fixed
that ;-)

> On 07/29, Peter Zijlstra wrote:
> >
> > On Mon, 2009-07-27 at 18:51 +0200, stephane eranian wrote:
> > >
> > > POSIX does not mandate that asynchronous signals be delivered
> > > to the thread in which they originated. Any thread in the process
> > > may process the signal, assuming it does not have the signal
> > > blocked.
> 
> Yes. I now nothing about POSIX, but this is what Linux does at least.
> I don't think we can/should change this behaviour.

Well, we have plenty exceptions to that rule already, we have itimer
extentions, tkill sys_rt_tgsigqueueinfo and plenty more..

> > fcntl(2) for F_SETOWN says:
> >
> > If a non-zero value is given to F_SETSIG  in  a  multi‐ threaded
> > process running with a threading library that supports thread groups
> > (e.g., NPTL),  then  a  positive value  given  to  F_SETOWN  has  a
> > different  meaning: instead of being a process ID identifying a whole
> > pro‐ cess,  it  is a thread ID identifying a specific thread within a
> > process.
> 
> Heh. Definitely this is not what Linux does ;)

Right, so the question is, did we ever? Why does the man page say this.

Looking at the .12 source (git start) we did:

  440 			if (!send_sig_info(fown->signum, &si, p))
  441 				break;
  442 		/* fall-through: fall back on the old plain SIGIO signal */
  443 		case 0:
  444 			send_group_sig_info(SIGIO, SEND_SIG_PRIV, p);

Which was 'corrected' in:

commit fc9c9ab22d5650977c417ef2032d02f455011b23
Author: Bharath Ramesh <bramesh@...edu>
Date:   Sat Apr 16 15:25:41 2005 -0700

    [PATCH] AYSNC IO using singals other than SIGIO

    A question on sigwaitinfo based IO mechanism in multithreaded applications.

    I am trying to use RT signals to notify me of IO events using RT signals
    instead of SIGIO in a multithreaded applications.  I noticed that there was
    some discussion on lkml during november 1999 with the subject of the
    discussion as "Signal driven IO".  In the thread I noticed that RT signals
    were being delivered to the worker thread.  I am running 2.6.10 kernel and
    I am trying to use the very same mechanism and I find that only SIGIO being
    propogated to the worker threads and RT signals only being propogated to
    the main thread and not the worker threads where I actually want them to be
    propogated too.  On further inspection I found that the following patch
    which I have attached solves the problem.

    I am not sure if this is a bug or feature in the kernel.


    Roland McGrath <roland@...hat.com> said:

    This relates only to fcntl F_SETSIG, which is a Linux extension.  So there is
    no POSIX issue.  When changing various things like the normal SIGIO signalling
    to do group signals, I was concerned strictly with the POSIX semantics and
    generally avoided touching things in the domain of Linux inventions.  That's
    why I didn't change this when I changed the call right next to it.  There is
    no reason I can see that F_SETSIG-requested signals shouldn't use a group
    signal like normal SIGIO does.  I'm happy to ACK this patch, there is nothing
    wrong with its change to the semantics in my book.  But neither POSIX nor I
    care a whit what F_SETSIG does.

    Signed-off-by: Andrew Morton <akpm@...l.org>
    Signed-off-by: Linus Torvalds <torvalds@...l.org>

> > Which seems to imply that when we feed fcntl(F_SETOWN) a TID instead of
> > a PID it should deliver SIGIO to the thread instead of the whole process
> > -- which, to me, seems a sane semantic.
> 
> I am not sure I understand the man above... But to me it looks like we
> should always send a private signal when fown->signum != 0 ?
> 
> The change should be simple, but as you pointed out we can break things.

Right, so the change I had in mind is like the below (except I don't
know if we can compare struct pid things by pointer value or if we
should look at the content).

In any case, we should either do something like the below (yay!), or
amend the manpage (Michael?) and introduce something like F_SETOWN2
which does have the below semantics :-(.

---
Index: linux-2.6/fs/fcntl.c
===================================================================
--- linux-2.6.orig/fs/fcntl.c
+++ linux-2.6/fs/fcntl.c
@@ -431,6 +431,16 @@ static void send_sigio_to_task(struct ta
 			       int fd,
 			       int reason)
 {
+	int (*send_sig)(int, struct siginfo *, struct task_struct *);
+
+	send_sig = group_send_sig_info;
+	/*
+	 * If the fown points to a specific TID instead of to a PID
+	 * we'll send the signal to the thread only.
+	 */
+	if (fown->pid_type == PIDTYPE_PID && fown->pid != task_tgid(p))
+		send_sig = send_sig_info;
+
 	/*
 	 * F_SETSIG can change ->signum lockless in parallel, make
 	 * sure we read it once and use the same value throughout.
@@ -461,11 +472,11 @@ static void send_sigio_to_task(struct ta
 			else
 				si.si_band = band_table[reason - POLL_IN];
 			si.si_fd    = fd;
-			if (!group_send_sig_info(signum, &si, p))
+			if (!send_sig(signum, &si, p))
 				break;
 		/* fall-through: fall back on the old plain SIGIO signal */
 		case 0:
-			group_send_sig_info(SIGIO, SEND_SIG_PRIV, p);
+			send_sig(SIGIO, SEND_SIG_PRIV, p);
 	}
 }
 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists