From: Marcelo Tosatti <mtosatti@redhat.com>
To: Nitesh Lal <nilal@redhat.com>
Cc: linux-kernel@vger.kernel.org,
Nicolas Saenz Julienne <nsaenzju@redhat.com>,
Frederic Weisbecker <frederic@kernel.org>,
Christoph Lameter <cl@linux.com>,
Juri Lelli <juri.lelli@redhat.com>,
Peter Zijlstra <peterz@infradead.org>,
Alex Belits <abelits@belits.com>, Peter Xu <peterx@redhat.com>
Subject: Re: [patch 1/4] add basic task isolation prctl interface
Date: Mon, 2 Aug 2021 11:16:11 -0300 [thread overview]
Message-ID: <20210802141611.GA40008@fuller.cnet> (raw)
In-Reply-To: <CAFki+LkQVQOe+5aNEKWDvLdnjWjxzKWOiqOvBZzeuPWX+G=XgA@mail.gmail.com>
On Mon, Aug 02, 2021 at 10:02:03AM -0400, Nitesh Lal wrote:
> On Fri, Jul 30, 2021 at 4:21 PM Marcelo Tosatti <mtosatti@redhat.com> wrote:
>
> > Add basic prctl task isolation interface, which allows
> > informing the kernel that application is executing
> > latency sensitive code (where interruptions are undesired).
> >
> > Interface is described by task_isolation.rst (added by this patch).
> >
> > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> >
> >
> [...]
>
> +extern void __tsk_isol_exit(struct task_struct *tsk);
> > +
> > +static inline void tsk_isol_exit(struct task_struct *tsk)
> > +{
> > + if (tsk->isol_info)
> > + __tsk_isol_exit(tsk);
> > +}
> > +
> > +
> >
>
> nit: we can get rid of this extra line.
>
>
> > +int prctl_task_isolation_feat(unsigned long arg2, unsigned long arg3,
> > + unsigned long arg4, unsigned long arg5);
> > +int prctl_task_isolation_get(unsigned long arg2, unsigned long arg3,
> > + unsigned long arg4, unsigned long arg5);
> > +int prctl_task_isolation_set(unsigned long arg2, unsigned long arg3,
> > + unsigned long arg4, unsigned long arg5);
> > +int prctl_task_isolation_ctrl_get(unsigned long arg2, unsigned long arg3,
> > + unsigned long arg4, unsigned long arg5);
> > +int prctl_task_isolation_ctrl_set(unsigned long arg2, unsigned long arg3,
> > + unsigned long arg4, unsigned long arg5);
> > +
> > +#else
> > +
> > +static inline void tsk_isol_exit(struct task_struct *tsk)
> > +{
> > +}
> > +
> > +static inline int prctl_task_isolation_feat(unsigned long arg2,
> > + unsigned long arg3,
> > + unsigned long arg4,
> > + unsigned long arg5)
> > +{
> > + return -EOPNOTSUPP;
> > +}
> > +
> > +static inline int prctl_task_isolation_get(unsigned long arg2,
> > + unsigned long arg3,
> > + unsigned long arg4,
> > + unsigned long arg5)
> > +{
> > + return -EOPNOTSUPP;
> > +}
> > +
> > +static inline int prctl_task_isolation_set(unsigned long arg2,
> > + unsigned long arg3,
> > + unsigned long arg4,
> > + unsigned long arg5)
> > +{
> > + return -EOPNOTSUPP;
> > +}
> > +
> > +static inline int prctl_task_isolation_ctrl_get(unsigned long arg2,
> > + unsigned long arg3,
> > + unsigned long arg4,
> > + unsigned long arg5)
> > +{
> > + return -EOPNOTSUPP;
> > +}
> > +
> > +static inline int prctl_task_isolation_ctrl_set(unsigned long arg2,
> > + unsigned long arg3,
> > + unsigned long arg4,
> > + unsigned long arg5)
> > +{
> > + return -EOPNOTSUPP;
> > +}
> > +
> > +#endif /* CONFIG_CPU_ISOLATION */
> > +
> > +#endif /* __LINUX_TASK_ISOL_H */
> > Index: linux-2.6/kernel/task_isolation.c
> > ===================================================================
> > --- /dev/null
> > +++ linux-2.6/kernel/task_isolation.c
> > @@ -0,0 +1,274 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Implementation of task isolation.
> > + *
> > + * Authors:
> > + * Chris Metcalf <cmetcalf@mellanox.com>
> > + * Alex Belits <abelits@belits.com>
> > + * Yuri Norov <ynorov@marvell.com>
> > + * Marcelo Tosatti <mtosatti@redhat.com>
> > + */
> > +
> > +#include <linux/sched.h>
> > +#include <linux/task_isolation.h>
> > +#include <linux/prctl.h>
> > +#include <linux/slab.h>
> > +#include <linux/kobject.h>
> > +#include <linux/string.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/init.h>
> > +
> > +static unsigned long default_quiesce_mask;
> > +
> > +static int tsk_isol_alloc_context(struct task_struct *task)
> > +{
> > + struct isol_info *info;
> > +
> > + info = kzalloc(sizeof(*info), GFP_KERNEL);
> > + if (unlikely(!info))
> > + return -ENOMEM;
> > +
> > + task->isol_info = info;
> > + return 0;
> > +}
> > +
> > +void __tsk_isol_exit(struct task_struct *tsk)
> > +{
> > + kfree(tsk->isol_info);
> > + tsk->isol_info = NULL;
> > +}
> > +
> > +static int prctl_task_isolation_feat_quiesce(unsigned long type)
> > +{
> > + switch (type) {
> > + case 0:
> > + return ISOL_F_QUIESCE_VMSTATS;
> > + case ISOL_F_QUIESCE_DEFMASK:
> > + return default_quiesce_mask;
> > + default:
> > + break;
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static int task_isolation_get_quiesce(void)
> > +{
> > + if (current->isol_info != NULL)
> >
>
> Should replace the above with just 'if (current->isol_info)'.
>
> + return current->isol_info->quiesce_mask;
> > +
> > + return 0;
> > +}
> > +
> > +static int task_isolation_set_quiesce(unsigned long quiesce_mask)
> > +{
> > + if (quiesce_mask != ISOL_F_QUIESCE_VMSTATS && quiesce_mask != 0)
> > + return -EINVAL;
> > +
> > + current->isol_info->quiesce_mask = quiesce_mask;
> > + return 0;
> > +}
> > +
> > +int prctl_task_isolation_feat(unsigned long feat, unsigned long arg3,
> > + unsigned long arg4, unsigned long arg5)
> > +{
> > + switch (feat) {
> > + case 0:
> > + return ISOL_F_QUIESCE;
> > + case ISOL_F_QUIESCE:
> > + return prctl_task_isolation_feat_quiesce(arg3);
> > + default:
> > + break;
> > + }
> > + return -EINVAL;
> > +}
> > +
> > +int prctl_task_isolation_get(unsigned long feat, unsigned long arg3,
> > + unsigned long arg4, unsigned long arg5)
> > +{
> > + switch (feat) {
> > + case ISOL_F_QUIESCE:
> > + return task_isolation_get_quiesce();
> > + default:
> > + break;
> > + }
> > + return -EINVAL;
> > +}
> > +
> > +int prctl_task_isolation_set(unsigned long feat, unsigned long arg3,
> > + unsigned long arg4, unsigned long arg5)
> > +{
> > + int ret;
> > + bool err_free_ctx = false;
> > +
> > + if (current->isol_info == NULL)
> >
>
> Can replace this with 'if (!current->isol_info).
> There are other places below where similar improvement can be done.
>
>
> > + err_free_ctx = true;
> > +
> > + ret = tsk_isol_alloc_context(current);
> > + if (ret)
> > + return ret;
> > +
> > + switch (feat) {
> > + case ISOL_F_QUIESCE:
> > + ret = task_isolation_set_quiesce(arg3);
> > + if (ret)
> > + break;
> > + return 0;
> > + default:
> > + break;
> > + }
> > +
> > + if (err_free_ctx)
> > + __tsk_isol_exit(current);
> > + return -EINVAL;
> > +}
> > +
> > +int prctl_task_isolation_ctrl_set(unsigned long feat, unsigned long arg3,
> > + unsigned long arg4, unsigned long arg5)
> > +{
> > + if (current->isol_info == NULL)
> > + return -EINVAL;
> > +
> > + if (feat != ISOL_F_QUIESCE && feat != 0)
> > + return -EINVAL;
> > +
> > + current->isol_info->active_mask = feat;
> > + return 0;
> > +}
> > +
> > +int prctl_task_isolation_ctrl_get(unsigned long arg2, unsigned long arg3,
> > + unsigned long arg4, unsigned long arg5)
> > +{
> > + if (current->isol_info == NULL)
> > + return 0;
> > +
> > + return current->isol_info->active_mask;
> > +}
> > +
> > +struct qoptions {
> > + unsigned long mask;
> > + char *name;
> > +};
> > +
> > +static struct qoptions qopts[] = {
> > + {ISOL_F_QUIESCE_VMSTATS, "vmstat"},
> > +};
> > +
> > +#define QLEN (sizeof(qopts) / sizeof(struct qoptions))
> > +
> > +static ssize_t default_quiesce_store(struct kobject *kobj,
> > + struct kobj_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + char *p, *s;
> > + unsigned long defmask = 0;
> > +
> > + s = (char *)buf;
> > + if (count == 1 && strlen(strim(s)) == 0) {
> > + default_quiesce_mask = 0;
> > + return count;
> > + }
> > +
> > + while ((p = strsep(&s, ",")) != NULL) {
> > + int i;
> > + bool found = false;
> > +
> > + if (!*p)
> > + continue;
> > +
> > + for (i = 0; i < QLEN; i++) {
> > + struct qoptions *opt = &qopts[i];
> > +
> > + if (strncmp(strim(p), opt->name,
> > strlen(opt->name)) == 0) {
> > + defmask |= opt->mask;
> > + found = true;
> > + break;
> > + }
> > + }
> > + if (found == true)
> > + continue;
> > + return -EINVAL;
> > + }
> > + default_quiesce_mask = defmask;
> > +
> > + return count;
> > +}
> > +
> > +#define MAXARRLEN 100
> > +
> > +static ssize_t default_quiesce_show(struct kobject *kobj,
> > + struct kobj_attribute *attr, char *buf)
> > +{
> > + int i;
> > + char tbuf[MAXARRLEN] = "";
> > +
> > + for (i = 0; i < QLEN; i++) {
> > + struct qoptions *opt = &qopts[i];
> > +
> > + if (default_quiesce_mask & opt->mask) {
> > + strlcat(tbuf, opt->name, MAXARRLEN);
> > + strlcat(tbuf, "\n", MAXARRLEN);
> > + }
> > + }
> > +
> > + return sprintf(buf, "%s", tbuf);
> > +}
> > +
> > +static struct kobj_attribute default_quiesce_attr =
> > + __ATTR_RW(default_quiesce);
> > +
> > +static ssize_t available_quiesce_show(struct kobject *kobj,
> > + struct kobj_attribute *attr, char
> > *buf)
> > +{
> > + int i;
> > + char tbuf[MAXARRLEN] = "";
> > +
> > + for (i = 0; i < QLEN; i++) {
> > + struct qoptions *opt = &qopts[i];
> > +
> > + strlcat(tbuf, opt->name, MAXARRLEN);
> > + strlcat(tbuf, "\n", MAXARRLEN);
> > + }
> > +
> > + return sprintf(buf, "%s", tbuf);
> > +}
> > +
> > +static struct kobj_attribute available_quiesce_attr =
> > + __ATTR_RO(available_quiesce);
> > +
> > +static struct attribute *task_isol_attrs[] = {
> > + &available_quiesce_attr.attr,
> > + &default_quiesce_attr.attr,
> > + NULL,
> > +};
> > +
> > +static const struct attribute_group task_isol_attr_group = {
> > + .attrs = task_isol_attrs,
> > + .bin_attrs = NULL,
> > +};
> > +
> > +static int __init task_isol_ksysfs_init(void)
> > +{
> > + int ret;
> > + struct kobject *task_isol_kobj;
> > +
> > + task_isol_kobj = kobject_create_and_add("task_isolation",
> > + kernel_kobj);
> > + if (!task_isol_kobj) {
> > + ret = -ENOMEM;
> > + goto out;
> > + }
> > +
> > + ret = sysfs_create_group(task_isol_kobj, &task_isol_attr_group);
> > + if (ret)
> > + goto out_task_isol_kobj;
> > +
> > + return 0;
> > +
> > +out_task_isol_kobj:
> > + kobject_put(task_isol_kobj);
> > +out:
> > + return ret;
> > +}
> > +
> > +arch_initcall(task_isol_ksysfs_init);
> > Index: linux-2.6/samples/Kconfig
> > ===================================================================
> > --- linux-2.6.orig/samples/Kconfig
> > +++ linux-2.6/samples/Kconfig
> > @@ -223,4 +223,11 @@ config SAMPLE_WATCH_QUEUE
> > Build example userspace program to use the new mount_notify(),
> > sb_notify() syscalls and the KEYCTL_WATCH_KEY keyctl() function.
> >
> > +config SAMPLE_TASK_ISOLATION
> > + bool "task isolation sample"
> > + depends on CC_CAN_LINK && HEADERS_INSTALL
> > + help
> > + Build example userspace program to use prctl task isolation
> > + interface.
> > +
> > endif # SAMPLES
> > Index: linux-2.6/samples/Makefile
> > ===================================================================
> > --- linux-2.6.orig/samples/Makefile
> > +++ linux-2.6/samples/Makefile
> > @@ -30,3 +30,4 @@ obj-$(CONFIG_SAMPLE_INTEL_MEI) += mei/
> > subdir-$(CONFIG_SAMPLE_WATCHDOG) += watchdog
> > subdir-$(CONFIG_SAMPLE_WATCH_QUEUE) += watch_queue
> > obj-$(CONFIG_DEBUG_KMEMLEAK_TEST) += kmemleak/
> > +subdir-$(CONFIG_SAMPLE_TASK_ISOLATION) += task_isolation
> > Index: linux-2.6/samples/task_isolation/Makefile
> > ===================================================================
> > --- /dev/null
> > +++ linux-2.6/samples/task_isolation/Makefile
> > @@ -0,0 +1,4 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +userprogs-always-y += task_isolation
> > +
> > +userccflags += -I usr/include
> >
> >
> >
> I am wondering if it is possible to further split this patch into smaller
> ones?
OK, will try to split in smaller patches and fix the
style issues.
Thanks.
next prev parent reply other threads:[~2021-08-02 14:17 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-30 20:18 [patch 0/4] extensible prctl task isolation interface and vmstat sync (v2) Marcelo Tosatti
2021-07-30 20:18 ` [patch 1/4] add basic task isolation prctl interface Marcelo Tosatti
[not found] ` <CAFki+Lnf0cs62Se0aPubzYxP9wh7xjMXn7RXEPvrmtBdYBrsow@mail.gmail.com>
2021-07-31 0:49 ` Marcelo Tosatti
2021-07-31 7:47 ` kernel test robot
2021-07-31 7:47 ` kernel test robot
[not found] ` <CAFki+LkQVQOe+5aNEKWDvLdnjWjxzKWOiqOvBZzeuPWX+G=XgA@mail.gmail.com>
2021-08-02 14:16 ` Marcelo Tosatti [this message]
2021-07-30 20:18 ` [patch 2/4] task isolation: sync vmstats on return to userspace Marcelo Tosatti
2021-08-03 15:13 ` nsaenzju
2021-08-03 16:44 ` Marcelo Tosatti
2021-07-30 20:18 ` [patch 3/4] mm: vmstat: move need_update Marcelo Tosatti
2021-07-30 20:18 ` [patch 4/4] mm: vmstat_refresh: avoid queueing work item if cpu stats are clean Marcelo Tosatti
2021-08-07 2:47 ` Nitesh Lal
2021-08-09 17:34 ` Marcelo Tosatti
2021-08-09 19:13 ` Nitesh Lal
2021-08-10 16:40 ` [patch 0/4] extensible prctl task isolation interface and vmstat sync (v2) Thomas Gleixner
2021-08-10 18:37 ` Marcelo Tosatti
2021-08-10 19:15 ` Marcelo Tosatti
-- strict thread matches above, loose matches on Subject: below --
2021-07-27 12:29 [patch 1/4] add basic task isolation prctl interface kernel test robot
2021-07-27 10:38 [patch 0/4] prctl task isolation interface and vmstat sync Marcelo Tosatti
2021-07-27 10:38 ` [patch 1/4] add basic task isolation prctl interface Marcelo Tosatti
2021-07-27 10:48 ` nsaenzju
2021-07-27 11:00 ` Marcelo Tosatti
2021-07-27 12:38 ` nsaenzju
2021-07-27 13:06 ` Marcelo Tosatti
2021-07-27 13:08 ` Marcelo Tosatti
2021-07-27 13:09 ` Frederic Weisbecker
2021-07-27 14:52 ` Marcelo Tosatti
2021-07-27 23:45 ` Frederic Weisbecker
2021-07-28 9:37 ` Marcelo Tosatti
2021-07-28 11:45 ` Frederic Weisbecker
2021-07-28 13:21 ` Marcelo Tosatti
2021-07-28 21:22 ` Frederic Weisbecker
2021-07-28 11:55 ` nsaenzju
2021-07-28 13:16 ` Marcelo Tosatti
[not found] ` <CAFki+LkQwoqVTKmgnwLQQM8ua-ixbLp8i+jUT6xF15k6X=89mw@mail.gmail.com>
2021-07-28 16:21 ` Marcelo Tosatti
2021-07-28 17:08 ` nsaenzju
[not found] ` <CAFki+LmHeXmSFze8YEHFNbYA5hLEtnZyk37Yjf-eyOuKa8Os4w@mail.gmail.com>
2021-07-28 16:17 ` Marcelo Tosatti
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210802141611.GA40008@fuller.cnet \
--to=mtosatti@redhat.com \
--cc=abelits@belits.com \
--cc=cl@linux.com \
--cc=frederic@kernel.org \
--cc=juri.lelli@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=nilal@redhat.com \
--cc=nsaenzju@redhat.com \
--cc=peterx@redhat.com \
--cc=peterz@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.