wg: Use plain loads/stores for keepalive management.

[jhb]

Summary

Using atomic loads after NET_EPOCH_ENTER or atomic stores before/after
NET_EPOCH_WAIT are not needed due to the existing barriers inside of
epoch_*().

[markj]

sys/dev/wg/if_wg.c

1048

I think this is an example of a place where we want to use atomic_load: there's no synchronization with writers so the values being loaded are immediately stale. That's not a problem in itself but it should be made obvious to anyone reading the code.

I wrote an implementation of plain atomic_load/store_bool(), will post it shortly.

What exactly does the net epoch section do for us here?

[jhb]

sys/dev/wg/if_wg.c

960

Hmm, I guess this comment is the reason for the NET_EPOCH around the callout_reset you asked about. It is not clear to me that a per-peer mutex used for managing callouts would be significantly less efficient? It would be easier to understand I think.

1048

I pointed to a comment above. It is abusing the epoch to serialize with the code that disables the peer.

[markj]

sys/dev/wg/if_wg.c

960

Oh. That's weird. I'm not familiar enough with these timers to say whether a mutex will be less efficient per se, but certainly contention could be a problem if the callout_pending() checks are done under a mutex.

1048

Ok. So I think the atomic_load/store_*s are not required for correctness, but would still serve as mildly helpful annotations. We do have atomic_load/store_bool in main now, BTW.

1159

Here's a situation where atomics would be helpful. There is zero synchronization between writers and readers of p_need_another_keepalive.

[jhb]

sys/dev/wg/if_wg.c

960

With the mutex you wouldn't need the callout_pending() calls at all. You would protect p_enabled with the mutex and could just use callout_stop under the lock in this function. Maybe I will mock up a version that uses callout_init_mtx and upload it for you to see how it looks instead.