On 10/15, Roland McGrath wrote:
>
> > 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.
Yes, I didn't claim this must be buggy. But yes, if we can't rely on
utrace_barrier() is the code like above, then perhaps it needs more
documentation to explain which exactly guarantees it provides.
> > 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?
I think yes, but s/smp_wmb/smp_mb/. "ptr = NULL" must be visible before
utrace_barrier() LOADs ->reporting, but wmb() can only serialize STOREs.
ptr = NULL;
// for utrace_barrier() which checks ->reporing
smp_mb();
utrace_barrier();
kfree();
Likewise,
my_report_callback()
{
if (ptr)
dereference(ptr);
// make sure the subsequent "->reporting == NULL
// can't be re-ordered.
smp_mb();
}
At least, this all is very unobvious.
Oleg.
