It should be spelled out and not padded. February 2, 2016

style: assign outside of declaration.

style: empty line needed

style: empty line needed

style: empty line needed

We

Convert.

Also suggest moving 'int timo' above this with a blank line after.

whitespace change

style consistency, no braces needed here.

period

period

style: empty line needed here

The existing style in the file seems to be 8 space line continuation, so I was synchronizing it with that. This seems fine.

Dang wrong name here. Fixed.

Usually sys/systm.h is the second included header, first being sys/param.h. Move it to the start of the #include blocks, before sys/ctype.h.

Style: space around binary op.

Why do you need explicitely sized type there ?

Style: no space between start and function name.

why is the TAILQ_EMPTY check needed ?

Are parallel modifications to the fe_entries list possible ?

Why _rel semantic is needed there ?

We usually say 'triggering' in this situation.

This really belongs to the declarations block, according to style.

At this moment, the thread lock should be equal to the sleepq_chain lock. Your thread_lock() recurses, it seems. Why do you need the thread lock there at all ?

Why do you need explicitely sized type there ?

Moving it caused the build to break, but it looks like it is superfluous anyway. I'm just going to remove that include.

Not needed. Changed back to int.

It isn't. Removed.

No; once fe_entries is created it is read only until refs go to 0, at which point at most one thread can garbage collect it. Having a single garbage collector is ensured by locking done during calls to the sysctl handler. What CAN change is:

• Each fail_point_entry in the list can have atomic operations performed on them
• The pointer in the fail point to a different fe_entries can be atomically swapped.

So the list itself is never modified in parallel.

Actually this should be acq: we don't want any accesses ordered before we increment here. Thanks for pointing that out.

Ah I didn't spot that lock dance. Great; I'll take the explicit lock/unlock of the thread lock out and replace it with a comment so it is bloody clear to the reader we hold it. Thanks.

Not necessary. I cleaned up the whitespace in this area too.

Why the barrier here? (Also, you should compare against 0, not use '!' per style(9)).

Why the barrier here?

What other memory operations are these barriers relative to? Also, this algorithm seems racy. Just because it's > 0 now doesn't mean it is still > 0 when you do the subtraction. You should instead use a loop with cmpset or possibly use fetchadd, e.g.:

val = ent->fe_count;
while (val > 0) {
    if (atomic_cmpset_32(&ent->fe_count, val, val - 1)) {
        val--;
        break;
    }
    val = ent->fe_count;
}
if (val == 0)
    ent->fe_stale = true;

Or using fetchadd:

val = ent->fe_count;
if (val > 0)
    val = atomic_fetchadd_32(&ent->fe_count, -1) - 1;
if (val <= 0)
    ent->fe_stale = true;

The first one would never allow fe_count to go negative. The second one would.

Fixed style issue.

This is the only form of load available in atomic.h according to atomic(9).

<type>
atomic_load_acq_<type>(volatile <type> *p);

I used an atomic load here out of an abundance of caution. My thought is that in an SMP environment I want to make sure that a modification of the refcnt cannot jump this load. Given this is garbage collection, though, I'm not sure it matters too much. It doesn't affect correctness either way since we are no longer linked to the fail point: the worst thing that would happen I think is that we wouldn't garbage collect something we otherwise could. So perhaps we can't justify the performance hit of a memory barrier here. John - does that sound right to you?

This is for the same reason as above: this is the only form of atomic load. But I think in this case that is more caution than is needed: if a store get reordered we are just going to loop again. I'm just going to remove the atomicity of the load. Does that make sense to you as well?

Right, so if the count becomes negative we know the entry is stale, just as though it was 0. We could end up setting stale to 'true' twice, but that is obviously idempotent. So this is a bit weird, but it is simple and not incorrect I think.

What none of the above solutions solve is that we can end up executing this entry more times than intended because we already checked the stale field above. At the time I designed this I wanted to side with simplicity because of how complex this function is becoming, so I accepted that. But perhaps I need to revisit that decision now. I could use a modified version of your first algorithm for this.

if (ent->fe_stale)
      continue;
if (....UNTRACKED) {
 val = ent->fe_count;
 execute = false
 while (val > 0) {
    if (atomic_cmpset_32(&ent->fe_count, val, val - 1)) {
        val--;
        execute = true;
        break;
    }
    val = ent->fe_count;
 }
 if (execute == false)
    /* We lost the race; consider the entry stale and bail now */
    continue;
 if (val == 0)
    ent->fe_stale = true;

}

If you don't need a barrier, just use x = y. There isn't an atomic op for simple loads and stores as you can do that with normal assignment (if your variable is volatile).

The barrier doesn't do anything to prevent some other CPU modifying the ref count or ordering the write of that other CPU with respect to your read. The barrier only prevents operations that this thread/CPU is doing from moving relative to the read of the ref_count. In this case there doesn't seem to be any clear relation of the operations after this relative to the read.

Yes.

Is this trying to only trigger the fail point N times? If so, I think your new code is fine.

Awesome background! Thanks John.

Yep.