> I still didn't find the time to read the code around set/clear ->reporting,
> it is subtle and needs a fresh head.
Ok. I had remembered that your earlier reviews included "review all use of
memory barriers in utrace", so I didn't realize this stuff needed review.
> But at least, I think finish_callback() needs an mb() before clearing
> ->reporting.
My thinking about that had been that ->reporting = NULL is the "all clear"
signal and we didn't have any issues with thinking the coast was clear too
late, only too soon. Since many engines' reports won't ever really care
about utrace_barrier() synchronization at all, I wanted to be sure not to
introduce any more barriers or other parallelism-hurting things into the
unconditional hot paths in reporting loops.
> We have a callback
>
> void my_report_callback(...)
> {
> if (ptr)
> dereference(ptr);
> }
>
> And the tracer does:
>
> void *tmp = ptr;
> ptr = NULL;
> utrace_barrier(my_engine);
> kfree(tmp);
>
> This should work correctly, right?
I never really thought about memory barriers for the callers own data.
utrace_barrier() just guarantees that your callback has returned and its
return value been processed (ordering them with respect to utrace_* calls
after utrace_barrier), nothing more. My first reaction is to think the
caller can do its own memory barriers when it wants to rely on them. But
perhaps utrace_barrier() should do more implicitly, and be documented to.
> But, I think this is racy because finish_callback() clears ->reporting
> without the barrier. this means (in theory) utrace_barrier() can see
> ->reporting == NULL before my_report_callback() sees ptr == NULL. And
> perhaps utrace_barrier() itself needs a barrier.
So today it would have to be:
void my_report_callback(...)
{
if (ptr)
dereference(ptr);
smp_rmb();
}
void *tmp = ptr;
ptr = NULL;
smp_wmb();
utrace_barrier(my_engine);
kfree(tmp);
Is that right?
Thanks,
Roland
