On Mon, Aug 22, 2022 at 10:42:05PM +0200, Alexander Bluhm wrote:
> 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 */
>
> [...]
>
> 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.
What are you trying to fix with READ_ONCE() here, within
mld6_fasttimeo()? The `mld_timers_are_running' is this too primitive
case from above. I'm not against volatile cast, but it seems unnecessary
here.
>
> > > - Make the comment more verbose to make clear this oportunistic
> > > read is MP safe.
> >
> > This one.
>
> Updated comments.
>
ok mvs@
> 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;
> }
> }
> }
>
