Introduce the new PF_KTHREAD flag to mark the kernel threads. It is set by
INIT_TASK() and copied to the forked childs (we could set it in kthreadd()
along with PF_NOFREEZE instead).
daemonize() was changed as well. In that case testing of PF_KTHREAD is racy,
but daemonize() is hopeless anyway.
This flag is cleared in do_execve(), before search_binary_handler(). Probably
not the best place, we can do this in exec_mmap() or in start_thread(), or
clear it along with PF_FORKNOEXEC. But I think this doesn't matter in practice,
and if do_execve() fails kthread should die soon.
Signed-off-by: Oleg Nesterov <[email protected]>
include/linux/sched.h | 1 +
include/linux/init_task.h | 2 +-
kernel/exit.c | 2 +-
fs/exec.c | 1 +
4 files changed, 4 insertions(+), 2 deletions(-)
--- 26-rc2/include/linux/sched.h~2_MAKE_PF_KTHREAD 2008-05-18 15:44:16.000000000 +0400
+++ 26-rc2/include/linux/sched.h 2008-05-18 20:08:13.000000000 +0400
@@ -1508,6 +1508,7 @@ static inline void put_task_struct(struc
#define PF_MEMPOLICY 0x10000000 /* Non-default NUMA mempolicy */
#define PF_MUTEX_TESTER 0x20000000 /* Thread belongs to the rt mutex tester */
#define PF_FREEZER_SKIP 0x40000000 /* Freezer should not count it as freezeable */
+#define PF_KTHREAD 0x80000000 /* I am a kernel thread */
/*
* Only the _current_ task can read/write to tsk->flags, but other
--- 26-rc2/include/linux/init_task.h~2_MAKE_PF_KTHREAD 2008-05-18 15:44:15.000000000 +0400
+++ 26-rc2/include/linux/init_task.h 2008-05-18 20:14:30.000000000 +0400
@@ -143,7 +143,7 @@ extern struct group_info init_groups;
.state = 0, \
.stack = &init_thread_info, \
.usage = ATOMIC_INIT(2), \
- .flags = 0, \
+ .flags = PF_KTHREAD, \
.lock_depth = -1, \
.prio = MAX_PRIO-20, \
.static_prio = MAX_PRIO-20, \
--- 26-rc2/kernel/exit.c~2_MAKE_PF_KTHREAD 2008-05-18 15:44:18.000000000 +0400
+++ 26-rc2/kernel/exit.c 2008-05-18 18:11:13.000000000 +0400
@@ -416,7 +416,7 @@ void daemonize(const char *name, ...)
* We don't want to have TIF_FREEZE set if the system-wide hibernation
* or suspend transition begins right now.
*/
- current->flags |= PF_NOFREEZE;
+ current->flags |= (PF_NOFREEZE | PF_KTHREAD);
if (current->nsproxy != &init_nsproxy) {
get_nsproxy(&init_nsproxy);
--- 26-rc2/fs/exec.c~2_MAKE_PF_KTHREAD 2008-05-18 15:44:00.000000000 +0400
+++ 26-rc2/fs/exec.c 2008-05-18 18:36:30.000000000 +0400
@@ -1317,6 +1317,7 @@ int do_execve(char * filename,
if (retval < 0)
goto out;
+ current->flags &= ~PF_KTHREAD;
retval = search_binary_handler(bprm,regs);
if (retval >= 0) {
/* execve success */
On Sun, 1 Jun 2008 19:30:42 +0400
Oleg Nesterov <[email protected]> wrote:
> Introduce the new PF_KTHREAD flag to mark the kernel threads. It is set by
> INIT_TASK() and copied to the forked childs (we could set it in kthreadd()
> along with PF_NOFREEZE instead).
>
> daemonize() was changed as well. In that case testing of PF_KTHREAD is racy,
> but daemonize() is hopeless anyway.
>
> This flag is cleared in do_execve(), before search_binary_handler(). Probably
> not the best place, we can do this in exec_mmap() or in start_thread(), or
> clear it along with PF_FORKNOEXEC. But I think this doesn't matter in practice,
> and if do_execve() fails kthread should die soon.
The changelog doesn't explain why this change is being made, and I
wasn't able to work that out.
Similarly, I can kinda see what benefit "[PATCH 2/3] kill
PF_BORROWED_MM in favour of PF_KTHREAD" is bringing us, but it would be
nice to see that spelled out please.
On 06/03, Andrew Morton wrote:
>
> On Sun, 1 Jun 2008 19:30:42 +0400
> Oleg Nesterov <[email protected]> wrote:
>
> > Introduce the new PF_KTHREAD flag to mark the kernel threads. It is set by
> > INIT_TASK() and copied to the forked childs (we could set it in kthreadd()
> > along with PF_NOFREEZE instead).
> >
> > daemonize() was changed as well. In that case testing of PF_KTHREAD is racy,
> > but daemonize() is hopeless anyway.
> >
> > This flag is cleared in do_execve(), before search_binary_handler(). Probably
> > not the best place, we can do this in exec_mmap() or in start_thread(), or
> > clear it along with PF_FORKNOEXEC. But I think this doesn't matter in practice,
> > and if do_execve() fails kthread should die soon.
>
> The changelog doesn't explain why this change is being made, and I
> wasn't able to work that out.
Currently it is not possible to see if the task is kthread or not.
In most cases we don't care, all we need to know is it has ->mm or not, but..
> Similarly, I can kinda see what benefit "[PATCH 2/3] kill
> PF_BORROWED_MM in favour of PF_KTHREAD" is bringing us, but it would be
> nice to see that spelled out please.
we can't use PF_BORROWED_MM for that without task_lock(), no matter how
many barriers we add/use.
The 3rd patch is just an example. We have a lot of "p->mm != NULL" checks
in (say) oom_kill.c, and they are not right. Without PF_KTHREAD which can
be checked lockless, we need something like
int has_mm(struct task_struct *p)
{
task_lock(p);
ret = p->mm && !(p->flags & PF_BORROWED_MM);
task_unlock(p);
return ret;
}
In short, PF_KTHREAD is more flexible and doesn't need task_lock(). If
it it cleared after the check, the task has the new ->mm.
Oleg.
On Sun, 1 Jun 2008 19:30:42 +0400
Oleg Nesterov <[email protected]> wrote:
> Introduce the new PF_KTHREAD flag to mark the kernel threads. It is set by
> INIT_TASK() and copied to the forked childs (we could set it in kthreadd()
> along with PF_NOFREEZE instead).
>
> daemonize() was changed as well. In that case testing of PF_KTHREAD is racy,
> but daemonize() is hopeless anyway.
>
> This flag is cleared in do_execve(), before search_binary_handler(). Probably
> not the best place, we can do this in exec_mmap() or in start_thread(), or
> clear it along with PF_FORKNOEXEC. But I think this doesn't matter in practice,
> and if do_execve() fails kthread should die soon.
>
> Signed-off-by: Oleg Nesterov <[email protected]>
>
> include/linux/sched.h | 1 +
> include/linux/init_task.h | 2 +-
> kernel/exit.c | 2 +-
> fs/exec.c | 1 +
> 4 files changed, 4 insertions(+), 2 deletions(-)
>
> --- 26-rc2/include/linux/sched.h~2_MAKE_PF_KTHREAD 2008-05-18 15:44:16.000000000 +0400
> +++ 26-rc2/include/linux/sched.h 2008-05-18 20:08:13.000000000 +0400
> @@ -1508,6 +1508,7 @@ static inline void put_task_struct(struc
> #define PF_MEMPOLICY 0x10000000 /* Non-default NUMA mempolicy */
> #define PF_MUTEX_TESTER 0x20000000 /* Thread belongs to the rt mutex tester */
> #define PF_FREEZER_SKIP 0x40000000 /* Freezer should not count it as freezeable */
> +#define PF_KTHREAD 0x80000000 /* I am a kernel thread */
Well "Freezer: Introduce PF_FREEZER_NOSIG" has cheerily come in
afterwards and stolen 0x80000000 from us. I'll redo this patch to use,
umm, 0x00000020. Please check that this is OK (if it isn't someone
needs thwapping for not adding a dont-use-this comment).
On Mon, 23 Jun 2008 13:40:37 -0700
Andrew Morton <[email protected]> wrote:
> On Sun, 1 Jun 2008 19:30:42 +0400
> Oleg Nesterov <[email protected]> wrote:
>
> > Introduce the new PF_KTHREAD flag to mark the kernel threads. It is set by
> > INIT_TASK() and copied to the forked childs (we could set it in kthreadd()
> > along with PF_NOFREEZE instead).
> >
> > daemonize() was changed as well. In that case testing of PF_KTHREAD is racy,
> > but daemonize() is hopeless anyway.
> >
> > This flag is cleared in do_execve(), before search_binary_handler(). Probably
> > not the best place, we can do this in exec_mmap() or in start_thread(), or
> > clear it along with PF_FORKNOEXEC. But I think this doesn't matter in practice,
> > and if do_execve() fails kthread should die soon.
> >
> > Signed-off-by: Oleg Nesterov <[email protected]>
> >
> > include/linux/sched.h | 1 +
> > include/linux/init_task.h | 2 +-
> > kernel/exit.c | 2 +-
> > fs/exec.c | 1 +
> > 4 files changed, 4 insertions(+), 2 deletions(-)
> >
> > --- 26-rc2/include/linux/sched.h~2_MAKE_PF_KTHREAD 2008-05-18 15:44:16.000000000 +0400
> > +++ 26-rc2/include/linux/sched.h 2008-05-18 20:08:13.000000000 +0400
> > @@ -1508,6 +1508,7 @@ static inline void put_task_struct(struc
> > #define PF_MEMPOLICY 0x10000000 /* Non-default NUMA mempolicy */
> > #define PF_MUTEX_TESTER 0x20000000 /* Thread belongs to the rt mutex tester */
> > #define PF_FREEZER_SKIP 0x40000000 /* Freezer should not count it as freezeable */
> > +#define PF_KTHREAD 0x80000000 /* I am a kernel thread */
>
> Well "Freezer: Introduce PF_FREEZER_NOSIG" has cheerily come in
> afterwards and stolen 0x80000000 from us. I'll redo this patch to use,
> umm, 0x00000020. Please check that this is OK (if it isn't someone
> needs thwapping for not adding a dont-use-this comment).
>
OK, it seems that in a later patch we switched to 0x00200000 anyway.
"Freezer: Introduce PF_FREEZER_NOSIG" has trashed this patch
series so I'll need to drop introduce-pf_kthread-flag.patch and
kill-pf_borrowed_mm-in-favour-of-pf_kthread.patch and
coredump-zap_threads-must-skip-kernel-threads.patch.
I don't yet know how much additional damage will happen as a result.
On Mon, 23 Jun 2008 13:47:06 -0700
Andrew Morton <[email protected]> wrote:
> I don't yet know how much additional damage will happen as a result.
Lots.
I restored the patches and just dropped the hunk:
static int has_mm(struct task_struct *p)
{
- return (p->mm && !(p->flags & PF_BORROWED_MM));
}
/**
--- 86,92 ----
static int has_mm(struct task_struct *p)
{
+ return (p->mm && !(p->flags & PF_KTHREAD));
}
due to that function having been turned into:
static inline bool should_send_signal(struct task_struct *p)
{
return !(p->flags & PF_FREEZER_NOSIG);
}
Please check the result?
On Monday, 23 of June 2008, Andrew Morton wrote:
> On Mon, 23 Jun 2008 13:47:06 -0700
> Andrew Morton <[email protected]> wrote:
>
> > I don't yet know how much additional damage will happen as a result.
>
> Lots.
Well, sorry about that. I can ask Len to drop the freezer patch temporarily
and rebase it on top of -next if that helps.
On Mon, 23 Jun 2008 23:36:59 +0200
"Rafael J. Wysocki" <[email protected]> wrote:
> On Monday, 23 of June 2008, Andrew Morton wrote:
> > On Mon, 23 Jun 2008 13:47:06 -0700
> > Andrew Morton <[email protected]> wrote:
> >
> > > I don't yet know how much additional damage will happen as a result.
> >
> > Lots.
>
> Well, sorry about that. I can ask Len to drop the freezer patch temporarily
> and rebase it on top of -next if that helps.
The freezer patch is already in linux-next. Assuming those changes are
planned for 2.6.27 then it's best to leave things as they are.
Some of these problems will go away once I get most-of-mm into
linux-next. But it will mean that people will need to start weaning
themselves off the practice of preparing patches against Linus
mainline.
On 06/23, Andrew Morton wrote:
>
> On Mon, 23 Jun 2008 13:47:06 -0700
> Andrew Morton <[email protected]> wrote:
>
> > I don't yet know how much additional damage will happen as a result.
>
> Lots.
>
> I restored the patches and just dropped the hunk:
>
> static int has_mm(struct task_struct *p)
> {
> - return (p->mm && !(p->flags & PF_BORROWED_MM));
> }
>
> /**
> --- 86,92 ----
>
> static int has_mm(struct task_struct *p)
> {
> + return (p->mm && !(p->flags & PF_KTHREAD));
> }
>
> due to that function having been turned into:
>
> static inline bool should_send_signal(struct task_struct *p)
> {
> return !(p->flags & PF_FREEZER_NOSIG);
> }
>
> Please check the result?
Thanks, this looks OK.
Rafael, can't freezer just use PF_KTHREAD (which btw kills PF_BORROWED_MM)
instead of the new PF_FREEZER_NOSIG flag? They look very similar, please
look at
"[PATCH 1/3] introduce PF_KTHREAD flag"
http://marc.info/?l=linux-kernel&m=121233423530812
"[PATCH 2/3] kill PF_BORROWED_MM in favour of PF_KTHREAD"
http://marc.info/?l=linux-kernel&m=121233423530820
Oleg.
On Tuesday, 24 of June 2008, Oleg Nesterov wrote:
> On 06/23, Andrew Morton wrote:
> >
> > On Mon, 23 Jun 2008 13:47:06 -0700
> > Andrew Morton <[email protected]> wrote:
> >
> > > I don't yet know how much additional damage will happen as a result.
> >
> > Lots.
> >
> > I restored the patches and just dropped the hunk:
> >
> > static int has_mm(struct task_struct *p)
> > {
> > - return (p->mm && !(p->flags & PF_BORROWED_MM));
> > }
> >
> > /**
> > --- 86,92 ----
> >
> > static int has_mm(struct task_struct *p)
> > {
> > + return (p->mm && !(p->flags & PF_KTHREAD));
> > }
> >
> > due to that function having been turned into:
> >
> > static inline bool should_send_signal(struct task_struct *p)
> > {
> > return !(p->flags & PF_FREEZER_NOSIG);
> > }
> >
> > Please check the result?
>
> Thanks, this looks OK.
>
> Rafael, can't freezer just use PF_KTHREAD (which btw kills PF_BORROWED_MM)
> instead of the new PF_FREEZER_NOSIG flag? They look very similar, please
> look at
>
> "[PATCH 1/3] introduce PF_KTHREAD flag"
> http://marc.info/?l=linux-kernel&m=121233423530812
>
> "[PATCH 2/3] kill PF_BORROWED_MM in favour of PF_KTHREAD"
> http://marc.info/?l=linux-kernel&m=121233423530820
The problem is that some kernel threads may actually want to clear
PF_FREEZER_NOSIG, but it would be invalid to clear PF_KTHREAD I think.
Hmm, well, in principle we could use two flags for that, with the combinations
of bits defined as follows:
11 - user space task (freezable with a fake signal)
10 - kernel thread freezable with a fake signal
01 - kernel thread freezable withoug a fake signal
00 - non-freezable kernel thread
Thanks,
Rafael