perf_counters issue with self-sampling threads

Hi,

I believe there is a problem with the current perf_counters (PCL) code for self-sampling threads. The problem is related to sample notifications via signal.

PCL (just like perfmon) is using SIGIO, an asynchronous signal, to notify user applications of the availability of data in the event buffer.

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.

[Reposting the message because of stupid MIME-encoding error on my part] Andi,

Looks like SIGPROF is calling _group_send_sig_info(), so I think it is subject to the same problem.

Quoted 5 lines · stephane eranian

[Reposting the message because of stupid MIME-encoding error on my part] Andi,

Looks like SIGPROF is calling _group_send_sig_info(), so I think it is subject to the same problem.

So sounds like a whole class of signals can't be POSIX compliant.

Likely others will run into the same problem.

Perhaps should define a new sigaction flag for this to make it all explicit.

Andi,

Looks like SIGPROF is calling _group_send_sig_info(), so I think it is subject to the same problem.

I believe there is a problem with the current perf_counters (PCL) code for self-sampling threads. The problem is related to sample notifications via signal.

PCL (just like perfmon) is using SIGIO, an asynchronous signal, to notify user applications of the availability of data in the event buffer.

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.

Quoted 4 lines · stephane eranian

I believe there is a problem with the current perf_counters (PCL) code for self-sampling threads. The problem is related to sample notifications via signal.

Quoted 8 lines

PCL (just like perfmon) is using SIGIO, an asynchronous signal, to notify user applications of the availability of data in the event buffer.

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.

This signal stuff makes my head spin a little, however:

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.  Consequently, it may be necessary to pass  F_SETOWN  the result of gettid(2) instead of get‐ pid(2) to get sensible results when F_SETSIG  is  used.  (In  current  Linux  threading implementations, a main thread’s thread ID is the same as its process ID.  This means  that  a  single-threaded program can equally use gettid(2) or getpid(2) in this scenario.)   Note,  how‐ ever,  that the  statements  in  this paragraph do not apply to the SIGURG signal generated  for  out-of-band data  on a socket: this signal is always sent to either a process or a process group, depending  on  the  value given  to  F_SETOWN.   Note  also  that Linux imposes a limit on the number of real-time signals  that  may  be queued  to  a  process (see getrlimit(2) and signal(7)) and if this limit is reached, then the kernel  reverts to  delivering  SIGIO,  and this signal is delivered to the entire process rather than to a specific thread.

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.

However,

 kill_fasync(SIGIO)
   __kill_fasync()
     send_sigio()
       /* if pid_type is a PIDTYPE_PID and pid a TID this should only iterate the one thread, I think */ do_each_pid_task() {
         send_sigio_to_task();
       } while_each_pid_task();

where:

 send_sigio_to_task() group_send_sig_info() __group_send_sig_info() send_signal(.group = 1) /* uh-ow trouble */ __send_signal()
           if (group)
              pending = &t->signal->shared_pending

which will result in the signal being send to the whole process anyway.

Now I was considering teaching send_sigio_to_task() to use specific_send_sig_info() when fown->pid != fown->group_leader->pid or something, but I'm not sure that won't break anything.

Alternatively, I've missed a detail and I either read the manpage wrong, or the code, or both of them.

Quoted 6 lines

Now I was considering teaching send_sigio_to_task() to use specific_send_sig_info() when fown->pid != fown->group_leader->pid or something, but I'm not sure that won't break anything.

Yes, that's the problem with touching this. I don't know if this will break things. That's why I was suggested creating a parallel code path which does what we want without modifying the existing path. Unless you know some signal expert at redhat or elsewhere.

Quoted 3 lines

Alternatively, I've missed a detail and I either read the manpage wrong, or the code, or both of them.

The code does not correspond to the manpage. Not clear which one is correct though. This F_SETOWN trick looks very Linux specific.

Quoted 5 lines · stephane eranian

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.

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.

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.

(add Roland)

Quoted 7 lines · stephane eranian

Quoted 5 lines

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.

Quoted 8 lines

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

Quoted 3 lines

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.

Quoted 1 line · Oleg Nesterov

(add Roland)

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

Quoted 9 lines · Peter Zijlstra

Quoted 7 lines · stephane eranian

Quoted 5 lines

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.

Quoted 2 lines

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

Quoted 10 lines

Quoted 8 lines

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 <[email protected]>
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.

Quoted 4 lines

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

fs/fcntl.c +0 -0

@@ -431,6 +431,16 @@ static void send_sigio_to_task(struct ta
 			       int fd,
 			       int reason)

Yes, this allows to send a private signal to sub-thread.

But this is a bit strange, because the user can't specify it wants a thread-specific signal to the main thread, its tid == pid.

I'll make a patch for F_SETOWN_TID then, unless someone comes up with a better name for the creature ;-)

Quoted 3 lines · Peter Zijlstra

I'll make a patch for F_SETOWN_TID then, unless someone comes up with a better name for the creature ;-)

I think you are right. It is not safe to change the current behaviour.

In order to direct the SIGIO signal to a particular thread of a multi-threaded application we cannot, like suggested by the manpage, put a TID into the regular fcntl(F_SETOWN) call. It will still be send to the whole process of which that thread is part.

Since people do want to properly direct SIGIO we introduce F_SETOWN_TID, which functions similarly to F_SETOWN, except positive arguments are interpreted as TIDs and negative arguments are interpreted as PIDs.

This extension is fully bug compatible with the old F_GETOWN implementation in that F_GETOWN_TID will be troubled by the negative return value for PIDs similarly to F_GETOWN's trouble with process groups.

[ compile tested only so far ]

Signed-off-by Peter Zijlstra <[email protected]>

---
 arch/parisc/include/asm/fcntl.h |    2 +
 fs/fcntl.c                      |   64 +++++++++++++++++++++++++++++++++----- include/asm-generic/fcntl.h     |    4 ++
 include/linux/fs.h              |   11 +++++-
 net/socket.c                    |    2 +-

 5 files changed, 71 insertions(+), 12 deletions(-)

arch/parisc/include/asm/fcntl.h +2 -0

index 1e1c824..5d5235a 100644
@@ -28,6 +28,8 @@
  #define F_SETOWN       12      /*  for sockets. */
  #define F_SETSIG       13      /*  for sockets. */
  #define F_GETSIG       14      /*  for sockets. */
+#define F_GETOWN_TID   15
+#define F_SETOWN_TID   16
 
  /* for posix fcntl() and lockf() */
  #define F_RDLCK                01

fs/fcntl.c +42 -6

