* [PATCH v2 1/3] sched: fix confusing PFA_NO_NEW_PRIVS constant
@ 2014-09-23 6:03 Zefan Li
2014-09-23 6:04 ` [PATCH v2 2/3] sched: add a macro to define bitops for task atomic flags Zefan Li
2014-09-23 6:05 ` [PATCH v2 3/3] cpuset: PF_SPREAD_PAGE and PF_SPREAD_SLAB should be " Zefan Li
0 siblings, 2 replies; 10+ messages in thread
From: Zefan Li @ 2014-09-23 6:03 UTC (permalink / raw)
To: Tejun Heo
Cc: Peter Zijlstra, Ingo Molnar, Tetsuo Handa, Miao Xie, LKML,
Cgroups, Kees Cook
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Commit 1d4457f99928 ("sched: move no_new_privs into new atomic flags")
defined PFA_NO_NEW_PRIVS as hexadecimal value, but it is confusing
because it is used as bit number. Redefine it as decimal bit number.
Note this changes the bit position of PFA_NOW_NEW_PRIVS from 1 to 0.
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Miao Xie <miaox@cn.fujitsu.com>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Acked-by: Kees Cook <keescook@chromium.org>
[ lizf: slightly modified subject and changelog ]
Signed-off-by: Zefan Li <lizefan@huawei.com>
---
include/linux/sched.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 5c2c885..4557765 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1957,7 +1957,7 @@ static inline void memalloc_noio_restore(unsigned int flags)
}
/* Per-process atomic flags. */
-#define PFA_NO_NEW_PRIVS 0x00000001 /* May not gain new privileges. */
+#define PFA_NO_NEW_PRIVS 0 /* May not gain new privileges. */
static inline bool task_no_new_privs(struct task_struct *p)
{
--
1.8.0.2
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v2 2/3] sched: add a macro to define bitops for task atomic flags 2014-09-23 6:03 [PATCH v2 1/3] sched: fix confusing PFA_NO_NEW_PRIVS constant Zefan Li @ 2014-09-23 6:04 ` Zefan Li 2014-09-23 6:37 ` Kees Cook 2014-09-23 6:05 ` [PATCH v2 3/3] cpuset: PF_SPREAD_PAGE and PF_SPREAD_SLAB should be " Zefan Li 1 sibling, 1 reply; 10+ messages in thread From: Zefan Li @ 2014-09-23 6:04 UTC (permalink / raw) To: Tejun Heo Cc: Peter Zijlstra, Ingo Molnar, Tetsuo Handa, Miao Xie, LKML, Cgroups, Kees Cook This will simplify code when we add new flags. v2: - updated scripts/tags.sh, suggested by Peter Cc: Peter Zijlstra <peterz@infradead.org> Cc: Ingo Molnar <mingo@kernel.org> Cc: Miao Xie <miaox@cn.fujitsu.com> Cc: Kees Cook <keescook@chromium.org> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Signed-off-by: Zefan Li <lizefan@huawei.com> --- include/linux/sched.h | 20 +++++++++++--------- scripts/tags.sh | 6 ++++++ 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/include/linux/sched.h b/include/linux/sched.h index 4557765..04a2ae2 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -1959,15 +1959,17 @@ static inline void memalloc_noio_restore(unsigned int flags) /* Per-process atomic flags. */ #define PFA_NO_NEW_PRIVS 0 /* May not gain new privileges. */ -static inline bool task_no_new_privs(struct task_struct *p) -{ - return test_bit(PFA_NO_NEW_PRIVS, &p->atomic_flags); -} - -static inline void task_set_no_new_privs(struct task_struct *p) -{ - set_bit(PFA_NO_NEW_PRIVS, &p->atomic_flags); -} +#define TASK_PFA_BITOPS(name, func) \ +static inline bool task_##func(struct task_struct *p) \ +{ return test_bit(PFA_##name, &p->atomic_flags); } \ + \ +static inline void task_set_##func(struct task_struct *p) \ +{ set_bit(PFA_##name, &p->atomic_flags); } \ + \ +static inline void task_clear_##func(struct task_struct *p) \ +{ clear_bit(PFA_##name, &p->atomic_flags); } + +TASK_PFA_BITOPS(NO_NEW_PRIVS, no_new_privs) /* * task->jobctl flags diff --git a/scripts/tags.sh b/scripts/tags.sh index cbfd269..8591b57 100755 --- a/scripts/tags.sh +++ b/scripts/tags.sh @@ -197,6 +197,9 @@ exuberant() --regex-c++='/SETPCGFLAG\(([^,)]*).*/SetPageCgroup\1/' \ --regex-c++='/CLEARPCGFLAG\(([^,)]*).*/ClearPageCgroup\1/' \ --regex-c++='/TESTCLEARPCGFLAG\(([^,)]*).*/TestClearPageCgroup\1/' \ + --regex-c++='/TASK_PFA_BITOPS\([^,]*,\s*([^)]*)\)/task_\1/' \ + --regex-c++='/TASK_PFA_BITOPS\([^,]*,\s*([^)]*)\)/task_set_\1/' \ + --regex-c++='/TASK_PFA_BITOPS\([^,]*,\s*([^)]*)\)/task_clear_\1/' \ --regex-c='/PCI_OP_READ\((\w*).*[1-4]\)/pci_bus_read_config_\1/' \ --regex-c='/PCI_OP_WRITE\((\w*).*[1-4]\)/pci_bus_write_config_\1/' \ --regex-c='/DEFINE_(MUTEX|SEMAPHORE|SPINLOCK)\((\w*)/\2/v/' \ @@ -260,6 +263,9 @@ emacs() --regex='/SETPCGFLAG\(([^,)]*).*/SetPageCgroup\1/' \ --regex='/CLEARPCGFLAG\(([^,)]*).*/ClearPageCgroup\1/' \ --regex='/TESTCLEARPCGFLAG\(([^,)]*).*/TestClearPageCgroup\1/' \ + --regex='/TASK_PFA_BITOPS\([^,]*,\s*([^)]*)\)/task_\1/' \ + --regex='/TASK_PFA_BITOPS\([^,]*,\s*([^)]*)\)/task_set_\1/' \ + --regex='/TASK_PFA_BITOPS\([^,]*,\s*([^)]*)\)/task_clear_\1/' \ --regex='/_PE(\([^,)]*\).*/PEVENT_ERRNO__\1/' \ --regex='/PCI_OP_READ(\([a-z]*[a-z]\).*[1-4])/pci_bus_read_config_\1/' \ --regex='/PCI_OP_WRITE(\([a-z]*[a-z]\).*[1-4])/pci_bus_write_config_\1/'\ -- 1.8.0.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/3] sched: add a macro to define bitops for task atomic flags 2014-09-23 6:04 ` [PATCH v2 2/3] sched: add a macro to define bitops for task atomic flags Zefan Li @ 2014-09-23 6:37 ` Kees Cook 2014-09-23 6:52 ` Zefan Li 0 siblings, 1 reply; 10+ messages in thread From: Kees Cook @ 2014-09-23 6:37 UTC (permalink / raw) To: Zefan Li Cc: Tejun Heo, Peter Zijlstra, Ingo Molnar, Tetsuo Handa, Miao Xie, LKML, Cgroups On Mon, Sep 22, 2014 at 11:04 PM, Zefan Li <lizefan@huawei.com> wrote: > This will simplify code when we add new flags. > > v2: > - updated scripts/tags.sh, suggested by Peter > > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Ingo Molnar <mingo@kernel.org> > Cc: Miao Xie <miaox@cn.fujitsu.com> > Cc: Kees Cook <keescook@chromium.org> > Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Signed-off-by: Zefan Li <lizefan@huawei.com> > --- > include/linux/sched.h | 20 +++++++++++--------- > scripts/tags.sh | 6 ++++++ > 2 files changed, 17 insertions(+), 9 deletions(-) > > diff --git a/include/linux/sched.h b/include/linux/sched.h > index 4557765..04a2ae2 100644 > --- a/include/linux/sched.h > +++ b/include/linux/sched.h > @@ -1959,15 +1959,17 @@ static inline void memalloc_noio_restore(unsigned int flags) > /* Per-process atomic flags. */ > #define PFA_NO_NEW_PRIVS 0 /* May not gain new privileges. */ > > -static inline bool task_no_new_privs(struct task_struct *p) > -{ > - return test_bit(PFA_NO_NEW_PRIVS, &p->atomic_flags); > -} > - > -static inline void task_set_no_new_privs(struct task_struct *p) > -{ > - set_bit(PFA_NO_NEW_PRIVS, &p->atomic_flags); > -} > +#define TASK_PFA_BITOPS(name, func) \ > +static inline bool task_##func(struct task_struct *p) \ > +{ return test_bit(PFA_##name, &p->atomic_flags); } \ > + \ > +static inline void task_set_##func(struct task_struct *p) \ > +{ set_bit(PFA_##name, &p->atomic_flags); } \ > + \ > +static inline void task_clear_##func(struct task_struct *p) \ > +{ clear_bit(PFA_##name, &p->atomic_flags); } > + > +TASK_PFA_BITOPS(NO_NEW_PRIVS, no_new_privs) One thing I don't like about this is that task_clear_no_new_privs() ends up getting defined, and it should absolutely never be used. NNP should never be cleared or there could be security issues. I realize this isn't a very useful nit-pick, but I'd rather the function wasn't even available for someone to accidentally use. Maybe break up the macro with some kind of "write only" version like: #define TASK_PFA_BITOPS_WO(name, func) \ static inline bool task_##func(struct task_struct *p) \ { return test_bit(PFA_##name, &p->atomic_flags); } \ static inline void task_set_##func(struct task_struct *p) \ { set_bit(PFA_##name, &p->atomic_flags); } #define TASK_PFA_BITOPS(name, func) \ TASK_PFA_BITOPS_WO(name, func); \ static inline void task_clear_##func(struct task_struct *p) \ { clear_bit(PFA_##name, &p->atomic_flags); } TASK_PFA_BITOPS_WO(NO_NEW_PRIVS, no_new_privs) And then all the new users can use TASK_PFA_BITOPS() normally since they expect to use "clear"? -Kees > > /* > * task->jobctl flags > diff --git a/scripts/tags.sh b/scripts/tags.sh > index cbfd269..8591b57 100755 > --- a/scripts/tags.sh > +++ b/scripts/tags.sh > @@ -197,6 +197,9 @@ exuberant() > --regex-c++='/SETPCGFLAG\(([^,)]*).*/SetPageCgroup\1/' \ > --regex-c++='/CLEARPCGFLAG\(([^,)]*).*/ClearPageCgroup\1/' \ > --regex-c++='/TESTCLEARPCGFLAG\(([^,)]*).*/TestClearPageCgroup\1/' \ > + --regex-c++='/TASK_PFA_BITOPS\([^,]*,\s*([^)]*)\)/task_\1/' \ > + --regex-c++='/TASK_PFA_BITOPS\([^,]*,\s*([^)]*)\)/task_set_\1/' \ > + --regex-c++='/TASK_PFA_BITOPS\([^,]*,\s*([^)]*)\)/task_clear_\1/' \ > --regex-c='/PCI_OP_READ\((\w*).*[1-4]\)/pci_bus_read_config_\1/' \ > --regex-c='/PCI_OP_WRITE\((\w*).*[1-4]\)/pci_bus_write_config_\1/' \ > --regex-c='/DEFINE_(MUTEX|SEMAPHORE|SPINLOCK)\((\w*)/\2/v/' \ > @@ -260,6 +263,9 @@ emacs() > --regex='/SETPCGFLAG\(([^,)]*).*/SetPageCgroup\1/' \ > --regex='/CLEARPCGFLAG\(([^,)]*).*/ClearPageCgroup\1/' \ > --regex='/TESTCLEARPCGFLAG\(([^,)]*).*/TestClearPageCgroup\1/' \ > + --regex='/TASK_PFA_BITOPS\([^,]*,\s*([^)]*)\)/task_\1/' \ > + --regex='/TASK_PFA_BITOPS\([^,]*,\s*([^)]*)\)/task_set_\1/' \ > + --regex='/TASK_PFA_BITOPS\([^,]*,\s*([^)]*)\)/task_clear_\1/' \ > --regex='/_PE(\([^,)]*\).*/PEVENT_ERRNO__\1/' \ > --regex='/PCI_OP_READ(\([a-z]*[a-z]\).*[1-4])/pci_bus_read_config_\1/' \ > --regex='/PCI_OP_WRITE(\([a-z]*[a-z]\).*[1-4])/pci_bus_write_config_\1/'\ > -- > 1.8.0.2 > -- Kees Cook Chrome OS Security ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/3] sched: add a macro to define bitops for task atomic flags 2014-09-23 6:37 ` Kees Cook @ 2014-09-23 6:52 ` Zefan Li 0 siblings, 0 replies; 10+ messages in thread From: Zefan Li @ 2014-09-23 6:52 UTC (permalink / raw) To: Kees Cook Cc: Tejun Heo, Peter Zijlstra, Ingo Molnar, Tetsuo Handa, Miao Xie, LKML, Cgroups >> -static inline bool task_no_new_privs(struct task_struct *p) >> -{ >> - return test_bit(PFA_NO_NEW_PRIVS, &p->atomic_flags); >> -} >> - >> -static inline void task_set_no_new_privs(struct task_struct *p) >> -{ >> - set_bit(PFA_NO_NEW_PRIVS, &p->atomic_flags); >> -} >> +#define TASK_PFA_BITOPS(name, func) \ >> +static inline bool task_##func(struct task_struct *p) \ >> +{ return test_bit(PFA_##name, &p->atomic_flags); } \ >> + \ >> +static inline void task_set_##func(struct task_struct *p) \ >> +{ set_bit(PFA_##name, &p->atomic_flags); } \ >> + \ >> +static inline void task_clear_##func(struct task_struct *p) \ >> +{ clear_bit(PFA_##name, &p->atomic_flags); } >> + >> +TASK_PFA_BITOPS(NO_NEW_PRIVS, no_new_privs) > > One thing I don't like about this is that task_clear_no_new_privs() > ends up getting defined, and it should absolutely never be used. NNP > should never be cleared or there could be security issues. I realize > this isn't a very useful nit-pick, but I'd rather the function wasn't > even available for someone to accidentally use. Maybe break up the > macro with some kind of "write only" version like: > > #define TASK_PFA_BITOPS_WO(name, func) \ > static inline bool task_##func(struct task_struct *p) \ > { return test_bit(PFA_##name, &p->atomic_flags); } \ > static inline void task_set_##func(struct task_struct *p) \ > { set_bit(PFA_##name, &p->atomic_flags); } > > #define TASK_PFA_BITOPS(name, func) \ > TASK_PFA_BITOPS_WO(name, func); \ > static inline void task_clear_##func(struct task_struct *p) \ > { clear_bit(PFA_##name, &p->atomic_flags); } > > TASK_PFA_BITOPS_WO(NO_NEW_PRIVS, no_new_privs) > > And then all the new users can use TASK_PFA_BITOPS() normally since > they expect to use "clear"? > Now I'm inclined to do this: +#define TASK_PFA_TEST(name, func) \ + static inline bool task_##func(struct task_struct *p) \ + { return test_bit(PFA_##name, &p->atomic_flags); } +#define TASK_PFA_SET(name, func) \ + static inline void task_set_##func(struct task_struct *p) \ + { set_bit(PFA_##name, &p->atomic_flags); } +#define TASK_PFA_CLEAR(name, func) \ + static inline void task_clear_##func(struct task_struct *p) \ + { clear_bit(PFA_##name, &p->atomic_flags); } + +TASK_PFA_TEST(NO_NEW_PRIVS, no_new_privs) +TASK_PFA_SET(NO_NEW_PRIVS, no_new_privs) ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 3/3] cpuset: PF_SPREAD_PAGE and PF_SPREAD_SLAB should be atomic flags 2014-09-23 6:03 [PATCH v2 1/3] sched: fix confusing PFA_NO_NEW_PRIVS constant Zefan Li 2014-09-23 6:04 ` [PATCH v2 2/3] sched: add a macro to define bitops for task atomic flags Zefan Li @ 2014-09-23 6:05 ` Zefan Li 2014-09-23 10:55 ` [PATCH v2 3/3] cpuset: PF_SPREAD_PAGE and PF_SPREAD_SLAB should beatomic flags Tetsuo Handa 1 sibling, 1 reply; 10+ messages in thread From: Zefan Li @ 2014-09-23 6:05 UTC (permalink / raw) To: Tejun Heo Cc: Peter Zijlstra, Ingo Molnar, Tetsuo Handa, Miao Xie, LKML, Cgroups, Kees Cook When we change cpuset.memory_spread_{page,slab}, cpuset will flip PF_SPREAD_{PAGE,SLAB} bit of tsk->flags for each task in that cpuset. This should be done using atomic bitops, but currently we don't, which is broken. Tetsuo reported a hard-to-reproduce kernel crash on RHEL6, which happend when one thread tried to clear PF_USED_MATH while at the same time another thread tried to flip PF_SPREAD_PAGE/PF_SPREAD_SLAB. They both operate on the same task. Here's the full report: https://lkml.org/lkml/2014/9/19/230 To fix this, we make PF_SPREAD_PAGE and PF_SPARED_SLAB atomic flags. Cc: Peter Zijlstra <peterz@infradead.org> Cc: Ingo Molnar <mingo@kernel.org> Cc: Miao Xie <miaox@cn.fujitsu.com> Cc: Kees Cook <keescook@chromium.org> Fixes: 950592f7b991 ("cpusets: update tasks' page/slab spread flags in time") Cc: <stable@vger.kernel.org> # 2.6.31+ Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Signed-off-by: Zefan Li <lizefan@huawei.com> --- include/linux/cpuset.h | 4 ++-- include/linux/sched.h | 6 ++++-- kernel/cpuset.c | 9 +++++---- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/include/linux/cpuset.h b/include/linux/cpuset.h index 0d4e067..2f073db 100644 --- a/include/linux/cpuset.h +++ b/include/linux/cpuset.h @@ -94,12 +94,12 @@ extern int cpuset_slab_spread_node(void); static inline int cpuset_do_page_mem_spread(void) { - return current->flags & PF_SPREAD_PAGE; + return task_spread_page(current); } static inline int cpuset_do_slab_mem_spread(void) { - return current->flags & PF_SPREAD_SLAB; + return task_spread_slab(current); } extern int current_cpuset_is_being_rebound(void); diff --git a/include/linux/sched.h b/include/linux/sched.h index 04a2ae2..ee634d1 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -1903,8 +1903,6 @@ extern void thread_group_cputime_adjusted(struct task_struct *p, cputime_t *ut, #define PF_KTHREAD 0x00200000 /* I am a kernel thread */ #define PF_RANDOMIZE 0x00400000 /* randomize virtual address space */ #define PF_SWAPWRITE 0x00800000 /* Allowed to write to swap */ -#define PF_SPREAD_PAGE 0x01000000 /* Spread page cache over cpuset */ -#define PF_SPREAD_SLAB 0x02000000 /* Spread some slab caches over cpuset */ #define PF_NO_SETAFFINITY 0x04000000 /* Userland is not allowed to meddle with cpus_allowed */ #define PF_MCE_EARLY 0x08000000 /* Early kill for mce process policy */ #define PF_MUTEX_TESTER 0x20000000 /* Thread belongs to the rt mutex tester */ @@ -1958,6 +1956,8 @@ static inline void memalloc_noio_restore(unsigned int flags) /* Per-process atomic flags. */ #define PFA_NO_NEW_PRIVS 0 /* May not gain new privileges. */ +#define PFA_SPREAD_PAGE 1 /* Spread page cache over cpuset */ +#define PFA_SPREAD_SLAB 2 /* Spread some slab caches over cpuset */ #define TASK_PFA_BITOPS(name, func) \ static inline bool task_##func(struct task_struct *p) \ @@ -1970,6 +1970,8 @@ static inline void task_clear_##func(struct task_struct *p) \ { clear_bit(PFA_##name, &p->atomic_flags); } TASK_PFA_BITOPS(NO_NEW_PRIVS, no_new_privs) +TASK_PFA_BITOPS(SPREAD_PAGE, spread_page) +TASK_PFA_BITOPS(SPREAD_SLAB, spread_slab) /* * task->jobctl flags diff --git a/kernel/cpuset.c b/kernel/cpuset.c index a37f4ed..1f107c7 100644 --- a/kernel/cpuset.c +++ b/kernel/cpuset.c @@ -365,13 +365,14 @@ static void cpuset_update_task_spread_flag(struct cpuset *cs, struct task_struct *tsk) { if (is_spread_page(cs)) - tsk->flags |= PF_SPREAD_PAGE; + task_set_spread_page(tsk); else - tsk->flags &= ~PF_SPREAD_PAGE; + task_clear_spread_page(tsk); + if (is_spread_slab(cs)) - tsk->flags |= PF_SPREAD_SLAB; + task_set_spread_slab(tsk); else - tsk->flags &= ~PF_SPREAD_SLAB; + task_clear_spread_slab(tsk); } /* -- 1.8.0.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/3] cpuset: PF_SPREAD_PAGE and PF_SPREAD_SLAB should beatomic flags 2014-09-23 6:05 ` [PATCH v2 3/3] cpuset: PF_SPREAD_PAGE and PF_SPREAD_SLAB should be " Zefan Li @ 2014-09-23 10:55 ` Tetsuo Handa 2014-09-23 13:13 ` Tejun Heo 2014-09-24 3:01 ` Zefan Li 0 siblings, 2 replies; 10+ messages in thread From: Tetsuo Handa @ 2014-09-23 10:55 UTC (permalink / raw) To: lizefan, tj; +Cc: peterz, mingo, miaox, linux-kernel, cgroups, keescook Zefan Li wrote: > Tetsuo reported a hard-to-reproduce kernel crash on RHEL6, which happend s/happend/happened/ > @@ -1972,6 +1973,14 @@ static inline void memalloc_noio_restore(unsigned int flags) > TASK_PFA_TEST(NO_NEW_PRIVS, no_new_privs) > TASK_PFA_SET(NO_NEW_PRIVS, no_new_privs) > > +TASK_PFA_TEST(SPREAD_PAGE, spread_page) > +TASK_PFA_SET(SPREAD_PAGE, spread_page) > +TASK_PFA_CLEAR(SPREAD_PAGE, spread_page) > + > +TASK_PFA_TEST(SPREAD_SLAB, spread_slab) > +TASK_PFA_SET(SPREAD_SLAB, spread_slab) > +TASK_PFA_CLEAR(SPREAD_SLAB, spread_slab) > + I wonder how adding 3 macro lines differs from 3 inlined functions. Personally, from LXR (source code browser) point of view, inlined functions are more friendly than macros. Also, I wonder about the cost of extracting macros in a file which is likely included by every file but referenced by few files. Speak of SPREAD_PAGE and SPREAD_SLAB, they should be defined as inlined functions in include/linux/cpuset.h rather than as macros in include/linux/sched.h ? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/3] cpuset: PF_SPREAD_PAGE and PF_SPREAD_SLAB should beatomic flags 2014-09-23 10:55 ` [PATCH v2 3/3] cpuset: PF_SPREAD_PAGE and PF_SPREAD_SLAB should beatomic flags Tetsuo Handa @ 2014-09-23 13:13 ` Tejun Heo 2014-09-24 3:01 ` Zefan Li 1 sibling, 0 replies; 10+ messages in thread From: Tejun Heo @ 2014-09-23 13:13 UTC (permalink / raw) To: Tetsuo Handa Cc: lizefan, peterz, mingo, miaox, linux-kernel, cgroups, keescook On Tue, Sep 23, 2014 at 07:55:48PM +0900, Tetsuo Handa wrote: > Zefan Li wrote: > > Tetsuo reported a hard-to-reproduce kernel crash on RHEL6, which happend > > s/happend/happened/ > > > @@ -1972,6 +1973,14 @@ static inline void memalloc_noio_restore(unsigned int flags) > > TASK_PFA_TEST(NO_NEW_PRIVS, no_new_privs) > > TASK_PFA_SET(NO_NEW_PRIVS, no_new_privs) > > > > +TASK_PFA_TEST(SPREAD_PAGE, spread_page) > > +TASK_PFA_SET(SPREAD_PAGE, spread_page) > > +TASK_PFA_CLEAR(SPREAD_PAGE, spread_page) > > + > > +TASK_PFA_TEST(SPREAD_SLAB, spread_slab) > > +TASK_PFA_SET(SPREAD_SLAB, spread_slab) > > +TASK_PFA_CLEAR(SPREAD_SLAB, spread_slab) > > + > > I wonder how adding 3 macro lines differs from 3 inlined functions. > Personally, from LXR (source code browser) point of view, inlined functions > are more friendly than macros. Also, I wonder about the cost of extracting > macros in a file which is likely included by every file but referenced > by few files. Speak of SPREAD_PAGE and SPREAD_SLAB, they should be defined > as inlined functions in include/linux/cpuset.h rather than as macros in > include/linux/sched.h ? I think sched.h is fine along w/ inlines for other flags but yeah we might be better off just open-coding them. Thanks. -- tejun ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/3] cpuset: PF_SPREAD_PAGE and PF_SPREAD_SLAB should beatomic flags 2014-09-23 10:55 ` [PATCH v2 3/3] cpuset: PF_SPREAD_PAGE and PF_SPREAD_SLAB should beatomic flags Tetsuo Handa 2014-09-23 13:13 ` Tejun Heo @ 2014-09-24 3:01 ` Zefan Li 2014-09-24 11:44 ` [PATCH v2 3/3] cpuset: PF_SPREAD_PAGE and PF_SPREAD_SLAB should be atomic flags Tetsuo Handa 1 sibling, 1 reply; 10+ messages in thread From: Zefan Li @ 2014-09-24 3:01 UTC (permalink / raw) To: Tetsuo Handa; +Cc: tj, peterz, mingo, miaox, linux-kernel, cgroups, keescook On 2014/9/23 18:55, Tetsuo Handa wrote: > Zefan Li wrote: >> Tetsuo reported a hard-to-reproduce kernel crash on RHEL6, which happend > > s/happend/happened/ > >> @@ -1972,6 +1973,14 @@ static inline void memalloc_noio_restore(unsigned int flags) >> TASK_PFA_TEST(NO_NEW_PRIVS, no_new_privs) >> TASK_PFA_SET(NO_NEW_PRIVS, no_new_privs) >> >> +TASK_PFA_TEST(SPREAD_PAGE, spread_page) >> +TASK_PFA_SET(SPREAD_PAGE, spread_page) >> +TASK_PFA_CLEAR(SPREAD_PAGE, spread_page) >> + >> +TASK_PFA_TEST(SPREAD_SLAB, spread_slab) >> +TASK_PFA_SET(SPREAD_SLAB, spread_slab) >> +TASK_PFA_CLEAR(SPREAD_SLAB, spread_slab) >> + > > I wonder how adding 3 macro lines differs from 3 inlined functions. > Personally, from LXR (source code browser) point of view, inlined functions > are more friendly than macros. Also, I wonder about the cost of extracting > macros in a file which is likely included by every file but referenced > by few files. Speak of SPREAD_PAGE and SPREAD_SLAB, they should be defined > as inlined functions in include/linux/cpuset.h rather than as macros in > include/linux/sched.h ? > . Though tsk->atomic_flags were newly introduced in 3.17 merge window, we already have 3 flags, and we may see more flags added. Those macros make the code easier to read, and emacs and cscope can also understand them. I'd vote for this: TASK_PFA_TEST(NO_NEW_PRIVS, no_new_privs) TASK_PFA_SET(NO_NEW_PRIVS, no_new_privs) TASK_PFA_TEST(SPREAD_PAGE, spread_page) TASK_PFA_SET(SPREAD_PAGE, spread_page) TASK_PFA_CLEAR(SPREAD_PAGE, spread_page) TASK_PFA_TEST(SPREAD_SLAB, spread_slab) TASK_PFA_SET(SPREAD_SLAB, spread_slab) TASK_PFA_CLEAR(SPREAD_SLAB, spread_slab) over this: static inline bool task_no_new_privs(struct task_struct *p) { return test_bit(PFA_NO_NEW_PRIVS, &p->atomic_flags); } static inline void task_set_new_privs(struct task_struct *p) { set_bit(PFA_NO_NEW_PRIVS, &p->atomic_flags); } static inline bool task_spread_page(struct task_struct *p) { return test_bit(PFA_SPREAD_PAGE, &p->atomic_flags); } static inline void task_set_spread_page(struct task_struct *p) { set_bit(PFA_SPREAD_PAGE, &p->atomic_flags); } static inline void task_clear_spread_page(struct task_struct *p) { clear_bit(PFA_SPREAD_PAGE, &p->atomic_flags); } static inline bool task_spread_slab(struct task_struct *p) { return test_bit(PFA_SPREAD_SLAB, &p->atomic_flags); } static inline void task_set_spread_slab(struct task_struct *p) { set_bit(PFA_SPREAD_SLAB, &p->atomic_flags); } static inline void task_clear_spread_slab(struct task_struct *p) { clear_bit(PFA_SPREAD_SLAB, &p->atomic_flags); } ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/3] cpuset: PF_SPREAD_PAGE and PF_SPREAD_SLAB should be atomic flags 2014-09-24 3:01 ` Zefan Li @ 2014-09-24 11:44 ` Tetsuo Handa 2014-09-24 13:06 ` Tejun Heo 0 siblings, 1 reply; 10+ messages in thread From: Tetsuo Handa @ 2014-09-24 11:44 UTC (permalink / raw) To: lizefan, peterz, mingo; +Cc: tj, miaox, linux-kernel, cgroups, keescook Zefan Li wrote: > Those macros make the code easier to read, and emacs and cscope can also > understand them. I'm using legacy LXR which cannot understand them. But > I'd vote for this: > > TASK_PFA_TEST(NO_NEW_PRIVS, no_new_privs) > TASK_PFA_SET(NO_NEW_PRIVS, no_new_privs) > > TASK_PFA_TEST(SPREAD_PAGE, spread_page) > TASK_PFA_SET(SPREAD_PAGE, spread_page) > TASK_PFA_CLEAR(SPREAD_PAGE, spread_page) > > TASK_PFA_TEST(SPREAD_SLAB, spread_slab) > TASK_PFA_SET(SPREAD_SLAB, spread_slab) > TASK_PFA_CLEAR(SPREAD_SLAB, spread_slab) > > over this: you can go ahead. This difference is not a stopper. Tejun Heo wrote: > All the patches look good to me. I can't say I'm a big fan of > function defining macros but I don't have prettier alternatives > either. Once Peter/Ingo acks the patches, I'll route them through > cgroup/for-3.17-fixes. Peter and Ingo, are these patches OK for you? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/3] cpuset: PF_SPREAD_PAGE and PF_SPREAD_SLAB should be atomic flags 2014-09-24 11:44 ` [PATCH v2 3/3] cpuset: PF_SPREAD_PAGE and PF_SPREAD_SLAB should be atomic flags Tetsuo Handa @ 2014-09-24 13:06 ` Tejun Heo 0 siblings, 0 replies; 10+ messages in thread From: Tejun Heo @ 2014-09-24 13:06 UTC (permalink / raw) To: Tetsuo Handa Cc: lizefan, peterz, mingo, miaox, linux-kernel, cgroups, keescook On Wed, Sep 24, 2014 at 08:44:43PM +0900, Tetsuo Handa wrote: > Tejun Heo wrote: > > All the patches look good to me. I can't say I'm a big fan of > > function defining macros but I don't have prettier alternatives > > either. Once Peter/Ingo acks the patches, I'll route them through > > cgroup/for-3.17-fixes. > > Peter and Ingo, are these patches OK for you? Peter acked. I'll apply the patches to cgroup/for-3.17-fixes. Thanks. -- tejun ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-09-24 13:06 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-09-23 6:03 [PATCH v2 1/3] sched: fix confusing PFA_NO_NEW_PRIVS constant Zefan Li 2014-09-23 6:04 ` [PATCH v2 2/3] sched: add a macro to define bitops for task atomic flags Zefan Li 2014-09-23 6:37 ` Kees Cook 2014-09-23 6:52 ` Zefan Li 2014-09-23 6:05 ` [PATCH v2 3/3] cpuset: PF_SPREAD_PAGE and PF_SPREAD_SLAB should be " Zefan Li 2014-09-23 10:55 ` [PATCH v2 3/3] cpuset: PF_SPREAD_PAGE and PF_SPREAD_SLAB should beatomic flags Tetsuo Handa 2014-09-23 13:13 ` Tejun Heo 2014-09-24 3:01 ` Zefan Li 2014-09-24 11:44 ` [PATCH v2 3/3] cpuset: PF_SPREAD_PAGE and PF_SPREAD_SLAB should be atomic flags Tetsuo Handa 2014-09-24 13:06 ` Tejun Heo
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).