I forgot about this one...
Once again, I agree with no more changes, but could you comment this
patch?
This looks like a bug to me. If it is not, I'd like to understand the
rationale behind the "spin until s/detached/NULL/".
On 08/19, Oleg Nesterov wrote:
>
> -------------------------------------------------------------------------------
> [PATCH v2] utrace_barrier(detached_engine) must not sping without checking
> ->reporting
>
> If engine is detached (has utrace_detached_ops), utrace_barrier(engine)
> spins until engine->ops becomes NULL. This is just wrong.
>
> Suppose that utrace_control(DETACH) returns -EINPROGRESS, now we should
> call utrace_barrier(). However, it is possible that -EINPROGRESS means
> we raced with, say, sys_sleep(A_LOT) doing report_syscall_entry().
>
> If start_callback() notices utrace_detached_ops and sets report->detaches
> everything is fine. But it is quite possible that this doesn't happen,
> and in this case utrace_barrier() will spin "forever" waiting for the
> next utrace_reset().
>
> Change get_utrace_lock() to succeed if the caller is utrace_barrier()
> and ops == &utrace_detached_ops. I do not see any reason why this case
> should be special from utrace_barrier's pov. It can just check
> ->reporting and return 0 or do another iteration.
>
> Note: we should also reconsider() utrace_barrier()->signal_pending()
> check. Also, it is not clear why utrace_barrier() needs utrace->lock,
> except to ensure it is safe to dereference target/utrace.
>
> Signed-off-by: Oleg Nesterov <o...@redhat.com>
> ---
>
> kernel/utrace.c | 11 ++++-------
> 1 file changed, 4 insertions(+), 7 deletions(-)
>
> --- kstub/kernel/utrace.c~9_utrace_barrier_and_detached 2010-08-18
> 17:47:53.000000000 +0200
> +++ kstub/kernel/utrace.c 2010-08-19 15:33:06.000000000 +0200
> @@ -416,23 +416,21 @@ static struct utrace *get_utrace_lock(st
> return ERR_PTR(-ESRCH);
> }
>
> - if (unlikely(engine->ops == &utrace_detached_ops)) {
> + if (attached && unlikely(engine->ops == &utrace_detached_ops)) {
> rcu_read_unlock();
> - return attached ? ERR_PTR(-ESRCH) : ERR_PTR(-ERESTARTSYS);
> + return ERR_PTR(-ESRCH);
> }
>
> utrace = task_utrace_struct(target);
> spin_lock(&utrace->lock);
> if (unlikely(utrace->reap) || unlikely(!engine->ops) ||
> - unlikely(engine->ops == &utrace_detached_ops)) {
> + (attached && unlikely(engine->ops == &utrace_detached_ops))) {
> /*
> * By the time we got the utrace lock,
> * it had been reaped or detached already.
> */
> spin_unlock(&utrace->lock);
> utrace = ERR_PTR(-ESRCH);
> - if (!attached && engine->ops == &utrace_detached_ops)
> - utrace = ERR_PTR(-ERESTARTSYS);
> }
> rcu_read_unlock();
>
> @@ -1286,8 +1284,7 @@ int utrace_barrier(struct task_struct *t
> utrace = get_utrace_lock(target, engine, false);
> if (unlikely(IS_ERR(utrace))) {
> ret = PTR_ERR(utrace);
> - if (ret != -ERESTARTSYS)
> - break;
> + break;
> } else {
> /*
> * All engine state changes are done while
