On Mon, Aug 22, 2022 at 08:14:05PM +0300, Vitaliy Makkoveev wrote:
> May be I misunderstood something, but what is the reason to compiler to
> drop this read? The value is not yet loaded, and we have no other
> unlocked access to this variable before rwlock.
>
> void
> mld6_fasttimeo(void)
> {
> struct ifnet *ifp;
>
> /*
> * Quick check to see if any work needs to be done, in order
> * to minimize the overhead of fasttimo processing.
> * Variable mld_timers_are_running is read atomically. In case we
> * miss a fast timer due to MP races, just run it next time.
> */
> if (!mld_timers_are_running)
> return;
>
This exammple is too primitive to get something wrong. But consider
int global_variable; /* may be changed by other cpu */
some_func()
{
if (global_variable)
do something that does not change global_variable
else
do something else
}
The compiler could change the code to something like
some_func()
{
if (global_variable)
do something that does not change global_variable
if (!global_variable)
do something else
}
If the other cpu changes global_variable between both if conditions,
something unexpected happens. It is not an else anymore.
The idiom below is safe from over optimization.
some_func()
{
if (READ_ONCE(global_variable))
do something that does not change global_variable
else
do something else
}
In our code we generally do not care about READ_ONCE(), but it works
as the compiler is not too evil. But I think we should start using
explicit reading once if we have lockless read access to MP modifyable
variables. When we get used to that idiom, the compiler cannot get
it wrong and the programmer sees that something with MP is going
on and has to be considered.
> > - Make the comment more verbose to make clear this oportunistic
> > read is MP safe.
>
> This one.
Updated comments.
bluhm
Index: netinet/igmp.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/igmp.c,v
retrieving revision 1.79
diff -u -p -r1.79 igmp.c
--- netinet/igmp.c 21 Aug 2022 23:04:45 -0000 1.79
+++ netinet/igmp.c 22 Aug 2022 18:56:17 -0000
@@ -96,7 +96,7 @@
#define IP_MULTICASTOPTS 0
-int igmp_timers_are_running;
+int igmp_timers_are_running; /* [N] shortcut for fast timer */
static LIST_HEAD(, router_info) rti_head;
static struct mbuf *router_alert;
struct cpumem *igmpcounters;
@@ -529,8 +529,9 @@ igmp_fasttimo(void)
/*
* Quick check to see if any work needs to be done, in order
* to minimize the overhead of fasttimo processing.
- * Variable igmp_timers_are_running is read atomically. In case we
- * miss a fast timer due to MP races, just run it next time.
+ * Variable igmp_timers_are_running is read atomically, but without
+ * lock intensionally. In case it is not set due to MP races, we may
+ * miss to check the timers. Then run the loop at next fast timeout.
*/
if (!igmp_timers_are_running)
return;
Index: netinet6/mld6.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/mld6.c,v
retrieving revision 1.57
diff -u -p -r1.57 mld6.c
--- netinet6/mld6.c 21 Aug 2022 23:04:45 -0000 1.57
+++ netinet6/mld6.c 22 Aug 2022 18:56:17 -0000
@@ -84,7 +84,7 @@
#include <netinet6/mld6_var.h>
static struct ip6_pktopts ip6_opts;
-static int mld_timers_are_running;
+int mld6_timers_are_running; /* [N] shortcut for fast timer */
/* XXX: These are necessary for KAME's link-local hack */
static struct in6_addr mld_all_nodes_linklocal =
IN6ADDR_LINKLOCAL_ALLNODES_INIT;
static struct in6_addr mld_all_routers_linklocal =
IN6ADDR_LINKLOCAL_ALLROUTERS_INIT;
@@ -99,7 +99,7 @@ mld6_init(void)
struct ip6_hbh *hbh = (struct ip6_hbh *)hbh_buf;
u_int16_t rtalert_code = htons((u_int16_t)IP6OPT_RTALERT_MLD);
- mld_timers_are_running = 0;
+ mld6_timers_are_running = 0;
/* ip6h_nxt will be fill in later */
hbh->ip6h_len = 0; /* (8 >> 3) - 1 */
@@ -136,7 +136,7 @@ mld6_start_listening(struct in6_multi *i
MLD_RANDOM_DELAY(MLD_V1_MAX_RI *
PR_FASTHZ);
in6m->in6m_state = MLD_IREPORTEDLAST;
- mld_timers_are_running = 1;
+ mld6_timers_are_running = 1;
}
}
@@ -266,7 +266,7 @@ mld6_input(struct mbuf *m, int off)
in6m->in6m_timer > timer) {
in6m->in6m_timer =
MLD_RANDOM_DELAY(timer);
- mld_timers_are_running = 1;
+ mld6_timers_are_running = 1;
}
}
}
@@ -330,15 +330,16 @@ mld6_fasttimeo(void)
/*
* Quick check to see if any work needs to be done, in order
* to minimize the overhead of fasttimo processing.
- * Variable mld_timers_are_running is read atomically. In case we
- * miss a fast timer due to MP races, just run it next time.
+ * Variable mld6_timers_are_running is read atomically, but without
+ * lock intensionally. In case it is not set due to MP races, we may
+ * miss to check the timers. Then run the loop at next fast timeout.
*/
- if (!mld_timers_are_running)
+ if (!mld6_timers_are_running)
return;
NET_LOCK();
- mld_timers_are_running = 0;
+ mld6_timers_are_running = 0;
TAILQ_FOREACH(ifp, &ifnet, if_list)
mld6_checktimer(ifp);
@@ -363,7 +364,7 @@ mld6_checktimer(struct ifnet *ifp)
mld6_sendpkt(in6m, MLD_LISTENER_REPORT, NULL);
in6m->in6m_state = MLD_IREPORTEDLAST;
} else {
- mld_timers_are_running = 1;
+ mld6_timers_are_running = 1;
}
}
}
