> On 11 Jul 2022, at 13:02, Alexander Bluhm <[email protected]> wrote:
>
> 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?
>
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 */
> };
>
