riscv: small counter(9) improvements

[mhorne]

Summary

• Prefer atomics to the critical section.
• Use CPU_FOREACH to skip absent CPUs

Seems that a nearly-identical change was made for arm64 in r313345.

[markj]

To be clear, the main motivation here and in r313345 is to ensure that zeroing is reliable? In the old version of this diff it is not, per the XXX comment, but neither the CR description or the commit log message for r313345 mention this explicitly, so maybe I'm missing something else.

[mhorne]

In D27536#615494, @markj wrote:

To be clear, the main motivation here and in r313345 is to ensure that zeroing is reliable? In the old version of this diff it is not, per the XXX comment, but neither the CR description or the commit log message for r313345 mention this explicitly, so maybe I'm missing something else.

Presumably you are asking for the exact meaning behind the XXXKIB comment?

Could you explain for me what makes the existing code unreliable for zeroing? It's not obvious to me why the critical section was not enough to protect the increment.

[markj]

In D27536#615591, @mhorne wrote:

In D27536#615494, @markj wrote:

To be clear, the main motivation here and in r313345 is to ensure that zeroing is reliable? In the old version of this diff it is not, per the XXX comment, but neither the CR description or the commit log message for r313345 mention this explicitly, so maybe I'm missing something else.

Presumably you are asking for the exact meaning behind the XXXKIB comment?

Could you explain for me what makes the existing code unreliable for zeroing? It's not obvious to me why the critical section was not enough to protect the increment.

counter_u64_zero_inline() works by raising an interrupt on each CPU; the interrupt handler calls counter_u64_zero_one_cpu() to clear the counter for that CPU.

In the old version of the diff this may fail to work because counter_u64_add() is not atomic with respect to interrupts. It loads the value into a register, modifies it, and stores the result. If the value is zeroed between the load and store, it will get reverted when the interrupt finishes and the interrupted thread continues running. I believe this is what the XXXKIB comment is stating.

[mhorne]

In D27536#615608, @markj wrote:

counter_u64_zero_inline() works by raising an interrupt on each CPU; the interrupt handler calls counter_u64_zero_one_cpu() to clear the counter for that CPU.

In the old version of the diff this may fail to work because counter_u64_add() is not atomic with respect to interrupts. It loads the value into a register, modifies it, and stores the result. If the value is zeroed between the load and store, it will get reverted when the interrupt finishes and the interrupted thread continues running. I believe this is what the XXXKIB comment is stating.

Thanks, that makes sense. counter_u64_zero_one being called via IPI is the detail I was forgetting. I can include a mention about the comment in the final commit message.

sys/riscv/include/counter.h

65 ↗

(On Diff #80519)

Most other machine/counter.h headers do this. Any reason not to use zpcpu_get() instead?

[markj]

sys/riscv/include/counter.h

65 ↗

(On Diff #80519)

I can't see a reason not use use zpcpu_get().