Implement non-thread dependant network address list locks

[• hselasky]

Summary

This patch reverts r340413 and implements the address lists locks for network interfaces like a per-CPU mutex protecting a per-CPU epoch tracker. This avoids simultaneous use of the epoch tracker inside the network stack and drivers. The right solution is to pass a priority tracker structure pointer to each and every epoch_enter_preempt() and epoch_exit_preempt() function call which is currently not possible due to frozen KBI's. This patch also saves a memory allocation for every thread structure.

[• hselasky]

Fix some whitespaces.

[• hselasky]

Move SYSINIT() a bit later to get all CPUs initialized.

[pho]

Could this one be related to your patch? "panic: freeing free block".

https://people.freebsd.org/~pho/stress/log/hselasky001.txt

[• hselasky]

@pho : No, I don't think so. I discussed this patch with @glebius and we agreed to not change it. Gleb's patch is temporary and will be removed.

[markj]

Is this approach suitable on stable/12? I am diagnosing panic reports from users that are hitting the problem fixed by Gleb's patch...

[glebius]

I think that if_maddr_rlock + accessing list head on ifnet is just a plain wrong KPI. The if_maddr_count+if_maddr_array is a bit better, but requires to allocate memory, while usually being used in a context where we can't wait.

I think that we should just totally hide how address lists are locked from drivers, as well as the fact that they are lists. They could be hashes or trees, or arrays, or whatever. In my non-finished projects/ifnet branch, there is a different
KPI to achieve traversal of address lists:

/*

• Traversing through interface address lists. */

typedef void ifaddr_cb_t(void *, struct sockaddr *, struct sockaddr *,

struct sockaddr *);

typedef void ifmaddr_cb_t(void *, struct sockaddr *);
void if_foreach_addr(if_t, ifaddr_cb_t, void *);
void if_foreach_maddr(if_t, ifmaddr_cb_t, void *);

The functions are internal to if.c and do whatever locking
is approprite in our kernel. Mutexes before, and epoch now.

And in the driver side this would look like this:

static void
bge_hash_maddr(void *arg, struct sockaddr *maddr)
{

struct sockaddr_dl *sdl = (struct sockaddr_dl *)maddr;
uint32_t *hashes = arg;
int h;

if (sdl->sdl_family != AF_LINK)
        return;

h = ether_crc32_le(LLADDR(sdl), ETHER_ADDR_LEN) & 0x7F;
hashes[(h & 0x60) >> 5] |= 1 << (h & 0x1F);

}

static void
bge_setmulti(struct bge_softc *sc)
{

uint32_t hashes[4] = { 0, 0, 0, 0 };
int i;

BGE_LOCK_ASSERT(sc);

if (sc->bge_if_flags & (IFF_ALLMULTI | IFF_PROMISC)) {
        for (i = 0; i < 4; i++)
                CSR_WRITE_4(sc, BGE_MAR0 + (i * 4), 0xFFFFFFFF);
        return;
}                                                                                                                                                                                                                                           

/* First, zot all the existing filters. */
for (i = 0; i < 4; i++)
        CSR_WRITE_4(sc, BGE_MAR0 + (i * 4), 0);

if_foreach_maddr(sc->bge_ifp, bge_hash_maddr, hashes);

for (i = 0; i < 4; i++)
        CSR_WRITE_4(sc, BGE_MAR0 + (i * 4), hashes[i]);

}

IMHO, much cleaner and independent of struct ifnet than
if_maddr_lock() + traversal, and better than if_multiaddr_count +
if_multiaddr_array since doesn't require allocating memory.

I'd say that there have been enough pain around interface address
traversal, so I would like to go forward with the following plan:

1. Introduce if_foreach_addr/if_foreach_maddr
2. Convert all drivers to this KPI.
3. Declare if_addr_lock/if_maddr_lock and if_multiaddr_count+if_multiaddr_array gone in 14.0
4. Indeed make them gone in head once stable/13 is forked. This will also make td->td_et disappear as well.

To sum up, I suggest to proceed with above, and do not overcomplicate the address list locking as this review suggests for sake of ugly KPI.

[markj]

In D17996#448682, @glebius wrote:

1. Introduce if_foreach_addr/if_foreach_maddr
2. Convert all drivers to this KPI.
3. Declare if_addr_lock/if_maddr_lock and if_multiaddr_count+if_multiaddr_array gone in 14.0
4. Indeed make them gone in head once stable/13 is forked. This will also make td->td_et disappear as well.

To sum up, I suggest to proceed with above, and do not overcomplicate the address list locking as this review suggests for sake of ugly KPI.

It seems reasonable to me. I agree that the current interface is ugly; I only touched this code to fix the race in stable/12 without changing KPIs/KBIs.