From 56d528e2caa42278860e5e3699c08266883f87b6 Mon Sep 17 00:00:00 2001 From: dlg Date: Thu, 10 Nov 2022 17:17:47 +0000 Subject: [PATCH] bring back r1.673: replace SRP with SMR in the if_idxmap. when i first wrote if_idxmap i didn't realise (and no one thought to tell me) that index 0 was special and means "no interface", so while here use the 0th slot in the interface map to store the length of the map instead of prepending the map with a length field. if_get() now special cases index 0 and returns NULL directly. this also means the size of the map is now always a power of 2, which is a nicer fit with what the kernel malloc aprovides. the problem with r1.673 that hrvoje popovski found was that attaching a lot of interfaces during autoconf would lock up when growing the map called smr_barrier. the fix in this diff is to (ab)use the usedidx bitmap to store an smr_entry and defer the freeing of the interface pointer map with it. tested by hrvoje popovski tweaks and ok visa@ --- sys/net/if.c | 239 ++++++++++++++++++++++++--------------------------- 1 file changed, 114 insertions(+), 125 deletions(-) diff --git a/sys/net/if.c b/sys/net/if.c index dcdc14c0eac..a29a011a32b 100644 --- a/sys/net/if.c +++ b/sys/net/if.c @@ -1,4 +1,4 @@ -/* $OpenBSD: if.c,v 1.676 2022/11/09 22:15:50 dlg Exp $ */ +/* $OpenBSD: if.c,v 1.677 2022/11/10 17:17:47 dlg Exp $ */ /* $NetBSD: if.c,v 1.35 1996/05/07 05:26:04 thorpej Exp $ */ /* @@ -87,6 +87,7 @@ #include #include /* uintptr_t */ #include +#include #include #include @@ -191,37 +192,32 @@ void if_qstart_compat(struct ifqueue *); * if_get(0) returns NULL. */ -void if_ifp_dtor(void *, void *); -void if_map_dtor(void *, void *); struct ifnet *if_ref(struct ifnet *); -/* - * struct if_map - * - * bounded array of ifnet srp pointers used to fetch references of live - * interfaces with if_get(). - */ - -struct if_map { - unsigned long limit; - /* followed by limit ifnet srp pointers */ -}; - /* * struct if_idxmap * * infrastructure to manage updates and accesses to the current if_map. + * + * interface index 0 is special and represents "no interface", so we + * use the 0th slot in map to store the length of the array. */ struct if_idxmap { - unsigned int serial; - unsigned int count; - struct srp map; - struct rwlock lock; - unsigned char *usedidx; /* bitmap of indices in use */ + unsigned int serial; + unsigned int count; + struct ifnet **map; /* SMR protected */ + struct rwlock lock; + unsigned char *usedidx; /* bitmap of indices in use */ +}; + +struct if_idxmap_dtor { + struct smr_entry smr; + struct ifnet **map; }; void if_idxmap_init(unsigned int); +void if_idxmap_free(void *); void if_idxmap_alloc(struct ifnet *); void if_idxmap_insert(struct ifnet *); void if_idxmap_remove(struct ifnet *); @@ -265,7 +261,7 @@ ifinit(void) * most machines boot with 4 or 5 interfaces, so size the initial map * to accommodate this */ - if_idxmap_init(8); + if_idxmap_init(8); /* 8 is a nice power of 2 for malloc */ for (i = 0; i < NET_TASKQ; i++) { nettqmp[i] = taskq_create("softnet", 1, IPL_NET, TASKQ_MPSAFE); @@ -274,48 +270,48 @@ ifinit(void) } } -static struct if_idxmap if_idxmap = { - 0, - 0, - SRP_INITIALIZER(), - RWLOCK_INITIALIZER("idxmaplk"), - NULL, -}; - -struct srp_gc if_ifp_gc = SRP_GC_INITIALIZER(if_ifp_dtor, NULL); -struct srp_gc if_map_gc = SRP_GC_INITIALIZER(if_map_dtor, NULL); +static struct if_idxmap if_idxmap; struct ifnet_head ifnetlist = TAILQ_HEAD_INITIALIZER(ifnetlist); +static inline unsigned int +if_idxmap_limit(struct ifnet **if_map) +{ + return ((uintptr_t)if_map[0]); +} + +static inline size_t +if_idxmap_usedidx_size(unsigned int limit) +{ + return (max(howmany(limit, NBBY), sizeof(struct if_idxmap_dtor))); +} + void if_idxmap_init(unsigned int limit) { - struct if_map *if_map; - struct srp *map; - unsigned int i; + struct ifnet **if_map; - if_idxmap.serial = 1; /* skip ifidx 0 so it can return NULL */ + rw_init(&if_idxmap.lock, "idxmaplk"); + if_idxmap.serial = 1; /* skip ifidx 0 */ - if_map = malloc(sizeof(*if_map) + limit * sizeof(*map), - M_IFADDR, M_WAITOK); + if_map = mallocarray(limit, sizeof(*if_map), M_IFADDR, + M_WAITOK | M_ZERO); - if_map->limit = limit; - map = (struct srp *)(if_map + 1); - for (i = 0; i < limit; i++) - srp_init(&map[i]); + if_map[0] = (struct ifnet *)(uintptr_t)limit; - if_idxmap.usedidx = malloc(howmany(limit, NBBY), + if_idxmap.usedidx = malloc(if_idxmap_usedidx_size(limit), M_IFADDR, M_WAITOK | M_ZERO); + setbit(if_idxmap.usedidx, 0); /* blacklist ifidx 0 */ /* this is called early so there's nothing to race with */ - srp_update_locked(&if_map_gc, &if_idxmap.map, if_map); + SMR_PTR_SET_LOCKED(&if_idxmap.map, if_map); } void if_idxmap_alloc(struct ifnet *ifp) { - struct if_map *if_map; - struct srp *map; + struct ifnet **if_map; + unsigned int limit; unsigned int index, i; refcnt_init(&ifp->if_refcnt); @@ -325,49 +321,50 @@ if_idxmap_alloc(struct ifnet *ifp) if (++if_idxmap.count >= USHRT_MAX) panic("too many interfaces"); - if_map = srp_get_locked(&if_idxmap.map); - map = (struct srp *)(if_map + 1); + if_map = SMR_PTR_GET_LOCKED(&if_idxmap.map); + limit = if_idxmap_limit(if_map); index = if_idxmap.serial++ & USHRT_MAX; - if (index >= if_map->limit) { - struct if_map *nif_map; - struct srp *nmap; - unsigned int nlimit; - struct ifnet *nifp; + if (index >= limit) { + struct if_idxmap_dtor *dtor; + struct ifnet **oif_map; + unsigned int olimit; unsigned char *nusedidx; - nlimit = if_map->limit * 2; - nif_map = malloc(sizeof(*nif_map) + nlimit * sizeof(*nmap), - M_IFADDR, M_WAITOK); - nmap = (struct srp *)(nif_map + 1); - - nif_map->limit = nlimit; - for (i = 0; i < if_map->limit; i++) { - srp_init(&nmap[i]); - nifp = srp_get_locked(&map[i]); - if (nifp != NULL) { - srp_update_locked(&if_ifp_gc, &nmap[i], - if_ref(nifp)); - } - } + oif_map = if_map; + olimit = limit; + + limit = olimit * 2; + if_map = mallocarray(limit, sizeof(*if_map), M_IFADDR, + M_WAITOK | M_ZERO); + if_map[0] = (struct ifnet *)(uintptr_t)limit; + + for (i = 1; i < olimit; i++) { + struct ifnet *oifp = SMR_PTR_GET_LOCKED(&oif_map[i]); + if (oifp == NULL) + continue; - while (i < nlimit) { - srp_init(&nmap[i]); - i++; + /* + * nif_map isn't visible yet, so don't need + * SMR_PTR_SET_LOCKED and its membar. + */ + if_map[i] = if_ref(oifp); } - nusedidx = malloc(howmany(nlimit, NBBY), + nusedidx = malloc(if_idxmap_usedidx_size(limit), M_IFADDR, M_WAITOK | M_ZERO); - memcpy(nusedidx, if_idxmap.usedidx, - howmany(if_map->limit, NBBY)); - free(if_idxmap.usedidx, M_IFADDR, - howmany(if_map->limit, NBBY)); + memcpy(nusedidx, if_idxmap.usedidx, howmany(olimit, NBBY)); + + /* use the old usedidx bitmap as an smr_entry for the if_map */ + dtor = (struct if_idxmap_dtor *)if_idxmap.usedidx; if_idxmap.usedidx = nusedidx; - srp_update_locked(&if_map_gc, &if_idxmap.map, nif_map); - if_map = nif_map; - map = nmap; + SMR_PTR_SET_LOCKED(&if_idxmap.map, if_map); + + dtor->map = oif_map; + smr_init(&dtor->smr); + smr_call(&dtor->smr, if_idxmap_free, dtor); } /* pick the next free index */ @@ -377,7 +374,7 @@ if_idxmap_alloc(struct ifnet *ifp) index = if_idxmap.serial++ & USHRT_MAX; } - KASSERT(index != 0 && index < if_map->limit); + KASSERT(index != 0 && index < limit); KASSERT(isclr(if_idxmap.usedidx, index)); setbit(if_idxmap.usedidx, index); @@ -386,23 +383,39 @@ if_idxmap_alloc(struct ifnet *ifp) rw_exit_write(&if_idxmap.lock); } +void +if_idxmap_free(void *arg) +{ + struct if_idxmap_dtor *dtor = arg; + struct ifnet **oif_map = dtor->map; + unsigned int olimit = if_idxmap_limit(oif_map); + unsigned int i; + + for (i = 1; i < olimit; i++) + if_put(oif_map[i]); + + free(oif_map, M_IFADDR, olimit * sizeof(*oif_map)); + free(dtor, M_IFADDR, if_idxmap_usedidx_size(olimit)); +} + void if_idxmap_insert(struct ifnet *ifp) { - struct if_map *if_map; - struct srp *map; + struct ifnet **if_map; unsigned int index = ifp->if_index; rw_enter_write(&if_idxmap.lock); - if_map = srp_get_locked(&if_idxmap.map); - map = (struct srp *)(if_map + 1); + if_map = SMR_PTR_GET_LOCKED(&if_idxmap.map); - KASSERT(index != 0 && index < if_map->limit); + KASSERTMSG(index != 0 && index < if_idxmap_limit(if_map), + "%s(%p) index %u vs limit %u", ifp->if_xname, ifp, index, + if_idxmap_limit(if_map)); + KASSERT(SMR_PTR_GET_LOCKED(&if_map[index]) == NULL); KASSERT(isset(if_idxmap.usedidx, index)); /* commit */ - srp_update_locked(&if_ifp_gc, &map[index], if_ref(ifp)); + SMR_PTR_SET_LOCKED(&if_map[index], if_ref(ifp)); rw_exit_write(&if_idxmap.lock); } @@ -410,53 +423,29 @@ if_idxmap_insert(struct ifnet *ifp) void if_idxmap_remove(struct ifnet *ifp) { - struct if_map *if_map; - struct srp *map; - unsigned int index; - - index = ifp->if_index; + struct ifnet **if_map; + unsigned int index = ifp->if_index; rw_enter_write(&if_idxmap.lock); - if_map = srp_get_locked(&if_idxmap.map); - KASSERT(index < if_map->limit); + if_map = SMR_PTR_GET_LOCKED(&if_idxmap.map); - map = (struct srp *)(if_map + 1); - KASSERT(ifp == (struct ifnet *)srp_get_locked(&map[index])); + KASSERT(index != 0 && index < if_idxmap_limit(if_map)); + KASSERT(SMR_PTR_GET_LOCKED(&if_map[index]) == ifp); + KASSERT(isset(if_idxmap.usedidx, index)); - srp_update_locked(&if_ifp_gc, &map[index], NULL); - if_idxmap.count--; + SMR_PTR_SET_LOCKED(&if_map[index], NULL); - KASSERT(isset(if_idxmap.usedidx, index)); + if_idxmap.count--; clrbit(if_idxmap.usedidx, index); /* end of if_idxmap modifications */ rw_exit_write(&if_idxmap.lock); -} -void -if_ifp_dtor(void *null, void *ifp) -{ + smr_barrier(); if_put(ifp); } -void -if_map_dtor(void *null, void *m) -{ - struct if_map *if_map = m; - struct srp *map = (struct srp *)(if_map + 1); - unsigned int i; - - /* - * dont need to serialize the use of update_locked since this is - * the last reference to this map. there's nothing to race against. - */ - for (i = 0; i < if_map->limit; i++) - srp_update_locked(&if_ifp_gc, &map[i], NULL); - - free(if_map, M_IFADDR, sizeof(*if_map) + if_map->limit * sizeof(*map)); -} - /* * Attach an interface to the * list of "active" interfaces. @@ -1780,22 +1769,22 @@ if_unit(const char *name) struct ifnet * if_get(unsigned int index) { - struct srp_ref sr; - struct if_map *if_map; - struct srp *map; + struct ifnet **if_map; struct ifnet *ifp = NULL; - if_map = srp_enter(&sr, &if_idxmap.map); - if (index < if_map->limit) { - map = (struct srp *)(if_map + 1); + if (index == 0) + return (NULL); - ifp = srp_follow(&sr, &map[index]); + smr_read_enter(); + if_map = SMR_PTR_GET(&if_idxmap.map); + if (index < if_idxmap_limit(if_map)) { + ifp = SMR_PTR_GET(&if_map[index]); if (ifp != NULL) { KASSERT(ifp->if_index == index); if_ref(ifp); } } - srp_leave(&sr); + smr_read_leave(); return (ifp); } -- 2.20.1