Re: [PATCH] sched/numa: fix unsafe get_task_struct() in task_numa_assign()

2014-10-20 Thread Kirill Tkhai
В Пн, 20/10/2014 в 11:13 +0200, Peter Zijlstra пишет: > OK, I think I'm finally awake enough to see what you're all talking > about :-) > > On Sun, Oct 19, 2014 at 09:37:44PM +0200, Oleg Nesterov wrote: > > > > RT tree has: > > > > > > > > https://git.kernel.org/cgit/linux/kernel/git/paulg/3.10-rt

Re: [PATCH] sched/numa: fix unsafe get_task_struct() in task_numa_assign()

2014-10-20 Thread Peter Zijlstra
OK, I think I'm finally awake enough to see what you're all talking about :-) On Sun, Oct 19, 2014 at 09:37:44PM +0200, Oleg Nesterov wrote: > > > RT tree has: > > > > > > https://git.kernel.org/cgit/linux/kernel/git/paulg/3.10-rt-patches.git/ > > > tree/patches/sched-delay-put-task.patch (answe

Re: [PATCH] sched/numa: fix unsafe get_task_struct() in task_numa_assign()

2014-10-20 Thread Kirill Tkhai
В Вс, 19/10/2014 в 21:43 +0200, Oleg Nesterov пишет: > On 10/19, Oleg Nesterov wrote: > > > > Forgot to mention... Or we can make task_struct_cachep SLAB_DESTROY_BY_RCU, > > in this case ->curr (or any other "task_struct *" ponter) can not go away > > under rcu_read_lock(). task_numa_compare() stil

Re: [PATCH] sched/numa: fix unsafe get_task_struct() in task_numa_assign()

2014-10-20 Thread Kirill Tkhai
В Вс, 19/10/2014 в 21:24 +0200, Oleg Nesterov пишет: > On 10/19, Kirill Tkhai wrote: > > > > 19.10.2014, 00:59, "Oleg Nesterov" : > > > > > No, I don't think this can work. Let's look at the current code: > > > > > > rcu_read_lock(); > > > cur = ACCESS_ONCE(dst_rq->curr); > > >

Re: [PATCH] sched/numa: fix unsafe get_task_struct() in task_numa_assign()

2014-10-20 Thread Kirill Tkhai
В Вс, 19/10/2014 в 23:38 +0200, Peter Zijlstra пишет: > On Sun, Oct 19, 2014 at 03:13:31AM +0400, Kirill Tkhai wrote: > > I'm too tired for all this, but: > > > + smp_rmb(); /* Pairs with dst_rq->lock unlocking which implies smp_wmb */ > > RELEASE does not imply a WMB. Thanks, please see, I'v

Re: [PATCH] sched/numa: fix unsafe get_task_struct() in task_numa_assign()

2014-10-19 Thread Peter Zijlstra
On Sun, Oct 19, 2014 at 03:13:31AM +0400, Kirill Tkhai wrote: I'm too tired for all this, but: > + smp_rmb(); /* Pairs with dst_rq->lock unlocking which implies smp_wmb */ RELEASE does not imply a WMB. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of

Re: [PATCH] sched/numa: fix unsafe get_task_struct() in task_numa_assign()

2014-10-19 Thread Oleg Nesterov
On 10/19, Oleg Nesterov wrote: > > Forgot to mention... Or we can make task_struct_cachep SLAB_DESTROY_BY_RCU, > in this case ->curr (or any other "task_struct *" ponter) can not go away > under rcu_read_lock(). task_numa_compare() still needs the PF_EXITING check, > but we do not need to recheck -

Re: [PATCH] sched/numa: fix unsafe get_task_struct() in task_numa_assign()

