WR? Did you mean RW, so they could be reset?

Isn't there an smr_lazy_advance() race here, where

• A reads ticks=X
• B reads ticks=X+1, proceeds, advances s->s_wr_seq to X+1
• A reads s->s_wr_seq=X+1, and then "advances" (reverts) it back to X

and is it a problem?

If so, I think we need to read ticks "after" s->s_wr_seq. I don't think we could just use SMR_SEQ_LEQ(t, s_wr_seq) as I think that would lead to the idle wrap problem.

Ought to add more parens around a and b.

I think you are right. Originally I didn't consider this but it would happen if ticks was ahead of wr_seq on an otherwise idle smr. And an acq barrier with a rel on the cmpset should resolve it.

sending wr_seq backwards and immediately polling would leave us considering a goal ticks calculated from the larger value as already expired.

I also wanted to add, I tried to write this as:

if s->s_ticks == ticks return s_wr_seq
monotonically increment s_wr_seq by SMR_SEQ_INCR,
s->s_ticks = ticks

I could not work out a way to do this safely with only barriers. I could use a 64bit cmpxchg. I could also use 64bit cmpxchg to fix s_rd_seq in a single atomic. I would prefer to keep the flexibility to make the counters 64bit instead though. I think the only s_rd_seq race below would harmlessly rewind it a little bit. If this were userspace and I could make fewer guarantees I would use the 64bit cmpxchg trick.

still need to do this.

Unused...

We grew a critical section around smr_default_advance (I assume you're okay with that...), so we can apply Mark's comments from the earlier review and s/counter_u64_add/counter_u64_add_protected/ (all three instances under advance).

GT

I don't understand this. Shouldn't the writer have called smr_advance()? Where did the goal come from if not?

s/SMR_SEQ_MAX_ADVANCE/SMR_SEQ_ADVANCE/ ?

Hmm. I think perhaps this comment should be expanded. I think what you're saying is, for SMR_DEFERRED, we can only be behind a valid goal by SMR_SEQ_INCR. For SMR_LAZY, we can be further behind, but we never have to catch up because we're always !wait. Is that right? Because it does seem possible that we drop out of this with goal > s_wr_seq for the SMR_LAZY case.

That is a good point. I am a little uncertain as to whether advance will continue to use critical. I will evaluate this again once I decide.

lazy only increments wr_seq lazily after ticks does. The goal is always given as later than the current ticks. So it's always in the future. If no other writers follow wr_seq won't be advanced and the goal will never be satisfied. This makes sure the clock is always moving as long as someone cares.

Yes thank you.

KASSERT(!wait || (zpcpu_get(smr)->c_flags & SMR_LAZY) == 0,

	    ("smr_poll: Blocking not allowed on lazy smrs."));

There is an assert at the top of the function. I'm considering a way to decouple the wr_seq from directly using ticks that would enable me to use IPI to advance if there is a blocking wait for LAZY. This may impact the critcal sections in advance.