[PATCH 2/4] utrace_reap: do not play with ->reporting

[Oleg Nesterov]

On 09/07, Roland McGrath wrote:
>
> > But more importantly, I think utrace_reap() never needs to synchronize
> > with utrace_barrier(). Once we drop utrace->lock, any other caller which
> > takes this lock must see both ->exit_state and utrace->reap. This means
> > utrace_control() or utrace_set_events() which sets/clears REAP must not
> > succeed.
>
> Right.  So this doesn't matter for the primary original purpose of
> utrace_barrier, i.e.:
>
>  * A successful return from utrace_barrier() guarantees its ordering
>  * with respect to utrace_set_events() and utrace_control() calls.

Yes.

> But IMHO it would be nice (and the least surprising thing) if this
> statement were really true too:
>
>  * A return value of zero means no callback from @target to @engine was
>  * in progress.

I was going to write that this doesn't make real sense, but now I think
you are right. For example,

	something_t *ptr;

	void my_report_reap(...)
	{
		if (ptr)
			dereference(ptr);
	}

	...

	void *tmp = ptr;
	ptr = NULL;
	utrace_barrier(my_engine);
	kfree(tmp);

With or without recent changes, this doesn't work.

> I'm not really sure about the barriers,

Me too.

In fact, recently I realized I don't really understand the barriers
around ->reporting, need to read this code with a fresh head. In
particular, I'd like to see

	1. Can't we set ->reporting _after_ checking engine->flags ?
	   (unlikely)

	2. Is utrace_barrier() correct???? Note the example above,
	   it assumes that utrace_barrier() itself is the barrier
	   wrt reporting. But, is it?

>  	list_for_each_entry_safe(engine, next, &utrace->attached, entry) {
> -		if (engine->flags & UTRACE_EVENT(REAP))
> +		if (engine->flags & UTRACE_EVENT(REAP)) {
> +			/*
> +			 * These barriers order anything ->report_reap()
> +			 * does to be while ->reporting is set.  That way
> +			 * any utrace_barrier() call that returns 0 is
> +			 * guaranteeing that the callback is complete.
> +			 */
> +			utrace->reporting = engine;
> +			smp_mb();
>  			engine->ops->report_reap(engine, target);
> +			smp_mb();
> +			utrace->reporting = NULL;

Agreed.


And yes, now it is important to clear ->ops _after_ calling
->report_reap(), otherwise utrace_barrier() can take the lock.

Oleg.