* [RFC PATCH v2] taskstats: add /proc/taskstats to fetch pid/tgid status @ 2020-12-17 17:00 Weiping Zhang 2020-12-28 14:10 ` Weiping Zhang 0 siblings, 1 reply; 13+ messages in thread From: Weiping Zhang @ 2020-12-17 17:00 UTC (permalink / raw) To: bsingharora; +Cc: linux-kernel 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v2] taskstats: add /proc/taskstats to fetch pid/tgid status 2020-12-17 17:00 [RFC PATCH v2] taskstats: add /proc/taskstats to fetch pid/tgid status Weiping Zhang @ 2020-12-28 14:10 ` Weiping Zhang 2021-01-22 14:07 ` Weiping Zhang 2021-01-27 11:07 ` Balbir Singh 0 siblings, 2 replies; 13+ messages in thread From: Weiping Zhang @ 2020-12-28 14:10 UTC (permalink / raw) To: davem, bsingharora, Linux Kernel Mailing List 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 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v2] taskstats: add /proc/taskstats to fetch pid/tgid status 2020-12-28 14:10 ` Weiping Zhang @ 2021-01-22 14:07 ` Weiping Zhang 2021-01-27 11:13 ` Balbir Singh 2021-01-27 11:07 ` Balbir Singh 1 sibling, 1 reply; 13+ messages in thread From: Weiping Zhang @ 2021-01-22 14:07 UTC (permalink / raw) To: sblbir, davem, bsingharora, Linux Kernel Mailing List 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 > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v2] taskstats: add /proc/taskstats to fetch pid/tgid status 2021-01-22 14:07 ` Weiping Zhang @ 2021-01-27 11:13 ` Balbir Singh 2021-01-31 9:16 ` Weiping Zhang 0 siblings, 1 reply; 13+ messages in thread From: Balbir Singh @ 2021-01-27 11:13 UTC (permalink / raw) To: Weiping Zhang; +Cc: sblbir, davem, Linux Kernel Mailing List 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v2] taskstats: add /proc/taskstats to fetch pid/tgid status 2021-01-27 11:13 ` Balbir Singh @ 2021-01-31 9:16 ` Weiping Zhang 2021-02-04 10:20 ` Balbir Singh 0 siblings, 1 reply; 13+ messages in thread From: Weiping Zhang @ 2021-01-31 9:16 UTC (permalink / raw) To: Balbir Singh; +Cc: sblbir, davem, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v2] taskstats: add /proc/taskstats to fetch pid/tgid status 2021-01-31 9:16 ` Weiping Zhang @ 2021-02-04 10:20 ` Balbir Singh 2021-02-04 14:37 ` Weiping Zhang 0 siblings, 1 reply; 13+ messages in thread From: Balbir Singh @ 2021-02-04 10:20 UTC (permalink / raw) To: Weiping Zhang; +Cc: sblbir, davem, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v2] taskstats: add /proc/taskstats to fetch pid/tgid status 2021-02-04 10:20 ` Balbir Singh @ 2021-02-04 14:37 ` Weiping Zhang 2021-02-05 0:08 ` Balbir Singh 0 siblings, 1 reply; 13+ messages in thread From: Weiping Zhang @ 2021-02-04 14:37 UTC (permalink / raw) To: Balbir Singh; +Cc: sblbir, davem, Linux Kernel Mailing List 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 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v2] taskstats: add /proc/taskstats to fetch pid/tgid status 2021-02-04 14:37 ` Weiping Zhang @ 2021-02-05 0:08 ` Balbir Singh 2021-02-05 2:43 ` Weiping Zhang 0 siblings, 1 reply; 13+ messages in thread From: Balbir Singh @ 2021-02-05 0:08 UTC (permalink / raw) To: Weiping Zhang; +Cc: sblbir, davem, Linux Kernel Mailing List 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v2] taskstats: add /proc/taskstats to fetch pid/tgid status 2021-02-05 0:08 ` Balbir Singh @ 2021-02-05 2:43 ` Weiping Zhang 2021-02-08 5:55 ` Balbir Singh 0 siblings, 1 reply; 13+ messages in thread From: Weiping Zhang @ 2021-02-05 2:43 UTC (permalink / raw) To: Balbir Singh; +Cc: sblbir, davem, Linux Kernel Mailing List 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v2] taskstats: add /proc/taskstats to fetch pid/tgid status 2021-02-05 2:43 ` Weiping Zhang @ 2021-02-08 5:55 ` Balbir Singh 2021-02-10 12:30 ` Weiping Zhang 0 siblings, 1 reply; 13+ messages in thread From: Balbir Singh @ 2021-02-08 5:55 UTC (permalink / raw) To: Weiping Zhang; +Cc: sblbir, davem, Linux Kernel Mailing List 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v2] taskstats: add /proc/taskstats to fetch pid/tgid status 2021-02-08 5:55 ` Balbir Singh @ 2021-02-10 12:30 ` Weiping Zhang 2021-02-11 2:35 ` Balbir Singh 0 siblings, 1 reply; 13+ messages in thread From: Weiping Zhang @ 2021-02-10 12:30 UTC (permalink / raw) To: Balbir Singh; +Cc: sblbir, davem, Linux Kernel Mailing List 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v2] taskstats: add /proc/taskstats to fetch pid/tgid status 2021-02-10 12:30 ` Weiping Zhang @ 2021-02-11 2:35 ` Balbir Singh 0 siblings, 0 replies; 13+ messages in thread From: Balbir Singh @ 2021-02-11 2:35 UTC (permalink / raw) To: Weiping Zhang; +Cc: sblbir, davem, Linux Kernel Mailing List > > 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH v2] taskstats: add /proc/taskstats to fetch pid/tgid status 2020-12-28 14:10 ` Weiping Zhang 2021-01-22 14:07 ` Weiping Zhang @ 2021-01-27 11:07 ` Balbir Singh 1 sibling, 0 replies; 13+ messages in thread From: Balbir Singh @ 2021-01-27 11:07 UTC (permalink / raw) To: Weiping Zhang; +Cc: davem, Linux Kernel Mailing List 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 > > ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-02-11 2:36 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-17 17:00 [RFC PATCH v2] taskstats: add /proc/taskstats to fetch pid/tgid status Weiping Zhang 2020-12-28 14:10 ` Weiping Zhang 2021-01-22 14:07 ` Weiping Zhang 2021-01-27 11:13 ` Balbir Singh 2021-01-31 9:16 ` Weiping Zhang 2021-02-04 10:20 ` Balbir Singh 2021-02-04 14:37 ` Weiping Zhang 2021-02-05 0:08 ` Balbir Singh 2021-02-05 2:43 ` Weiping Zhang 2021-02-08 5:55 ` Balbir Singh 2021-02-10 12:30 ` Weiping Zhang 2021-02-11 2:35 ` Balbir Singh 2021-01-27 11:07 ` Balbir Singh
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).