Download raw body.
domain protocol timer unlock
On Sat, May 18, 2024 at 12:03:32PM +0200, Claudio Jeker wrote:
> Stupid compilers are broken. Some of these transformations they do are
> just not side-effect free. So no you are not too paranoid. You are not
> paranoid enough since the code would probably need some memory barriers as
> well (at least on the producer side). Right now I think it is fine but if
> someone changes the NET_LOCK in the timeout even a shared NET_LOCK then we
> are in trouble.
Here is a variant with atomic operations and memory barriers for
igmp_timers_are_running and mld6_timers_are_running. The memory
barriers should not be necessary as long net lock protects us. But
we want to reduce net lock, so we can to it correctly right now.
This diff is independent of the question whether all these timeouts
should use the timeout(9) framework. My gaol is to remove kernel
lock from network code. So I have to make multicast MP good enough.
ok?
bluhm
Index: kern/uipc_domain.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_domain.c,v
diff -u -p -r1.65 uipc_domain.c
--- kern/uipc_domain.c 11 Jan 2024 14:15:11 -0000 1.65
+++ kern/uipc_domain.c 29 Jul 2024 12:42:28 -0000
@@ -90,8 +90,10 @@ domaininit(void)
max_linkhdr = 64;
max_hdr = max_linkhdr + max_protohdr;
- timeout_set_proc(&pffast_timeout, pffasttimo, &pffast_timeout);
- timeout_set_proc(&pfslow_timeout, pfslowtimo, &pfslow_timeout);
+ timeout_set_flags(&pffast_timeout, pffasttimo, &pffast_timeout,
+ KCLOCK_NONE, TIMEOUT_PROC | TIMEOUT_MPSAFE);
+ timeout_set_flags(&pfslow_timeout, pfslowtimo, &pfslow_timeout,
+ KCLOCK_NONE, TIMEOUT_PROC | TIMEOUT_MPSAFE);
timeout_add(&pffast_timeout, 1);
timeout_add(&pfslow_timeout, 1);
}
Index: netinet/igmp.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/igmp.c,v
diff -u -p -r1.83 igmp.c
--- netinet/igmp.c 16 Sep 2023 09:33:27 -0000 1.83
+++ netinet/igmp.c 29 Jul 2024 13:33:43 -0000
@@ -96,12 +96,12 @@
#define IP_MULTICASTOPTS 0
-int igmp_timers_are_running; /* [N] shortcut for fast timer */
+int igmp_timers_are_running; /* [a] shortcut for fast timer */
static LIST_HEAD(, router_info) rti_head;
static struct mbuf *router_alert;
struct cpumem *igmpcounters;
-void igmp_checktimer(struct ifnet *);
+int igmp_checktimer(struct ifnet *);
void igmp_sendpkt(struct ifnet *, struct in_multi *, int, in_addr_t);
int rti_fill(struct in_multi *);
struct router_info * rti_find(struct ifnet *);
@@ -228,7 +228,7 @@ igmp_input_if(struct ifnet *ifp, struct
struct in_multi *inm;
struct router_info *rti;
struct in_ifaddr *ia;
- int timer;
+ int timer, running = 0;
igmplen = ntohs(ip->ip_len) - iphlen;
@@ -300,7 +300,7 @@ igmp_input_if(struct ifnet *ifp, struct
inm->inm_state = IGMP_DELAYING_MEMBER;
inm->inm_timer = IGMP_RANDOM_DELAY(
IGMP_MAX_HOST_REPORT_DELAY * PR_FASTHZ);
- igmp_timers_are_running = 1;
+ running = 1;
}
}
} else {
@@ -341,7 +341,7 @@ igmp_input_if(struct ifnet *ifp, struct
IGMP_DELAYING_MEMBER;
inm->inm_timer =
IGMP_RANDOM_DELAY(timer);
- igmp_timers_are_running = 1;
+ running = 1;
break;
case IGMP_SLEEPING_MEMBER:
inm->inm_state =
@@ -475,6 +475,11 @@ igmp_input_if(struct ifnet *ifp, struct
}
+ if (running) {
+ membar_producer();
+ atomic_store_int(&igmp_timers_are_running, running);
+ }
+
/*
* Pass all valid IGMP packets up to any process(es) listening
* on a raw IGMP socket.
@@ -485,7 +490,7 @@ igmp_input_if(struct ifnet *ifp, struct
void
igmp_joingroup(struct in_multi *inm, struct ifnet *ifp)
{
- int i;
+ int i, running = 0;
inm->inm_state = IGMP_IDLE_MEMBER;
@@ -496,9 +501,14 @@ igmp_joingroup(struct in_multi *inm, str
inm->inm_state = IGMP_DELAYING_MEMBER;
inm->inm_timer = IGMP_RANDOM_DELAY(
IGMP_MAX_HOST_REPORT_DELAY * PR_FASTHZ);
- igmp_timers_are_running = 1;
+ running = 1;
} else
inm->inm_timer = 0;
+
+ if (running) {
+ membar_producer();
+ atomic_store_int(&igmp_timers_are_running, running);
+ }
}
void
@@ -525,6 +535,7 @@ void
igmp_fasttimo(void)
{
struct ifnet *ifp;
+ int running = 0;
/*
* Quick check to see if any work needs to be done, in order
@@ -533,23 +544,29 @@ igmp_fasttimo(void)
* lock intentionally. 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)
+ membar_consumer();
+ if (!atomic_load_int(&igmp_timers_are_running))
return;
NET_LOCK();
- igmp_timers_are_running = 0;
- TAILQ_FOREACH(ifp, &ifnetlist, if_list)
- igmp_checktimer(ifp);
+ TAILQ_FOREACH(ifp, &ifnetlist, if_list) {
+ if (igmp_checktimer(ifp))
+ running = 1;
+ }
+
+ membar_producer();
+ atomic_store_int(&igmp_timers_are_running, running);
NET_UNLOCK();
}
-void
+int
igmp_checktimer(struct ifnet *ifp)
{
struct in_multi *inm;
struct ifmaddr *ifma;
+ int running = 0;
NET_ASSERT_LOCKED();
@@ -570,9 +587,11 @@ igmp_checktimer(struct ifnet *ifp)
inm->inm_state = IGMP_IDLE_MEMBER;
}
} else {
- igmp_timers_are_running = 1;
+ running = 1;
}
}
+
+ return (running);
}
void
Index: netinet6/icmp6.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/icmp6.c,v
diff -u -p -r1.254 icmp6.c
--- netinet6/icmp6.c 14 Jul 2024 18:53:39 -0000 1.254
+++ netinet6/icmp6.c 29 Jul 2024 12:42:28 -0000
@@ -1198,7 +1198,6 @@ icmp6_reflect(struct mbuf **mp, size_t o
void
icmp6_fasttimo(void)
{
-
mld6_fasttimeo();
}
Index: netinet6/mld6.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/mld6.c,v
diff -u -p -r1.62 mld6.c
--- netinet6/mld6.c 13 Feb 2024 12:22:09 -0000 1.62
+++ netinet6/mld6.c 29 Jul 2024 13:36:57 -0000
@@ -85,9 +85,9 @@
#include <netinet6/mld6_var.h>
static struct ip6_pktopts ip6_opts;
-int mld6_timers_are_running; /* [N] shortcut for fast timer */
+int mld6_timers_are_running; /* [a] shortcut for fast timer */
-void mld6_checktimer(struct ifnet *);
+int mld6_checktimer(struct ifnet *);
static void mld6_sendpkt(struct in6_multi *, int, const struct in6_addr *);
void
@@ -118,6 +118,7 @@ mld6_start_listening(struct in6_multi *i
{
/* XXX: These are necessary for KAME's link-local hack */
struct in6_addr all_nodes = IN6ADDR_LINKLOCAL_ALLNODES_INIT;
+ int running = 0;
/*
* RFC2710 page 10:
@@ -138,7 +139,12 @@ mld6_start_listening(struct in6_multi *i
MLD_RANDOM_DELAY(MLD_V1_MAX_RI *
PR_FASTHZ);
in6m->in6m_state = MLD_IREPORTEDLAST;
- mld6_timers_are_running = 1;
+ running = 1;
+ }
+
+ if (running) {
+ membar_producer();
+ atomic_store_int(&mld6_timers_are_running, running);
}
}
@@ -169,6 +175,7 @@ mld6_input(struct mbuf *m, int off)
struct in6_multi *in6m;
struct ifmaddr *ifma;
int timer; /* timer value in the MLD query header */
+ int running = 0;
/* XXX: These are necessary for KAME's link-local hack */
struct in6_addr all_nodes = IN6ADDR_LINKLOCAL_ALLNODES_INIT;
@@ -272,7 +279,7 @@ mld6_input(struct mbuf *m, int off)
in6m->in6m_timer > timer) {
in6m->in6m_timer =
MLD_RANDOM_DELAY(timer);
- mld6_timers_are_running = 1;
+ running = 1;
}
}
}
@@ -323,8 +330,13 @@ mld6_input(struct mbuf *m, int off)
#endif
break;
}
- if_put(ifp);
+ if (running) {
+ membar_producer();
+ atomic_store_int(&mld6_timers_are_running, running);
+ }
+
+ if_put(ifp);
m_freem(m);
}
@@ -332,6 +344,7 @@ void
mld6_fasttimeo(void)
{
struct ifnet *ifp;
+ int running;
/*
* Quick check to see if any work needs to be done, in order
@@ -340,23 +353,29 @@ mld6_fasttimeo(void)
* lock intentionally. 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 (!mld6_timers_are_running)
+ membar_consumer();
+ if (!atomic_load_int(&mld6_timers_are_running))
return;
NET_LOCK();
- mld6_timers_are_running = 0;
- TAILQ_FOREACH(ifp, &ifnetlist, if_list)
- mld6_checktimer(ifp);
+ TAILQ_FOREACH(ifp, &ifnetlist, if_list) {
+ if (mld6_checktimer(ifp))
+ running = 1;
+ }
+
+ membar_producer();
+ atomic_store_int(&mld6_timers_are_running, running);
NET_UNLOCK();
}
-void
+int
mld6_checktimer(struct ifnet *ifp)
{
struct in6_multi *in6m;
struct ifmaddr *ifma;
+ int running = 0;
NET_ASSERT_LOCKED();
@@ -370,9 +389,11 @@ mld6_checktimer(struct ifnet *ifp)
mld6_sendpkt(in6m, MLD_LISTENER_REPORT, NULL);
in6m->in6m_state = MLD_IREPORTEDLAST;
} else {
- mld6_timers_are_running = 1;
+ running = 1;
}
}
+
+ return (running);
}
static void
domain protocol timer unlock