From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934124AbcKMN7t (ORCPT ); Sun, 13 Nov 2016 08:59:49 -0500 Received: from mout.gmx.net ([212.227.15.18]:57049 "EHLO mout.gmx.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933365AbcKMN7r (ORCPT ); Sun, 13 Nov 2016 08:59:47 -0500 Message-ID: <1479045544.12006.8.camel@gmx.de> Subject: Re: sched/autogroup: race if !sysctl_sched_autogroup_enabled ? From: Mike Galbraith To: Oleg Nesterov , Peter Zijlstra Cc: Ingo Molnar , Linus Torvalds , hartsjc@redhat.com, vbendel@redhat.com, vlovejoy@redhat.com, linux-kernel@vger.kernel.org Date: Sun, 13 Nov 2016 14:59:04 +0100 In-Reply-To: <20161111165743.GA29869@redhat.com> References: <20161109165933.GA26071@redhat.com> <20161109175005.GS3142@twins.programming.kicks-ass.net> <20161110130913.GA11933@redhat.com> <20161111165743.GA29869@redhat.com> Content-Type: text/plain; charset="us-ascii" X-Mailer: Evolution 3.16.5 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-Provags-ID: V03:K0:W0TAqF9NvzMrjS1AfLUmCRQ7S8Y0y+7IuEKPHMZ18wOrFzbVMSO qXZc6qXKY5m/hkvQqIX0jGJ3WYWu624U3jO35fD9DedHQWo0fE7RHA6uIF6WGzOsX2Oo6m1 YYMK/jYbQpuGO6GoxoUlBWRX6+59+lmT5b/mmXPDTzabdIF/cimIqe1azCcJQhEEuzi7Pcx vewzBr+ydQfp1vzYMYcKg== X-UI-Out-Filterresults: notjunk:1;V01:K0:wfrSTjTlbVU=:jEsY9Ij85y5r9gD0pMMKPM xXnZtTXULxRb3rBckQmpeV5T5EkW6nLmXiOqLQ3LZl+OzSQDM4ADi4tTuHXiiEjvMaqC5XZ2q WigrJUBeWW4tCp+p38FyL0iWavc8wNTOYdoMwyMO/jcdtW7awhA8s4Xd3u5Gk8kFKzDT9nDkR lbDa5IWLFD+le/m35NO460yuza4HimXeoM6X8c4MxAZcjXpPphStuSzUT3vvImKuDaYxxpvPt JYuXm4J9M6/i98rA0QnNXPhYOxrGxH9SprHupYtEkdPGLuMibgtxp6o82rscFIUMyApw2foFS LgQenW6AXYpR5AjYMUYbV3MEkQ5n5vlWRnKG4Ibka7v7A9Y2SkPGfyipO20bKBHEQLWVkx/NP o89v90X0cnHp+F/02A90t2PTm1ZMD1Hp7wGCC2aJl4iaH7uuUPRej+WNWf2wlYVTgKvhG5toY iuuKApXVqguMy9HIhpmWadI6yxTaWSpgDEn1XLc8dG2fE9NmwUyN5RXtusyNmmZBBPk1P0ufR hxyaMLautIEPdXdpuqZPM8BY5v2UNWzmjvKE7YePdoQsqde/QEUfLZysqPLp/QIO0jFB3ijbg b8RQZ15f3BeYQ3l/RKIabaVYRHmB5Gv0oQKO0Huku5Vo6V21Rt2GC02zzsnpIkFTpNtnr4TUT Zq0RIUqVEQSljD3/7fUhWID7t/vz8bDcDRGRNqgdaIC4emA92HnkKDgtwzvECW8H2YMoCuzYg JWpm5cUDFN8hYqDbpjsdHmPbOISFqqlMMDPk0r+4fzrWjsOTOFf8ehREFN+rcF/RUZcCTg6hC vR9Uct5 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 2016-11-11 at 17:57 +0100, Oleg Nesterov wrote: > I am starting to think that we should just move ->autogroup from signal_struct > to task_struct. This will simplify the code and fix all these problems. But > I need a simple fix for backporting anyway. How about just rip out knobs that should have never been born. More can probably go as well. (sorry for slowness btw.. on vacation, trying the "go outside" thing;) sched/autogroup: Rip out dynamic knobs Autogroup never should have had knobs in the first place, and now, Oleg has discovered that the dynamic enable/disable capability either has become, or perhaps always was racy. Rip it all out, leaving only commandline disable. Signed-off-by: Mike Galbraith --- fs/proc/base.c | 34 --------------------------- include/linux/sched/sysctl.h | 4 --- kernel/sched/auto_group.c | 53 +++++++------------------------------------ kernel/sched/auto_group.h | 5 ---- kernel/sysctl.c | 11 -------- 5 files changed, 10 insertions(+), 97 deletions(-) --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -1455,39 +1455,6 @@ static int sched_autogroup_show(struct s return 0; } -static ssize_t -sched_autogroup_write(struct file *file, const char __user *buf, - size_t count, loff_t *offset) -{ - struct inode *inode = file_inode(file); - struct task_struct *p; - char buffer[PROC_NUMBUF]; - int nice; - int err; - - memset(buffer, 0, sizeof(buffer)); - if (count > sizeof(buffer) - 1) - count = sizeof(buffer) - 1; - if (copy_from_user(buffer, buf, count)) - return -EFAULT; - - err = kstrtoint(strstrip(buffer), 0, &nice); - if (err < 0) - return err; - - p = get_proc_task(inode); - if (!p) - return -ESRCH; - - err = proc_sched_autogroup_set_nice(p, nice); - if (err) - count = err; - - put_task_struct(p); - - return count; -} - static int sched_autogroup_open(struct inode *inode, struct file *filp) { int ret; @@ -1504,7 +1471,6 @@ static int sched_autogroup_open(struct i static const struct file_operations proc_pid_sched_autogroup_operations = { .open = sched_autogroup_open, .read = seq_read, - .write = sched_autogroup_write, .llseek = seq_lseek, .release = single_release, }; --- a/include/linux/sched/sysctl.h +++ b/include/linux/sched/sysctl.h @@ -60,10 +60,6 @@ extern int sysctl_sched_rt_runtime; extern unsigned int sysctl_sched_cfs_bandwidth_slice; #endif -#ifdef CONFIG_SCHED_AUTOGROUP -extern unsigned int sysctl_sched_autogroup_enabled; -#endif - extern int sched_rr_timeslice; extern int sched_rr_handler(struct ctl_table *table, int write, --- a/kernel/sched/auto_group.c +++ b/kernel/sched/auto_group.c @@ -7,7 +7,7 @@ #include #include -unsigned int __read_mostly sysctl_sched_autogroup_enabled = 1; +unsigned int __read_mostly sched_autogroup_enabled = 1; static struct autogroup autogroup_default; static atomic_t autogroup_seq_nr; @@ -109,7 +109,7 @@ static inline struct autogroup *autogrou bool task_wants_autogroup(struct task_struct *p, struct task_group *tg) { - if (tg != &root_task_group) + if (!sched_autogroup_enabled || tg != &root_task_group) return false; /* @@ -139,12 +139,9 @@ autogroup_move_group(struct task_struct p->signal->autogroup = autogroup_kref_get(ag); - if (!READ_ONCE(sysctl_sched_autogroup_enabled)) - goto out; - for_each_thread(p, t) sched_move_task(t); -out: + unlock_task_sighand(p, &flags); autogroup_kref_put(prev); } @@ -152,8 +149,11 @@ autogroup_move_group(struct task_struct /* Allocates GFP_KERNEL, cannot be called under any spinlock */ void sched_autogroup_create_attach(struct task_struct *p) { - struct autogroup *ag = autogroup_create(); + struct autogroup *ag; + if (!sched_autogroup_enabled) + return; + ag = autogroup_create(); autogroup_move_group(p, ag); /* drop extra reference added by autogroup_create() */ autogroup_kref_put(ag); @@ -179,7 +179,7 @@ void sched_autogroup_exit(struct signal_ static int __init setup_autogroup(char *str) { - sysctl_sched_autogroup_enabled = 0; + sched_autogroup_enabled = 0; return 1; } @@ -187,41 +187,6 @@ static int __init setup_autogroup(char * __setup("noautogroup", setup_autogroup); #ifdef CONFIG_PROC_FS - -int proc_sched_autogroup_set_nice(struct task_struct *p, int nice) -{ - static unsigned long next = INITIAL_JIFFIES; - struct autogroup *ag; - int err; - - if (nice < MIN_NICE || nice > MAX_NICE) - return -EINVAL; - - err = security_task_setnice(current, nice); - if (err) - return err; - - if (nice < 0 && !can_nice(current, nice)) - return -EPERM; - - /* this is a heavy operation taking global locks.. */ - if (!capable(CAP_SYS_ADMIN) && time_before(jiffies, next)) - return -EAGAIN; - - next = HZ / 10 + jiffies; - ag = autogroup_task_get(p); - - down_write(&ag->lock); - err = sched_group_set_shares(ag->tg, sched_prio_to_weight[nice + 20]); - if (!err) - ag->nice = nice; - up_write(&ag->lock); - - autogroup_kref_put(ag); - - return err; -} - void proc_sched_autogroup_show_task(struct task_struct *p, struct seq_file *m) { struct autogroup *ag = autogroup_task_get(p); @@ -230,7 +195,7 @@ void proc_sched_autogroup_show_task(stru goto out; down_read(&ag->lock); - seq_printf(m, "/autogroup-%ld nice %d\n", ag->id, ag->nice); + seq_printf(m, "/autogroup-%ld\n", ag->id); up_read(&ag->lock); out: --- a/kernel/sched/auto_group.h +++ b/kernel/sched/auto_group.h @@ -13,7 +13,6 @@ struct autogroup { struct task_group *tg; struct rw_semaphore lock; unsigned long id; - int nice; }; extern void autogroup_init(struct task_struct *init_task); @@ -29,9 +28,7 @@ extern bool task_wants_autogroup(struct static inline struct task_group * autogroup_task_group(struct task_struct *p, struct task_group *tg) { - int enabled = READ_ONCE(sysctl_sched_autogroup_enabled); - - if (enabled && task_wants_autogroup(p, tg)) + if (task_wants_autogroup(p, tg)) return p->signal->autogroup->tg; return tg; --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -437,17 +437,6 @@ static struct ctl_table kern_table[] = { .mode = 0644, .proc_handler = sched_rr_handler, }, -#ifdef CONFIG_SCHED_AUTOGROUP - { - .procname = "sched_autogroup_enabled", - .data = &sysctl_sched_autogroup_enabled, - .maxlen = sizeof(unsigned int), - .mode = 0644, - .proc_handler = proc_dointvec_minmax, - .extra1 = &zero, - .extra2 = &one, - }, -#endif #ifdef CONFIG_CFS_BANDWIDTH { .procname = "sched_cfs_bandwidth_slice_us",