On 10/21, Peter Zijlstra wrote: > > On Mon, Oct 20, 2014 at 10:50:06PM +0200, Oleg Nesterov wrote: > > Let me explain what I personally dislike in v3: > > > > - I think that we do not have enough reasons for > > SLAB_DESTROY_BY_RCU. This is the serious change. > > What exactly would the downsides be? SDBR has very limited space > overhead iirc.
Yes, SDBR is nice (and it could probably have more users), but my concern is not overhead. Please see below. > > - Again, perhaps we should start we a simple and stupid fix. > > We can do get_task_struct() under rq->lock or, if nothing > > else, just > > > > raw_spin_lock_irq(&rq->lock); > > cur = rq->curr; > > if (is_idle_task(cur) || (cur->flags & PF_EXITING)) > > cur = NULL; > > raw_spin_unlock_irq(&rq->lock); > > I think I agree with you, this is the simple safe option and is > something we can easily backport. After that we can add creative bits on > top. Agreed. Kirill, could you please make a patch? > I think I prefer the SLAB_DESTROY_BY_RCU thing over the probe_kernel > thing I won't really insist, but let me try to explain why I dislike it in this particular case. - It is not clear who else (except task_numa_compare) will need it. And it looks at bit strange to make task_struct SLAB_DESTROY_BY_RCU just to read a word in task_numa_compare(). - In some sense, the usage of SDBR looks simply "wrong" in this case. IOW, I agree that probe_kernel_read() is ugly, but in this case SDBR acts exactly the same way as probe_kernel_read(). SDBR does not make the object rcu-safe, it only protects the object type plus ensures that this memory can't go away. It was designed for the case when you read the stable members initialized in ctor (usually a lock) and verify/lock the object. But in this case we can not detect that the object is still alive without the additional trick, so when you read ->sighand or ->flags, the fact that this memory is still "struct task_struct" doesn't help and doesn't matter at all. Only the subsequent "task == rq->curr" check proves that yes, this is task_struct. OTOH, (afaics) we only need probe_kernel_read() if CONFIG_DEBUG_SLAB. And in fact I think that "read the valid but potentially freed kernel pointer" deserves another helper, it can have more users. For example, please look at get_freepointer_safe(). What if we add get_kernel(x, ptr) macro to factor out the IS_ENABLED() of ifdef hack? Or inline function... This way the new xxx() helper we discussed won't look that bad. But again, I agree that this subjective, I won't really argue. Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