index ae41308..8d0b7f9 100644
@@ -197,13 +197,15 @@ static int setfl(int fd, struct file * filp, unsigned long arg)
  }
 
  static void f_modown(struct file *filp, struct pid *pid, enum pid_type type,
-                     int force)
+                     int flags)
  {
        write_lock_irq(&filp->f_owner.lock);
-       if (force || !filp->f_owner.pid) {
+       if ((flags & FF_SETOWN_FORCE) || !filp->f_owner.pid) {
                put_pid(filp->f_owner.pid);
                filp->f_owner.pid = get_pid(pid);
                filp->f_owner.pid_type = type;
+               filp->f_owner.task_only =
+                       (type == PIDTYPE_PID && (flags & FF_SETOWN_TID));
 
                if (pid) {
                        const struct cred *cred = current_cred();

@@ -215,7 +217,7 @@ static void f_modown(struct file *filp, struct pid *pid, enum pid_type type,
  }
 
  int __f_setown(struct file *filp, struct pid *pid, enum pid_type type,
-               int force)
+               int flags)
  {
        int err;
 

@@ -223,12 +225,12 @@ int __f_setown(struct file *filp, struct pid *pid, enum pid_type type,
        if (err)
                return err;
 
-       f_modown(filp, pid, type, force);
+       f_modown(filp, pid, type, flags);
        return 0;
  }
  EXPORT_SYMBOL(__f_setown);
 
-int f_setown(struct file *filp, unsigned long arg, int force)
+int f_setown(struct file *filp, unsigned long arg, int flags)
  {
        enum pid_type type;
        struct pid *pid;

@@ -241,7 +243,7 @@ int f_setown(struct file *filp, unsigned long arg, int force)
        }
        rcu_read_lock();
        pid = find_vpid(who);
-       result = __f_setown(filp, pid, type, force);
+       result = __f_setown(filp, pid, type, flags);
        rcu_read_unlock();
        return result;
  }

@@ -263,6 +265,40 @@ pid_t f_getown(struct file *filp)
        return pid;
  }
 
+static int f_setown_tid(struct file *filp, unsigned long arg)
+{
+       int flags = FF_SETOWN_FORCE;
+       struct pid *pid;
+       int who = arg;
+       int ret = 0;
+
+       if (who < 0)
+               who = -who;
+       else
+               flags |= FF_SETOWN_TID;
+
+       rcu_read_lock();
+       pid = find_vpid(who);
+       ret = __f_setown(filp, pid, PIDTYPE_PID, flags);
+       rcu_read_unlock();
+
+       return ret;
+}
+
+static pid_t f_getown_tid(struct file *filp)
+{
+       pid_t tid;
+
+       read_lock(&filp->f_owner.lock);
+       tid = pid_vnr(filp->f_owner.pid);
+       if (filp->f_owner.pid_type == PIDTYPE_PGID)
+               tid = 0;
+       if (!filp->f_owner.task_only)
+               tid = -tid;
+       read_unlock(&filp->f_owner.lock);
+       return tid;
+}
+
  static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
                struct file *filp)
  {

@@ -311,7 +347,14 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
                force_successful_syscall_return();
                break;

       case F_SETOWN:
-               err = f_setown(filp, arg, 1);
+               err = f_setown(filp, arg, FF_SETOWN_FORCE);
+               break;
+       case F_GETOWN_TID:
+               err = f_getown_tid(filp);
+               force_successful_syscall_return();
+               break;
+       case F_SETOWN_TID:
+               err = f_setown_tid(filp, arg); break;
       case F_GETSIG:
               err = filp->f_owner.signum;

fs/fcntl.c +3 -0

@@ -431,6 +474,7 @@ static void send_sigio_to_task(struct task_struct *p,
                               int fd,
                               int reason)
  {
+       int (*send_sig)(int, struct siginfo *, struct task_struct *);
        /*
         * F_SETSIG can change ->signum lockless in parallel, make
         * sure we read it once and use the same value throughout.

@@ -440,6 +484,8 @@ static void send_sigio_to_task(struct task_struct *p,
        if (!sigio_perm(p, fown, signum))
                return;
 
+       send_sig = fown->task_only ? send_sig_info : group_send_sig_info;
+
        switch (signum) {
                siginfo_t si;

               default:

fs/fcntl.c +0 -0

@@ -461,11 +507,11 @@ static void send_sigio_to_task(struct task_struct *p,

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

include/asm-generic/fcntl.h +4 -0

index 4d3e483..d7906b8 100644
@@ -73,6 +73,10 @@
  #define F_SETSIG       10      /* for sockets. */
  #define F_GETSIG       11      /* for sockets. */
  #endif
+#ifndef F_SETOWN_TID
+#define F_SETOWN_TID   12
+#define F_GETOWN_TID   13
+#endif
 
  /* for F_[GET|SET]FL */
  #define FD_CLOEXEC     1       /* actually anything with low bit set goes */

include/linux/fs.h +0 -0

index 0872372..42697e7 100644
@@ -872,6 +872,7 @@ struct fown_struct {

       rwlock_t lock;          /* protects pid, uid, euid fields */ struct pid *pid;        /* pid or -pgrp where SIGIO should be sent */ enum pid_type pid_type; /* Kind of process group SIGIO should be sent to */
+       bool task_only;         /* task or group signal */
       uid_t uid, euid;        /* uid/euid of process setting the owner */ int signum;             /* posix.1b rt signal to be delivered on IO */ };

include/linux/fs.h +8 -2

@@ -1291,8 +1292,14 @@ extern void kill_fasync(struct fasync_struct **, int, int);
  /* only for net: no internal synchronization */
  extern void __kill_fasync(struct fasync_struct *, int, int);
 
-extern int __f_setown(struct file *filp, struct pid *, enum pid_type, int force);
-extern int f_setown(struct file *filp, unsigned long arg, int force);
+/*
+ * setown flags
+ */
+#define FF_SETOWN_FORCE                1
+#define FF_SETOWN_TID          2
+
+extern int __f_setown(struct file *filp, struct pid *, enum pid_type, int flags);
+extern int f_setown(struct file *filp, unsigned long arg, int flags);
  extern void f_delown(struct file *filp);
  extern pid_t f_getown(struct file *filp);
  extern int send_sigurg(struct fown_struct *fown);

net/socket.c +1 -1

index 791d71a..ac57f8e 100644
@@ -916,7 +916,7 @@ static long sock_ioctl(struct file *file, unsigned cmd, unsigned long arg)
                        err = -EFAULT;
                        if (get_user(pid, (int __user *)argp))
                                break;
-                       err = f_setown(sock->file, pid, 1);
+                       err = f_setown(sock->file, pid, FF_SETOWN_FORCE);
                        break;

               case FIOGETOWN:
               case SIOCGPGRP:

In order to direct the SIGIO signal to a particular thread of a multi-threaded application we cannot, like suggested by the manpage, put a TID into the regular fcntl(F_SETOWN) call. It will still be send to the whole process of which that thread is part.

Since people do want to properly direct SIGIO we introduce F_SETOWN_TID, which functions similarly to F_SETOWN, except positive arguments are interpreted as TIDs and negative arguments are interpreted as PIDs.

In order to direct the SIGIO signal to a particular thread of a multi-threaded application we cannot, like suggested by the manpage, put a TID into the regular fcntl(F_SETOWN) call. It will still be send to the whole process of which that thread is part.

Since people do want to properly direct SIGIO we introduce F_SETOWN_TID, which functions similarly to F_SETOWN, except positive arguments are interpreted as TIDs and negative arguments are interpreted as PIDs.

This extension is fully bug compatible with the old F_GETOWN implementation in that F_GETOWN_TID will be troubled by the negative return value for PIDs similarly to F_GETOWN's trouble with process groups.

[ compile tested only so far ]

arch/parisc/include/asm/fcntl.h |    2 +
fs/fcntl.c                      |   64 +++++++++++++++++++++++++++++++++-----
include/asm-generic/fcntl.h     |    4 ++
include/linux/fs.h              |   11 +++++-
net/socket.c                    |    2 +-

fs/fcntl.c +0 -1

@@ -197,13 +197,15 @@ static int setfl(int fd, struct file * f
 }
 
 static void f_modown(struct file *filp, struct pid *pid, enum pid_type type,
-                     int force)

fs/fcntl.c static int setfl(int fd, struct file * f · line 208

Quoted 2 lines

fs/fcntl.c +2 -0

+		filp->f_owner.task_only =
+			(type == PIDTYPE_PID && (flags & FF_SETOWN_TID));

Do we need type == PIDTYPE_PID check? FF_SETOWN_TID must imply PIDTYPE_PID, it is only used by f_setown_tid().

fs/fcntl.c pid_t f_getown(struct file *filp) · line 278

Quoted 11 lines

fs/fcntl.c +11 -0

+static int f_setown_tid(struct file *filp, unsigned long arg)
+{
+	int flags = FF_SETOWN_FORCE;
+	struct pid *pid;
+	int who = arg;
+	int ret = 0;
+
+	if (who < 0)
+		who = -who;
+	else
+		flags |= FF_SETOWN_TID;

Hmm, OK. so fcntl(F_SETOWN_TID, -666) <=> fcntl(F_SETOWN, +666).

Not that I disagree, but I think this should be discussed. Perhaps F_SETOWN_TID can just reject who < 0.

fs/fcntl.c pid_t f_getown(struct file *filp) · line 297

Quoted 10 lines

fs/fcntl.c +10 -0

+static pid_t f_getown_tid(struct file *filp)
+{
+	pid_t tid;
+
+	read_lock(&filp->f_owner.lock);
+	tid = pid_vnr(filp->f_owner.pid);
+	if (filp->f_owner.pid_type == PIDTYPE_PGID)
+		tid = 0;
+	if (!filp->f_owner.task_only)
+		tid = -tid;

I didn't think about this before... What should F_GETOWN_TID return if we did F_GETOWN ? (and vice versa). f_getown_tid() returns < 0 if !task_only and ->piD != 0, this helps.

but the caller of F_GETOWN can't know what the returned value actually means if F_GETOWN_TID was used.

Do we really need fown->task_only? Not only this enlarges fown_struct, we have to modify f_modown() and f_setown().

Perhaps we can just add

	#define F_PIDTYPE_THREAD	PIDTYPE_MAX

into fcntl.c ? Then,

	static int f_setown_xxx(struct file *filp, unsigned long arg, int force, bool group)
	{
		enum pid_type type;
		struct pid *pid;
		int who = arg;
		int result;

		type = PIDTYPE_PID;
		if (!group)
			type = F_PIDTYPE_THREAD
		else if (who < 0) {
			type = PIDTYPE_PGID;
			who = -who;
		}

		rcu_read_lock();
		pid = find_vpid(who);
		result = __f_setown(filp, pid, type, force);
		rcu_read_unlock();
		return result;
	}

	int f_setown(struct file *filp, unsigned long arg, int force)
	{
		return f_setown_xxx(..., true);
	}

Now we should also change send_sigio/send_sigurg, but this is trivial

		type = fown->pid_type;
	+	if (type == F_PIDTYPE_THREAD)
			type = PIDTYPE_PID;

send_sigio_to_task() is trivial too.

What do you think? I agree, this is a bit hackish, but otoh this lessens the changes outside of fcntl.h.

Quoted 4 lines · Oleg Nesterov

Hmm, OK. so fcntl(F_SETOWN_TID, -666) <=> fcntl(F_SETOWN, +666).

Not that I disagree, but I think this should be discussed. Perhaps F_SETOWN_TID can just reject who < 0.

Yeah, I considered that. Opinions?

Quoted 2 lines

but the caller of F_GETOWN can't know what the returned value actually means if F_GETOWN_TID was used.

Ah, I made GETOWN_TID deal with !PID but forgot the TID case in GETOWN. Yeah, icky, esp since there is no room for errors in the return value :/ I guess I could make it return 0.

Quoted 6 lines

Perhaps we can just add

	#define F_PIDTYPE_THREAD	PIDTYPE_MAX

into fcntl.c ? Then,

Right, I considered adding PIDTYPE_TID, but then I'd have to go through the kernel and make everything consistent, which is where I gave up ;-)

dunno if people will go for it though..

As for F_XXXOWN/F_XXXWOWN_TID interaction. Another option, perhaps, is add F_{SET,GET}OWN_EX which accepts a use-visible

struct f_setown_struct {
int pid; // > 0
int type; // enumerates PIDTYPE_PID, PIDTYPE_PGID, F_PIDTYPE_THREAD
}

pointer via arg. Instead of F_XXXOWN_TID + int who.

This way at least the users of new api can't be confused.

Quoted 9 lines · Oleg Nesterov

As for F_XXXOWN/F_XXXWOWN_TID interaction. Another option, perhaps, is add F_{SET,GET}OWN_EX which accepts a use-visible

struct f_setown_struct {
int pid; // > 0
int type; // enumerates PIDTYPE_PID, PIDTYPE_PGID, F_PIDTYPE_THREAD
}

Quoted 3 lines

pointer via arg. Instead of F_XXXOWN_TID + int who.

This way at least the users of new api can't be confused.

This would expose PIDTYPE* to userspace,

To clarify, I am not really sure this interface is better. Just for discussion. Because it will be very painful to change this api later. It is better to at least consider all options now.

fs/fcntl.c +15 -0

+static int f_setown_ex(struct file *filp, unsigned long arg)
+{
+	struct f_owner_ex * __user owner_p = (void * __user)arg;
+	struct f_owner_ex owner;
+	struct pid *pid;
+	int type;
+	int ret;
+
+	ret = copy_from_user(&owner, owner_p, sizeof(owner));
+	if (ret)
+		return ret;
+
+	switch (owner.type) {
+	case F_OWNER_TID:
+		type = PIDTYPE_MAX;

Quoted 16 lines · Peter Zijlstra

(quoted diff) +15 -0

+static int f_setown_ex(struct file *filp, unsigned long arg)
+{
+	struct f_owner_ex * __user owner_p = (void * __user)arg;
+	struct f_owner_ex owner;
+	struct pid *pid;
+	int type;
+	int ret;
+
+	ret = copy_from_user(&owner, owner_p, sizeof(owner));
+	if (ret)
+		return ret;
+
+	switch (owner.type) {
+	case F_OWNER_TID:
+		type = PIDTYPE_MAX;

Review

Quoted 10 lines

(quoted diff) +10 -0

+		break;
+
+	case F_OWNER_PID:
+		type = PIDTYPE_PID;
+		break;
+
+	case F_OWNER_GID:
+		type = PIDTYPE_PGID;
+		break;
+	}

Note that send_sigio()->do_each_pid_task(type) must use the valid type < PIDTYPE_MAX, or we can crash/etc.

This means f_setown_ex() should be careful with the wrong owner->type, the switch() above needs

	default:
		return -EINVAL;

Review

Quoted 6 lines

(quoted diff) +6 -0

+	rcu_read_lock();
+	pid = find_vpid(owner.pid);
+	ret = __f_setown(filp, pid, type, 1);
+	rcu_read_unlock();
+
+	return ret;

Perhaps it makes sense to return -ESRCH if owner.pid && !pid, not sure.

Quoted 6 lines

(quoted diff) +1 -0

@@ -474,16 +540,23 @@ void send_sigio(struct fown_struct *fown
 	struct task_struct *p;
 	enum pid_type type;
 	struct pid *pid;
+	int group = 1;
 

Review void send_sigio(struct fown_struct *fown · line 551

Quoted 7 lines

(quoted diff) +5 -0

 	read_lock(&fown->lock);
+
 	type = fown->pid_type;
+	if (type == PIDTYPE_MAX) {
+		group = 0;
+		type = PIDTYPE_PID;
+	}

And send_sigurg() needs the same change. I am not sure we should teach send_sigurg_to_task() to handle the F_OWNER_TID, but we must ensure send_sigurg()->do_each_pid_task() won't be called with PIDTYPE_MAX.

Otherwise, personally I think this is what we need to solve the problem.

Quoted 9 lines · Oleg Nesterov

Review

Quoted 6 lines

(quoted diff) +6 -0

+	rcu_read_lock();
+	pid = find_vpid(owner.pid);
+	ret = __f_setown(filp, pid, type, 1);
+	rcu_read_unlock();
+
+	return ret;

Perhaps it makes sense to return -ESRCH if owner.pid && !pid, not sure.

We'd need that case to unset/clear the owner, so returning -ESRCH might confuse users I think.

How about the below delta, it changes send_sigurg_to_task() to also use do_send_sig_info() which looses the check_kill_permission() check, but your previous changes lost that same thing from SIGIO -- so I'm hoping that's ok.

Subject: fcntl: F_[SG]ETOWN_EX
From: Peter Zijlstra <[email protected]>
Date: Fri, 31 Jul 2009 10:35:30 +0200

In order to direct the SIGIO signal to a particular thread of a multi-threaded application we cannot, like suggested by the manpage, put a TID into the regular fcntl(F_SETOWN) call. It will still be send to the whole process of which that thread is part.

Since people do want to properly direct SIGIO we introduce F_SETOWN_EX.

The need to direct SIGIO comes from self-monitoring profiling such as with perf-counters. Perf-counters uses SIGIO to notify that new sample data is available. If the signal is delivered to the same task that generated the new sample it can augment that data by inspecting the task's user-space state right after it returns from the kernel. This is esp. convenient for interpreted or virtual machine driven environments.

Both F_SETOWN_EX and F_GETOWN_EX take a pointer to a struct f_owner_ex as argument:

struct f_owner_ex {
	int   type;
	pid_t pid;
};

Where type is one of F_OWNER_TID, F_OWNER_PID or F_OWNER_GID.

Signed-off-by Peter Zijlstra <[email protected]>

---
 arch/alpha/include/asm/fcntl.h  |    2 
 arch/parisc/include/asm/fcntl.h |    2 
 fs/fcntl.c                      |  108 +++++++++++++++++++++++++++++++++++++---
 include/asm-generic/fcntl.h     |   13 ++++
 4 files changed, 117 insertions(+), 8 deletions(-)

Index: linux-2.6/arch/parisc/include/asm/fcntl.h
===================================================================

arch/parisc/include/asm/fcntl.h +2 -0

@@ -28,6 +28,8 @@
 #define F_SETOWN	12	/*  for sockets. */
 #define F_SETSIG	13	/*  for sockets. */
 #define F_GETSIG	14	/*  for sockets. */
+#define F_GETOWN_EX	15
+#define F_SETOWN_EX	16
 
 /* for posix fcntl() and lockf() */
 #define F_RDLCK		01

Index: linux-2.6/fs/fcntl.c
===================================================================

fs/fcntl.c +100 -8

@@ -263,6 +263,79 @@ pid_t f_getown(struct file *filp)
 	return pid;
 }
 
+static int f_setown_ex(struct file *filp, unsigned long arg)
+{
+	struct f_owner_ex * __user owner_p = (void * __user)arg;
+	struct f_owner_ex owner;
+	struct pid *pid;
+	int type;
+	int ret;
+
+	ret = copy_from_user(&owner, owner_p, sizeof(owner));
+	if (ret)
+		return ret;
+
+	switch (owner.type) {
+	case F_OWNER_TID:
+		type = PIDTYPE_MAX;
+		break;
+
+	case F_OWNER_PID:
+		type = PIDTYPE_PID;
+		break;
+
+	case F_OWNER_GID:
+		type = PIDTYPE_PGID;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	rcu_read_lock();
+	pid = find_vpid(owner.pid);
+	if (owner.pid && !pid)
+		ret = -ESRCH;
+	else
+		ret = __f_setown(filp, pid, type, 1);
+	rcu_read_unlock();
+
+	return ret;
+}
+
+static int f_getown_ex(struct file *filp, unsigned long arg)
+{
+	struct f_owner_ex * __user owner_p = (void * __user)arg;
+	struct f_owner_ex owner;
+	int ret = 0;
+
+	read_lock(&filp->f_owner.lock);
+	owner.pid = pid_vnr(filp->f_owner.pid);
+	switch (filp->f_owner.pid_type) {
+	case PIDTYPE_MAX:
+		owner.type = F_OWNER_TID;
+		break;
+
+	case PIDTYPE_PID:
+		owner.type = F_OWNER_PID;
+		break;
+
+	case PIDTYPE_PGID:
+		owner.type = F_OWNER_GID;
+		break;
+
+	default:
+		WARN_ON(1);
+		ret = -EINVAL;
+		break;
+	}
+	read_unlock(&filp->f_owner.lock);
+
+	if (!ret)
+		ret = copy_to_user(owner_p, &owner, sizeof(owner));
+	return ret;
+}
+
 static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
 		struct file *filp)
 {

@@ -313,6 +386,12 @@ static long do_fcntl(int fd, unsigned in
 	case F_SETOWN:
 		err = f_setown(filp, arg, 1);
 		break;
+	case F_GETOWN_EX:
+		err = f_getown_ex(filp, arg);
+		break;
+	case F_SETOWN_EX:
+		err = f_setown_ex(filp, arg);
+		break;
 	case F_GETSIG:
 		err = filp->f_owner.signum;
 		break;

@@ -428,8 +507,7 @@ static inline int sigio_perm(struct task
 
 static void send_sigio_to_task(struct task_struct *p,
 			       struct fown_struct *fown,
-			       int fd,
-			       int reason)
+			       int fd, int reason, int group)
 {
 	/*
 	 * F_SETSIG can change ->signum lockless in parallel, make

@@ -461,11 +539,11 @@ static void send_sigio_to_task(struct ta
 			else
 				si.si_band = band_table[reason - POLL_IN];
 			si.si_fd    = fd;
-			if (!do_send_sig_info(signum, &si, p, true))
+			if (!do_send_sig_info(signum, &si, p, group))
 				break;
 		/* fall-through: fall back on the old plain SIGIO signal */
 		case 0:
-			do_send_sig_info(SIGIO, SEND_SIG_PRIV, p, true);
+			do_send_sig_info(SIGIO, SEND_SIG_PRIV, p, group);
 	}
 }
 

@@ -474,16 +552,23 @@ void send_sigio(struct fown_struct *fown
 	struct task_struct *p;
 	enum pid_type type;
 	struct pid *pid;
+	int group = 1;
 
 	read_lock(&fown->lock);
+
 	type = fown->pid_type;
+	if (type == PIDTYPE_MAX) {
+		group = 0;
+		type = PIDTYPE_PID;
+	}
+
 	pid = fown->pid;
 	if (!pid)
 		goto out_unlock_fown;
 
 	read_lock(&tasklist_lock);
 	do_each_pid_task(pid, type, p) {
-		send_sigio_to_task(p, fown, fd, band);
+		send_sigio_to_task(p, fown, fd, band, group);
 	} while_each_pid_task(pid, type, p);
 	read_unlock(&tasklist_lock);
  out_unlock_fown:

@@ -491,10 +576,10 @@ void send_sigio(struct fown_struct *fown
 }
 
 static void send_sigurg_to_task(struct task_struct *p,
-                                struct fown_struct *fown)
+				struct fown_struct *fown, int group)
 {
 	if (sigio_perm(p, fown, SIGURG))
-		group_send_sig_info(SIGURG, SEND_SIG_PRIV, p);
+		do_send_sig_info(SIGURG, SEND_SIG_PRIV, p, group);
 }
 
 int send_sigurg(struct fown_struct *fown)

@@ -502,10 +587,17 @@ int send_sigurg(struct fown_struct *fown
 	struct task_struct *p;
 	enum pid_type type;
 	struct pid *pid;
+	int group = 1;
 	int ret = 0;
 
 	read_lock(&fown->lock);
+
 	type = fown->pid_type;
+	if (type == PIDTYPE_MAX) {
+		group = 0;
+		type = PIDTYPE_PID;
+	}
+
 	pid = fown->pid;
 	if (!pid)
 		goto out_unlock_fown;

@@ -514,7 +606,7 @@ int send_sigurg(struct fown_struct *fown
 
 	read_lock(&tasklist_lock);
 	do_each_pid_task(pid, type, p) {
-		send_sigurg_to_task(p, fown);
+		send_sigurg_to_task(p, fown, group);
 	} while_each_pid_task(pid, type, p);
 	read_unlock(&tasklist_lock);
  out_unlock_fown:

Index: linux-2.6/include/asm-generic/fcntl.h
===================================================================

include/asm-generic/fcntl.h +13 -0

@@ -73,6 +73,19 @@
 #define F_SETSIG	10	/* for sockets. */
 #define F_GETSIG	11	/* for sockets. */
 #endif
+#ifndef F_SETOWN_EX
+#define F_SETOWN_EX	12
+#define F_GETOWN_EX	13
+#endif
+
+#define F_OWNER_TID	0
+#define F_OWNER_PID	1
+#define F_OWNER_GID	2
+
+struct f_owner_ex {
+	int	type;
+	pid_t	pid;
+};
 
 /* for F_[GET|SET]FL */
 #define FD_CLOEXEC	1	/* actually anything with low bit set goes */

Index: linux-2.6/arch/alpha/include/asm/fcntl.h
===================================================================

arch/alpha/include/asm/fcntl.h +2 -0

@@ -26,6 +26,8 @@
 #define F_GETOWN	6	/*  for sockets. */
 #define F_SETSIG	10	/*  for sockets. */
 #define F_GETSIG	11	/*  for sockets. */
+#define F_SETOWN_EX	12
+#define F_GETOWN_EX	13
 
 /* for posix fcntl() and lockf() */
 #define F_RDLCK		1

Quoted 28 lines · Peter Zijlstra

Subject: fcntl: F_[SG]ETOWN_EX
From: Peter Zijlstra <[email protected]>
Date: Fri, 31 Jul 2009 10:35:30 +0200

In order to direct the SIGIO signal to a particular thread of a multi-threaded application we cannot, like suggested by the manpage, put a TID into the regular fcntl(F_SETOWN) call. It will still be send to the whole process of which that thread is part.

Since people do want to properly direct SIGIO we introduce F_SETOWN_EX.

The need to direct SIGIO comes from self-monitoring profiling such as with perf-counters. Perf-counters uses SIGIO to notify that new sample data is available. If the signal is delivered to the same task that generated the new sample it can augment that data by inspecting the task's user-space state right after it returns from the kernel. This is esp. convenient for interpreted or virtual machine driven environments.

Both F_SETOWN_EX and F_GETOWN_EX take a pointer to a struct f_owner_ex as argument:

struct f_owner_ex {
      int   type;
      pid_t pid;
};

Quoted 1 line

Where type is one of F_OWNER_TID, F_OWNER_PID or F_OWNER_GID.

I think the patch is right.

Reviewed-by Oleg Nesterov <[email protected]>

Quoted 246 lines

Signed-off-by Peter Zijlstra <[email protected]>

---
 arch/alpha/include/asm/fcntl.h  |    2 arch/parisc/include/asm/fcntl.h |    2
 fs/fcntl.c                      |  108 +++++++++++++++++++++++++++++++++++++--- include/asm-generic/fcntl.h     |   13 ++++

 4 files changed, 117 insertions(+), 8 deletions(-)

Index: linux-2.6/arch/parisc/include/asm/fcntl.h
===================================================================

arch/parisc/include/asm/fcntl.h +2 -0

@@ -28,6 +28,8 @@
  #define F_SETOWN     12      /*  for sockets. */
  #define F_SETSIG     13      /*  for sockets. */
  #define F_GETSIG     14      /*  for sockets. */
+#define F_GETOWN_EX  15
+#define F_SETOWN_EX  16
 
  /* for posix fcntl() and lockf() */
  #define F_RDLCK              01

Index: linux-2.6/fs/fcntl.c
===================================================================

fs/fcntl.c +73 -0

@@ -263,6 +263,79 @@ pid_t f_getown(struct file *filp)
       return pid;
  }
 
+static int f_setown_ex(struct file *filp, unsigned long arg)
+{
+     struct f_owner_ex * __user owner_p = (void * __user)arg;
+     struct f_owner_ex owner;
+     struct pid *pid;
+     int type;
+     int ret;
+
+     ret = copy_from_user(&owner, owner_p, sizeof(owner));
+     if (ret)
+             return ret;
+
+     switch (owner.type) {
+     case F_OWNER_TID:
+             type = PIDTYPE_MAX;
+             break;
+
+     case F_OWNER_PID:
+             type = PIDTYPE_PID;
+             break;
+
+     case F_OWNER_GID:
+             type = PIDTYPE_PGID;
+             break;
+
+     default:
+             return -EINVAL;
+     }
+
+     rcu_read_lock();
+     pid = find_vpid(owner.pid);
+     if (owner.pid && !pid)
+             ret = -ESRCH;
+     else
+             ret = __f_setown(filp, pid, type, 1);
+     rcu_read_unlock();
+
+     return ret;
+}
+
+static int f_getown_ex(struct file *filp, unsigned long arg)
+{
+     struct f_owner_ex * __user owner_p = (void * __user)arg;
+     struct f_owner_ex owner;
+     int ret = 0;
+
+     read_lock(&filp->f_owner.lock);
+     owner.pid = pid_vnr(filp->f_owner.pid);
+     switch (filp->f_owner.pid_type) {
+     case PIDTYPE_MAX:
+             owner.type = F_OWNER_TID;
+             break;
+
+     case PIDTYPE_PID:
+             owner.type = F_OWNER_PID;
+             break;
+
+     case PIDTYPE_PGID:
+             owner.type = F_OWNER_GID;
+             break;
+
+     default:
+             WARN_ON(1);
+             ret = -EINVAL;
+             break;
+     }
+     read_unlock(&filp->f_owner.lock);
+
+     if (!ret)
+             ret = copy_to_user(owner_p, &owner, sizeof(owner));
+     return ret;
+}
+
  static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
               struct file *filp)
  {

@@ -313,6 +386,12 @@ static long do_fcntl(int fd, unsigned in

      case F_SETOWN:
              err = f_setown(filp, arg, 1);
              break;
+     case F_GETOWN_EX:
+             err = f_getown_ex(filp, arg);
+             break;
+     case F_SETOWN_EX:
+             err = f_setown_ex(filp, arg);
+             break;
      case F_GETSIG:
              err = filp->f_owner.signum;
              break;

fs/fcntl.c +1 -2

@@ -428,8 +507,7 @@ static inline int sigio_perm(struct task
 
  static void send_sigio_to_task(struct task_struct *p,
                              struct fown_struct *fown,
-                            int fd,
-                            int reason)
+                            int fd, int reason, int group)
  {
       /*
        * F_SETSIG can change ->signum lockless in parallel, make

@@ -461,11 +539,11 @@ static void send_sigio_to_task(struct ta

                      else
                              si.si_band = band_table[reason - POLL_IN]; si.si_fd    = fd;
-                     if (!do_send_sig_info(signum, &si, p, true))
+                     if (!do_send_sig_info(signum, &si, p, group)) break;
              /* fall-through: fall back on the old plain SIGIO signal */ case 0:
-                     do_send_sig_info(SIGIO, SEND_SIG_PRIV, p, true);
+                     do_send_sig_info(SIGIO, SEND_SIG_PRIV, p, group); }
 }

fs/fcntl.c +8 -1

@@ -474,16 +552,23 @@ void send_sigio(struct fown_struct *fown
       struct task_struct *p;
       enum pid_type type;
       struct pid *pid;
+     int group = 1;
 
       read_lock(&fown->lock);
+
       type = fown->pid_type;
+     if (type == PIDTYPE_MAX) {
+             group = 0;
+             type = PIDTYPE_PID;
+     }
+
       pid = fown->pid;
       if (!pid)
               goto out_unlock_fown;
 
       read_lock(&tasklist_lock);
       do_each_pid_task(pid, type, p) {
-             send_sigio_to_task(p, fown, fd, band);
+             send_sigio_to_task(p, fown, fd, band, group);
       } while_each_pid_task(pid, type, p);
       read_unlock(&tasklist_lock);

  out_unlock_fown:

fs/fcntl.c +10 -3

@@ -491,10 +576,10 @@ void send_sigio(struct fown_struct *fown
  }
 
  static void send_sigurg_to_task(struct task_struct *p,
-                                struct fown_struct *fown)
+                             struct fown_struct *fown, int group)
  {
       if (sigio_perm(p, fown, SIGURG))
-             group_send_sig_info(SIGURG, SEND_SIG_PRIV, p);
+             do_send_sig_info(SIGURG, SEND_SIG_PRIV, p, group);
  }
 
  int send_sigurg(struct fown_struct *fown)

@@ -502,10 +587,17 @@ int send_sigurg(struct fown_struct *fown
       struct task_struct *p;
       enum pid_type type;
       struct pid *pid;
+     int group = 1;
       int ret = 0;
 
       read_lock(&fown->lock);
+
       type = fown->pid_type;
+     if (type == PIDTYPE_MAX) {
+             group = 0;
+             type = PIDTYPE_PID;
+     }
+
       pid = fown->pid;
       if (!pid)
               goto out_unlock_fown;

@@ -514,7 +606,7 @@ int send_sigurg(struct fown_struct *fown
 
       read_lock(&tasklist_lock);
       do_each_pid_task(pid, type, p) {
-             send_sigurg_to_task(p, fown);
+             send_sigurg_to_task(p, fown, group);
       } while_each_pid_task(pid, type, p);
       read_unlock(&tasklist_lock);

  out_unlock_fown:
Index: linux-2.6/include/asm-generic/fcntl.h
===================================================================

include/asm-generic/fcntl.h +13 -0

@@ -73,6 +73,19 @@
  #define F_SETSIG     10      /* for sockets. */
  #define F_GETSIG     11      /* for sockets. */
  #endif
+#ifndef F_SETOWN_EX
+#define F_SETOWN_EX  12
+#define F_GETOWN_EX  13
+#endif
+
+#define F_OWNER_TID  0
+#define F_OWNER_PID  1
+#define F_OWNER_GID  2
+
+struct f_owner_ex {
+     int     type;
+     pid_t   pid;
+};
 
  /* for F_[GET|SET]FL */
  #define FD_CLOEXEC   1       /* actually anything with low bit set goes */

Index: linux-2.6/arch/alpha/include/asm/fcntl.h
===================================================================

arch/alpha/include/asm/fcntl.h +2 -0

@@ -26,6 +26,8 @@
  #define F_GETOWN     6       /*  for sockets. */
  #define F_SETSIG     10      /*  for sockets. */
  #define F_GETSIG     11      /*  for sockets. */
+#define F_SETOWN_EX  12
+#define F_GETOWN_EX  13
 
  /* for posix fcntl() and lockf() */
  #define F_RDLCK              1

Quoted 9 lines · On Fri, 31 Jul 2009 10:35:20 +0200 Peter Zijlstra <[email protected]>…

In order to direct the SIGIO signal to a particular thread of a multi-threaded application we cannot, like suggested by the manpage, put a TID into the regular fcntl(F_SETOWN) call. It will still be send to the whole process of which that thread is part.

Since people do want to properly direct SIGIO we introduce F_SETOWN_TID, which functions similarly to F_SETOWN, except positive arguments are interpreted as TIDs and negative arguments are interpreted as PIDs.

Quoted 4 lines

This extension is fully bug compatible with the old F_GETOWN implementation in that F_GETOWN_TID will be troubled by the negative return value for PIDs similarly to F_GETOWN's trouble with process groups.

I'd be interested in seeing a bit more explanation about the "people do want to properly direct SIGIO" thing - use cases, how the current code causes them problems, etc.  As it stands, it's a bit of a mystery-patch.

Quoted 1 line

[ compile tested only so far ]

I will continue to lurk :)

Quoted 5 lines

arch/parisc/include/asm/fcntl.h |    2 +
fs/fcntl.c                      |   64 +++++++++++++++++++++++++++++++++----- include/asm-generic/fcntl.h     |    4 ++
include/linux/fs.h              |   11 +++++-
net/socket.c                    |    2 +-

OK.

Alpha has private definitions of F_SETSIG and F_GETSIG which are identical to the generic ones.  That's somewhat logical, given that alpha's F_SETOWN/F_GETOWN _differ_ from the asm-generic ones.

Alpha appears to have made the decision to spell out _all_ the F_* flags, given that some of them are different.  That has some merit, I guess.

But your patch broke that.

As Peter found out, the man page seems to indicate that if you specify a TID instead a PID with F_SETOWN, then the kernel should interpret this as meaning this particular thread, but this is not what is implemented right now.

Quoted 3 lines · stephane eranian

As Peter found out, the man page seems to indicate that if you specify a TID instead a PID with F_SETOWN, then the kernel should interpret this as meaning this particular thread, but this is not what is implemented right now.

The behaviour was changed quietly in kernel 2.6.12.

I wrote that part of the man page, and it was true at the time.  SIGIO signals _were_ thread-specific.

Starting in kernel 2.5.60, SIGIO signals were thread-specific when F_SETSIG was used to enable queued siginfo.

Prompted by this F_SETOWN_TID patch, I checked through old kernel patches, and found that signals set by F_SIGSIG were changed to process-wide in 2.6.12:

   @@ -437,7 +438,7 @@ static void send_sigio_to_task(struct ta else
                                   si.si_band = band_table[reason - POLL_IN]; si.si_fd    = fd;
   -                       if (!send_sig_info(fown->signum, &si, p))
   +                       if (!send_group_sig_info(fown->signum, &si, p)) break;
                   /* fall-through: fall back on the old plain SIGIO signal */ case 0:

That's a bit annoying, because it breaks a library which uses queued I/O signals as an I/O event mechanism when used with multiple threads. (Fortunatelly epoll is available nowadays; it does I/O events much better, but sometimes you want SIGIO from epoll's descriptor...)

So the man page is now incorrect, but if F_SETOWN_TID is added and has the behaviour which F_SETOWN used to have, then the text can be shuffled around.

If anyone is interested, I have a fairly detailed test program which checks queued "SIGIO" (F_SETSIG) signals due to I/O on a socket, including SIGURG and SIGIO on overflow, and checks whether each one is delivered properly and is thread-specific.  I found quite a few Glibc and kernel bugs with it in the past.

-- Jamie

You must use F_SETSIG on SIGIO if you want your signal handler to receive the file descriptor in siginfo. This is useful if you want to perform some actions on the descriptor. That is the case in perfmon and this is the case in certain situations with perfcounters as well.

Setting SA_SIGINFO provides siginfo, but the si_fd field is NOT set correctly without F_SETSIG. I have verified this with perfcounters, and this is indeed the case.

This behavior seems kind of odd to me.

Quoted 11 lines · stephane eranian

You must use F_SETSIG on SIGIO if you want your signal handler to receive the file descriptor in siginfo. This is useful if you want to perform some actions on the descriptor. That is the case in perfmon and this is the case in certain situations with perfcounters as well.

Setting SA_SIGINFO provides siginfo, but the si_fd field is NOT set correctly without F_SETSIG. I have verified this with perfcounters, and this is indeed the case.

This behavior seems kind of odd to me.

Agreed, this looks a bit odd. But at least this is documented. From man 2 fcntl:

       By using F_SETSIG with a nonzero value, and setting SA_SIGINFO for the signal handler (see sigac- tion(2)), extra information about I/O events is passed to the handler in a  siginfo_t  structure.  If  the si_code field indicates the source is SI_SIGIO, the si_fd field gives the file descriptor associated with the event.  Otherwise, there is no indication which file descriptors are pending,

Not sure if it is safe to change the historical behaviour.

From Jamie, it seems F_SETSIG was not added just for this file descriptor trick.

(the manpage is not exactly right though, and the comment in send_sigio_to_task() is not right too: SI_SIGIO (and, btw, SI_QUEUE/SI_DETHREAD) is never used).

Oleg.

Quoted 23 lines · Oleg Nesterov

Quoted 11 lines · stephane eranian

You must use F_SETSIG on SIGIO if you want your signal handler to receive the file descriptor in siginfo. This is useful if you want to perform some actions on the descriptor. That is the case in perfmon and this is the case in certain situations with perfcounters as well.

Setting SA_SIGINFO provides siginfo, but the si_fd field is NOT set correctly without F_SETSIG. I have verified this with perfcounters, and this is indeed the case.

This behavior seems kind of odd to me.

Agreed, this looks a bit odd. But at least this is documented. From man 2 fcntl:

       By using F_SETSIG with a nonzero value, and setting SA_SIGINFO for the signal handler (see sigac- tion(2)), extra information about I/O events is passed to the handler in a  siginfo_t  structure.  If  the si_code field indicates the source is SI_SIGIO, the si_fd field gives the file descriptor associated with the event.  Otherwise, there is no indication which file descriptors are pending,

Quoted 2 lines

Not sure if it is safe to change the historical behaviour.

Don't need to change it.

But for SIGIO, if you see SA_SIGINFO, then pass the si_fd.

And again, this is even documented. The change is trivial but user-space visible, it may confuse the (stupid) app which uses SIGIO + SA_SIGINFO without F_SETSIG.

Sorry for late reply...

Quoted 27 lines · stephane eranian

Quoted 25 lines · Oleg Nesterov

Quoted 11 lines · stephane eranian

You must use F_SETSIG on SIGIO if you want your signal handler to receive the file descriptor in siginfo. This is useful if you want to perform some actions on the descriptor. That is the case in perfmon and this is the case in certain situations with perfcounters as well.

Setting SA_SIGINFO provides siginfo, but the si_fd field is NOT set correctly without F_SETSIG. I have verified this with perfcounters, and this is indeed the case.

This behavior seems kind of odd to me.

Agreed, this looks a bit odd. But at least this is documented. From man 2 fcntl:

       By using F_SETSIG with a nonzero value, and setting SA_SIGINFO for the signal handler (see sigac- tion(2)), extra information about I/O events is passed to the handler in a  siginfo_t  structure.  If  the si_code field indicates the source is SI_SIGIO, the si_fd field gives the file descriptor associated with the event.  Otherwise, there is no indication which file descriptors are pending,

Not sure if it is safe to change the historical behaviour.

Quoted 1 line

Don't need to change it.

Good,

Quoted 1 line

But for SIGIO, if you see SA_SIGINFO, then pass the si_fd.

But this means we do change the behaviour ;) Confused.

In any case. We should not look at SA_SIGINFO at all. If sys_sigaction() was called without SA_SIGINFO, then it doesn'matter if we send SEND_SIG_PRIV or siginfo_t with the correct si_fd/etc.

And again, this is even documented. The change is trivial but user-space visible, it may confuse the (stupid) app which uses SIGIO + SA_SIGINFO without F_SETSIG.

Quoted 10 lines · Oleg Nesterov

Sorry for late reply...

Quoted 6 lines · stephane eranian

Quoted 3 lines · Oleg Nesterov

Not sure if it is safe to change the historical behaviour.

Don't need to change it.

Quoted 6 lines

Good,

Quoted 1 line

But for SIGIO, if you see SA_SIGINFO, then pass the si_fd.

But this means we do change the behaviour ;) Confused.

I meant do not remove F_SETSIG and its side-effect on si_fd.

Quoted 4 lines

In any case. We should not look at SA_SIGINFO at all. If sys_sigaction() was called without SA_SIGINFO, then it doesn'matter if we send SEND_SIG_PRIV or siginfo_t with the correct si_fd/etc.

What's the official role of SA_SIGINFO? Pass a siginfo struct?

Does POSIX describe the rules governing the content of si_fd? Or is si_fd a Linux-ony extension in which case it goes with F_SETSIG.

Quoted 4 lines

And again, this is even documented. The change is trivial but user-space visible, it may confuse the (stupid) app which uses SIGIO + SA_SIGINFO without F_SETSIG.

That would be an app that uses SIGINFO and fiddles with si_fd which has no defined content. What kind of app would that be?

It would seem natural that in the siginfo passed to the handler of SIGIO, the file descriptor be passed by default. That is all I am trying to say here.

Quoted 4 lines · stephane eranian

Quoted 4 lines

In any case. We should not look at SA_SIGINFO at all. If sys_sigaction() was called without SA_SIGINFO, then it doesn'matter if we send SEND_SIG_PRIV or siginfo_t with the correct si_fd/etc.

Quoted 4 lines

What's the official role of SA_SIGINFO? Pass a siginfo struct?

Does POSIX describe the rules governing the content of si_fd? Or is si_fd a Linux-ony extension in which case it goes with F_SETSIG.

Not sure I understand your concern...

OK. You suggest to pass siginfo_t with .si_fd/etc when we detect SA_SIGINFO.

But, in that case we can _always_ pass siginfo_t, regardless of SA_SIGINFO. If the task has a signal handler and sigaction() was called without SA_SIGINFO, then the handler must not look into *info (the second arg of sigaction->sa_sigaction). And in fact, __setup_rt_frame() doesn't even copy the info to the user-space:

       if (ka->sa.sa_flags & SA_SIGINFO) {
               if (copy_siginfo_to_user(&frame->info, info)) return -EFAULT;
       }

OK? Or I missed something?

Or. Suppose that some app does:

      void io_handler(int sig, siginfo_t *info, void *u)
      {
              if ((info->si_code & __SI_MASK) != SI_POLL) {
                     // RT signal failed! sig MUST be == SIGIO
                      recover();
              } else {
                     do_something(info->si_fd);
              }
      }

      int main(void)
      {
              sigaction(SIGRTMIN, { SA_SIGINFO, io_handler });
              sigaction(SIGIO,    { SA_SIGINFO, io_handler });
              ...
      }

Quoted 2 lines

It would seem natural that in the siginfo passed to the handler of SIGIO, the file descriptor be passed by default. That is all I am trying to say here.

Completely agreed! I was always puzzled by send_sigio_to_task(). I was never able to understand why it looks so strange.

So, I think it should be

       static void send_sigio_to_task(struct task_struct *p, struct fown_struct *fown, int fd,
                                      int reason)
       {
               siginfo_t si;
               /*
                * F_SETSIG can change ->signum lockless in parallel, make
                * sure we read it once and use the same value throughout. */
               int signum = ACCESS_ONCE(fown->signum) ?: SIGIO;

               if (!sigio_perm(p, fown, signum))
                       return;

               si.si_signo = signum;
               si.si_errno = 0;
               si.si_code  = reason;
               si.si_fd    = fd;
               /* Make sure we are called with one of the POLL_* reasons, otherwise we could leak kernel stack into userspace.  */
               BUG_ON((reason & __SI_MASK) != __SI_POLL); if (reason - POLL_IN >= NSIGPOLL) si.si_band  = ~0L;
               else
                       si.si_band = band_table[reason - POLL_IN];

               /* Failure to queue an rt signal must be reported as SIGIO */ if (!group_send_sig_info(signum, &si, p)) group_send_sig_info(SIGIO, SEND_SIG_PRIV, p); }

(except it should be on top of fcntl-add-f_etown_ex.patch). This way, at least we don't break the "detect RT signal failed" above.

What do you think?

But let me repeat: I just can't convince myself we have a good reason to change the strange, but carefully documented behaviour.

Agreed, this looks a bit odd. But at least this is documented. From man 2 fcntl:

By using F_SETSIG with a nonzero value, and setting SA_SIGINFO for the signal handler (see sigaction(2)), extra information about I/O events is passed to the handler in a siginfo_t structure. If the si_code field indicates the source is SI_SIGIO, the si_fd field gives the file descriptor associated with the event. Otherwise, there is no indication which file descriptors are pending,

Not sure if it is safe to change the historical behaviour.

(the manpage is not exactly right though, and the comment in send_sigio_to_task() is not right too: SI_SIGIO (and, btw, SI_QUEUE/SI_DETHREAD) is never used).

Quoted 11 lines · Oleg Nesterov

Agreed, this looks a bit odd. But at least this is documented. From man 2 fcntl:

By using F_SETSIG with a nonzero value, and setting SA_SIGINFO for the signal handler (see sigaction(2)), extra information about I/O events is passed to the handler in a siginfo_t structure. If the si_code field indicates the source is SI_SIGIO, the si_fd field gives the file descriptor associated with the event. Otherwise, there is no indication which file descriptors are pending,

Quoted 1 line

Not sure if it is safe to change the historical behaviour.

The change in 2.6.12 breaks some code of mine, which uses RT queued I/O signals on multiple threads but as far as I know it's not used anywhere now.

In the <= 2.4 era, there were lots of web servers and benchmarks using queued I/O signals for scalable event-driven I/O, but I don't know of any implementation who dared do it with multiple threads, except mine.

It was regarded as "beware ye who enter here" territory, which I can attest to from the long time it took to get it right and the multitude of kernel bugs and version changes needing to be worked around.

Since 2.6, everyone uses epoll which is much better, except that occasionally SIGIO comes in handy when an async notification is required.

So the change in 2.6.12 does break something that probably isn't much used, but it's too late now.

Occasionally thread-specific SIGIO (or
F_SETSIG) is useful; F_SETOWN_TID makes that nice and clear.

I would drop the pseudo-"bug compatible" behaviour of using negative tid to mean pid; that's pointless.

I'd also make F_GETOWN return an
error when F_SETOWN_TID has been used,

and F_GETOWN_TID return an
error when F_SETOWN has been used.

Quoted 7 lines

arch/parisc/include/asm/fcntl.h |    2 +
fs/fcntl.c                      |   64 +++++++++++++++++++++++++++++++++-----
include/asm-generic/fcntl.h     |    4 ++
include/linux/fs.h              |   11 +++++-
net/socket.c                    |    2 +-

OK.

Alpha has private definitions of F_SETSIG and F_GETSIG which are identical to the generic ones. That's somewhat logical, given that alpha's F_SETOWN/F_GETOWN _differ_ from the asm-generic ones.

Alpha appears to have made the decision to spell out _all_ the F_* flags, given that some of them are different. That has some merit, I guess.

But your patch broke that.