Re: replace SRP with SMR in the if_idxmap commit
On Thu, Nov 10, 2022 at 11:59:02PM +1000, David Gwynne wrote: > On Thu, Nov 10, 2022 at 09:04:22PM +1000, David Gwynne wrote: > > On Thu, Nov 10, 2022 at 08:10:35AM +1000, David Gwynne wrote: > > > I know what this is. The barrier at the end of if_idxmap_alloc is > > > sleeping waiting for cpus to run that aren't running cos we haven't > > > finished booting yet. > > > > > > I'll back it out and fix it up when I'm actually awake. > > > > i woke up, so here's a diff. > > > > this uses the usedidx as an smr_entry so we can use smr_call instead of > > smr_barrier during autoconf. > > > > this works for me on a box with a lot of hardware interfaces, which > > forces allocation of a new interface map and therefore destruction of > > the initial one. > > > > there is still an smr_barrier in if_idxmap_remove, but remove only > > happens when an interface goes away. we could use part of struct ifnet > > (eg, if_description) as an smr_entry if needed. > > > > this one is even better. Please add #include . > + SMR_PTR_SET_LOCKED(_idxmap.map, if_map); > + > + smr_init(>smr); > + dtor->map = oif_map; I think smr_call could be moved here. The call is non-blocking. Then the scope of the dtor variable could be reduced too. dtor->map = oif_map; smr_init(>smr); smr_call(>smr, if_idxmap_free, dtor); OK visa@
Re: replace SRP with SMR in the if_idxmap commit
On 10.11.2022. 14:59, David Gwynne wrote: > On Thu, Nov 10, 2022 at 09:04:22PM +1000, David Gwynne wrote: >> On Thu, Nov 10, 2022 at 08:10:35AM +1000, David Gwynne wrote: >>> I know what this is. The barrier at the end of if_idxmap_alloc is sleeping >>> waiting for cpus to run that aren't running cos we haven't finished booting >>> yet. >>> >>> I'll back it out and fix it up when I'm actually awake. >> i woke up, so here's a diff. >> >> this uses the usedidx as an smr_entry so we can use smr_call instead of >> smr_barrier during autoconf. >> >> this works for me on a box with a lot of hardware interfaces, which >> forces allocation of a new interface map and therefore destruction of >> the initial one. >> >> there is still an smr_barrier in if_idxmap_remove, but remove only >> happens when an interface goes away. we could use part of struct ifnet >> (eg, if_description) as an smr_entry if needed. >> > this one is even better. Hi, with this diff my machines boot as they should. Thank you
Re: replace SRP with SMR in the if_idxmap commit
On Thu, Nov 10, 2022 at 09:04:22PM +1000, David Gwynne wrote:
> On Thu, Nov 10, 2022 at 08:10:35AM +1000, David Gwynne wrote:
> > I know what this is. The barrier at the end of if_idxmap_alloc is sleeping
> > waiting for cpus to run that aren't running cos we haven't finished booting
> > yet.
> >
> > I'll back it out and fix it up when I'm actually awake.
>
> i woke up, so here's a diff.
>
> this uses the usedidx as an smr_entry so we can use smr_call instead of
> smr_barrier during autoconf.
>
> this works for me on a box with a lot of hardware interfaces, which
> forces allocation of a new interface map and therefore destruction of
> the initial one.
>
> there is still an smr_barrier in if_idxmap_remove, but remove only
> happens when an interface goes away. we could use part of struct ifnet
> (eg, if_description) as an smr_entry if needed.
>
this one is even better.
Index: if.c
===
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.676
diff -u -p -r1.676 if.c
--- if.c9 Nov 2022 22:15:50 - 1.676
+++ if.c10 Nov 2022 13:55:39 -
@@ -196,32 +196,29 @@ 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 longlimit;
- /* 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 rwlocklock;
- 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 +262,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 +271,49 @@ 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(_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([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(_map_gc, _idxmap.map, if_map);
+ SMR_PTR_SET_LOCKED(_idxmap.map, if_map);
}
void
if_idxmap_alloc(struct ifnet *ifp)
{
- struct if_map *if_map;
- struct srp *map;
+ struct if_idxmap_dtor *dtor = NULL;
+ struct ifnet **if_map;
+ unsigned int limit;
unsigned int
Re: replace SRP with SMR in the if_idxmap commit
On Thu, Nov 10, 2022 at 08:10:35AM +1000, David Gwynne wrote:
> I know what this is. The barrier at the end of if_idxmap_alloc is sleeping
> waiting for cpus to run that aren't running cos we haven't finished booting
> yet.
>
> I'll back it out and fix it up when I'm actually awake.
i woke up, so here's a diff.
this uses the usedidx as an smr_entry so we can use smr_call instead of
smr_barrier during autoconf.
this works for me on a box with a lot of hardware interfaces, which
forces allocation of a new interface map and therefore destruction of
the initial one.
there is still an smr_barrier in if_idxmap_remove, but remove only
happens when an interface goes away. we could use part of struct ifnet
(eg, if_description) as an smr_entry if needed.
Index: if.c
===
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.676
diff -u -p -r1.676 if.c
--- if.c9 Nov 2022 22:15:50 - 1.676
+++ if.c10 Nov 2022 10:58:30 -
@@ -196,32 +196,29 @@ 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 longlimit;
- /* 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 rwlocklock;
- 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 +262,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 +271,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(_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([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);
/* this is called early so there's nothing to race with */
- srp_update_locked(_map_gc, _idxmap.map, if_map);
+ SMR_PTR_SET_LOCKED(_idxmap.map, if_map);
}
void
if_idxmap_alloc(struct ifnet *ifp)
{
- struct if_map *if_map;
- struct srp *map;
+ struct ifnet **if_map, **oif_map;
+ struct if_idxmap_dtor *dtor = NULL;
+ unsigned int limit, olimit;
unsigned int index, i;
refcnt_init(>if_refcnt);
@@ -325,49 +322,46 @@ if_idxmap_alloc(struct ifnet *ifp)
if (++if_idxmap.count >= USHRT_MAX)
Re: replace SRP with SMR in the if_idxmap commit
I know what this is. The barrier at the end of if_idxmap_alloc is sleeping waiting for cpus to run that aren't running cos we haven't finished booting yet. I'll back it out and fix it up when I'm actually awake. dlg > On 10 Nov 2022, at 6:28 am, Hrvoje Popovski wrote: > > Hi all, > > I've checkout cvs half an hour ago on two boxes and both boxes won't > properly boot. > > First one stops here > > ppb10 at pci1 dev 28 function 4 "Intel 8 Series PCIE" rev 0xd5: msi > pci12 at ppb10 bus 13 > em4 at pci12 dev 0 function 0 "Intel I350" rev 0x01: msi, address > 00:25:90:5d:c9:9a > em5 at pci12 dev 0 function 1 "Intel I350" rev 0x01: msi > > > second one stop here > vmm0 at mainbus0: VMX/EPT > > so I've change if.c revision to r1.672 and with that change box boots > but with r1.673 won't boot... > I've compile kernel with WITNESS and WITNESS_LOCKTRACE but with that > there isn't any more information. > > Did anyone experience that or I just me? > > > > >
replace SRP with SMR in the if_idxmap commit
Hi all, I've checkout cvs half an hour ago on two boxes and both boxes won't properly boot. First one stops here ppb10 at pci1 dev 28 function 4 "Intel 8 Series PCIE" rev 0xd5: msi pci12 at ppb10 bus 13 em4 at pci12 dev 0 function 0 "Intel I350" rev 0x01: msi, address 00:25:90:5d:c9:9a em5 at pci12 dev 0 function 1 "Intel I350" rev 0x01: msi second one stop here vmm0 at mainbus0: VMX/EPT so I've change if.c revision to r1.672 and with that change box boots but with r1.673 won't boot... I've compile kernel with WITNESS and WITNESS_LOCKTRACE but with that there isn't any more information. Did anyone experience that or I just me?
