On Mon, Feb 03, 2014 at 01:24:41AM +0000, [email protected] wrote:
> ---- Original Message ----
> From: "Andrey Wagin" <[email protected]>
> To: "Shawn Landden" <[email protected]>
> CC: "LKML" <[email protected]>, "[email protected]"
> <[email protected]>, "Alexander Viro" <[email protected]>,
> [email protected], "Thomas Gleixner" <[email protected]>
> Sent: Wed, Dec 25, 2013, 12:46 AM
> Subject: Re: [CRIU] [PATCH] timerfd: show procfs fdinfo helper
>
>
> 2013/12/24 Shawn Landden <[email protected]>:
>
> | pos: 0
> | flags: 02004002
> | clockid: 0
>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Alexander Viro <[email protected]>
> Signed-off-by: Shawn Landden <[email protected]>
> ---
> fs/timerfd.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/fs/timerfd.c b/fs/timerfd.c
> index 9293121..e5fa587 100644
> --- a/fs/timerfd.c
> +++ b/fs/timerfd.c
> @@ -25,6 +25,7 @@
> #include <linux/syscalls.h>
> #include <linux/compat.h>
> #include <linux/rcupdate.h>
> +#include <linux/seq_file.h>
>
> struct timerfd_ctx {
> union {
> @@ -284,7 +285,23 @@ static ssize_t timerfd_read(struct file *file,
> char __user *buf, size_t count,
> return res;
> }
>
> +#ifdef CONFIG_PROC_FS
> +static int timerfd_show_fdinfo(struct seq_file *m, struct file *f)
> +{
> + struct timerfd_ctx *ctx = f->private_data;
> + int clockid;
> +
> + clockid = ctx->clockid;
> + seq_printf(m, "clockid:\t%d\n", clockid);
>
>
>
> I think we can show ctx->ticks, itimerspec here. The ctx->ticks is
> required for proper dumping and restoring timerfd.
>
>
> How? Shouldn't the itemerspec (from timerfd_gettime and restored with
> timerfd_settime) and clockid be enough?
ctx->ticks is avaliable to userspace programs, so it must be restored,
otherwise you can break applications, which use this parameter in own
logic.
> How do we put the ctx->ticks back into
> the restored timerfd if we get it out with procfs?
read(2) returns ctx->ticks, so write() can be used for set it. It you
don't like this, you can add ioctl for setting ctx->ticks.
>
> +
> + return 0;
> +}
> +#endif
> +
> static const struct file_operations timerfd_fops = {
> +#ifdef CONFIG_PROC_FS
> + .show_fdinfo = timerfd_show_fdinfo,
> +#endif
> .release = timerfd_release,
> .poll = timerfd_poll,
> .read = timerfd_read,
> --
> 1.8.5.2.297.g3e57c29
>
> _______________________________________________
> CRIU mailing list
> [email protected]
> https://lists.openvz.org/mailman/listinfo/criu
>
> _______________________________________________
> CRIU mailing list
> [email protected]
> https://lists.openvz.org/mailman/listinfo/criu
| pos: 0
| flags: 02004002
| clockid: 0
| ticks: 6
Cc: Thomas Gleixner <[email protected]>
Cc: Alexander Viro <[email protected]>
Signed-off-by: Shawn Landden <[email protected]>
---
fs/timerfd.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 64 insertions(+)
diff --git a/fs/timerfd.c b/fs/timerfd.c
index 9293121..2e81bdb 100644
--- a/fs/timerfd.c
+++ b/fs/timerfd.c
@@ -25,6 +25,7 @@
#include <linux/syscalls.h>
#include <linux/compat.h>
#include <linux/rcupdate.h>
+#include <linux/seq_file.h>
struct timerfd_ctx {
union {
@@ -284,10 +285,73 @@ static ssize_t timerfd_read(struct file *file, char __user *buf, size_t count,
return res;
}
+#ifdef CONFIG_PROC_FS
+static int timerfd_show_fdinfo(struct seq_file *m, struct file *f)
+{
+ struct timerfd_ctx *ctx = f->private_data;
+
+ seq_printf(m, "clockid:\t%d\n"
+ "ticks:\t%llu\n", ctx->clockid, ctx->ticks);
+
+ return 0;
+}
+#endif
+
+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;
+ ssize_t res;
+ __u64 ucnt;
+ DECLARE_WAITQUEUE(wait, current);
+
+ if (count < sizeof(ucnt))
+ return -EINVAL;
+ if (copy_from_user(&ucnt, buf, sizeof(ucnt)))
+ return -EFAULT;
+ if (ucnt == ULLONG_MAX)
+ return -EINVAL;
+ spin_lock_irq(&ctx->wqh.lock);
+ res = -EAGAIN;
+ if (ULLONG_MAX - ctx->ticks > ucnt)
+ res = sizeof(ucnt);
+ else if (!(file->f_flags & O_NONBLOCK)) {
+ __add_wait_queue(&ctx->wqh, &wait);
+ for (res = 0;;) {
+ set_current_state(TASK_INTERRUPTIBLE);
+ if (ULLONG_MAX - ctx->ticks > ucnt) {
+ res = sizeof(ucnt);
+ break;
+ }
+ if (signal_pending(current)) {
+ res = -ERESTARTSYS;
+ break;
+ }
+ spin_unlock_irq(&ctx->wqh.lock);
+ schedule();
+ spin_lock_irq(&ctx->wqh.lock);
+ }
+ __remove_wait_queue(&ctx->wqh, &wait);
+ __set_current_state(TASK_RUNNING);
+ }
+ if (likely(res > 0)) {
+ ctx->ticks += ucnt;
+ if (waitqueue_active(&ctx->wqh))
+ wake_up_locked_poll(&ctx->wqh, POLLIN);
+ }
+ spin_unlock_irq(&ctx->wqh.lock);
+
+ return res;
+}
+
static const struct file_operations timerfd_fops = {
+#ifdef CONFIG_PROC_FS
+ .show_fdinfo = timerfd_show_fdinfo,
+#endif
.release = timerfd_release,
.poll = timerfd_poll,
.read = timerfd_read,
+ .write = timerfd_write,
.llseek = noop_llseek,
};
--
1.8.5.2.297.g3e57c29
On Mon, 3 Feb 2014, Shawn Landden wrote:
> | pos: 0
> | flags: 02004002
> | clockid: 0
> | ticks: 6
This changelog is completely useless. It neither explains WHY we need
to expose that fdinfo nor WHAT the write function is supposed to do.
Aside of that the code lacks any comment and there is no documentation
for the new user ABI.
> Signed-off-by: Shawn Landden <[email protected]>
I'm not going to find and read the bible of the churchofgit to figure
out what this is all about.
Thanks,
tglx