[RFC PATCH v2] taskstats: add /proc/taskstats to fetch pid/tgid status

[RFC PATCH v2] taskstats: add /proc/taskstats to fetch pid/tgid status

[Weiping Zhang]

If a program needs monitor lots of process's status, it needs two
syscalls for every process. The first one is telling kernel which
pid/tgid should be monitored by send a command(write socket) to kernel.
The second one is read the statistics by read socket. This patch add
a new interface /proc/taskstats to reduce two syscalls to one ioctl.
The user just set the target pid/tgid to the struct taskstats.ac_pid,
then kernel will collect statistics for that pid/tgid.

Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>
---
Changes since v1:
 * use /proc/taskstats instead of /dev/taskstats

 include/uapi/linux/taskstats.h |  5 +++
 kernel/taskstats.c             | 73 +++++++++++++++++++++++++++++++++-
 2 files changed, 77 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/taskstats.h b/include/uapi/linux/taskstats.h
index ccbd08709321..077eab84c77e 100644
--- a/include/uapi/linux/taskstats.h
+++ b/include/uapi/linux/taskstats.h
@@ -214,6 +214,11 @@ enum {
 
 #define TASKSTATS_CMD_ATTR_MAX (__TASKSTATS_CMD_ATTR_MAX - 1)
 
+/* ioctl command */
+#define TASKSTATS_IOC_ATTR_PID	_IO('T', TASKSTATS_CMD_ATTR_PID)
+#define TASKSTATS_IOC_ATTR_TGID	_IO('T', TASKSTATS_CMD_ATTR_TGID)
+
+
 /* NETLINK_GENERIC related info */
 
 #define TASKSTATS_GENL_NAME	"TASKSTATS"
diff --git a/kernel/taskstats.c b/kernel/taskstats.c
index a2802b6ff4bb..c0f9e2f2308b 100644
--- a/kernel/taskstats.c
+++ b/kernel/taskstats.c
@@ -21,6 +21,8 @@
 #include <net/genetlink.h>
 #include <linux/atomic.h>
 #include <linux/sched/cputime.h>
+#include <linux/proc_fs.h>
+#include <linux/uio.h>
 
 /*
  * Maximum length of a cpumask that can be specified in
@@ -28,6 +30,10 @@
  */
 #define TASKSTATS_CPUMASK_MAXLEN	(100+6*NR_CPUS)
 
+#ifdef CONFIG_PROC_FS
+#define PROC_TASKSTATS	"taskstats"
+#endif
+
 static DEFINE_PER_CPU(__u32, taskstats_seqnum);
 static int family_registered;
 struct kmem_cache *taskstats_cache;
@@ -666,6 +672,60 @@ static struct genl_family family __ro_after_init = {
 	.n_ops		= ARRAY_SIZE(taskstats_ops),
 };
 
+#ifdef CONFIG_PROC_FS
+static long taskstats_ioctl_pid_tgid(unsigned long arg, bool tgid)
+{
+	struct taskstats kstat;
+	struct taskstats *ustat = (struct taskstats *)arg;
+	u32 id;
+	unsigned long offset = offsetof(struct taskstats, ac_pid);
+	long ret;
+
+	/* userspace set monitored pid/tgid to the field "ac_pid" */
+	if (unlikely(copy_from_user(&id, (void *)(arg + offset), sizeof(u32))))
+		return -EFAULT;
+
+	if (tgid)
+		ret = fill_stats_for_tgid(id, &kstat);
+	else
+		ret = fill_stats_for_pid(id, &kstat);
+	if (ret < 0)
+		return ret;
+
+	if (unlikely(copy_to_user(ustat, &kstat, sizeof(struct taskstats))))
+		return -EFAULT;
+
+	return 0;
+}
+
+static long taskstats_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	long ret;
+
+	switch (cmd) {
+	case TASKSTATS_IOC_ATTR_PID:
+		ret = taskstats_ioctl_pid_tgid(arg, false);
+		break;
+	case TASKSTATS_IOC_ATTR_TGID:
+		ret = taskstats_ioctl_pid_tgid(arg, true);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
+static const struct proc_ops taskstats_proc_ops = {
+	.proc_ioctl = taskstats_ioctl,
+#ifdef CONFIG_COMPAT
+	.proc_compat_ioctl = taskstats_ioctl,
+#endif
+};
+#endif
+
+
 /* Needed early in initialization */
 void __init taskstats_init_early(void)
 {
@@ -682,9 +742,20 @@ static int __init taskstats_init(void)
 {
 	int rc;
 
+#ifdef CONFIG_PROC_FS
+	if (!proc_create(PROC_TASKSTATS,  0, NULL, &taskstats_proc_ops)) {
+		pr_err("failed to create /proc/%s\n", PROC_TASKSTATS);
+		return -1;
+	}
+#endif
+
 	rc = genl_register_family(&family);
-	if (rc)
+	if (rc) {
+#ifdef CONFIG_PROC_FS
+		remove_proc_entry(PROC_TASKSTATS, NULL);
+#endif
 		return rc;
+	}
 
 	family_registered = 1;
 	pr_info("registered taskstats version %d\n", TASKSTATS_GENL_VERSION);
-- 
2.17.2

Re: [RFC PATCH v2] taskstats: add /proc/taskstats to fetch pid/tgid status

[Weiping Zhang]

Hi David,

Could you help review this patch ?

thanks

On Fri, Dec 18, 2020 at 1:24 AM Weiping Zhang
<zhangweiping@didiglobal.com> wrote:
>
> If a program needs monitor lots of process's status, it needs two
> syscalls for every process. The first one is telling kernel which
> pid/tgid should be monitored by send a command(write socket) to kernel.
> The second one is read the statistics by read socket. This patch add
> a new interface /proc/taskstats to reduce two syscalls to one ioctl.
> The user just set the target pid/tgid to the struct taskstats.ac_pid,
> then kernel will collect statistics for that pid/tgid.
>
> Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>
> ---
> Changes since v1:
>  * use /proc/taskstats instead of /dev/taskstats
>
>  include/uapi/linux/taskstats.h |  5 +++
>  kernel/taskstats.c             | 73 +++++++++++++++++++++++++++++++++-
>  2 files changed, 77 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/taskstats.h b/include/uapi/linux/taskstats.h
> index ccbd08709321..077eab84c77e 100644
> --- a/include/uapi/linux/taskstats.h
> +++ b/include/uapi/linux/taskstats.h
> @@ -214,6 +214,11 @@ enum {
>
>  #define TASKSTATS_CMD_ATTR_MAX (__TASKSTATS_CMD_ATTR_MAX - 1)
>
> +/* ioctl command */
> +#define TASKSTATS_IOC_ATTR_PID _IO('T', TASKSTATS_CMD_ATTR_PID)
> +#define TASKSTATS_IOC_ATTR_TGID        _IO('T', TASKSTATS_CMD_ATTR_TGID)
> +
> +
>  /* NETLINK_GENERIC related info */
>
>  #define TASKSTATS_GENL_NAME    "TASKSTATS"
> diff --git a/kernel/taskstats.c b/kernel/taskstats.c
> index a2802b6ff4bb..c0f9e2f2308b 100644
> --- a/kernel/taskstats.c
> +++ b/kernel/taskstats.c
> @@ -21,6 +21,8 @@
>  #include <net/genetlink.h>
>  #include <linux/atomic.h>
>  #include <linux/sched/cputime.h>
> +#include <linux/proc_fs.h>
> +#include <linux/uio.h>
>
>  /*
>   * Maximum length of a cpumask that can be specified in
> @@ -28,6 +30,10 @@
>   */
>  #define TASKSTATS_CPUMASK_MAXLEN       (100+6*NR_CPUS)
>
> +#ifdef CONFIG_PROC_FS
> +#define PROC_TASKSTATS "taskstats"
> +#endif
> +
>  static DEFINE_PER_CPU(__u32, taskstats_seqnum);
>  static int family_registered;
>  struct kmem_cache *taskstats_cache;
> @@ -666,6 +672,60 @@ static struct genl_family family __ro_after_init = {
>         .n_ops          = ARRAY_SIZE(taskstats_ops),
>  };
>
> +#ifdef CONFIG_PROC_FS
> +static long taskstats_ioctl_pid_tgid(unsigned long arg, bool tgid)
> +{
> +       struct taskstats kstat;
> +       struct taskstats *ustat = (struct taskstats *)arg;
> +       u32 id;
> +       unsigned long offset = offsetof(struct taskstats, ac_pid);
> +       long ret;
> +
> +       /* userspace set monitored pid/tgid to the field "ac_pid" */
> +       if (unlikely(copy_from_user(&id, (void *)(arg + offset), sizeof(u32))))
> +               return -EFAULT;
> +
> +       if (tgid)
> +               ret = fill_stats_for_tgid(id, &kstat);
> +       else
> +               ret = fill_stats_for_pid(id, &kstat);
> +       if (ret < 0)
> +               return ret;
> +
> +       if (unlikely(copy_to_user(ustat, &kstat, sizeof(struct taskstats))))
> +               return -EFAULT;
> +
> +       return 0;
> +}
> +
> +static long taskstats_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> +       long ret;
> +
> +       switch (cmd) {
> +       case TASKSTATS_IOC_ATTR_PID:
> +               ret = taskstats_ioctl_pid_tgid(arg, false);
> +               break;
> +       case TASKSTATS_IOC_ATTR_TGID:
> +               ret = taskstats_ioctl_pid_tgid(arg, true);
> +               break;
> +       default:
> +               ret = -EINVAL;
> +               break;
> +       }
> +
> +       return ret;
> +}
> +
> +static const struct proc_ops taskstats_proc_ops = {
> +       .proc_ioctl = taskstats_ioctl,
> +#ifdef CONFIG_COMPAT
> +       .proc_compat_ioctl = taskstats_ioctl,
> +#endif
> +};
> +#endif
> +
> +
>  /* Needed early in initialization */
>  void __init taskstats_init_early(void)
>  {
> @@ -682,9 +742,20 @@ static int __init taskstats_init(void)
>  {
>         int rc;
>
> +#ifdef CONFIG_PROC_FS
> +       if (!proc_create(PROC_TASKSTATS,  0, NULL, &taskstats_proc_ops)) {
> +               pr_err("failed to create /proc/%s\n", PROC_TASKSTATS);
> +               return -1;
> +       }
> +#endif
> +
>         rc = genl_register_family(&family);
> -       if (rc)
> +       if (rc) {
> +#ifdef CONFIG_PROC_FS
> +               remove_proc_entry(PROC_TASKSTATS, NULL);
> +#endif
>                 return rc;
> +       }
>
>         family_registered = 1;
>         pr_info("registered taskstats version %d\n", TASKSTATS_GENL_VERSION);
> --
> 2.17.2
>

Re: [RFC PATCH v2] taskstats: add /proc/taskstats to fetch pid/tgid status

[Weiping Zhang]

Hello Balbir Singh,

Could you help review this patch, thanks

On Mon, Dec 28, 2020 at 10:10 PM Weiping Zhang <zwp10758@gmail.com> wrote:
>
> Hi David,
>
> Could you help review this patch ?
>
> thanks
>
> On Fri, Dec 18, 2020 at 1:24 AM Weiping Zhang
> <zhangweiping@didiglobal.com> wrote:
> >
> > If a program needs monitor lots of process's status, it needs two
> > syscalls for every process. The first one is telling kernel which
> > pid/tgid should be monitored by send a command(write socket) to kernel.
> > The second one is read the statistics by read socket. This patch add
> > a new interface /proc/taskstats to reduce two syscalls to one ioctl.
> > The user just set the target pid/tgid to the struct taskstats.ac_pid,
> > then kernel will collect statistics for that pid/tgid.
> >
> > Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>
> > ---
> > Changes since v1:
> >  * use /proc/taskstats instead of /dev/taskstats
> >
> >  include/uapi/linux/taskstats.h |  5 +++
> >  kernel/taskstats.c             | 73 +++++++++++++++++++++++++++++++++-
> >  2 files changed, 77 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/taskstats.h b/include/uapi/linux/taskstats.h
> > index ccbd08709321..077eab84c77e 100644
> > --- a/include/uapi/linux/taskstats.h
> > +++ b/include/uapi/linux/taskstats.h
> > @@ -214,6 +214,11 @@ enum {
> >
> >  #define TASKSTATS_CMD_ATTR_MAX (__TASKSTATS_CMD_ATTR_MAX - 1)
> >
> > +/* ioctl command */
> > +#define TASKSTATS_IOC_ATTR_PID _IO('T', TASKSTATS_CMD_ATTR_PID)
> > +#define TASKSTATS_IOC_ATTR_TGID        _IO('T', TASKSTATS_CMD_ATTR_TGID)
> > +
> > +
> >  /* NETLINK_GENERIC related info */
> >
> >  #define TASKSTATS_GENL_NAME    "TASKSTATS"
> > diff --git a/kernel/taskstats.c b/kernel/taskstats.c
> > index a2802b6ff4bb..c0f9e2f2308b 100644
> > --- a/kernel/taskstats.c
> > +++ b/kernel/taskstats.c
> > @@ -21,6 +21,8 @@
> >  #include <net/genetlink.h>
> >  #include <linux/atomic.h>
> >  #include <linux/sched/cputime.h>
> > +#include <linux/proc_fs.h>
> > +#include <linux/uio.h>
> >
> >  /*
> >   * Maximum length of a cpumask that can be specified in
> > @@ -28,6 +30,10 @@
> >   */
> >  #define TASKSTATS_CPUMASK_MAXLEN       (100+6*NR_CPUS)
> >
> > +#ifdef CONFIG_PROC_FS
> > +#define PROC_TASKSTATS "taskstats"
> > +#endif
> > +
> >  static DEFINE_PER_CPU(__u32, taskstats_seqnum);
> >  static int family_registered;
> >  struct kmem_cache *taskstats_cache;
> > @@ -666,6 +672,60 @@ static struct genl_family family __ro_after_init = {
> >         .n_ops          = ARRAY_SIZE(taskstats_ops),
> >  };
> >
> > +#ifdef CONFIG_PROC_FS
> > +static long taskstats_ioctl_pid_tgid(unsigned long arg, bool tgid)
> > +{
> > +       struct taskstats kstat;
> > +       struct taskstats *ustat = (struct taskstats *)arg;
> > +       u32 id;
> > +       unsigned long offset = offsetof(struct taskstats, ac_pid);
> > +       long ret;
> > +
> > +       /* userspace set monitored pid/tgid to the field "ac_pid" */
> > +       if (unlikely(copy_from_user(&id, (void *)(arg + offset), sizeof(u32))))
> > +               return -EFAULT;
> > +
> > +       if (tgid)
> > +               ret = fill_stats_for_tgid(id, &kstat);
> > +       else
> > +               ret = fill_stats_for_pid(id, &kstat);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       if (unlikely(copy_to_user(ustat, &kstat, sizeof(struct taskstats))))
> > +               return -EFAULT;
> > +
> > +       return 0;
> > +}
> > +
> > +static long taskstats_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> > +{
> > +       long ret;
> > +
> > +       switch (cmd) {
> > +       case TASKSTATS_IOC_ATTR_PID:
> > +               ret = taskstats_ioctl_pid_tgid(arg, false);
> > +               break;
> > +       case TASKSTATS_IOC_ATTR_TGID:
> > +               ret = taskstats_ioctl_pid_tgid(arg, true);
> > +               break;
> > +       default:
> > +               ret = -EINVAL;
> > +               break;
> > +       }
> > +
> > +       return ret;
> > +}
> > +
> > +static const struct proc_ops taskstats_proc_ops = {
> > +       .proc_ioctl = taskstats_ioctl,
> > +#ifdef CONFIG_COMPAT
> > +       .proc_compat_ioctl = taskstats_ioctl,
> > +#endif
> > +};
> > +#endif
> > +
> > +
> >  /* Needed early in initialization */
> >  void __init taskstats_init_early(void)
> >  {
> > @@ -682,9 +742,20 @@ static int __init taskstats_init(void)
> >  {
> >         int rc;
> >
> > +#ifdef CONFIG_PROC_FS
> > +       if (!proc_create(PROC_TASKSTATS,  0, NULL, &taskstats_proc_ops)) {
> > +               pr_err("failed to create /proc/%s\n", PROC_TASKSTATS);
> > +               return -1;
> > +       }
> > +#endif
> > +
> >         rc = genl_register_family(&family);
> > -       if (rc)
> > +       if (rc) {
> > +#ifdef CONFIG_PROC_FS
> > +               remove_proc_entry(PROC_TASKSTATS, NULL);
> > +#endif
> >                 return rc;
> > +       }
> >
> >         family_registered = 1;
> >         pr_info("registered taskstats version %d\n", TASKSTATS_GENL_VERSION);
> > --
> > 2.17.2
> >

Re: [RFC PATCH v2] taskstats: add /proc/taskstats to fetch pid/tgid status

[Balbir Singh]

On Mon, Dec 28, 2020 at 10:10:03PM +0800, Weiping Zhang wrote:
> Hi David,
> 
> Could you help review this patch ?
> 
> thanks

I've got it on my review list, thanks for the ping!
You should hear back from me soon.

Balbir Singh.

> 
> On Fri, Dec 18, 2020 at 1:24 AM Weiping Zhang
> <zhangweiping@didiglobal.com> wrote:
> >
> > If a program needs monitor lots of process's status, it needs two
> > syscalls for every process. The first one is telling kernel which
> > pid/tgid should be monitored by send a command(write socket) to kernel.
> > The second one is read the statistics by read socket. This patch add
> > a new interface /proc/taskstats to reduce two syscalls to one ioctl.
> > The user just set the target pid/tgid to the struct taskstats.ac_pid,
> > then kernel will collect statistics for that pid/tgid.
> >
> > Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>
> > ---
> > Changes since v1:
> >  * use /proc/taskstats instead of /dev/taskstats
> >
> >  include/uapi/linux/taskstats.h |  5 +++
> >  kernel/taskstats.c             | 73 +++++++++++++++++++++++++++++++++-
> >  2 files changed, 77 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/taskstats.h b/include/uapi/linux/taskstats.h
> > index ccbd08709321..077eab84c77e 100644
> > --- a/include/uapi/linux/taskstats.h
> > +++ b/include/uapi/linux/taskstats.h
> > @@ -214,6 +214,11 @@ enum {
> >
> >  #define TASKSTATS_CMD_ATTR_MAX (__TASKSTATS_CMD_ATTR_MAX - 1)
> >
> > +/* ioctl command */
> > +#define TASKSTATS_IOC_ATTR_PID _IO('T', TASKSTATS_CMD_ATTR_PID)
> > +#define TASKSTATS_IOC_ATTR_TGID        _IO('T', TASKSTATS_CMD_ATTR_TGID)
> > +
> > +
> >  /* NETLINK_GENERIC related info */
> >
> >  #define TASKSTATS_GENL_NAME    "TASKSTATS"
> > diff --git a/kernel/taskstats.c b/kernel/taskstats.c
> > index a2802b6ff4bb..c0f9e2f2308b 100644
> > --- a/kernel/taskstats.c
> > +++ b/kernel/taskstats.c
> > @@ -21,6 +21,8 @@
> >  #include <net/genetlink.h>
> >  #include <linux/atomic.h>
> >  #include <linux/sched/cputime.h>
> > +#include <linux/proc_fs.h>
> > +#include <linux/uio.h>
> >
> >  /*
> >   * Maximum length of a cpumask that can be specified in
> > @@ -28,6 +30,10 @@
> >   */
> >  #define TASKSTATS_CPUMASK_MAXLEN       (100+6*NR_CPUS)
> >
> > +#ifdef CONFIG_PROC_FS
> > +#define PROC_TASKSTATS "taskstats"
> > +#endif
> > +
> >  static DEFINE_PER_CPU(__u32, taskstats_seqnum);
> >  static int family_registered;
> >  struct kmem_cache *taskstats_cache;
> > @@ -666,6 +672,60 @@ static struct genl_family family __ro_after_init = {
> >         .n_ops          = ARRAY_SIZE(taskstats_ops),
> >  };
> >
> > +#ifdef CONFIG_PROC_FS
> > +static long taskstats_ioctl_pid_tgid(unsigned long arg, bool tgid)
> > +{
> > +       struct taskstats kstat;
> > +       struct taskstats *ustat = (struct taskstats *)arg;
> > +       u32 id;
> > +       unsigned long offset = offsetof(struct taskstats, ac_pid);
> > +       long ret;
> > +
> > +       /* userspace set monitored pid/tgid to the field "ac_pid" */
> > +       if (unlikely(copy_from_user(&id, (void *)(arg + offset), sizeof(u32))))
> > +               return -EFAULT;
> > +
> > +       if (tgid)
> > +               ret = fill_stats_for_tgid(id, &kstat);
> > +       else
> > +               ret = fill_stats_for_pid(id, &kstat);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       if (unlikely(copy_to_user(ustat, &kstat, sizeof(struct taskstats))))
> > +               return -EFAULT;
> > +
> > +       return 0;
> > +}
> > +
> > +static long taskstats_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> > +{
> > +       long ret;
> > +
> > +       switch (cmd) {
> > +       case TASKSTATS_IOC_ATTR_PID:
> > +               ret = taskstats_ioctl_pid_tgid(arg, false);
> > +               break;
> > +       case TASKSTATS_IOC_ATTR_TGID:
> > +               ret = taskstats_ioctl_pid_tgid(arg, true);
> > +               break;
> > +       default:
> > +               ret = -EINVAL;
> > +               break;
> > +       }
> > +
> > +       return ret;
> > +}
> > +
> > +static const struct proc_ops taskstats_proc_ops = {
> > +       .proc_ioctl = taskstats_ioctl,
> > +#ifdef CONFIG_COMPAT
> > +       .proc_compat_ioctl = taskstats_ioctl,
> > +#endif
> > +};
> > +#endif
> > +
> > +
> >  /* Needed early in initialization */
> >  void __init taskstats_init_early(void)
> >  {
> > @@ -682,9 +742,20 @@ static int __init taskstats_init(void)
> >  {
> >         int rc;
> >
> > +#ifdef CONFIG_PROC_FS
> > +       if (!proc_create(PROC_TASKSTATS,  0, NULL, &taskstats_proc_ops)) {
> > +               pr_err("failed to create /proc/%s\n", PROC_TASKSTATS);
> > +               return -1;
> > +       }
> > +#endif
> > +
> >         rc = genl_register_family(&family);
> > -       if (rc)
> > +       if (rc) {
> > +#ifdef CONFIG_PROC_FS
> > +               remove_proc_entry(PROC_TASKSTATS, NULL);
> > +#endif
> >                 return rc;
> > +       }
> >
> >         family_registered = 1;
> >         pr_info("registered taskstats version %d\n", TASKSTATS_GENL_VERSION);
> > --
> > 2.17.2
> >

Re: [RFC PATCH v2] taskstats: add /proc/taskstats to fetch pid/tgid status

[Balbir Singh]

On Fri, Jan 22, 2021 at 10:07:50PM +0800, Weiping Zhang wrote:
> Hello Balbir Singh,
> 
> Could you help review this patch, thanks
> 
> On Mon, Dec 28, 2020 at 10:10 PM Weiping Zhang <zwp10758@gmail.com> wrote:
> >
> > Hi David,
> >
> > Could you help review this patch ?
> >
> > thanks
> >
> > On Fri, Dec 18, 2020 at 1:24 AM Weiping Zhang
> > <zhangweiping@didiglobal.com> wrote:
> > >
> > > If a program needs monitor lots of process's status, it needs two
> > > syscalls for every process. The first one is telling kernel which
> > > pid/tgid should be monitored by send a command(write socket) to kernel.
> > > The second one is read the statistics by read socket. This patch add
> > > a new interface /proc/taskstats to reduce two syscalls to one ioctl.
> > > The user just set the target pid/tgid to the struct taskstats.ac_pid,
> > > then kernel will collect statistics for that pid/tgid.
> > >
> > > Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>

Could you elaborate on the overhead your seeing for the syscalls? I am not
in favour of adding new IOCTL's.

Balbir Singh.

Re: [RFC PATCH v2] taskstats: add /proc/taskstats to fetch pid/tgid status

[Weiping Zhang]

On Wed, Jan 27, 2021 at 7:13 PM Balbir Singh <bsingharora@gmail.com> wrote:
>
> On Fri, Jan 22, 2021 at 10:07:50PM +0800, Weiping Zhang wrote:
> > Hello Balbir Singh,
> >
> > Could you help review this patch, thanks
> >
> > On Mon, Dec 28, 2020 at 10:10 PM Weiping Zhang <zwp10758@gmail.com> wrote:
> > >
> > > Hi David,
> > >
> > > Could you help review this patch ?
> > >
> > > thanks
> > >
> > > On Fri, Dec 18, 2020 at 1:24 AM Weiping Zhang
> > > <zhangweiping@didiglobal.com> wrote:
> > > >
> > > > If a program needs monitor lots of process's status, it needs two
> > > > syscalls for every process. The first one is telling kernel which
> > > > pid/tgid should be monitored by send a command(write socket) to kernel.
> > > > The second one is read the statistics by read socket. This patch add
> > > > a new interface /proc/taskstats to reduce two syscalls to one ioctl.
> > > > The user just set the target pid/tgid to the struct taskstats.ac_pid,
> > > > then kernel will collect statistics for that pid/tgid.
> > > >
> > > > Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>
>
> Could you elaborate on the overhead your seeing for the syscalls? I am not
> in favour of adding new IOCTL's.
>
> Balbir Singh.

Hello Balbir Singh,

Sorry for late reply,

I do a performance test between netlink mode and ioctl mode,
monitor 1000 and 10000 sleep processes,
the netlink mode cost more time than ioctl mode, that is to say
ioctl mode can save some cpu resource and has a quickly reponse
especially when monitor lot of process.

proccess-count    netlink         ioctl
---------------------------------------------
1000              0.004446851     0.001553733
10000             0.047024986     0.023290664

you can get the test demo code from the following link
https://github.com/dublio/tools/tree/master/c/taskstat

Thanks
Weiping

Re: [RFC PATCH v2] taskstats: add /proc/taskstats to fetch pid/tgid status

[Balbir Singh]

On Sun, Jan 31, 2021 at 05:16:47PM +0800, Weiping Zhang wrote:
> On Wed, Jan 27, 2021 at 7:13 PM Balbir Singh <bsingharora@gmail.com> wrote:
> >
> > On Fri, Jan 22, 2021 at 10:07:50PM +0800, Weiping Zhang wrote:
> > > Hello Balbir Singh,
> > >
> > > Could you help review this patch, thanks
> > >
> > > On Mon, Dec 28, 2020 at 10:10 PM Weiping Zhang <zwp10758@gmail.com> wrote:
> > > >
> > > > Hi David,
> > > >
> > > > Could you help review this patch ?
> > > >
> > > > thanks
> > > >
> > > > On Fri, Dec 18, 2020 at 1:24 AM Weiping Zhang
> > > > <zhangweiping@didiglobal.com> wrote:
> > > > >
> > > > > If a program needs monitor lots of process's status, it needs two
> > > > > syscalls for every process. The first one is telling kernel which
> > > > > pid/tgid should be monitored by send a command(write socket) to kernel.
> > > > > The second one is read the statistics by read socket. This patch add
> > > > > a new interface /proc/taskstats to reduce two syscalls to one ioctl.
> > > > > The user just set the target pid/tgid to the struct taskstats.ac_pid,
> > > > > then kernel will collect statistics for that pid/tgid.
> > > > >
> > > > > Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>
> >
> > Could you elaborate on the overhead your seeing for the syscalls? I am not
> > in favour of adding new IOCTL's.
> >
> > Balbir Singh.
> 
> Hello Balbir Singh,
> 
> Sorry for late reply,
> 
> I do a performance test between netlink mode and ioctl mode,
> monitor 1000 and 10000 sleep processes,
> the netlink mode cost more time than ioctl mode, that is to say
> ioctl mode can save some cpu resource and has a quickly reponse
> especially when monitor lot of process.
> 
> proccess-count    netlink         ioctl
> ---------------------------------------------
> 1000              0.004446851     0.001553733
> 10000             0.047024986     0.023290664
> 
> you can get the test demo code from the following link
> https://github.com/dublio/tools/tree/master/c/taskstat
>

Let me try it out, I am opposed to adding the new IOCTL interface
you propose. How frequently do you monitor this data and how much
time in spent in making decision on the data? I presume the data
mentioned is the cost per call in seconds?

Balbir Singh
 

Re: [RFC PATCH v2] taskstats: add /proc/taskstats to fetch pid/tgid status

[Weiping Zhang]

On Thu, Feb 4, 2021 at 6:20 PM Balbir Singh <bsingharora@gmail.com> wrote:
>
> On Sun, Jan 31, 2021 at 05:16:47PM +0800, Weiping Zhang wrote:
> > On Wed, Jan 27, 2021 at 7:13 PM Balbir Singh <bsingharora@gmail.com> wrote:
> > >
> > > On Fri, Jan 22, 2021 at 10:07:50PM +0800, Weiping Zhang wrote:
> > > > Hello Balbir Singh,
> > > >
> > > > Could you help review this patch, thanks
> > > >
> > > > On Mon, Dec 28, 2020 at 10:10 PM Weiping Zhang <zwp10758@gmail.com> wrote:
> > > > >
> > > > > Hi David,
> > > > >
> > > > > Could you help review this patch ?
> > > > >
> > > > > thanks
> > > > >
> > > > > On Fri, Dec 18, 2020 at 1:24 AM Weiping Zhang
> > > > > <zhangweiping@didiglobal.com> wrote:
> > > > > >
> > > > > > If a program needs monitor lots of process's status, it needs two
> > > > > > syscalls for every process. The first one is telling kernel which
> > > > > > pid/tgid should be monitored by send a command(write socket) to kernel.
> > > > > > The second one is read the statistics by read socket. This patch add
> > > > > > a new interface /proc/taskstats to reduce two syscalls to one ioctl.
> > > > > > The user just set the target pid/tgid to the struct taskstats.ac_pid,
> > > > > > then kernel will collect statistics for that pid/tgid.
> > > > > >
> > > > > > Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>
> > >
> > > Could you elaborate on the overhead your seeing for the syscalls? I am not
> > > in favour of adding new IOCTL's.
> > >
> > > Balbir Singh.
> >
> > Hello Balbir Singh,
> >
> > Sorry for late reply,
> >
> > I do a performance test between netlink mode and ioctl mode,
> > monitor 1000 and 10000 sleep processes,
> > the netlink mode cost more time than ioctl mode, that is to say
> > ioctl mode can save some cpu resource and has a quickly reponse
> > especially when monitor lot of process.
> >
> > proccess-count    netlink         ioctl
> > ---------------------------------------------
> > 1000              0.004446851     0.001553733
> > 10000             0.047024986     0.023290664
> >
> > you can get the test demo code from the following link
> > https://github.com/dublio/tools/tree/master/c/taskstat
> >
>
> Let me try it out, I am opposed to adding the new IOCTL interface
> you propose. How frequently do you monitor this data and how much
> time in spent in making decision on the data? I presume the data
> mentioned is the cost per call in seconds?
>
This program just read every process's taskstats from kernel and do not
any extra data calculation, that is to say it just test the time spend on
these syscalls. It read data every 1 second, the output is delta time spend to
read all 1000 or 10000 processes's taskstat.

t1 = clock_gettime();
for_each_pid /* 1000 or 10000 */
        read_pid_taskstat
t2 = clock_gettime();

delta = t2 - t1.

> > proccess-count    netlink         ioctl
> > ---------------------------------------------
> > 1000              0.004446851     0.001553733
> > 10000             0.047024986     0.023290664

Since netlink mode needs two syscall and ioctl mode needs one syscall
the test result shows  netlink cost double time compare to ioctl.
So I want to add this interface to reduce the time cost by syscall.

You can get the test script from:
https://github.com/dublio/tools/tree/master/c/taskstat#test-the-performance-between-netlink-and-ioctl-mode

Thanks

> Balbir Singh
>

Re: [RFC PATCH v2] taskstats: add /proc/taskstats to fetch pid/tgid status

[Balbir Singh]

On Thu, Feb 04, 2021 at 10:37:20PM +0800, Weiping Zhang wrote:
> On Thu, Feb 4, 2021 at 6:20 PM Balbir Singh <bsingharora@gmail.com> wrote:
> >
> > On Sun, Jan 31, 2021 at 05:16:47PM +0800, Weiping Zhang wrote:
> > > On Wed, Jan 27, 2021 at 7:13 PM Balbir Singh <bsingharora@gmail.com> wrote:
> > > >
> > > > On Fri, Jan 22, 2021 at 10:07:50PM +0800, Weiping Zhang wrote:
> > > > > Hello Balbir Singh,
> > > > >
> > > > > Could you help review this patch, thanks
> > > > >
> > > > > On Mon, Dec 28, 2020 at 10:10 PM Weiping Zhang <zwp10758@gmail.com> wrote:
> > > > > >
> > > > > > Hi David,
> > > > > >
> > > > > > Could you help review this patch ?
> > > > > >
> > > > > > thanks
> > > > > >
> > > > > > On Fri, Dec 18, 2020 at 1:24 AM Weiping Zhang
> > > > > > <zhangweiping@didiglobal.com> wrote:
> > > > > > >
> > > > > > > If a program needs monitor lots of process's status, it needs two
> > > > > > > syscalls for every process. The first one is telling kernel which
> > > > > > > pid/tgid should be monitored by send a command(write socket) to kernel.
> > > > > > > The second one is read the statistics by read socket. This patch add
> > > > > > > a new interface /proc/taskstats to reduce two syscalls to one ioctl.
> > > > > > > The user just set the target pid/tgid to the struct taskstats.ac_pid,
> > > > > > > then kernel will collect statistics for that pid/tgid.
> > > > > > >
> > > > > > > Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>
> > > >
> > > > Could you elaborate on the overhead your seeing for the syscalls? I am not
> > > > in favour of adding new IOCTL's.
> > > >
> > > > Balbir Singh.
> > >
> > > Hello Balbir Singh,
> > >
> > > Sorry for late reply,
> > >
> > > I do a performance test between netlink mode and ioctl mode,
> > > monitor 1000 and 10000 sleep processes,
> > > the netlink mode cost more time than ioctl mode, that is to say
> > > ioctl mode can save some cpu resource and has a quickly reponse
> > > especially when monitor lot of process.
> > >
> > > proccess-count    netlink         ioctl
> > > ---------------------------------------------
> > > 1000              0.004446851     0.001553733
> > > 10000             0.047024986     0.023290664
> > >
> > > you can get the test demo code from the following link
> > > https://github.com/dublio/tools/tree/master/c/taskstat
> > >
> >
> > Let me try it out, I am opposed to adding the new IOCTL interface
> > you propose. How frequently do you monitor this data and how much
> > time in spent in making decision on the data? I presume the data
> > mentioned is the cost per call in seconds?
> >
> This program just read every process's taskstats from kernel and do not
> any extra data calculation, that is to say it just test the time spend on
> these syscalls. It read data every 1 second, the output is delta time spend to
> read all 1000 or 10000 processes's taskstat.
> 
> t1 = clock_gettime();
> for_each_pid /* 1000 or 10000 */
>         read_pid_taskstat
> t2 = clock_gettime();
> 
> delta = t2 - t1.
> 
> > > proccess-count    netlink         ioctl
> > > ---------------------------------------------
> > > 1000              0.004446851     0.001553733
> > > 10000             0.047024986     0.023290664
> 
> Since netlink mode needs two syscall and ioctl mode needs one syscall
> the test result shows  netlink cost double time compare to ioctl.
> So I want to add this interface to reduce the time cost by syscall.
> 
> You can get the test script from:
> https://github.com/dublio/tools/tree/master/c/taskstat#test-the-performance-between-netlink-and-ioctl-mode
> 
> Thanks
>

Have you looked at the listener interface in taskstats, where one
can register to listen on a cpumask against all exiting processes?

That provides a register once and listen and filter interface (based
on pids/tgids returned) and lets the task be done on exit as opposed
to polling for data.

Balbir Singh. 

Re: [RFC PATCH v2] taskstats: add /proc/taskstats to fetch pid/tgid status

[Weiping Zhang]

On Fri, Feb 5, 2021 at 8:08 AM Balbir Singh <bsingharora@gmail.com> wrote:
>
> On Thu, Feb 04, 2021 at 10:37:20PM +0800, Weiping Zhang wrote:
> > On Thu, Feb 4, 2021 at 6:20 PM Balbir Singh <bsingharora@gmail.com> wrote:
> > >
> > > On Sun, Jan 31, 2021 at 05:16:47PM +0800, Weiping Zhang wrote:
> > > > On Wed, Jan 27, 2021 at 7:13 PM Balbir Singh <bsingharora@gmail.com> wrote:
> > > > >
> > > > > On Fri, Jan 22, 2021 at 10:07:50PM +0800, Weiping Zhang wrote:
> > > > > > Hello Balbir Singh,
> > > > > >
> > > > > > Could you help review this patch, thanks
> > > > > >
> > > > > > On Mon, Dec 28, 2020 at 10:10 PM Weiping Zhang <zwp10758@gmail.com> wrote:
> > > > > > >
> > > > > > > Hi David,
> > > > > > >
> > > > > > > Could you help review this patch ?
> > > > > > >
> > > > > > > thanks
> > > > > > >
> > > > > > > On Fri, Dec 18, 2020 at 1:24 AM Weiping Zhang
> > > > > > > <zhangweiping@didiglobal.com> wrote:
> > > > > > > >
> > > > > > > > If a program needs monitor lots of process's status, it needs two
> > > > > > > > syscalls for every process. The first one is telling kernel which
> > > > > > > > pid/tgid should be monitored by send a command(write socket) to kernel.
> > > > > > > > The second one is read the statistics by read socket. This patch add
> > > > > > > > a new interface /proc/taskstats to reduce two syscalls to one ioctl.
> > > > > > > > The user just set the target pid/tgid to the struct taskstats.ac_pid,
> > > > > > > > then kernel will collect statistics for that pid/tgid.
> > > > > > > >
> > > > > > > > Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>
> > > > >
> > > > > Could you elaborate on the overhead your seeing for the syscalls? I am not
> > > > > in favour of adding new IOCTL's.
> > > > >
> > > > > Balbir Singh.
> > > >
> > > > Hello Balbir Singh,
> > > >
> > > > Sorry for late reply,
> > > >
> > > > I do a performance test between netlink mode and ioctl mode,
> > > > monitor 1000 and 10000 sleep processes,
> > > > the netlink mode cost more time than ioctl mode, that is to say
> > > > ioctl mode can save some cpu resource and has a quickly reponse
> > > > especially when monitor lot of process.
> > > >
> > > > proccess-count    netlink         ioctl
> > > > ---------------------------------------------
> > > > 1000              0.004446851     0.001553733
> > > > 10000             0.047024986     0.023290664
> > > >
> > > > you can get the test demo code from the following link
> > > > https://github.com/dublio/tools/tree/master/c/taskstat
> > > >
> > >
> > > Let me try it out, I am opposed to adding the new IOCTL interface
> > > you propose. How frequently do you monitor this data and how much
> > > time in spent in making decision on the data? I presume the data
> > > mentioned is the cost per call in seconds?
> > >
> > This program just read every process's taskstats from kernel and do not
> > any extra data calculation, that is to say it just test the time spend on
> > these syscalls. It read data every 1 second, the output is delta time spend to
> > read all 1000 or 10000 processes's taskstat.
> >
> > t1 = clock_gettime();
> > for_each_pid /* 1000 or 10000 */
> >         read_pid_taskstat
> > t2 = clock_gettime();
> >
> > delta = t2 - t1.
> >
> > > > proccess-count    netlink         ioctl
> > > > ---------------------------------------------
> > > > 1000              0.004446851     0.001553733
> > > > 10000             0.047024986     0.023290664
> >
> > Since netlink mode needs two syscall and ioctl mode needs one syscall
> > the test result shows  netlink cost double time compare to ioctl.
> > So I want to add this interface to reduce the time cost by syscall.
> >
> > You can get the test script from:
> > https://github.com/dublio/tools/tree/master/c/taskstat#test-the-performance-between-netlink-and-ioctl-mode
> >
> > Thanks
> >
>
> Have you looked at the listener interface in taskstats, where one
> can register to listen on a cpumask against all exiting processes?
>
> That provides a register once and listen and filter interface (based
> on pids/tgids returned) and lets the task be done on exit as opposed
> to polling for data.
>
That is a good feature to collect data async mode, now I want to collect
those long-time running process's data in a fixed frequency, like iotop.
So I try to reduce the overhead cost by these syscalls when I polling
a lot of long-time running processes.

Thanks a ton

> Balbir Singh.

Re: [RFC PATCH v2] taskstats: add /proc/taskstats to fetch pid/tgid status

[Balbir Singh]

On Fri, Feb 05, 2021 at 10:43:02AM +0800, Weiping Zhang wrote:
> On Fri, Feb 5, 2021 at 8:08 AM Balbir Singh <bsingharora@gmail.com> wrote:
> >
> > On Thu, Feb 04, 2021 at 10:37:20PM +0800, Weiping Zhang wrote:
> > > On Thu, Feb 4, 2021 at 6:20 PM Balbir Singh <bsingharora@gmail.com> wrote:
> > > >
> > > > On Sun, Jan 31, 2021 at 05:16:47PM +0800, Weiping Zhang wrote:
> > > > > On Wed, Jan 27, 2021 at 7:13 PM Balbir Singh <bsingharora@gmail.com> wrote:
> > > > > >
> > > > > > On Fri, Jan 22, 2021 at 10:07:50PM +0800, Weiping Zhang wrote:
> > > > > > > Hello Balbir Singh,
> > > > > > >
> > > > > > > Could you help review this patch, thanks
> > > > > > >
> > > > > > > On Mon, Dec 28, 2020 at 10:10 PM Weiping Zhang <zwp10758@gmail.com> wrote:
> > > > > > > >
> > > > > > > > Hi David,
> > > > > > > >
> > > > > > > > Could you help review this patch ?
> > > > > > > >
> > > > > > > > thanks
> > > > > > > >
> > > > > > > > On Fri, Dec 18, 2020 at 1:24 AM Weiping Zhang
> > > > > > > > <zhangweiping@didiglobal.com> wrote:
> > > > > > > > >
> > > > > > > > > If a program needs monitor lots of process's status, it needs two
> > > > > > > > > syscalls for every process. The first one is telling kernel which
> > > > > > > > > pid/tgid should be monitored by send a command(write socket) to kernel.
> > > > > > > > > The second one is read the statistics by read socket. This patch add
> > > > > > > > > a new interface /proc/taskstats to reduce two syscalls to one ioctl.
> > > > > > > > > The user just set the target pid/tgid to the struct taskstats.ac_pid,
> > > > > > > > > then kernel will collect statistics for that pid/tgid.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>
> > > > > >
> > > > > > Could you elaborate on the overhead your seeing for the syscalls? I am not
> > > > > > in favour of adding new IOCTL's.
> > > > > >
> > > > > > Balbir Singh.
> > > > >
> > > > > Hello Balbir Singh,
> > > > >
> > > > > Sorry for late reply,
> > > > >
> > > > > I do a performance test between netlink mode and ioctl mode,
> > > > > monitor 1000 and 10000 sleep processes,
> > > > > the netlink mode cost more time than ioctl mode, that is to say
> > > > > ioctl mode can save some cpu resource and has a quickly reponse
> > > > > especially when monitor lot of process.
> > > > >
> > > > > proccess-count    netlink         ioctl
> > > > > ---------------------------------------------
> > > > > 1000              0.004446851     0.001553733
> > > > > 10000             0.047024986     0.023290664
> > > > >
> > > > > you can get the test demo code from the following link
> > > > > https://github.com/dublio/tools/tree/master/c/taskstat
> > > > >
> > > >
> > > > Let me try it out, I am opposed to adding the new IOCTL interface
> > > > you propose. How frequently do you monitor this data and how much
> > > > time in spent in making decision on the data? I presume the data
> > > > mentioned is the cost per call in seconds?
> > > >
> > > This program just read every process's taskstats from kernel and do not
> > > any extra data calculation, that is to say it just test the time spend on
> > > these syscalls. It read data every 1 second, the output is delta time spend to
> > > read all 1000 or 10000 processes's taskstat.
> > >
> > > t1 = clock_gettime();
> > > for_each_pid /* 1000 or 10000 */
> > >         read_pid_taskstat
> > > t2 = clock_gettime();
> > >
> > > delta = t2 - t1.
> > >
> > > > > proccess-count    netlink         ioctl
> > > > > ---------------------------------------------
> > > > > 1000              0.004446851     0.001553733
> > > > > 10000             0.047024986     0.023290664
> > >
> > > Since netlink mode needs two syscall and ioctl mode needs one syscall
> > > the test result shows  netlink cost double time compare to ioctl.
> > > So I want to add this interface to reduce the time cost by syscall.
> > >
> > > You can get the test script from:
> > > https://github.com/dublio/tools/tree/master/c/taskstat#test-the-performance-between-netlink-and-ioctl-mode
> > >
> > > Thanks
> > >
> >
> > Have you looked at the listener interface in taskstats, where one
> > can register to listen on a cpumask against all exiting processes?
> >
> > That provides a register once and listen and filter interface (based
> > on pids/tgids returned) and lets the task be done on exit as opposed
> > to polling for data.
> >
> That is a good feature to collect data async mode, now I want to collect
> those long-time running process's data in a fixed frequency, like iotop.
> So I try to reduce the overhead cost by these syscalls when I polling
> a lot of long-time running processes.
> 
> Thanks a ton

Still not convinced about it, I played around with it. The reason we did not
use ioctl in the first place is to get the benefits of TLA with netlink, which
ioctl's miss. IMHO, the overhead is not very significant even for
10,000 processes in your experiment. I am open to considering enhancing the
interface to do a set of pid's

Balbir Singh.

Re: [RFC PATCH v2] taskstats: add /proc/taskstats to fetch pid/tgid status

[Weiping Zhang]

On Mon, Feb 8, 2021 at 1:55 PM Balbir Singh <bsingharora@gmail.com> wrote:
>
> On Fri, Feb 05, 2021 at 10:43:02AM +0800, Weiping Zhang wrote:
> > On Fri, Feb 5, 2021 at 8:08 AM Balbir Singh <bsingharora@gmail.com> wrote:
> > >
> > > On Thu, Feb 04, 2021 at 10:37:20PM +0800, Weiping Zhang wrote:
> > > > On Thu, Feb 4, 2021 at 6:20 PM Balbir Singh <bsingharora@gmail.com> wrote:
> > > > >
> > > > > On Sun, Jan 31, 2021 at 05:16:47PM +0800, Weiping Zhang wrote:
> > > > > > On Wed, Jan 27, 2021 at 7:13 PM Balbir Singh <bsingharora@gmail.com> wrote:
> > > > > > >
> > > > > > > On Fri, Jan 22, 2021 at 10:07:50PM +0800, Weiping Zhang wrote:
> > > > > > > > Hello Balbir Singh,
> > > > > > > >
> > > > > > > > Could you help review this patch, thanks
> > > > > > > >
> > > > > > > > On Mon, Dec 28, 2020 at 10:10 PM Weiping Zhang <zwp10758@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > Hi David,
> > > > > > > > >
> > > > > > > > > Could you help review this patch ?
> > > > > > > > >
> > > > > > > > > thanks
> > > > > > > > >
> > > > > > > > > On Fri, Dec 18, 2020 at 1:24 AM Weiping Zhang
> > > > > > > > > <zhangweiping@didiglobal.com> wrote:
> > > > > > > > > >
> > > > > > > > > > If a program needs monitor lots of process's status, it needs two
> > > > > > > > > > syscalls for every process. The first one is telling kernel which
> > > > > > > > > > pid/tgid should be monitored by send a command(write socket) to kernel.
> > > > > > > > > > The second one is read the statistics by read socket. This patch add
> > > > > > > > > > a new interface /proc/taskstats to reduce two syscalls to one ioctl.
> > > > > > > > > > The user just set the target pid/tgid to the struct taskstats.ac_pid,
> > > > > > > > > > then kernel will collect statistics for that pid/tgid.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>
> > > > > > >
> > > > > > > Could you elaborate on the overhead your seeing for the syscalls? I am not
> > > > > > > in favour of adding new IOCTL's.
> > > > > > >
> > > > > > > Balbir Singh.
> > > > > >
> > > > > > Hello Balbir Singh,
> > > > > >
> > > > > > Sorry for late reply,
> > > > > >
> > > > > > I do a performance test between netlink mode and ioctl mode,
> > > > > > monitor 1000 and 10000 sleep processes,
> > > > > > the netlink mode cost more time than ioctl mode, that is to say
> > > > > > ioctl mode can save some cpu resource and has a quickly reponse
> > > > > > especially when monitor lot of process.
> > > > > >
> > > > > > proccess-count    netlink         ioctl
> > > > > > ---------------------------------------------
> > > > > > 1000              0.004446851     0.001553733
> > > > > > 10000             0.047024986     0.023290664
> > > > > >
> > > > > > you can get the test demo code from the following link
> > > > > > https://github.com/dublio/tools/tree/master/c/taskstat
> > > > > >
> > > > >
> > > > > Let me try it out, I am opposed to adding the new IOCTL interface
> > > > > you propose. How frequently do you monitor this data and how much
> > > > > time in spent in making decision on the data? I presume the data
> > > > > mentioned is the cost per call in seconds?
> > > > >
> > > > This program just read every process's taskstats from kernel and do not
> > > > any extra data calculation, that is to say it just test the time spend on
> > > > these syscalls. It read data every 1 second, the output is delta time spend to
> > > > read all 1000 or 10000 processes's taskstat.
> > > >
> > > > t1 = clock_gettime();
> > > > for_each_pid /* 1000 or 10000 */
> > > >         read_pid_taskstat
> > > > t2 = clock_gettime();
> > > >
> > > > delta = t2 - t1.
> > > >
> > > > > > proccess-count    netlink         ioctl
> > > > > > ---------------------------------------------
> > > > > > 1000              0.004446851     0.001553733
> > > > > > 10000             0.047024986     0.023290664
> > > >
> > > > Since netlink mode needs two syscall and ioctl mode needs one syscall
> > > > the test result shows  netlink cost double time compare to ioctl.
> > > > So I want to add this interface to reduce the time cost by syscall.
> > > >
> > > > You can get the test script from:
> > > > https://github.com/dublio/tools/tree/master/c/taskstat#test-the-performance-between-netlink-and-ioctl-mode
> > > >
> > > > Thanks
> > > >
> > >
> > > Have you looked at the listener interface in taskstats, where one
> > > can register to listen on a cpumask against all exiting processes?
> > >
> > > That provides a register once and listen and filter interface (based
> > > on pids/tgids returned) and lets the task be done on exit as opposed
> > > to polling for data.
> > >
> > That is a good feature to collect data async mode, now I want to collect
> > those long-time running process's data in a fixed frequency, like iotop.
> > So I try to reduce the overhead cost by these syscalls when I polling
> > a lot of long-time running processes.
> >
> > Thanks a ton
>
> Still not convinced about it, I played around with it. The reason we did not
> use ioctl in the first place is to get the benefits of TLA with netlink, which
For monitoring long-time-running process the ioctl can meet our requirement,
it is more simple than netlink when we get the real user data(struct taskstats).
The netlink mode needs construct/parse extra strcutures like struct msgtemplate,
struct nlmsghdr, struct genlmsghdr. The ioctl mode only has one
structure (struct taskstats).
For complicated user case the netlink mode is more suitable, for this
simple user case
the ioctl mode is more suitable. From the test results we can see that
ioctl can save CPU
resource, it's useful to build a light-weight monitor tools.
> ioctl's miss. IMHO, the overhead is not very significant even for
> 10,000 processes in your experiment. I am open to considering enhancing the
> interface to do a set of pid's.
It's a good approach to collect data in batch mode, I think we can support it in
both netlink and ioctl mode.

Add ioctl can give user mode choice and make user code more simple, it seems no
harm to taskstats framework, I'd like to support it.

Thanks very much
>
> Balbir Singh.

Re: [RFC PATCH v2] taskstats: add /proc/taskstats to fetch pid/tgid status

[Balbir Singh]

> > Still not convinced about it, I played around with it. The reason we did not
> > use ioctl in the first place is to get the benefits of TLA with netlink, which
> For monitoring long-time-running process the ioctl can meet our requirement,
> it is more simple than netlink when we get the real user data(struct taskstats).
> The netlink mode needs construct/parse extra strcutures like struct msgtemplate,
> struct nlmsghdr, struct genlmsghdr. The ioctl mode only has one
> structure (struct taskstats).
> For complicated user case the netlink mode is more suitable, for this
> simple user case
> the ioctl mode is more suitable. From the test results we can see that
> ioctl can save CPU
> resource, it's useful to build a light-weight monitor tools.

I think your missing the value of TLA and the advantages of async
send vs recv

> > ioctl's miss. IMHO, the overhead is not very significant even for
> > 10,000 processes in your experiment. I am open to considering enhancing the
> > interface to do a set of pid's.
> It's a good approach to collect data in batch mode, I think we can support it in
> both netlink and ioctl mode.
> 
> Add ioctl can give user mode choice and make user code more simple, it seems no
> harm to taskstats framework, I'd like to support it.
> 
> Thanks very much

In general the ioctl interface is quite fragile, conflicts in ioctl numbers,
inability to check the types of the parameters passed in and out makes it
not so good. Not to mention versioning issues, with the genl interface we have
the flexibility to version requests. I would really hate to have two ways to
do the same thing.

The overhead is there, do you see the overhead of 20ms per 10,000 calls significant?
Does it affect your use case significantly?

Balbir Singh