posix timers: Extend kernel API to report more info about timers

PATCH In review Pavel Emelyanov 4 participants 10 messages Feb 14, 2013 – Feb 21, 2013

Subsystems (3)

x86 Scheduler ABI/API

lore

Share & tools

lore

Pavel Emelyanov

author

2013-02-14 16:18 UTC #0 lore raw

cover posix timers: Extend kernel API to report more info about timers

Hi.

I'm working on the checkpoint-restore project (http://criu.org), briefly it's aim is to collect information about process' state and saving it so that later it is possible to recreate the processes in the very same state as they were, using the collected information.

One part of the task's state is the posix timers that this task has created. Currently kernel doesn't provide any API for getting information about what timers are currently created by process and in which state they are. I'd like to extend the posix timers API to provide more information about timers.

Another problem with timers is the timer ID. Currently IDs are generated from global IDR and this makes it impossible to restore a timer from the saved state in general, as the required ID may be already busy at the time of restore.

That said, I propose to

1. Change the way timer IDs are generated. This was done some time ago, so I'm just re-sending this patch;
2. Add a system call that will list timer IDs created by the calling process;
3. Add a system call that will allow to get the sigevent information about particular timer in the sigaction-like manner.

This is actually an RFC to start discussion about how the described problems can be addressed. Thus, if the approach with new system calls is not acceptable, I'm OK to implement this in any other form.

Thanks,
Pavel

Pavel Emelyanov

author

2013-02-14 16:19 UTC #1 lore raw

PATCH 1/3 posix timers: Allocate timer id per process

From: Stanislav Kinsbursky <[email protected]>

Patch replaces global idr with global hash table for posix timers and makes timer ids unique not globally, but per process. Next free timer id is type of integer and stored on signal struct (posix_timer_id). If free timer id reaches negative value on timer creation, it will be dropped to zero and -EAGAIN will be returned to user.

Hash table has 512 slots.
Key is constructed as follows:
key = hash_32(hash_32(current->signal) ^ posix_timer_id));

Note: with this patch, id, returned to user, is not the minimal free amymore. It means, that id, returned to user space in loop, listed below, will be increasing on each iteration till INT_MAX and then dropped to zero:

while(1) {
	id = timer_create(...);
	timer_delete(id);
}

Signed-off-by Stanislav Kinsbursky <[email protected]>

Signed-off-by Pavel Emelyanov <[email protected]>

---
 include/linux/posix-timers.h |    1 +
 include/linux/sched.h        |    3 +-
 kernel/posix-timers.c        |  114 +++++++++++++++++++++++++++--------------
 3 files changed, 78 insertions(+), 40 deletions(-)

include/linux/posix-timers.h +1 -0

index 042058f..60bac69 100644
@@ -55,6 +55,7 @@ struct cpu_timer_list {
 /* POSIX.1b interval timer structure. */
 struct k_itimer {
 	struct list_head list;		/* free/ allocate list */
+	struct hlist_node t_hash;
 	spinlock_t it_lock;
 	clockid_t it_clock;		/* which timer type */
 	timer_t it_id;			/* timer id */

include/linux/sched.h +2 -1

index d211247..d0d9a8b 100644
@@ -561,7 +561,8 @@ struct signal_struct {
 	unsigned int		has_child_subreaper:1;
 
 	/* POSIX.1b Interval Timers */
-	struct list_head posix_timers;
+	int			posix_timer_id;
+	struct list_head	posix_timers;
 
 	/* ITIMER_REAL timer for the process */
 	struct hrtimer real_timer;

kernel/posix-timers.c +75 ~~-39~~

168 lines not on this page · raw message

Signature · 1 line

1.7.6.5

Sasha Levin

2013-02-14 20:13 UTC #2 lore raw

Re: [PATCH 1/3] posix timers: Allocate timer id per process

Quoted 16 lines · Pavel Emelyanov

From: Stanislav Kinsbursky <[email protected]>
Patch replaces global idr with global hash table for posix timers and makes timer ids unique not globally, but per process. Next free timer id is type of integer and stored on signal struct (posix_timer_id). If free timer id reaches negative value on timer creation, it will be dropped to zero and -EAGAIN will be returned to user.
Hash table has 512 slots.
Key is constructed as follows:
key = hash_32(hash_32(current->signal) ^ posix_timer_id));
Note: with this patch, id, returned to user, is not the minimal free amymore. It means, that id, returned to user space in loop, listed below, will be increasing on each iteration till INT_MAX and then dropped to zero:

Quoted 7 lines

while(1) {
        id = timer_create(...);
        timer_delete(id);
}
Signed-off-by Stanislav Kinsbursky <[email protected]>
Signed-off-by Pavel Emelyanov <[email protected]>

Hi Pavel,

Why not use linux/hashtable.h?

Thanks,
Sasha

Pavel Emelyanov

author

2013-02-15 05:13 UTC #3 lore raw

Re: [PATCH 1/3] posix timers: Allocate timer id per process

Quoted 22 lines · Sasha Levin

Quoted 21 lines · Pavel Emelyanov
From: Stanislav Kinsbursky <[email protected]>
Patch replaces global idr with global hash table for posix timers and makes timer ids unique not globally, but per process. Next free timer id is type of integer and stored on signal struct (posix_timer_id). If free timer id reaches negative value on timer creation, it will be dropped to zero and -EAGAIN will be returned to user.
Hash table has 512 slots.
Key is constructed as follows:
key = hash_32(hash_32(current->signal) ^ posix_timer_id));
Note: with this patch, id, returned to user, is not the minimal free amymore. It means, that id, returned to user space in loop, listed below, will be increasing on each iteration till INT_MAX and then dropped to zero:
while(1) {
        id = timer_create(...);
        timer_delete(id);
}

Quoted 6 lines

Quoted 2 lines
Signed-off-by Stanislav Kinsbursky <[email protected]>
Signed-off-by Pavel Emelyanov <[email protected]>
Hi Pavel,
Why not use linux/hashtable.h?

Simply because this patch was just picked from the previous discussions as is :) I will tune it to use hashtable.h in the next iteration.

