On Mon, Aug 22, 2022 at 03:59:39PM +0200, Alexander Bluhm wrote:
> On Sun, Aug 21, 2022 at 10:10:53PM +0300, Vitaliy Makkoveev wrote:
> > On Sun, Aug 21, 2022 at 07:19:23PM +0200, Alexander Bluhm wrote:
> > > When testing parallel UDP input, I see contention on the netlock
> > > from the protocol timer. There is a check to avoid loop traversal
> > > if igmp and mld6 are not active. Move the shortcut to avoid grabbing
> > > netlock. This should be save as variables igmp_timers_are_running
> > > and mld_timers_are_running are read atomically. In case we miss
> > > an fast timer due to MP races, we just run it next time.
> >
> > Could we have a commentary like this before unlocked check? The rest of
> > diff looks ok by me.
> >
> > > This should be save as variables igmp_timers_are_running
> > > and mld_timers_are_running are read atomically. In case we miss
> > > an fast timer due to MP races, we just run it next time.
>
> I commited it with comment. I wonder if we should use read once.
> It is an unprotected atomic acces. Read once makes this explicit.
>
READ_ONCE() is useless here. It will be loaded in any case. But since
this check and `*_timers_are_running' modifications are not serialized,
we could miss this timer run.
> * Variable igmp_timers_are_running is read atomically. In case we
> * miss a fast timer due to MP races, just run it next time.
> */
> - if (!igmp_timers_are_running)
> + if (!READ_ONCE(igmp_timers_are_running))
> return;
> The common prefix is mld6_ for mld6.c. Document that
> igmp_timers_are_running and mld_timers_are_running are protected
> by netlock.
>
These hunks are ok by me.
> ok?
>
> 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 13:19:51 -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;
> @@ -532,7 +532,7 @@ igmp_fasttimo(void)
> * Variable igmp_timers_are_running is read atomically. In case we
> * miss a fast timer due to MP races, just run it next time.
> */
> - if (!igmp_timers_are_running)
> + if (!READ_ONCE(igmp_timers_are_running))
> return;
>
> NET_LOCK();
> 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 13:22:58 -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,15 @@ 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
> + * Variable mld6_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)
> + if (!READ_ONCE(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 +363,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;
> }
> }
> }
