[patch 0/3] timerfd -- implement missing parts to checkpoint and restore timerfd state, v2

[patch 0/3] timerfd -- implement missing parts to checkpoint and restore timerfd state, v2

[Cyrill Gorcunov]

Hi, here is an updated series, changes mostly in show_fdinfo code which now
produces a way more information (and documentation update). Please take a look
once time permit. Thanks!

[patch 1/3] timerfd: Implement show_fdinfo method

[Cyrill Gorcunov]

[-- Attachment #1: timerfd-show-fdinfo-3 --]
[-- Type: text/plain, Size: 2974 bytes --]

For checkpoint/restore of timerfd files we need to know how exactly
the timer were armed to be able to handle it. Thus implement show_fdinfo
method which provides enough information for timer re-creation.

One of significant changes I think is addition of
timerfd_ctx::settime_flags member. Currently there are
two flags TFD_TIMER_ABSTIME and TFD_TIMER_CANCEL_ON_SET,
and the second can be found from @might_cancel variable
but in case if the flags will be extended in future we
most probably will have to somehow remember them explicitly
anyway so I guss doing that right now won't hurt.

To not bloat the timerfd_ctx structure I've converted
@expired to short integer and defined @settime_flags
as short as well.

v2 (by avagin@, vdavydov@ and tglx@):

 - Add it_value/it_interval fields
 - Save flags being used in timerfd_setup in context

CC: Shawn Landden <shawn@churchofgit.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Andrey Vagin <avagin@openvz.org>
CC: Pavel Emelyanov <xemul@parallels.com>
CC: Vladimir Davydov <vdavydov@parallels.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---
 fs/timerfd.c |   30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

Index: linux-2.6.git/fs/timerfd.c
===================================================================
--- linux-2.6.git.orig/fs/timerfd.c
+++ linux-2.6.git/fs/timerfd.c
@@ -35,8 +35,9 @@ struct timerfd_ctx {
 	ktime_t moffs;
 	wait_queue_head_t wqh;
 	u64 ticks;
-	int expired;
 	int clockid;
+	short unsigned expired;
+	short unsigned settime_flags;	/* to show in fdinfo */
 	struct rcu_head rcu;
 	struct list_head clist;
 	bool might_cancel;
@@ -196,6 +197,8 @@ static int timerfd_setup(struct timerfd_
 		if (timerfd_canceled(ctx))
 			return -ECANCELED;
 	}
+
+	ctx->settime_flags = flags & TFD_SETTIME_FLAGS;
 	return 0;
 }
 
@@ -284,11 +287,36 @@ static ssize_t timerfd_read(struct file
 	return res;
 }
 
+static int timerfd_show(struct seq_file *m, struct file *file)
+{
+	struct timerfd_ctx *ctx = file->private_data;
+	struct itimerspec t;
+
+	spin_lock_irq(&ctx->wqh.lock);
+	t.it_value = ktime_to_timespec(timerfd_get_remaining(ctx));
+	t.it_interval = ktime_to_timespec(ctx->tintv);
+	spin_unlock_irq(&ctx->wqh.lock);
+
+	return seq_printf(m,
+			  "clockid: %d\n"
+			  "ticks: %llu\n"
+			  "settime flags: 0%o\n"
+			  "it_value: (%llu, %llu)\n"
+			  "it_interval: (%llu, %llu)\n",
+			  ctx->clockid, (unsigned long long)ctx->ticks,
+			  ctx->settime_flags,
+			  (unsigned long long)t.it_value.tv_sec,
+			  (unsigned long long)t.it_value.tv_nsec,
+			  (unsigned long long)t.it_interval.tv_sec,
+			  (unsigned long long)t.it_interval.tv_nsec);
+}
+
 static const struct file_operations timerfd_fops = {
 	.release	= timerfd_release,
 	.poll		= timerfd_poll,
 	.read		= timerfd_read,
 	.llseek		= noop_llseek,
+	.show_fdinfo	= timerfd_show,
 };
 
 static int timerfd_fget(int fd, struct fd *p)

Re: [patch 1/3] timerfd: Implement show_fdinfo method

[Vladimir Davydov]

Hi Cyrill,

On 04/07/2014 09:47 PM, Cyrill Gorcunov wrote:
> For checkpoint/restore of timerfd files we need to know how exactly
> the timer were armed to be able to handle it. Thus implement show_fdinfo
> method which provides enough information for timer re-creation.
>
> One of significant changes I think is addition of
> timerfd_ctx::settime_flags member. Currently there are
> two flags TFD_TIMER_ABSTIME and TFD_TIMER_CANCEL_ON_SET,
> and the second can be found from @might_cancel variable
> but in case if the flags will be extended in future we
> most probably will have to somehow remember them explicitly
> anyway so I guss doing that right now won't hurt.
>
> To not bloat the timerfd_ctx structure I've converted
> @expired to short integer and defined @settime_flags
> as short as well.
>
> v2 (by avagin@, vdavydov@ and tglx@):
>
>  - Add it_value/it_interval fields
>  - Save flags being used in timerfd_setup in context
>
> CC: Shawn Landden <shawn@churchofgit.com>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: Andrey Vagin <avagin@openvz.org>
> CC: Pavel Emelyanov <xemul@parallels.com>
> CC: Vladimir Davydov <vdavydov@parallels.com>
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> ---
>  fs/timerfd.c |   30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
>
> Index: linux-2.6.git/fs/timerfd.c
> ===================================================================
> --- linux-2.6.git.orig/fs/timerfd.c
> +++ linux-2.6.git/fs/timerfd.c
> @@ -35,8 +35,9 @@ struct timerfd_ctx {
>  	ktime_t moffs;
>  	wait_queue_head_t wqh;
>  	u64 ticks;
> -	int expired;
>  	int clockid;
> +	short unsigned expired;
> +	short unsigned settime_flags;	/* to show in fdinfo */
>  	struct rcu_head rcu;
>  	struct list_head clist;
>  	bool might_cancel;
> @@ -196,6 +197,8 @@ static int timerfd_setup(struct timerfd_
>  		if (timerfd_canceled(ctx))
>  			return -ECANCELED;
>  	}
> +
> +	ctx->settime_flags = flags & TFD_SETTIME_FLAGS;
>  	return 0;
>  }
>  
> @@ -284,11 +287,36 @@ static ssize_t timerfd_read(struct file
>  	return res;
>  }
>  
> +static int timerfd_show(struct seq_file *m, struct file *file)
> +{
> +	struct timerfd_ctx *ctx = file->private_data;
> +	struct itimerspec t;
> +
> +	spin_lock_irq(&ctx->wqh.lock);
> +	t.it_value = ktime_to_timespec(timerfd_get_remaining(ctx));
> +	t.it_interval = ktime_to_timespec(ctx->tintv);
> +	spin_unlock_irq(&ctx->wqh.lock);
> +
> +	return seq_printf(m,
> +			  "clockid: %d\n"
> +			  "ticks: %llu\n"
> +			  "settime flags: 0%o\n"
> +			  "it_value: (%llu, %llu)\n"
> +			  "it_interval: (%llu, %llu)\n",

IMO, one would expect to setup the timer on restore by passing the
values of settime_flags, it_value, and it_interval obtained from the
fdinfo to sys_timerfd_settime, but this will be incorrect, because AFAIU
the it_value you report here is always relative to the current time, no
matter whether TFD_TIMER_ABSTIME was set in settime_flags or not. Is it OK?

Thanks.

> +			  ctx->clockid, (unsigned long long)ctx->ticks,
> +			  ctx->settime_flags,
> +			  (unsigned long long)t.it_value.tv_sec,
> +			  (unsigned long long)t.it_value.tv_nsec,
> +			  (unsigned long long)t.it_interval.tv_sec,
> +			  (unsigned long long)t.it_interval.tv_nsec);
> +}
> +
>  static const struct file_operations timerfd_fops = {
>  	.release	= timerfd_release,
>  	.poll		= timerfd_poll,
>  	.read		= timerfd_read,
>  	.llseek		= noop_llseek,
> +	.show_fdinfo	= timerfd_show,
>  };
>  
>  static int timerfd_fget(int fd, struct fd *p)

Re: [patch 1/3] timerfd: Implement show_fdinfo method

[Cyrill Gorcunov]

On Tue, Apr 08, 2014 at 10:17:14AM +0400, Vladimir Davydov wrote:
...
> > +static int timerfd_show(struct seq_file *m, struct file *file)
> > +{
> > +	struct timerfd_ctx *ctx = file->private_data;
> > +	struct itimerspec t;
> > +
> > +	spin_lock_irq(&ctx->wqh.lock);
> > +	t.it_value = ktime_to_timespec(timerfd_get_remaining(ctx));
> > +	t.it_interval = ktime_to_timespec(ctx->tintv);
> > +	spin_unlock_irq(&ctx->wqh.lock);
> > +
> > +	return seq_printf(m,
> > +			  "clockid: %d\n"
> > +			  "ticks: %llu\n"
> > +			  "settime flags: 0%o\n"
> > +			  "it_value: (%llu, %llu)\n"
> > +			  "it_interval: (%llu, %llu)\n",
> 
> IMO, one would expect to setup the timer on restore by passing the
> values of settime_flags, it_value, and it_interval obtained from the
> fdinfo to sys_timerfd_settime, but this will be incorrect, because AFAIU
> the it_value you report here is always relative to the current time, no
> matter whether TFD_TIMER_ABSTIME was set in settime_flags or not. Is it OK?

Hi Vladimir! Well it_value returns remaining time so it's up to user to
adjust this value when restore with abs time. That said one can examinate
if abs flag was set and restore accordingly. If this is vague I'm open
to change it to more clear way. Ideas?

Re: [patch 1/3] timerfd: Implement show_fdinfo method

[Vladimir Davydov]

On 04/08/2014 10:42 AM, Cyrill Gorcunov wrote:
> On Tue, Apr 08, 2014 at 10:17:14AM +0400, Vladimir Davydov wrote:
> ...
>>> +static int timerfd_show(struct seq_file *m, struct file *file)
>>> +{
>>> +	struct timerfd_ctx *ctx = file->private_data;
>>> +	struct itimerspec t;
>>> +
>>> +	spin_lock_irq(&ctx->wqh.lock);
>>> +	t.it_value = ktime_to_timespec(timerfd_get_remaining(ctx));
>>> +	t.it_interval = ktime_to_timespec(ctx->tintv);
>>> +	spin_unlock_irq(&ctx->wqh.lock);
>>> +
>>> +	return seq_printf(m,
>>> +			  "clockid: %d\n"
>>> +			  "ticks: %llu\n"
>>> +			  "settime flags: 0%o\n"
>>> +			  "it_value: (%llu, %llu)\n"
>>> +			  "it_interval: (%llu, %llu)\n",
>> IMO, one would expect to setup the timer on restore by passing the
>> values of settime_flags, it_value, and it_interval obtained from the
>> fdinfo to sys_timerfd_settime, but this will be incorrect, because AFAIU
>> the it_value you report here is always relative to the current time, no
>> matter whether TFD_TIMER_ABSTIME was set in settime_flags or not. Is it OK?
> Hi Vladimir! Well it_value returns remaining time so it's up to user to
> adjust this value when restore with abs time. That said one can examinate
> if abs flag was set and restore accordingly. If this is vague I'm open
> to change it to more clear way. Ideas?

If it's intended, it should be documented explicitly I think. Currently
in the doc patch I see nothing about whether expiration time is absolute
or relative:

> 'it_value' and 'it_interval' are the expiration time and interval for the timer.

Thanks.

Re: [patch 1/3] timerfd: Implement show_fdinfo method

[Cyrill Gorcunov]

On Tue, Apr 08, 2014 at 10:54:45AM +0400, Vladimir Davydov wrote:
> > Hi Vladimir! Well it_value returns remaining time so it's up to user to
> > adjust this value when restore with abs time. That said one can examinate
> > if abs flag was set and restore accordingly. If this is vague I'm open
> > to change it to more clear way. Ideas?
> 
> If it's intended, it should be documented explicitly I think. Currently
> in the doc patch I see nothing about whether expiration time is absolute
> or relative:
> 
> > 'it_value' and 'it_interval' are the expiration time and interval for the timer.

I'll update, thanks!

[patch 2/3] docs: procfs -- Document timerfd output

[Cyrill Gorcunov]

[-- Attachment #1: timerfd-doc-fdinfo --]
[-- Type: text/plain, Size: 1437 bytes --]

CC: Shawn Landden <shawn@churchofgit.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Andrey Vagin <avagin@openvz.org>
CC: Pavel Emelyanov <xemul@parallels.com>
CC: Vladimir Davydov <vdavydov@parallels.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---
 Documentation/filesystems/proc.txt |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Index: linux-2.6.git/Documentation/filesystems/proc.txt
===================================================================
--- linux-2.6.git.orig/Documentation/filesystems/proc.txt
+++ linux-2.6.git/Documentation/filesystems/proc.txt
@@ -1741,6 +1741,23 @@ pair provide additional information part
 	While the first three lines are mandatory and always printed, the rest is
 	optional and may be omitted if no marks created yet.
 
+	Timerfd files
+	~~~~~~~~~~~~~
+
+	pos:	0
+	flags:	02
+	mnt_id:	9
+	clockid: 0
+	ticks: 0
+	settime flags: 01
+	it_value: (0, 49406829)
+	it_interval: (1, 0)
+
+	where 'clockid' is the clock type and 'ticks' is the number of the timer expirations
+	that have occurred [see timerfd_create(2) for details]. 'settime flags' are
+	flags in octal form used to setup the timer [see timerfd_settime(2) for
+	details]. 'it_value' and 'it_interval' are the expiration time and interval
+	for the timer.
 
 ------------------------------------------------------------------------------
 Configuring procfs

[patch 2/3] docs: procfs -- Document timerfd output, v2

[Cyrill Gorcunov]

CC: Shawn Landden <shawn@churchofgit.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Andrey Vagin <avagin@openvz.org>
CC: Pavel Emelyanov <xemul@parallels.com>
CC: Vladimir Davydov <vdavydov@parallels.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---

Vladimir, would the below text be more clear?

 Documentation/filesystems/proc.txt |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Index: linux-2.6.git/Documentation/filesystems/proc.txt
===================================================================
--- linux-2.6.git.orig/Documentation/filesystems/proc.txt
+++ linux-2.6.git/Documentation/filesystems/proc.txt
@@ -1741,6 +1741,23 @@ pair provide additional information part
 	While the first three lines are mandatory and always printed, the rest is
 	optional and may be omitted if no marks created yet.
 
+	Timerfd files
+	~~~~~~~~~~~~~
+
+	pos:	0
+	flags:	02
+	mnt_id:	9
+	clockid: 0
+	ticks: 0
+	settime flags: 01
+	it_value: (0, 49406829)
+	it_interval: (1, 0)
+
+	where 'clockid' is the clock type and 'ticks' is the number of the timer expirations
+	that have occurred [see timerfd_create(2) for details]. 'settime flags' are
+	flags in octal form used to setup the timer [see timerfd_settime(2) for
+	details]. 'it_value' is remaining time until the timer exiration.
+	'it_interval' is the interval for the timer.
 
 ------------------------------------------------------------------------------
 Configuring procfs

Re: [patch 2/3] docs: procfs -- Document timerfd output, v2

[Vladimir Davydov]

On 04/08/2014 11:43 AM, Cyrill Gorcunov wrote:
> CC: Shawn Landden <shawn@churchofgit.com>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: Andrey Vagin <avagin@openvz.org>
> CC: Pavel Emelyanov <xemul@parallels.com>
> CC: Vladimir Davydov <vdavydov@parallels.com>
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> ---
>
> Vladimir, would the below text be more clear?

It looks good to me.

Thanks.

>
>  Documentation/filesystems/proc.txt |   17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> Index: linux-2.6.git/Documentation/filesystems/proc.txt
> ===================================================================
> --- linux-2.6.git.orig/Documentation/filesystems/proc.txt
> +++ linux-2.6.git/Documentation/filesystems/proc.txt
> @@ -1741,6 +1741,23 @@ pair provide additional information part
>  	While the first three lines are mandatory and always printed, the rest is
>  	optional and may be omitted if no marks created yet.
>  
> +	Timerfd files
> +	~~~~~~~~~~~~~
> +
> +	pos:	0
> +	flags:	02
> +	mnt_id:	9
> +	clockid: 0
> +	ticks: 0
> +	settime flags: 01
> +	it_value: (0, 49406829)
> +	it_interval: (1, 0)
> +
> +	where 'clockid' is the clock type and 'ticks' is the number of the timer expirations
> +	that have occurred [see timerfd_create(2) for details]. 'settime flags' are
> +	flags in octal form used to setup the timer [see timerfd_settime(2) for
> +	details]. 'it_value' is remaining time until the timer exiration.
> +	'it_interval' is the interval for the timer.
>  
>  ------------------------------------------------------------------------------
>  Configuring procfs

[patch 3/3] timerfd: Implement write method

[Cyrill Gorcunov]

[-- Attachment #1: timerfd-write-ticks --]
[-- Type: text/plain, Size: 1659 bytes --]

The read() of timerfd files allows to fetch the number of
timer ticks while there is no way to set it back from userspace.

To restore the timer state as it was at checkpoint moment we need
a way to setup ticks back. So as a counterpart of read() the write()
takes ticks number from the userspace and updates internal timer
ticks accordingly.

CC: Shawn Landden <shawn@churchofgit.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Andrey Vagin <avagin@openvz.org>
CC: Pavel Emelyanov <xemul@parallels.com>
CC: Vladimir Davydov <vdavydov@parallels.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---
 fs/timerfd.c |   21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

Index: linux-2.6.git/fs/timerfd.c
===================================================================
--- linux-2.6.git.orig/fs/timerfd.c
+++ linux-2.6.git/fs/timerfd.c
@@ -311,10 +311,31 @@ static int timerfd_show(struct seq_file
 			  (unsigned long long)t.it_interval.tv_nsec);
 }
 
+
+static ssize_t timerfd_write(struct file *file, const char __user *buf,
+			     size_t count, loff_t *ppos)
+{
+	struct timerfd_ctx *ctx = file->private_data;
+	u64 ticks = 0;
+
+	if (count < sizeof(ticks))
+		return -EINVAL;
+
+	if (get_user(ticks, (u64 __user *) buf))
+		return -EFAULT;
+
+	spin_lock_irq(&ctx->wqh.lock);
+	ctx->ticks = ticks;
+	spin_unlock_irq(&ctx->wqh.lock);
+
+	return sizeof(ticks);
+}
+
 static const struct file_operations timerfd_fops = {
 	.release	= timerfd_release,
 	.poll		= timerfd_poll,
 	.read		= timerfd_read,
+	.write		= timerfd_write,
 	.llseek		= noop_llseek,
 	.show_fdinfo	= timerfd_show,
 };

Re: [patch 0/3] timerfd -- implement missing parts to checkpoint and restore timerfd state, v2

[Cyrill Gorcunov]

On Mon, Apr 07, 2014 at 09:47:01PM +0400, Cyrill Gorcunov wrote:
> Hi, here is an updated series, changes mostly in show_fdinfo code which now
> produces a way more information (and documentation update). Please take a look
> once time permit. Thanks!

Hi, any new comments on this series?

Re: [patch 0/3] timerfd -- implement missing parts to checkpoint and restore timerfd state, v2

[Thomas Gleixner]

On Tue, 29 Apr 2014, Cyrill Gorcunov wrote:

> On Mon, Apr 07, 2014 at 09:47:01PM +0400, Cyrill Gorcunov wrote:
> > Hi, here is an updated series, changes mostly in show_fdinfo code which now
> > produces a way more information (and documentation update). Please take a look
> > once time permit. Thanks!
> 
> Hi, any new comments on this series?

Looks ok and I did not do anything about it, as I saw a "will update
in next version" from you in the discussion.

Thanks,

	tglx

Re: [patch 0/3] timerfd -- implement missing parts to checkpoint and restore timerfd state, v2

[Cyrill Gorcunov]

On Mon, Apr 28, 2014 at 10:59:34PM +0200, Thomas Gleixner wrote:
> On Tue, 29 Apr 2014, Cyrill Gorcunov wrote:
> 
> > On Mon, Apr 07, 2014 at 09:47:01PM +0400, Cyrill Gorcunov wrote:
> > > Hi, here is an updated series, changes mostly in show_fdinfo code which now
> > > produces a way more information (and documentation update). Please take a look
> > > once time permit. Thanks!
> > 
> > Hi, any new comments on this series?
> 
> Looks ok and I did not do anything about it, as I saw a "will update
> in next version" from you in the discussion.

Silly me :( I promised to update the series a bit, and forgot. Thanks, Thomas!

[patch 0/3] timerfd -- implement missing parts to checkpoint and restore timerfd state, v3

[Cyrill Gorcunov]

Hello! This is (I hope) the final version. Most changes from v2 are sitting in
documentation, where I tried to reflect that the time we're printing out is
remaining until the timer expiration. Please take a look.

[patch 3/3] timerfd: Implement write method

[Cyrill Gorcunov]

[-- Attachment #1: timerfd-write-ticks --]
[-- Type: text/plain, Size: 1659 bytes --]

The read() of timerfd files allows to fetch the number of
timer ticks while there is no way to set it back from userspace.

To restore the timer state as it was at checkpoint moment we need
a way to setup ticks back. So as a counterpart of read() the write()
takes ticks number from the userspace and updates internal timer
ticks accordingly.

CC: Shawn Landden <shawn@churchofgit.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Andrey Vagin <avagin@openvz.org>
CC: Pavel Emelyanov <xemul@parallels.com>
CC: Vladimir Davydov <vdavydov@parallels.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---
 fs/timerfd.c |   21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

Index: linux-2.6.git/fs/timerfd.c
===================================================================
--- linux-2.6.git.orig/fs/timerfd.c
+++ linux-2.6.git/fs/timerfd.c
@@ -311,10 +311,31 @@ static int timerfd_show(struct seq_file
 			  (unsigned long long)t.it_interval.tv_nsec);
 }
 
+
+static ssize_t timerfd_write(struct file *file, const char __user *buf,
+			     size_t count, loff_t *ppos)
+{
+	struct timerfd_ctx *ctx = file->private_data;
+	u64 ticks = 0;
+
+	if (count < sizeof(ticks))
+		return -EINVAL;
+
+	if (get_user(ticks, (u64 __user *) buf))
+		return -EFAULT;
+
+	spin_lock_irq(&ctx->wqh.lock);
+	ctx->ticks = ticks;
+	spin_unlock_irq(&ctx->wqh.lock);
+
+	return sizeof(ticks);
+}
+
 static const struct file_operations timerfd_fops = {
 	.release	= timerfd_release,
 	.poll		= timerfd_poll,
 	.read		= timerfd_read,
+	.write		= timerfd_write,
 	.llseek		= noop_llseek,
 	.show_fdinfo	= timerfd_show,
 };

Re: [patch 3/3] timerfd: Implement write method

[Thomas Gleixner]

On Tue, 29 Apr 2014, Cyrill Gorcunov wrote:
> +
> +static ssize_t timerfd_write(struct file *file, const char __user *buf,
> +			     size_t count, loff_t *ppos)
> +{
> +	struct timerfd_ctx *ctx = file->private_data;
> +	u64 ticks = 0;
> +
> +	if (count < sizeof(ticks))
> +		return -EINVAL;
> +
> +	if (get_user(ticks, (u64 __user *) buf))
> +		return -EFAULT;
> +
> +	spin_lock_irq(&ctx->wqh.lock);
> +	ctx->ticks = ticks;

So what wakes a potential waiter in read/poll?

Thanks,

	tglx

Re: [patch 3/3] timerfd: Implement write method

[Cyrill Gorcunov]

On Thu, May 22, 2014 at 06:43:08AM +0900, Thomas Gleixner wrote:
> On Tue, 29 Apr 2014, Cyrill Gorcunov wrote:
> > +static ssize_t timerfd_write(struct file *file, const char __user *buf,
> > +			     size_t count, loff_t *ppos)
> > +{
> > +	struct timerfd_ctx *ctx = file->private_data;
> > +	u64 ticks = 0;
> > +
> > +	if (count < sizeof(ticks))
> > +		return -EINVAL;
> > +
> > +	if (get_user(ticks, (u64 __user *) buf))
> > +		return -EFAULT;
> > +
> > +	spin_lock_irq(&ctx->wqh.lock);
> > +	ctx->ticks = ticks;
> 
> So what wakes a potential waiter in read/poll?

Why should it? You mean the scenario when timer is armed then
someone writes nonzero @ticks and we should wake waiters?
The idea was to setup this ticks on timer restore without
waking anyone. If it breaks the logic of timerfd in general,
then sure I need to rework. Hm?

Re: [patch 3/3] timerfd: Implement write method

[Thomas Gleixner]

On Thu, 22 May 2014, Cyrill Gorcunov wrote:
> On Thu, May 22, 2014 at 06:43:08AM +0900, Thomas Gleixner wrote:
> > On Tue, 29 Apr 2014, Cyrill Gorcunov wrote:
> > > +static ssize_t timerfd_write(struct file *file, const char __user *buf,
> > > +			     size_t count, loff_t *ppos)
> > > +{
> > > +	struct timerfd_ctx *ctx = file->private_data;
> > > +	u64 ticks = 0;
> > > +
> > > +	if (count < sizeof(ticks))
> > > +		return -EINVAL;
> > > +
> > > +	if (get_user(ticks, (u64 __user *) buf))
> > > +		return -EFAULT;
> > > +
> > > +	spin_lock_irq(&ctx->wqh.lock);
> > > +	ctx->ticks = ticks;
> > 
> > So what wakes a potential waiter in read/poll?
> 
> Why should it? You mean the scenario when timer is armed then
> someone writes nonzero @ticks and we should wake waiters?
> The idea was to setup this ticks on timer restore without
> waking anyone. If it breaks the logic of timerfd in general,
> then sure I need to rework. Hm?

There is a world outside of checkpoint/restore, really.

So what's the semantics of that write function? We really want to have
that agreed on and documented in the man page.

Right now the write will just update the ticks and nothing else. So
what if there is a waiter already? What if there is a timer armed?

Can you please describe how checkpoint/restore is going to use all of
this. How is the timer restored and how/when is the reader which was
waiting in read/poll at the time of suspend reattached to it.

Thanks,

	tglx

Re: [patch 3/3] timerfd: Implement write method

[Cyrill Gorcunov]

On Thu, May 22, 2014 at 07:12:30AM +0900, Thomas Gleixner wrote:
> 
> There is a world outside of checkpoint/restore, really.

Yes, I simply don't know who else might use this write()
functionality for other purpose, I mean i don't see a
point to use it for anything else.

> So what's the semantics of that write function? We really want to have
> that agreed on and documented in the man page.

The idea was to provide a way to setup @ticks into (nonzero) value
which we get from show_fdinfo output. Then when we restore it
we setup the timer and set @ticks to the value it had at dump
moment.

> Right now the write will just update the ticks and nothing else. So
> what if there is a waiter already? What if there is a timer armed?
> 
> Can you please describe how checkpoint/restore is going to use all of
> this. How is the timer restored and how/when is the reader which was
> waiting in read/poll at the time of suspend reattached to it.

Thomas, I see what you mean. Need to think (I must admit I forgot about
polling of timerfds :( I were to restore timerfds like this

 - fetch data from fdinfo
 - use timer_create/settime to arm it
 - write @ticks then

but i didn't try restore polling waiters, my bad. Letme rework this
trying addressing your comments.

Re: [patch 3/3] timerfd: Implement write method

[Thomas Gleixner]

On Thu, 22 May 2014, Cyrill Gorcunov wrote:
> On Thu, May 22, 2014 at 07:12:30AM +0900, Thomas Gleixner wrote:
> > 
> > There is a world outside of checkpoint/restore, really.
> 
> Yes, I simply don't know who else might use this write()
> functionality for other purpose, I mean i don't see a
> point to use it for anything else.
> 
> > So what's the semantics of that write function? We really want to have
> > that agreed on and documented in the man page.
> 
> The idea was to provide a way to setup @ticks into (nonzero) value
> which we get from show_fdinfo output. Then when we restore it
> we setup the timer and set @ticks to the value it had at dump
> moment.

That's not describing the semantics. It's describing what you use it
for.
 
> > Right now the write will just update the ticks and nothing else. So
> > what if there is a waiter already? What if there is a timer armed?
> > 
> > Can you please describe how checkpoint/restore is going to use all of
> > this. How is the timer restored and how/when is the reader which was
> > waiting in read/poll at the time of suspend reattached to it.
> 
> Thomas, I see what you mean. Need to think (I must admit I forgot about
> polling of timerfds :( I were to restore timerfds like this
> 
>  - fetch data from fdinfo
>  - use timer_create/settime to arm it
>  - write @ticks then

That's clear to me.

So again you have to answer the questions:

   Do we just allow the write unconditionally?
   Do we care about waking readers/pollers?
 
Whatever the answer is, it needs to be documented coherently in the
changelog, in the code and in the man page.

Thanks,

	tglx

Re: [patch 3/3] timerfd: Implement write method

[Cyrill Gorcunov]

On Thu, May 22, 2014 at 08:30:04AM +0900, Thomas Gleixner wrote:
> > 
> > > So what's the semantics of that write function? We really want to have
> > > that agreed on and documented in the man page.
> > 
> > The idea was to provide a way to setup @ticks into (nonzero) value
> > which we get from show_fdinfo output. Then when we restore it
> > we setup the timer and set @ticks to the value it had at dump
> > moment.
> 
> That's not describing the semantics. It's describing what you use it
> for.

That's I've been intending to use it for and as result the semantic
was to write unconditionally. But because I missed polling in first
place now I think such semantic is wrong and write() should be
a complete counterpart of read() method and wake up waiters.

> > > Right now the write will just update the ticks and nothing else. So
> > > what if there is a waiter already? What if there is a timer armed?
> > > 
> > > Can you please describe how checkpoint/restore is going to use all of
> > > this. How is the timer restored and how/when is the reader which was
> > > waiting in read/poll at the time of suspend reattached to it.
> > 
> > Thomas, I see what you mean. Need to think (I must admit I forgot about
> > polling of timerfds :( I were to restore timerfds like this
> > 
> >  - fetch data from fdinfo
> >  - use timer_create/settime to arm it
> >  - write @ticks then
> 
> That's clear to me.
> 
> So again you have to answer the questions:
> 
>    Do we just allow the write unconditionally?
>    Do we care about waking readers/pollers?
>  
> Whatever the answer is, it needs to be documented coherently in the
> changelog, in the code and in the man page.

"Yes" to both questions I think. Thomas I'll return with a new patchset,
testcase and man update.

Re: [patch 3/3] timerfd: Implement write method

[Michael Kerrisk]

[Thomas, thanks for pinging me on this.]

Hi Cyril

Please CC linux-api on changes that affect kernel-user-space ABI/API.

On Thu, May 22, 2014 at 12:35 AM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> On Thu, May 22, 2014 at 07:12:30AM +0900, Thomas Gleixner wrote:
>>
>> There is a world outside of checkpoint/restore, really.
>
> Yes, I simply don't know who else might use this write()
> functionality for other purpose, I mean i don't see a
> point to use it for anything else.

Nevertheless, when making API changes like this, we should be thinking
as generally as possible, rather than looking from the perspective of
a single use case.

>> So what's the semantics of that write function? We really want to have
>> that agreed on and documented in the man page.
>
> The idea was to provide a way to setup @ticks into (nonzero) value
> which we get from show_fdinfo output. Then when we restore it
> we setup the timer and set @ticks to the value it had at dump
> moment.
>
>> Right now the write will just update the ticks and nothing else. So
>> what if there is a waiter already? What if there is a timer armed?
>>
>> Can you please describe how checkpoint/restore is going to use all of
>> this. How is the timer restored and how/when is the reader which was
>> waiting in read/poll at the time of suspend reattached to it.
>
> Thomas, I see what you mean. Need to think (I must admit I forgot about
> polling of timerfds :( I were to restore timerfds like this
>
>  - fetch data from fdinfo
>  - use timer_create/settime to arm it
>  - write @ticks then
>
> but i didn't try restore polling waiters, my bad. Letme rework this
> trying addressing your comments.

Great. Please CC me and linux-api@ on the next round.

Cheers,

Michael

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



-- 
Michael Kerrisk Linux man-pages maintainer;
http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface", http://blog.man7.org/

Re: [patch 3/3] timerfd: Implement write method

[Cyrill Gorcunov]

On Thu, May 22, 2014 at 08:32:45AM +0200, Michael Kerrisk wrote:
> [Thomas, thanks for pinging me on this.]
> 
> Hi Cyril
> 
> Please CC linux-api on changes that affect kernel-user-space ABI/API.

Sure!

Re: [patch 3/3] timerfd: Implement write method

[Thomas Gleixner]

On Thu, 22 May 2014, Thomas Gleixner wrote:

> On Tue, 29 Apr 2014, Cyrill Gorcunov wrote:
> > +
> > +static ssize_t timerfd_write(struct file *file, const char __user *buf,
> > +			     size_t count, loff_t *ppos)
> > +{
> > +	struct timerfd_ctx *ctx = file->private_data;
> > +	u64 ticks = 0;
> > +
> > +	if (count < sizeof(ticks))
> > +		return -EINVAL;
> > +
> > +	if (get_user(ticks, (u64 __user *) buf))
> > +		return -EFAULT;
> > +
> > +	spin_lock_irq(&ctx->wqh.lock);
> > +	ctx->ticks = ticks;
> 
> So what wakes a potential waiter in read/poll?

And who is updating timerfd_create(2) ?

Thanks,

	tglx

Re: [patch 3/3] timerfd: Implement write method

[Cyrill Gorcunov]

On Thu, May 22, 2014 at 06:58:19AM +0900, Thomas Gleixner wrote:
> > 
> > So what wakes a potential waiter in read/poll?
> 
> And who is updating timerfd_create(2) ?

Thomas, could you please take a look if the approach below is acceptable?
If it will be fine I update manpage then.
---
From: Cyrill Gorcunov <gorcunov@openvz.org>
Subject: timerfd: Implement timerfd_ioctl method to restore timerfd_ctx::ticks

The read() of timerfd files allows to fetch the number of timer ticks
while there is no way to set it back from userspace.

To restore the timer's state as it was at checkpoint moment we need
a path to bring @ticks back. Initially I thought about writing ticks
back via write() interface but it seems such API is somehow obscure.

Instead implement timerfd_ioctl() method with TFD_IOC_SET_TICKS
command which requires CAP_SYS_RESOURCE capability to be able to
set @ticks into arbitrary value. Note this command doesn't wake
up readers/waiters and its purpose only to serve C/R needs
(for same sake I wrapped code with CONFIG_CHECKPOINT_RESTORE).
Still if needed the ioctl may be extended for new commands
and CONFIG_CHECKPOINT_RESTORE dropped off.

CC: Thomas Gleixner <tglx@linutronix.de>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Andrey Vagin <avagin@openvz.org>
CC: Pavel Emelyanov <xemul@parallels.com>
CC: Vladimir Davydov <vdavydov@parallels.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---
 fs/timerfd.c            |   31 +++++++++++++++++++++++++++++++
 include/linux/timerfd.h |    5 +++++
 2 files changed, 36 insertions(+)

Index: linux-2.6.git/fs/timerfd.c
===================================================================
--- linux-2.6.git.orig/fs/timerfd.c
+++ linux-2.6.git/fs/timerfd.c
@@ -313,11 +313,42 @@ static int timerfd_show(struct seq_file
 }
 #endif
 
+#ifdef CONFIG_CHECKPOINT_RESTORE
+static long timerfd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	struct timerfd_ctx *ctx = file->private_data;
+	int ret = 0;
+
+	switch (cmd) {
+	case TFD_IOC_SET_TICKS: {
+		u64 ticks;
+
+		if (!capable(CAP_SYS_RESOURCE))
+			return -EPERM;
+		if (get_user(ticks, (u64 __user *)arg))
+			return -EFAULT;
+		spin_lock_irq(&ctx->wqh.lock);
+		ctx->ticks = ticks;
+		spin_unlock_irq(&ctx->wqh.lock);
+		break;
+	}
+	default:
+		ret = -ENOTTY;
+		break;
+	}
+
+	return ret;
+}
+#endif
+
 static const struct file_operations timerfd_fops = {
 	.release	= timerfd_release,
 	.poll		= timerfd_poll,
 	.read		= timerfd_read,
 	.llseek		= noop_llseek,
+#ifdef CONFIG_CHECKPOINT_RESTORE
+	.unlocked_ioctl	= timerfd_ioctl,
+#endif
 #ifdef CONFIG_PROC_FS
 	.show_fdinfo	= timerfd_show,
 #endif
Index: linux-2.6.git/include/linux/timerfd.h
===================================================================
--- linux-2.6.git.orig/include/linux/timerfd.h
+++ linux-2.6.git/include/linux/timerfd.h
@@ -11,6 +11,9 @@
 /* For O_CLOEXEC and O_NONBLOCK */
 #include <linux/fcntl.h>
 
+/* For _IO helpers */
+#include <linux/ioctl.h>
+
 /*
  * CAREFUL: Check include/asm-generic/fcntl.h when defining
  * new flags, since they might collide with O_* ones. We want
@@ -29,4 +32,6 @@
 /* Flags for timerfd_settime.  */
 #define TFD_SETTIME_FLAGS (TFD_TIMER_ABSTIME | TFD_TIMER_CANCEL_ON_SET)
 
+#define TFD_IOC_SET_TICKS	_IOW('T', 0, u64)
+
 #endif /* _LINUX_TIMERFD_H */

Re: [patch 3/3] timerfd: Implement write method

[Michael Kerrisk (man-pages)]

[CC += linux-api@]

On Tue, Jun 10, 2014 at 6:35 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> On Thu, May 22, 2014 at 06:58:19AM +0900, Thomas Gleixner wrote:
>> >
>> > So what wakes a potential waiter in read/poll?
>>
>> And who is updating timerfd_create(2) ?
>
> Thomas, could you please take a look if the approach below is acceptable?
> If it will be fine I update manpage then.
> ---
> From: Cyrill Gorcunov <gorcunov@openvz.org>
> Subject: timerfd: Implement timerfd_ioctl method to restore timerfd_ctx::ticks
>
> The read() of timerfd files allows to fetch the number of timer ticks
> while there is no way to set it back from userspace.
>
> To restore the timer's state as it was at checkpoint moment we need
> a path to bring @ticks back. Initially I thought about writing ticks
> back via write() interface but it seems such API is somehow obscure.
>
> Instead implement timerfd_ioctl() method with TFD_IOC_SET_TICKS
> command which requires CAP_SYS_RESOURCE capability to be able to
> set @ticks into arbitrary value. Note this command doesn't wake
> up readers/waiters and its purpose only to serve C/R needs
> (for same sake I wrapped code with CONFIG_CHECKPOINT_RESTORE).
> Still if needed the ioctl may be extended for new commands
> and CONFIG_CHECKPOINT_RESTORE dropped off.
>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: Andrey Vagin <avagin@openvz.org>
> CC: Pavel Emelyanov <xemul@parallels.com>
> CC: Vladimir Davydov <vdavydov@parallels.com>
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> ---
>  fs/timerfd.c            |   31 +++++++++++++++++++++++++++++++
>  include/linux/timerfd.h |    5 +++++
>  2 files changed, 36 insertions(+)
>
> Index: linux-2.6.git/fs/timerfd.c
> ===================================================================
> --- linux-2.6.git.orig/fs/timerfd.c
> +++ linux-2.6.git/fs/timerfd.c
> @@ -313,11 +313,42 @@ static int timerfd_show(struct seq_file
>  }
>  #endif
>
> +#ifdef CONFIG_CHECKPOINT_RESTORE
> +static long timerfd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> +       struct timerfd_ctx *ctx = file->private_data;
> +       int ret = 0;
> +
> +       switch (cmd) {
> +       case TFD_IOC_SET_TICKS: {
> +               u64 ticks;
> +
> +               if (!capable(CAP_SYS_RESOURCE))
> +                       return -EPERM;
> +               if (get_user(ticks, (u64 __user *)arg))
> +                       return -EFAULT;
> +               spin_lock_irq(&ctx->wqh.lock);
> +               ctx->ticks = ticks;
> +               spin_unlock_irq(&ctx->wqh.lock);
> +               break;
> +       }
> +       default:
> +               ret = -ENOTTY;
> +               break;
> +       }
> +
> +       return ret;
> +}
> +#endif
> +
>  static const struct file_operations timerfd_fops = {
>         .release        = timerfd_release,
>         .poll           = timerfd_poll,
>         .read           = timerfd_read,
>         .llseek         = noop_llseek,
> +#ifdef CONFIG_CHECKPOINT_RESTORE
> +       .unlocked_ioctl = timerfd_ioctl,
> +#endif
>  #ifdef CONFIG_PROC_FS
>         .show_fdinfo    = timerfd_show,
>  #endif
> Index: linux-2.6.git/include/linux/timerfd.h
> ===================================================================
> --- linux-2.6.git.orig/include/linux/timerfd.h
> +++ linux-2.6.git/include/linux/timerfd.h
> @@ -11,6 +11,9 @@
>  /* For O_CLOEXEC and O_NONBLOCK */
>  #include <linux/fcntl.h>
>
> +/* For _IO helpers */
> +#include <linux/ioctl.h>
> +
>  /*
>   * CAREFUL: Check include/asm-generic/fcntl.h when defining
>   * new flags, since they might collide with O_* ones. We want
> @@ -29,4 +32,6 @@
>  /* Flags for timerfd_settime.  */
>  #define TFD_SETTIME_FLAGS (TFD_TIMER_ABSTIME | TFD_TIMER_CANCEL_ON_SET)
>
> +#define TFD_IOC_SET_TICKS      _IOW('T', 0, u64)
> +
>  #endif /* _LINUX_TIMERFD_H */



-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [patch 3/3] timerfd: Implement write method

[Andy Lutomirski]

On Tue, Jun 10, 2014 at 1:03 PM, Michael Kerrisk (man-pages)
<mtk.manpages@gmail.com> wrote:
> [CC += linux-api@]
>
> On Tue, Jun 10, 2014 at 6:35 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>> On Thu, May 22, 2014 at 06:58:19AM +0900, Thomas Gleixner wrote:
>>> >
>>> > So what wakes a potential waiter in read/poll?
>>>
>>> And who is updating timerfd_create(2) ?
>>
>> Thomas, could you please take a look if the approach below is acceptable?
>> If it will be fine I update manpage then.
>> ---
>> From: Cyrill Gorcunov <gorcunov@openvz.org>
>> Subject: timerfd: Implement timerfd_ioctl method to restore timerfd_ctx::ticks
>>
>> The read() of timerfd files allows to fetch the number of timer ticks
>> while there is no way to set it back from userspace.
>>
>> To restore the timer's state as it was at checkpoint moment we need
>> a path to bring @ticks back. Initially I thought about writing ticks
>> back via write() interface but it seems such API is somehow obscure.
>>
>> Instead implement timerfd_ioctl() method with TFD_IOC_SET_TICKS
>> command which requires CAP_SYS_RESOURCE capability to be able to
>> set @ticks into arbitrary value. Note this command doesn't wake
>> up readers/waiters and its purpose only to serve C/R needs
>> (for same sake I wrapped code with CONFIG_CHECKPOINT_RESTORE).
>> Still if needed the ioctl may be extended for new commands
>> and CONFIG_CHECKPOINT_RESTORE dropped off.

Why does this need CAP_SYS_RESOURCE?

--Andy

Re: [patch 3/3] timerfd: Implement write method

[Cyrill Gorcunov]

On Tue, Jun 10, 2014 at 01:05:22PM -0700, Andy Lutomirski wrote:
> On Tue, Jun 10, 2014 at 1:03 PM, Michael Kerrisk (man-pages)
> <mtk.manpages@gmail.com> wrote:
> > [CC += linux-api@]

Thanks Michael!

> > On Tue, Jun 10, 2014 at 6:35 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> >> On Thu, May 22, 2014 at 06:58:19AM +0900, Thomas Gleixner wrote:
> >>> >
> >>> > So what wakes a potential waiter in read/poll?
> >>>
> >>> And who is updating timerfd_create(2) ?
> >>
> >> Thomas, could you please take a look if the approach below is acceptable?
> >> If it will be fine I update manpage then.
> >> ---
> >> From: Cyrill Gorcunov <gorcunov@openvz.org>
> >> Subject: timerfd: Implement timerfd_ioctl method to restore timerfd_ctx::ticks
> >>
> >> The read() of timerfd files allows to fetch the number of timer ticks
> >> while there is no way to set it back from userspace.
> >>
> >> To restore the timer's state as it was at checkpoint moment we need
> >> a path to bring @ticks back. Initially I thought about writing ticks
> >> back via write() interface but it seems such API is somehow obscure.
> >>
> >> Instead implement timerfd_ioctl() method with TFD_IOC_SET_TICKS
> >> command which requires CAP_SYS_RESOURCE capability to be able to
> >> set @ticks into arbitrary value. Note this command doesn't wake
> >> up readers/waiters and its purpose only to serve C/R needs
> >> (for same sake I wrapped code with CONFIG_CHECKPOINT_RESTORE).
> >> Still if needed the ioctl may be extended for new commands
> >> and CONFIG_CHECKPOINT_RESTORE dropped off.
> 
> Why does this need CAP_SYS_RESOURCE?

  Because I think this interface should not be used by a regular
applications, the only purpose is to restore the @ticks after
checkpoint. Requiring CAP_SYS_RESOURCE means that at least
program which use it knows what it's doing.

  Still if someone has a scenarion where we might need this
intarface out of this cap requirement -- we always can
drop it of without breaking existing users, but not the
reverse.

P.S. I remember Thomas' words about existence of the other
word out of c/r, still I treat this ioctl as exception
(as in prctl codes we use for c/r).

Re: [patch 3/3] timerfd: Implement write method

[Andrew Vagin]

On Tue, Jun 10, 2014 at 08:35:30PM +0400, Cyrill Gorcunov wrote:
> On Thu, May 22, 2014 at 06:58:19AM +0900, Thomas Gleixner wrote:
> > > 
> > > So what wakes a potential waiter in read/poll?
> > 
> > And who is updating timerfd_create(2) ?
> 
> Thomas, could you please take a look if the approach below is acceptable?
> If it will be fine I update manpage then.
> ---
> From: Cyrill Gorcunov <gorcunov@openvz.org>
> Subject: timerfd: Implement timerfd_ioctl method to restore timerfd_ctx::ticks
> 
> The read() of timerfd files allows to fetch the number of timer ticks
> while there is no way to set it back from userspace.
> 
> To restore the timer's state as it was at checkpoint moment we need
> a path to bring @ticks back. Initially I thought about writing ticks
> back via write() interface but it seems such API is somehow obscure.
> 
> Instead implement timerfd_ioctl() method with TFD_IOC_SET_TICKS
> command which requires CAP_SYS_RESOURCE capability to be able to
> set @ticks into arbitrary value. Note this command doesn't wake
> up readers/waiters and its purpose only to serve C/R needs
> (for same sake I wrapped code with CONFIG_CHECKPOINT_RESTORE).
> Still if needed the ioctl may be extended for new commands
> and CONFIG_CHECKPOINT_RESTORE dropped off.
> 
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: Andrey Vagin <avagin@openvz.org>
> CC: Pavel Emelyanov <xemul@parallels.com>
> CC: Vladimir Davydov <vdavydov@parallels.com>
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> ---
>  fs/timerfd.c            |   31 +++++++++++++++++++++++++++++++
>  include/linux/timerfd.h |    5 +++++
>  2 files changed, 36 insertions(+)
> 
> Index: linux-2.6.git/fs/timerfd.c
> ===================================================================
> --- linux-2.6.git.orig/fs/timerfd.c
> +++ linux-2.6.git/fs/timerfd.c
> @@ -313,11 +313,42 @@ static int timerfd_show(struct seq_file
>  }
>  #endif
>  
> +#ifdef CONFIG_CHECKPOINT_RESTORE
> +static long timerfd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> +	struct timerfd_ctx *ctx = file->private_data;
> +	int ret = 0;
> +
> +	switch (cmd) {
> +	case TFD_IOC_SET_TICKS: {
> +		u64 ticks;
> +
> +		if (!capable(CAP_SYS_RESOURCE))
> +			return -EPERM;

I think it is too strong. It will not work in userns.

Why do we need to check CAP_SYS_RESOURCE here?
Can we replace capable on ns_capable?

> +		if (get_user(ticks, (u64 __user *)arg))
> +			return -EFAULT;
> +		spin_lock_irq(&ctx->wqh.lock);
> +		ctx->ticks = ticks;

I think we need to wakt up readers here if ctx->ticks isn't zero.

> +		spin_unlock_irq(&ctx->wqh.lock);
> +		break;
> +	}
> +	default:
> +		ret = -ENOTTY;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +#endif
> +
>  static const struct file_operations timerfd_fops = {
>  	.release	= timerfd_release,
>  	.poll		= timerfd_poll,
>  	.read		= timerfd_read,
>  	.llseek		= noop_llseek,
> +#ifdef CONFIG_CHECKPOINT_RESTORE
> +	.unlocked_ioctl	= timerfd_ioctl,
> +#endif
>  #ifdef CONFIG_PROC_FS
>  	.show_fdinfo	= timerfd_show,
>  #endif
> Index: linux-2.6.git/include/linux/timerfd.h
> ===================================================================
> --- linux-2.6.git.orig/include/linux/timerfd.h
> +++ linux-2.6.git/include/linux/timerfd.h
> @@ -11,6 +11,9 @@
>  /* For O_CLOEXEC and O_NONBLOCK */
>  #include <linux/fcntl.h>
>  
> +/* For _IO helpers */
> +#include <linux/ioctl.h>
> +
>  /*
>   * CAREFUL: Check include/asm-generic/fcntl.h when defining
>   * new flags, since they might collide with O_* ones. We want
> @@ -29,4 +32,6 @@
>  /* Flags for timerfd_settime.  */
>  #define TFD_SETTIME_FLAGS (TFD_TIMER_ABSTIME | TFD_TIMER_CANCEL_ON_SET)
>  
> +#define TFD_IOC_SET_TICKS	_IOW('T', 0, u64)
> +
>  #endif /* _LINUX_TIMERFD_H */

Re: [patch 3/3] timerfd: Implement write method

[Cyrill Gorcunov]

On Wed, Jun 11, 2014 at 11:27:43AM +0400, Andrew Vagin wrote:
> > +	case TFD_IOC_SET_TICKS: {
> > +		u64 ticks;
> > +
> > +		if (!capable(CAP_SYS_RESOURCE))
> > +			return -EPERM;
> 
> I think it is too strong. It will not work in userns.
> 
> Why do we need to check CAP_SYS_RESOURCE here?
> Can we replace capable on ns_capable?
> 
> > +		if (get_user(ticks, (u64 __user *)arg))
> > +			return -EFAULT;
> > +		spin_lock_irq(&ctx->wqh.lock);
> > +		ctx->ticks = ticks;
> 
> I think we need to wakt up readers here if ctx->ticks isn't zero.

  I used this caps to prevent arbitrary changing the ticks from a
regular programs because i didn't wake up readers. But after thinking
more i think indeed this case (write ticks but not wake up anyone)
leads to strange situation -- currently (without the patch) there can't
be scenario when we have a waiter not woken up with nonzero @ticks,
but with this patch applied the timerfd logic become vague. One can
set up @ticks to nonzero value and still have waiter looping around.

  Thanks Andrew! How about the patch below? We allow to modify @ticks
and wake up waiter on non-zero @ticks assignment. I think this should
fit current timerfd logic.

(once such or anything else approach get approved -- I'll resend new
 series with man updated).
---
From: Cyrill Gorcunov <gorcunov@openvz.org>
Subject: timerfd: Implement timerfd_ioctl method to restore timerfd_ctx::ticks

The read() of timerfd files allows to fetch the number of timer ticks
while there is no way to set it back from userspace.

To restore the timer's state as it was at checkpoint moment we need
a path to bring @ticks back. Initially I thought about writing ticks
back via write() interface but it seems such API is somehow obscure.

Instead implement timerfd_ioctl() method with TFD_IOC_SET_TICKS
command which allows to adjust @ticks into arbitrary value. In
case if new value is non-zero we are waking up the waiters.
I wrapped code with CONFIG_CHECKPOINT_RESTORE which can be
dropped off if there users except c/r camp appear.

CC: Thomas Gleixner <tglx@linutronix.de>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Andrey Vagin <avagin@openvz.org>
CC: Pavel Emelyanov <xemul@parallels.com>
CC: Vladimir Davydov <vdavydov@parallels.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---
 fs/timerfd.c            |   31 +++++++++++++++++++++++++++++++
 include/linux/timerfd.h |    5 +++++
 2 files changed, 36 insertions(+)

Index: linux-2.6.git/fs/timerfd.c
===================================================================
--- linux-2.6.git.orig/fs/timerfd.c
+++ linux-2.6.git/fs/timerfd.c
@@ -313,11 +313,42 @@ static int timerfd_show(struct seq_file
 }
 #endif
 
+#ifdef CONFIG_CHECKPOINT_RESTORE
+static long timerfd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	struct timerfd_ctx *ctx = file->private_data;
+	int ret = 0;
+
+	switch (cmd) {
+	case TFD_IOC_SET_TICKS: {
+		u64 ticks;
+
+		if (get_user(ticks, (u64 __user *)arg))
+			return -EFAULT;
+		spin_lock_irq(&ctx->wqh.lock);
+		ctx->ticks = ticks;
+		if (ticks)
+			wake_up_locked(&ctx->wqh);
+		spin_unlock_irq(&ctx->wqh.lock);
+		break;
+	}
+	default:
+		ret = -ENOTTY;
+		break;
+	}
+
+	return ret;
+}
+#endif
+
 static const struct file_operations timerfd_fops = {
 	.release	= timerfd_release,
 	.poll		= timerfd_poll,
 	.read		= timerfd_read,
 	.llseek		= noop_llseek,
+#ifdef CONFIG_CHECKPOINT_RESTORE
+	.unlocked_ioctl	= timerfd_ioctl,
+#endif
 #ifdef CONFIG_PROC_FS
 	.show_fdinfo	= timerfd_show,
 #endif
Index: linux-2.6.git/include/linux/timerfd.h
===================================================================
--- linux-2.6.git.orig/include/linux/timerfd.h
+++ linux-2.6.git/include/linux/timerfd.h
@@ -11,6 +11,9 @@
 /* For O_CLOEXEC and O_NONBLOCK */
 #include <linux/fcntl.h>
 
+/* For _IO helpers */
+#include <linux/ioctl.h>
+
 /*
  * CAREFUL: Check include/asm-generic/fcntl.h when defining
  * new flags, since they might collide with O_* ones. We want
@@ -29,4 +32,6 @@
 /* Flags for timerfd_settime.  */
 #define TFD_SETTIME_FLAGS (TFD_TIMER_ABSTIME | TFD_TIMER_CANCEL_ON_SET)
 
+#define TFD_IOC_SET_TICKS	_IOW('T', 0, u64)
+
 #endif /* _LINUX_TIMERFD_H */

Re: [patch 3/3] timerfd: Implement write method

[Andrew Vagin]

On Wed, Jun 11, 2014 at 11:51:25AM +0400, Cyrill Gorcunov wrote:
> On Wed, Jun 11, 2014 at 11:27:43AM +0400, Andrew Vagin wrote:

...

> +#ifdef CONFIG_CHECKPOINT_RESTORE
> +static long timerfd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> +	struct timerfd_ctx *ctx = file->private_data;
> +	int ret = 0;
> +
> +	switch (cmd) {
> +	case TFD_IOC_SET_TICKS: {
> +		u64 ticks;
> +
> +		if (get_user(ticks, (u64 __user *)arg))
> +			return -EFAULT;
> +		spin_lock_irq(&ctx->wqh.lock);
> +		ctx->ticks = ticks;
> +		if (ticks)
> +			wake_up_locked(&ctx->wqh);

Setting ticks to zero is equivalent to timerfd_read(), isn't it?
So do we need to re-arme the timer, if it's periodic?

> +		spin_unlock_irq(&ctx->wqh.lock);
> +		break;
> +	}
> +	default:
> +		ret = -ENOTTY;
> +		break;
> +	}
> +
> +V	return ret;
> +}
> +#endif

Re: [patch 3/3] timerfd: Implement write method

[Cyrill Gorcunov]

On Wed, Jun 11, 2014 at 01:09:15PM +0400, Andrew Vagin wrote:
> On Wed, Jun 11, 2014 at 11:51:25AM +0400, Cyrill Gorcunov wrote:
> > On Wed, Jun 11, 2014 at 11:27:43AM +0400, Andrew Vagin wrote:
> 
> ...
> 
> > +#ifdef CONFIG_CHECKPOINT_RESTORE
> > +static long timerfd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> > +{
> > +	struct timerfd_ctx *ctx = file->private_data;
> > +	int ret = 0;
> > +
> > +	switch (cmd) {
> > +	case TFD_IOC_SET_TICKS: {
> > +		u64 ticks;
> > +
> > +		if (get_user(ticks, (u64 __user *)arg))
> > +			return -EFAULT;
> > +		spin_lock_irq(&ctx->wqh.lock);
> > +		ctx->ticks = ticks;
> > +		if (ticks)
> > +			wake_up_locked(&ctx->wqh);
> 
> Setting ticks to zero is equivalent to timerfd_read(), isn't it?
> So do we need to re-arme the timer, if it's periodic?

I must admit I'm not really sure if we should rearm it in such
case. In general @ticks are zeroified in case of timer-setup/cancel/read.

 - lets consider someone armed the timer it triggered but no read done
   yet, instead ioctl called and @ticks are set to zero, then call for
   read() and it returns zero to caller not rearming the timer (in
   current patch approach and non-block read)

 - in turn if we rearm timer on @ticks = 0 in ioctl this makes it
   close to behaviour of read() function (which in turn look to
   me as a duplication of read() interface).

That said, I'm not sure yet...

Re: [patch 3/3] timerfd: Implement write method

[Cyrill Gorcunov]

On Wed, Jun 11, 2014 at 01:52:46PM +0400, Cyrill Gorcunov wrote:
> On Wed, Jun 11, 2014 at 01:09:15PM +0400, Andrew Vagin wrote:
> > On Wed, Jun 11, 2014 at 11:51:25AM +0400, Cyrill Gorcunov wrote:
> > > On Wed, Jun 11, 2014 at 11:27:43AM +0400, Andrew Vagin wrote:
> > 
> > Setting ticks to zero is equivalent to timerfd_read(), isn't it?
> > So do we need to re-arme the timer, if it's periodic?
> 
> I must admit I'm not really sure if we should rearm it in such
> case. In general @ticks are zeroified in case of timer-setup/cancel/read.
> 
>  - lets consider someone armed the timer it triggered but no read done
>    yet, instead ioctl called and @ticks are set to zero, then call for
>    read() and it returns zero to caller not rearming the timer (in
>    current patch approach and non-block read)
> 
>  - in turn if we rearm timer on @ticks = 0 in ioctl this makes it
>    close to behaviour of read() function (which in turn look to
>    me as a duplication of read() interface).
> 
> That said, I'm not sure yet...

What if we prohibit setting non-zero values here? @ticks are set to
zero on timerfd_setup thus there is always a way to create a timer
with fields zeroified. Something like

	case TFD_IOC_SET_TICKS: {
		u64 ticks;

		if (get_user(ticks, (u64 __user *)arg))
			return -EFAULT;
		if (!ticks)
			return -EINVAL;

		spin_lock_irq(&ctx->wqh.lock);
		if (!timerfd_canceled(ctx)) {
			ctx->ticks = ticks;
			if (ticks)
				wake_up_locked(&ctx->wqh);
			else
		} else
			ret = -ECANCELED;
		spin_unlock_irq(&ctx->wqh.lock);
		break;
	}
?