Quoted 5 lines

Thanks,
Sasha
.

Pavel Emelyanov

author

2013-02-14 16:19 UTC #4 lore raw

PATCH 2/3 posix timers: Add syscall that lists timer IDs armed by process

In reply to Pavel Emelyanov

The sys_timer_list syscall accepts clock id, buffer size to store the timers ids and the pointer to the buffer itself.

The number of timers of clockid type is returned and these timers' ids are put into the provided buffer. If the buffer is not enough for all timers, then only part of ids are put into it, but the actual number of timers is reported anyway. If the buffer is bigger than required, then its tail is left untouched and the number of timers returned from the syscall denotes how much of ids are there.

Signed-off-by Pavel Emelyanov <[email protected]>

---
 arch/x86/syscalls/syscall_64.tbl |    1 +
 include/linux/syscalls.h         |    2 +
 kernel/posix-timers.c            |   48 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 51 insertions(+), 0 deletions(-)

arch/x86/syscalls/syscall_64.tbl +1 -0

index dc97328..72b6ee6 100644
@@ -320,6 +320,7 @@
 311	64	process_vm_writev	sys_process_vm_writev
 312	common	kcmp			sys_kcmp
 313	common	finit_module		sys_finit_module
+314	common	timer_list		sys_timer_list
 
 #
 # x32-specific system call numbers start at 512 to avoid cache impact

include/linux/syscalls.h +2 -0

index 45e2db2..dc951da 100644
@@ -319,6 +319,8 @@ asmlinkage long sys_timer_settime(timer_t timer_id, int flags,
 				const struct itimerspec __user *new_setting,
 				struct itimerspec __user *old_setting);
 asmlinkage long sys_timer_delete(timer_t timer_id);
