Something that's always been an issue in the utrace interface is the
management of struct utrace_attached_engine. It's tricky that you have to
use RCU and/or follow picayune rules in the attach+set_events->callbacks
sequence to be sure your engine pointer is valid. From the beginning, I
expected to revisit this aspect of the interface and think it through again
at some point. But I hadn't really thought about it in a long time now.
The discussion here on the asynchronous detach problem has helped me think
through the details that impinge on the concerns that had led me to write
it this way originally. I'm grateful to David and Ananth for the detailed
exchange that helps air these things out. Please keep it up!
We've been talking about all these corner cases with races, where I list
all the caveats for interactions with detach. What these all come down to
is that your engine pointer might have been freed. RCU can protect against
that, but it gets sticky if you want to block. The common mechanism for
dealing with this kind of issue in interfaces is reference counted objects.
In the original implementation, I was resistant to using reference counts
on engines for a few reasons.
One reason was the idea that the interface could make it hard to leak.
I do still think it's good that e.g. the interface not encourage you to
hold task refs (i.e. you don't need them because you'll get a final
report_reap callback before the struct goes away). But for the engine
structs, the option opposite leaks is to make the failure mode likely in
sloppy modules that they use a dangling engine pointer. That is most
likely more dangerous than leaks, and especially likely to do something
bad to someone else's engine (since the pointer will be reused). So
that objection to reference counts just doesn't hold any water.
Other concerns had to do with having lockless reporting loops, and
originally that seemed to require RCU for the structs anyway. But that
part of the implementation has been straightened out since. How we
manage the lockless list now is completely independent of RCU lists.
I've also objected to reference counts being touched for reporting,
and I still do. But they don't have to be.
The callback synchronization thing is simpler when we have an engine
pointer that can be relied on, even when you block. I'm guessing
everyone will be happy to see reference counts replace RCU in the
interface for dealing with struct utrace_attached_engine.
So here is what I'm thinking:
utrace_attach returns a ref for the caller. You can use
utrace_engine_get(engine) and utrace_engine_put(engine) to manage the
pointer. When you drop the last ref, the struct is freed. There are no
release/destructor callbacks for that, the refcount is purely for the
purpose of keeping the pointer valid.
An implicit ref is held while the engine is attached. The engine
pointer can always be used during a callback because of this ref. Your
callback can add a new ref even if you had dropped all other refs. If a
callback returns UTRACE_DETACH, then this ref will be dropped (and might
be the last).
Since it's easy to keep the engine pointer on hand, we really only need
one synchronizing call. So now there will be no *_sync calls after all.
utrace_set_events and utrace_control(,UTRACE_DETACH) can return
-EINPROGRESS as I described before. This means the change of event
mask, or the detach, will take effect--but there might be a callback
already in progress or unavoidably on its way. To block until that
callback is done, you can call utrace_barrier.
So now the safe detach procedure would be:
ret = utrace_control(task, engine, UTRACE_DETACH);
if (ret == -EINPROGRESS)
ret = utrace_barrier(task, engine);
if (ret == -ERESTARTSYS) /* interrupted wait, maybe callback pending */
return ret;
utrace_engine_put(engine);
I haven't really tested the new code at all. But this took not much
work at all to change in the implementation. If this is the right
direction, I'll give the code a modicum of testing and update the
documentation bits after another once-over on Monday.
Thanks,
Roland
