On Thu, Jun 24, 2021 at 09:31:20AM +0200, Alexandr Nedvedicky wrote:
> Hello David,
>
> </snip>
> >
> > i think we can get away with not refcounting eb_entry structures at all.
> > either they're in the etherbridge map/table or they're not, and the
> > thing that takes them out of the map while holding the eb_lock mutex
> > becomes responsible for their cleanup.
> >
> > i feel like most of the original logic can still hold up if we fix my
> > stupid refcnting mistake(s) and do a better job of avoiding a double
> > free.
>
> I'm not sure. It seems to me the code in your diff deals with
> insert vs. insert race properly. how about delete vs. insert?
hrm. i can half convince myself that the outcome of losing a delete vs
insert race would have a semantically correct outcome, but while i'm
trying to do that it occurs to me that i'm trying to make the code too
clever and i should dumb it down.
> 350 mtx_enter(&eb->eb_lock);
> 351 num = eb->eb_num + (oebe == NULL);
>
> 352 if (num <= eb->eb_max && ebt_insert(eb, nebe) == oebe) {
>
> 353 /* we won, do the update */
>
> 354 ebl_insert(ebl, nebe);
> 355
> 356 if (oebe != NULL) {
> 357 ebl_remove(ebl, oebe);
> 358 ebt_replace(eb, oebe, nebe);
> 359 }
> 360
> 361 nebe = NULL; /* give nebe reference to the table */
> 362 eb->eb_num = num;
>
> 363 } else {
>
> 364 /* we lost, we didn't end up replacing oebe */
> 365 oebe = NULL;
> 366 }
> 367 mtx_leave(&eb->eb_lock);
> 368
>
> assume cpu0 got oebe and assumes it is going to perform update (oebe !=
> NULL).
> the cpu1 runs ahead and won mutex (->eb_lock) in etherbridge_del_addr()
> and
> removed the entry successfully. as soon as cpu1 leaves ->eb_lock, it's
> cpu0's turn. In this case ebt_insert() returns NULL, because there is
> no conflict any more. However 'NULL != oebe'.
in this situation it would look like etherbridge_del_addr ran after nebe
was inserted, which i think is a plausible history.
> I'm not sure we can fix insert vs. delete race properly without atomic
> reference counter.
during the drive to work it occurred to me that we should basically have
the same logic around whether we should insert or replace or do nothing
in both the smr and mutex critical sections.
it at least makes the code easier to understand. i think?
Index: if_etherbridge.c
===================================================================
RCS file: /cvs/src/sys/net/if_etherbridge.c,v
retrieving revision 1.6
diff -u -p -r1.6 if_etherbridge.c
--- if_etherbridge.c 10 Mar 2021 10:21:47 -0000 1.6
+++ if_etherbridge.c 25 Jun 2021 03:56:37 -0000
@@ -44,7 +44,6 @@
#include <net/if_etherbridge.h>
-static inline void ebe_take(struct eb_entry *);
static inline void ebe_rele(struct eb_entry *);
static void ebe_free(void *);
@@ -233,16 +232,9 @@ ebt_remove(struct etherbridge *eb, struc
}
static inline void
-ebe_take(struct eb_entry *ebe)
-{
- refcnt_take(&ebe->ebe_refs);
-}
-
-static void
ebe_rele(struct eb_entry *ebe)
{
- if (refcnt_rele(&ebe->ebe_refs))
- smr_call(&ebe->ebe_smr_entry, ebe_free, ebe);
+ smr_call(&ebe->ebe_smr_entry, ebe_free, ebe);
}
static void
@@ -309,19 +301,21 @@ etherbridge_map(struct etherbridge *eb,
smr_read_enter();
oebe = ebl_find(ebl, eba);
- if (oebe == NULL)
- new = 1;
- else {
+ if (oebe == NULL) {
+ /*
+ * peek at the space to see if it's worth trying
+ * to make a new entry.
+ */
+ if (eb->eb_num < eb->eb_max)
+ new = 1;
+ } else {
if (oebe->ebe_age != now)
oebe->ebe_age = now;
/* does this entry need to be replaced? */
if (oebe->ebe_type == EBE_DYNAMIC &&
- !eb_port_eq(eb, oebe->ebe_port, port)) {
+ !eb_port_eq(eb, oebe->ebe_port, port))
new = 1;
- ebe_take(oebe);
- } else
- oebe = NULL;
}
smr_read_leave();
@@ -342,7 +336,6 @@ etherbridge_map(struct etherbridge *eb,
}
smr_init(&nebe->ebe_smr_entry);
- refcnt_init(&nebe->ebe_refs);
nebe->ebe_etherbridge = eb;
nebe->ebe_addr = eba;
@@ -351,40 +344,49 @@ etherbridge_map(struct etherbridge *eb,
nebe->ebe_age = now;
mtx_enter(&eb->eb_lock);
- num = eb->eb_num + (oebe == NULL);
- if (num <= eb->eb_max && ebt_insert(eb, nebe) == oebe) {
- /* we won, do the update */
- ebl_insert(ebl, nebe);
-
- if (oebe != NULL) {
- ebl_remove(ebl, oebe);
- ebt_replace(eb, oebe, nebe);
-
- /* take the table reference away */
- if (refcnt_rele(&oebe->ebe_refs)) {
- panic("%s: eb %p oebe %p refcnt",
- __func__, eb, oebe);
+ oebe = ebt_find(eb, nebe);
+ if (oebe == NULL) {
+ num = eb->eb_num + 1;
+ if (num <= eb->eb_max) {
+ ebl_insert(ebl, nebe);
+
+ oebe = ebt_insert(eb, nebe);
+ if (oebe != NULL) {
+ panic("etherbridge %p changed while locked",
+ eb);
}
+
+ /* great success */
+ eb->eb_num = num;
+ nebe = NULL; /* give ref to table */
}
+ } else if (oebe->ebe_type == EBE_DYNAMIC) {
+ /* do the update */
+ ebl_insert(ebl, nebe);
- nebe = NULL;
- eb->eb_num = num;
+ ebl_remove(ebl, oebe);
+ ebt_replace(eb, oebe, nebe);
+
+ nebe = NULL; /* give ref to table */
+ } else {
+ /*
+ * oebe is not a dynamic entry, so don't replace it.
+ */
+ oebe = NULL;
}
mtx_leave(&eb->eb_lock);
if (nebe != NULL) {
/*
* the new entry didn't make it into the
- * table, so it can be freed directly.
+ * table so it can be freed directly.
*/
ebe_free(nebe);
}
if (oebe != NULL) {
/*
- * the old entry could be referenced in
- * multiple places, including an smr read
- * section, so release it properly.
+ * we replaced this entry, it needs to be released.
*/
ebe_rele(oebe);
}
@@ -415,7 +417,6 @@ etherbridge_add_addr(struct etherbridge
}
smr_init(&nebe->ebe_smr_entry);
- refcnt_init(&nebe->ebe_refs);
nebe->ebe_etherbridge = eb;
nebe->ebe_addr = eba;
@@ -551,12 +552,18 @@ etherbridge_detach_port(struct etherbrid
mtx_leave(&eb->eb_lock);
}
- smr_barrier(); /* try and do it once for all the entries */
+ if (TAILQ_EMPTY(&ebq))
+ return;
+
+ /*
+ * do one smr barrier for all the entries rather than an
+ * smr_call each.
+ */
+ smr_barrier();
TAILQ_FOREACH_SAFE(ebe, &ebq, ebe_qentry, nebe) {
TAILQ_REMOVE(&ebq, ebe, ebe_qentry);
- if (refcnt_rele(&ebe->ebe_refs))
- ebe_free(ebe);
+ ebe_free(ebe);
}
}
@@ -587,12 +594,18 @@ etherbridge_flush(struct etherbridge *eb
mtx_leave(&eb->eb_lock);
}
- smr_barrier(); /* try and do it once for all the entries */
+ if (TAILQ_EMPTY(&ebq))
+ return;
+
+ /*
+ * do one smr barrier for all the entries rather than an
+ * smr_call each.
+ */
+ smr_barrier();
TAILQ_FOREACH_SAFE(ebe, &ebq, ebe_qentry, nebe) {
TAILQ_REMOVE(&ebq, ebe, ebe_qentry);
- if (refcnt_rele(&ebe->ebe_refs))
- ebe_free(ebe);
+ ebe_free(ebe);
}
}
Index: if_etherbridge.h
===================================================================
RCS file: /cvs/src/sys/net/if_etherbridge.h,v
retrieving revision 1.3
diff -u -p -r1.3 if_etherbridge.h
--- if_etherbridge.h 26 Feb 2021 01:28:51 -0000 1.3
+++ if_etherbridge.h 25 Jun 2021 03:56:37 -0000
@@ -51,7 +51,6 @@ struct eb_entry {
time_t ebe_age;
struct etherbridge *ebe_etherbridge;
- struct refcnt ebe_refs;
struct smr_entry ebe_smr_entry;
};