+asmlinkage long sys_timer_list(const clockid_t clock_id, int nr,
+				timer_t __user *timer_ids);
 asmlinkage long sys_clock_settime(clockid_t which_clock,
 				const struct timespec __user *tp);
 asmlinkage long sys_clock_gettime(clockid_t which_clock,

kernel/posix-timers.c +48 -0

51 lines not on this page · raw message

Signature · 1 line

1.7.6.5

Pavel Emelyanov

author

2013-02-14 16:19 UTC #5 lore raw

PATCH 3/3 posix timers: Add syscall that works on timer sigevent

In reply to Pavel Emelyanov

The intention is to make a syscall, that works like sigaction but on a posix timer. I.e. -- puts (if provided) new sigevent on the timer and reports the previous value (if requested).

That said, the syscall accepts timer id to work on, a pointer to the new sigevent (may be NULL, meaning that the existing sigevent remains intact) and pointer to memory buffer where to put the existing sigevent (may be NULL as well, sigevent is not reported in this case).

For how I didn't implement the new sigevent assignment, as it requires a little bit of trickery with timer. If the proposed idea itself is OK useful I will implement this part as well in the v2 series.

Signed-off-by Pavel Emelyanov <[email protected]>

---
 arch/x86/syscalls/syscall_64.tbl |    1 +
 include/linux/syscalls.h         |    3 +++
 kernel/posix-timers.c            |   28 ++++++++++++++++++++++++++++
 3 files changed, 32 insertions(+), 0 deletions(-)

arch/x86/syscalls/syscall_64.tbl +1 -0

index 72b6ee6..c2c1743 100644
@@ -321,6 +321,7 @@
 312	common	kcmp			sys_kcmp
 313	common	finit_module		sys_finit_module
 314	common	timer_list		sys_timer_list
+315	64	timer_sigevent		sys_timer_sigevent
 
 #
 # x32-specific system call numbers start at 512 to avoid cache impact

include/linux/syscalls.h +3 -0

index dc951da..7d121c5 100644
@@ -312,6 +312,9 @@ asmlinkage long sys_setitimer(int which,
 asmlinkage long sys_timer_create(clockid_t which_clock,
 				 struct sigevent __user *timer_event_spec,
 				 timer_t __user * created_timer_id);
+asmlinkage long sys_timer_sigevent(timer_t timer_id,
+				struct sigevent __user *new_event,
+				struct sigevent __user *old_event);
 asmlinkage long sys_timer_gettime(timer_t timer_id,
 				struct itimerspec __user *setting);
 asmlinkage long sys_timer_getoverrun(timer_t timer_id);

kernel/posix-timers.c +28 -0

index 46cee59..29de379 100644
@@ -1151,3 +1151,31 @@ try_again:
 out:
 	return nr;
 }
+
+SYSCALL_DEFINE3(timer_sigevent, timer_t, timer_id, struct sigevent __user *, new_event,
+		struct sigevent __user *, old_event)
+{
+	struct k_itimer *timer;
+	unsigned long flags;
+	struct sigevent event;
+	int ret = 0;
+
+	if (new_event)
+		return -EINVAL;
+
+	timer = lock_timer(timer_id, &flags);
+	if (!timer)
+		return -EINVAL;
+
+	event.sigev_notify = timer->it_sigev_notify;
+	event.sigev_signo = timer->sigq->info.si_signo;
+	event.sigev_value = timer->sigq->info.si_value;
+	event.sigev_notify_thread_id = pid_vnr(timer->it_pid);
+
+	unlock_timer(timer, flags);
+
+	if (old_event && copy_to_user(old_event, &event, sizeof(event)))
+		ret = -EFAULT;
+
+	return ret;
+}

Signature · 1 line

1.7.6.5

Jann Horn

2013-02-17 13:42 UTC #6 lore raw

Re: [PATCH 3/3] posix timers: Add syscall that works on timer sigevent

Quoted 1 line · Pavel Emelyanov

kernel/posix-timers.c +1 -0
+	struct sigevent event;

[...]

Quoted 4 lines

kernel/posix-timers.c +4 -0

+	event.sigev_notify = timer->it_sigev_notify;
+	event.sigev_signo = timer->sigq->info.si_signo;
+	event.sigev_value = timer->sigq->info.si_value;
+	event.sigev_notify_thread_id = pid_vnr(timer->it_pid);

[...]

kernel/posix-timers.c try_again: · line 1177

Quoted 1 line

kernel/posix-timers.c +1 -0

+	if (old_event && copy_to_user(old_event, &event, sizeof(event)))

Won't this leak uninitialized kernel stack data to userspace? As far as I can see, the _sigev_un union is bigger than the _tid field in it, so the rest of it will be copied over without initialization, right?

Jann

Pavel Emelyanov

author

2013-02-17 18:53 UTC #7 lore raw

Re: [PATCH 3/3] posix timers: Add syscall that works on timer sigevent

Quoted 8 lines · Jann Horn

Quoted 1 line · Pavel Emelyanov

kernel/posix-timers.c +1 -0
+	struct sigevent event;

[...]

Quoted 4 lines

kernel/posix-timers.c +4 -0

+	event.sigev_notify = timer->it_sigev_notify;
+	event.sigev_signo = timer->sigq->info.si_signo;
+	event.sigev_value = timer->sigq->info.si_value;
+	event.sigev_notify_thread_id = pid_vnr(timer->it_pid);

[...]

Quoted 5 lines

kernel/posix-timers.c try_again: · line 1177
Quoted 1 line
kernel/posix-timers.c +1 -0
+	if (old_event && copy_to_user(old_event, &event, sizeof(event)))
Won't this leak uninitialized kernel stack data to userspace? As far as I can see, the _sigev_un union is bigger than the _tid field in it, so the rest of it will be copied over without initialization, right?

Indeed :( Thanks for catching this, I will memezro the thing before copying to the user-space.

Quoted 2 lines

Jann

Matthew Helsley

2013-02-21 01:21 UTC #8 lore raw

In reply to Pavel Emelyanov

Quoted 18 lines · Pavel Emelyanov

Hi.
I'm working on the checkpoint-restore project (http://criu.org), briefly it's aim is to collect information about process' state and saving it so that later it is possible to recreate the processes in the very same state as they were, using the collected information.
One part of the task's state is the posix timers that this task has created. Currently kernel doesn't provide any API for getting information about what timers are currently created by process and in which state they are. I'd like to extend the posix timers API to provide more information about timers.
Another problem with timers is the timer ID. Currently IDs are generated from global IDR and this makes it impossible to restore a timer from the saved state in general, as the required ID may be already busy at the time of restore.

Quoted 4 lines

That said, I propose to
1. Change the way timer IDs are generated. This was done some time ago, so I'm just re-sending this patch;

Seems fine in principle. Aside: I noticed there were some important-looking patches to the idr usage in timer id allocation today...

Quoted 1 line

2. Add a system call that will list timer IDs created by the calling process;

If timers were listed in /proc like fds then you wouldn't need this syscall. If we keep adding new syscalls like this CRIU will be needlessly x86-specific when it could have been written more portably.

Quoted 2 lines

3. Add a system call that will allow to get the sigevent information about particular timer in the sigaction-like manner.

You mentioned "extending the POSIX timer API". Isn't that something best left to standards bodies lest your changes conflict with theirs? Again, if this were a /proc interface you wouldn't have that issue (you'll have others ;)).

Quoted 4 lines

This is actually an RFC to start discussion about how the described problems can be addressed. Thus, if the approach with new system calls is not acceptable, I'm OK to implement this in any other form.

My preference is for "other form" for the reasons above.

Cheers,
-Matt Helsley

Pavel Emelyanov

author

2013-02-21 10:35 UTC #9 lore raw

Quoted 19 lines · Matthew Helsley

Quoted 18 lines · Pavel Emelyanov
Hi.
I'm working on the checkpoint-restore project (http://criu.org), briefly it's aim is to collect information about process' state and saving it so that later it is possible to recreate the processes in the very same state as they were, using the collected information.
One part of the task's state is the posix timers that this task has created. Currently kernel doesn't provide any API for getting information about what timers are currently created by process and in which state they are. I'd like to extend the posix timers API to provide more information about timers.
Another problem with timers is the timer ID. Currently IDs are generated from global IDR and this makes it impossible to restore a timer from the saved state in general, as the required ID may be already busy at the time of restore.

Quoted 8 lines

Quoted 4 lines
That said, I propose to
1. Change the way timer IDs are generated. This was done some time ago, so I'm just re-sending this patch;
Seems fine in principle. Aside: I noticed there were some important-looking patches to the idr usage in timer id allocation today...

Hm, OK, will try to find one.

Quoted 15 lines

Quoted 1 line
2. Add a system call that will list timer IDs created by the calling process;
If timers were listed in /proc like fds then you wouldn't need this syscall. If we keep adding new syscalls like this CRIU will be needlessly x86-specific when it could have been written more portably.
Quoted 2 lines
3. Add a system call that will allow to get the sigevent information about particular timer in the sigaction-like manner.
You mentioned "extending the POSIX timer API". Isn't that something best left to standards bodies lest your changes conflict with theirs? Again, if this were a /proc interface you wouldn't have that issue (you'll have others ;)).
Quoted 1 line

Quoted 5 lines

Quoted 3 lines
This is actually an RFC to start discussion about how the described problems can be addressed. Thus, if the approach with new system calls is not acceptable, I'm OK to implement this in any other form.
My preference is for "other form" for the reasons above.

No problem, proc is OK for me as well. I will look at what can be done here.

Thanks for the feedback!

Quoted 4 lines

Cheers,
-Matt Helsley
.