2014-10-19 Thread Oleg Nesterov
On 10/19, Oleg Nesterov wrote: > > On 10/19, Kirill Tkhai wrote: > > > > --- a/kernel/sched/fair.c > > +++ b/kernel/sched/fair.c > > @@ -1165,7 +1165,30 @@ static void task_numa_compare(struct task_numa_env > > *env, > > > > rcu_read_lock(); > > cur = ACCESS_ONCE(dst_rq->curr); > > - if

Re: [PATCH] sched/numa: fix unsafe get_task_struct() in task_numa_assign()

2014-10-19 Thread Oleg Nesterov
On 10/19, Kirill Tkhai wrote: > > 19.10.2014, 00:59, "Oleg Nesterov" : > > >  No, I don't think this can work. Let's look at the current code: > > > >  rcu_read_lock(); > >  cur = ACCESS_ONCE(dst_rq->curr); > >  if (cur->pid == 0) /* idle */ > > > >  And any dereference, eve

Re: [PATCH] sched/numa: fix unsafe get_task_struct() in task_numa_assign()

2014-10-19 Thread Kirill Tkhai
On 18.10.2014 23:36, Peter Zijlstra wrote: > On Sat, Oct 18, 2014 at 12:33:27PM +0400, Kirill Tkhai wrote: >> How about this? >> >> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c >> index b78280c..d46427e 100644 >> --- a/kernel/sched/fair.c >> +++ b/kernel/sched/fair.c >> @@ -1165,7 +1165,2

Re: [PATCH] sched/numa: fix unsafe get_task_struct() in task_numa_assign()

2014-10-18 Thread Kirill Tkhai
19.10.2014, 00:59, "Oleg Nesterov" : >  On 10/18, Kirill Tkhai wrote: >>   18.10.2014, 01:40, "Oleg Nesterov" : >>>   ... >>>   The >>>   task_struct itself can't go away, >>>   ... >>>   --- a/kernel/sched/fair.c >>>   +++ b/kernel/sched/fair.c >>>   @@ -1158,7 +1158,13 @@ static void task_numa_co

Re: [PATCH] sched/numa: fix unsafe get_task_struct() in task_numa_assign()

2014-10-18 Thread Oleg Nesterov
On 10/18, Peter Zijlstra wrote: > > So you worry about the refcount doing 0->1 ? In which case the above is > still wrong and we should be using atomic_inc_not_zero() in order to > acquire the reference count. It is actually worse, please see my reply to Kirill. We simply can't dereference foreign

Re: [PATCH] sched/numa: fix unsafe get_task_struct() in task_numa_assign()

2014-10-18 Thread Oleg Nesterov
On 10/18, Kirill Tkhai wrote: > > 18.10.2014, 01:40, "Oleg Nesterov" : > > ... > > The > > task_struct itself can't go away, > > ... > > --- a/kernel/sched/fair.c > > +++ b/kernel/sched/fair.c > > @@ -1158,7 +1158,13 @@ static void task_numa_compare(struct task_numa_env > > *env, > > > >  

Re: [PATCH] sched/numa: fix unsafe get_task_struct() in task_numa_assign()

2014-10-18 Thread Peter Zijlstra
On Sat, Oct 18, 2014 at 12:33:27PM +0400, Kirill Tkhai wrote: > How about this? > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > index b78280c..d46427e 100644 > --- a/kernel/sched/fair.c > +++ b/kernel/sched/fair.c > @@ -1165,7 +1165,21 @@ static void task_numa_compare(struct task_numa

Re:[PATCH] sched/numa: fix unsafe get_task_struct() in task_numa_assign()

2014-10-18 Thread Kirill Tkhai
And smp_rmb() beetween ifs which is pairs with rq unlocking > 18.10.2014, 12:15, "Kirill Tkhai" : > >> 18.10.2014, 01:40, "Oleg Nesterov" : >> >>> The lockless get_task_struct(tsk) is only safe if tsk == current >>> and didn't pass exit_notify(), or if this tsk was found on a rcu >>> protected li

Re: [PATCH] sched/numa: fix unsafe get_task_struct() in task_numa_assign()

2014-10-18 Thread Kirill Tkhai
18.10.2014, 12:15, "Kirill Tkhai" : > 18.10.2014, 01:40, "Oleg Nesterov" : >>  The lockless get_task_struct(tsk) is only safe if tsk == current >>  and didn't pass exit_notify(), or if this tsk was found on a rcu >>  protected list (say, for_each_process() or find_task_by_vpid()). >>  IOW, it is on

Re: [PATCH] sched/numa: fix unsafe get_task_struct() in task_numa_assign()

2014-10-18 Thread Kirill Tkhai
18.10.2014, 01:40, "Oleg Nesterov" : > The lockless get_task_struct(tsk) is only safe if tsk == current > and didn't pass exit_notify(), or if this tsk was found on a rcu > protected list (say, for_each_process() or find_task_by_vpid()). > IOW, it is only safe if release_task() was not called befor