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

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

[Michael Kerrisk (man-pages)]

[CC += linux-api@]

On Tue, Jun 10, 2014 at 6:35 PM, Cyrill Gorcunov <gorcunov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-GEFAQzZX7r8dnm+yROfE0A@public.gmane.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-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> CC: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
> CC: Andrey Vagin <avagin-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org>
> CC: Pavel Emelyanov <xemul-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> CC: Vladimir Davydov <vdavydov-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> Signed-off-by: Cyrill Gorcunov <gorcunov-GEFAQzZX7r8dnm+yROfE0A@public.gmane.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).