Hi,
Customer complains that routing video stream over OpenBSD stutters
when snmpd is runnning. It looks like ioctl(SIOCGIFMEDIA) may be
the culprit, so I want to get it out of the netlock.
Start with a mutex around interface media list.
ok?
bluhm
Index: dev/ic/gem.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/dev/ic/gem.c,v
retrieving revision 1.126
diff -u -p -r1.126 gem.c
--- dev/ic/gem.c 12 Dec 2020 11:48:52 -0000 1.126
+++ dev/ic/gem.c 11 Jul 2022 06:16:15 -0000
@@ -332,6 +332,7 @@ gem_config(struct gem_softc *sc)
}
/* Check if we support GigE media. */
+ mtx_enter(&ifmedia_mtx);
TAILQ_FOREACH(ifm, &sc->sc_media.ifm_list, ifm_list) {
if (IFM_SUBTYPE(ifm->ifm_media) == IFM_1000_T ||
IFM_SUBTYPE(ifm->ifm_media) == IFM_1000_SX ||
@@ -341,6 +342,7 @@ gem_config(struct gem_softc *sc)
break;
}
}
+ mtx_leave(&ifmedia_mtx);
/* Attach the interface. */
if_attach(ifp);
Index: net/if_media.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/net/if_media.c,v
retrieving revision 1.33
diff -u -p -r1.33 if_media.c
--- net/if_media.c 10 Jul 2022 21:13:41 -0000 1.33
+++ net/if_media.c 11 Jul 2022 06:24:28 -0000
@@ -82,6 +82,7 @@
#include <sys/ioctl.h>
#include <sys/socket.h>
#include <sys/malloc.h>
+#include <sys/mutex.h>
#include <net/if.h>
#ifdef IFMEDIA_DEBUG
@@ -102,6 +103,8 @@ int ifmedia_debug = 0;
static void ifmedia_printword(uint64_t);
#endif
+struct mutex ifmedia_mtx = MUTEX_INITIALIZER(IPL_NET);
+
/*
* Initialize if_media struct for a specific interface instance.
*/
@@ -110,6 +113,7 @@ ifmedia_init(struct ifmedia *ifm, uint64
ifm_change_cb_t change_callback, ifm_stat_cb_t status_callback)
{
TAILQ_INIT(&ifm->ifm_list);
+ ifm->ifm_nwords = 0;
ifm->ifm_cur = NULL;
ifm->ifm_media = 0;
ifm->ifm_mask = dontcare_mask; /* IF don't-care bits */
@@ -145,7 +149,10 @@ ifmedia_add(struct ifmedia *ifm, uint64_
entry->ifm_data = data;
entry->ifm_aux = aux;
+ mtx_enter(&ifmedia_mtx);
TAILQ_INSERT_TAIL(&ifm->ifm_list, entry, ifm_list);
+ ifm->ifm_nwords++;
+ mtx_leave(&ifmedia_mtx);
}
/*
@@ -290,7 +297,6 @@ ifmedia_ioctl(struct ifnet *ifp, struct
case SIOCGIFMEDIA:
{
struct ifmediareq *ifmr = (struct ifmediareq *) ifr;
- struct ifmedia_entry *ep;
size_t nwords;
if (ifmr->ifm_count < 0)
@@ -302,37 +308,54 @@ ifmedia_ioctl(struct ifnet *ifp, struct
ifmr->ifm_status = 0;
(*ifm->ifm_status_cb)(ifp, ifmr);
- /*
- * Count them so we know a-priori how much is the max we'll
- * need.
- */
- ep = TAILQ_FIRST(&ifm->ifm_list);
- for (nwords = 0; ep != NULL; ep = TAILQ_NEXT(ep, ifm_list))
- nwords++;
+ mtx_enter(&ifmedia_mtx);
+ nwords = ifm->ifm_nwords;
+ mtx_leave(&ifmedia_mtx);
- if (ifmr->ifm_count != 0) {
- size_t minwords, ksiz;
+ if (ifmr->ifm_count == 0) {
+ ifmr->ifm_count = nwords;
+ return (0);
+ }
+
+ do {
+ struct ifmedia_entry *ife;
uint64_t *kptr;
+ size_t ksiz;
- minwords = nwords > (size_t)ifmr->ifm_count ?
- (size_t)ifmr->ifm_count : nwords;
kptr = mallocarray(nwords, sizeof(*kptr), M_TEMP,
M_WAITOK | M_ZERO);
ksiz = nwords * sizeof(*kptr);
+
+ mtx_enter(&ifmedia_mtx);
+ /* Madia list might grow during malloc(). */
+ if (nwords < ifm->ifm_nwords) {
+ nwords = ifm->ifm_nwords;
+ mtx_leave(&ifmedia_mtx);
+ free(kptr, M_TEMP, ksiz);
+ continue;
+ }
+ /* Request memory too small, set error and ifm_count. */
+ if (ifmr->ifm_count < ifm->ifm_nwords) {
+ nwords = ifm->ifm_nwords;
+ mtx_leave(&ifmedia_mtx);
+ free(kptr, M_TEMP, ksiz);
+ error = E2BIG;
+ break;
+ }
/*
* Get the media words from the interface's list.
*/
- ep = TAILQ_FIRST(&ifm->ifm_list);
- for (nwords = 0; ep != NULL && nwords < minwords;
- ep = TAILQ_NEXT(ep, ifm_list))
- kptr[nwords++] = ep->ifm_media;
- if (ep == NULL)
- error = copyout(kptr, ifmr->ifm_ulist,
- nwords * sizeof(*kptr));
- else
- error = E2BIG;
+ nwords = 0;
+ TAILQ_FOREACH(ife, &ifm->ifm_list, ifm_list) {
+ kptr[nwords++] = ife->ifm_media;
+ }
+ KASSERT(nwords == ifm->ifm_nwords);
+ mtx_leave(&ifmedia_mtx);
+
+ error = copyout(kptr, ifmr->ifm_ulist,
+ nwords * sizeof(*kptr));
free(kptr, M_TEMP, ksiz);
- }
+ } while (0);
ifmr->ifm_count = nwords;
break;
}
@@ -355,6 +378,7 @@ ifmedia_match(struct ifmedia *ifm, uint6
match = NULL;
mask = ~mask;
+ mtx_enter(&ifmedia_mtx);
TAILQ_FOREACH(next, &ifm->ifm_list, ifm_list) {
if ((next->ifm_media & mask) == (target & mask)) {
if (match) {
@@ -368,6 +392,7 @@ ifmedia_match(struct ifmedia *ifm, uint6
match = next;
}
}
+ mtx_leave(&ifmedia_mtx);
return (match);
}
@@ -379,13 +404,25 @@ void
ifmedia_delete_instance(struct ifmedia *ifm, uint64_t inst)
{
struct ifmedia_entry *ife, *nife;
+ struct ifmedia_list ifmlist;
+
+ TAILQ_INIT(&ifmlist);
+ mtx_enter(&ifmedia_mtx);
TAILQ_FOREACH_SAFE(ife, &ifm->ifm_list, ifm_list, nife) {
if (inst == IFM_INST_ANY ||
inst == IFM_INST(ife->ifm_media)) {
TAILQ_REMOVE(&ifm->ifm_list, ife, ifm_list);
- free(ife, M_IFADDR, sizeof *ife);
+ ifm->ifm_nwords--;
+ TAILQ_INSERT_TAIL(&ifmlist, ife, ifm_list);
}
+ }
+ mtx_leave(&ifmedia_mtx);
+
+ /* Do not hold mutex longer than necessary, call free() without. */
+ while((ife = TAILQ_FIRST(&ifmlist)) != NULL) {
+ TAILQ_REMOVE(&ifmlist, ife, ifm_list);
+ free(ife, M_IFADDR, sizeof *ife);
}
}
Index: net/if_media.h
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/net/if_media.h,v
retrieving revision 1.43
diff -u -p -r1.43 if_media.h
--- net/if_media.h 10 Jul 2022 21:13:41 -0000 1.43
+++ net/if_media.h 11 Jul 2022 06:16:15 -0000
@@ -71,6 +71,7 @@
#ifdef _KERNEL
struct ifnet;
+extern struct mutex ifmedia_mtx;
#include <sys/queue.h>
/*
@@ -80,6 +81,11 @@ typedef int (*ifm_change_cb_t)(struct if
typedef void (*ifm_stat_cb_t)(struct ifnet *, struct ifmediareq *);
/*
+ * Locks used to protect struct members in this file:
+ * M ifmedia_mtx interface media mutex
+ */
+
+/*
* In-kernel representation of a single supported media type.
*/
struct ifmedia_entry {
@@ -89,6 +95,7 @@ struct ifmedia_entry {
void *ifm_aux; /* for driver-specific use */
};
+TAILQ_HEAD(ifmedia_list, ifmedia_entry);
/*
* One of these goes into a network interface's softc structure.
* It is used to keep general media state.
@@ -97,7 +104,8 @@ struct ifmedia {
uint64_t ifm_mask; /* mask of changes we don't care about
*/
uint64_t ifm_media; /* current user-set media word */
struct ifmedia_entry *ifm_cur; /* currently selected media */
- TAILQ_HEAD(, ifmedia_entry) ifm_list; /* list of all supported media */
+ struct ifmedia_list ifm_list; /* [M] list of all supported media */
+ size_t ifm_nwords; /* [M] number of ifm_list entries */
ifm_change_cb_t ifm_change_cb; /* media change driver callback */
ifm_stat_cb_t ifm_status_cb; /* media status driver callback */
};
