Looks good to me.

In D56942#1314738, @markj wrote:

In D56942#1314723, @zlei wrote:

In D56942#1314704, @markj wrote:

In D56942#1314685, @zlei wrote:

I'm OK with this revision, although I think an explicit memory barrier while increasing / decreasing sc->sc_count will make the code more clear than a NULL check in the reader side (lagg_rr_start).

A memory barrier isn't sufficient. For sc_count and length(sc_ports) to be consistent with each other, you need a mutex.

The writer side is lagg_port_create() and lagg_port_destroy(). They are serialized. The reader side always checks sc_count before accessing sc_ports, so this order should be sufficient for the writer side,

diff --git a/sys/net/if_lagg.c b/sys/net/if_lagg.c
index 0333162da0d4..226565633b3a 100644
--- a/sys/net/if_lagg.c
+++ b/sys/net/if_lagg.c
@@ -879,7 +879,7 @@ lagg_port_create(struct lagg_softc *sc, struct ifnet *ifp)
                CK_SLIST_INSERT_AFTER(tlp, lp, lp_entries);
        else
                CK_SLIST_INSERT_HEAD(&sc->sc_ports, lp, lp_entries);
-       sc->sc_count++;
+       atomic_add_rel_int(&sc->sc_count, 1);
 
        lagg_setmulti(lp);
 
@@ -962,8 +962,8 @@ lagg_port_destroy(struct lagg_port *lp, int rundelport)
        }
 
        /* Finally, remove the port from the lagg */
+       atomic_add_rel_int(&sc->sc_count, -1);
        CK_SLIST_REMOVE(&sc->sc_ports, lp, lagg_port, lp_entries);
-       sc->sc_count--;
 
        /* Update the primary interface */
        if (lp == sc->sc_primary) {

The reader side use atomic_load_acq_int() to get sc_count, then iterate sc_ports.

This is still not sufficient. The barrier just enforces an ordering of memory accesses.

Suppose a reader loads sc_count into a register, and then an interrupt arrives and the reader thread is preempted for a long time. Meanwhile, writer threads are updating the port list. When the reader resumes execution, the list might be completely different.

In D56942#1314704, @markj wrote:

In D56942#1314685, @zlei wrote:

I'm OK with this revision, although I think an explicit memory barrier while increasing / decreasing sc->sc_count will make the code more clear than a NULL check in the reader side (lagg_rr_start).

A memory barrier isn't sufficient. For sc_count and length(sc_ports) to be consistent with each other, you need a mutex.

Looks good to me.

In D57154#1310272, @bms wrote:

This is giving me 2003 flashbacks.

I'm OK with this revision, although I think an explicit memory barrier while increasing / decreasing sc->sc_count will make the code more clear than a NULL check in the reader side (lagg_rr_start).

In D57094#1308941, @pouria wrote:

After @zlei work on VNET. I think it's better to remove is_exclusive=true.
Tested, LGTM

@glebius
Of course acquiring per-interface lock is better than global one and looks like in this case it is possible.

Yes, that is perfect.

In D55203#1305963, @rmacklem wrote:

Could you please take another look at this?
kib@ seems to think this is preferred to just
enabling it for all NICs.

I understand that it is only a "hint", since
routing can change at any time.
However, for an NFS server, routing is
unlikely to change without a NFS server
restart.

In D56942#1304350, @markj wrote:

In D56942#1304346, @zlei wrote:

Actually there are other sc members ( and other places sc->sc_count invoved ) not synchronized properly, I think on architectures with weak memory order they are easier to spot. Found that while I was working on PR 289017 .

Could you be more specific? I'm sure there are other problems, but this one is easy to trigger in practice.

In D56944#1304407, @markj wrote:

In D56944#1304393, @zlei wrote:

Emm, the if_vxlan(4) is not virtualized yet. @hrs attempted the first, me the second, and @kp the third. It is better to proceed to have tests for it.

Sorry, I don't quite follow what you're asking for?

Emm, the if_vxlan(4) is not virtualized yet. @hrs attempted the first, me the second, and @kp the third. It is better to proceed to have tests for it.

Oops ! My stupid mistake.

Actually there are other sc members ( and other places sc->sc_count invoved ) not synchronized properly, I think on architectures with weak memory order they are easier to spot. Found that while I was working on PR 289017 .

Hi @oliver , I was about to MFC one fix to tests/sys/netinet/carp.sh and found this. Is stable/15 not affected by this issue ?

Hi @oliver , I was about to MFC one fix to tests/sys/netinet/carp.sh and found this. Is stable/15 not affected by this issue ?

Spent some time reading the RFC. I'm not sure how useful the Optimistic DAD feature will be. Normal DAD can finish within seconds, is not that enough ?

Indeed the net stack lacks proper ioctl locks. I found that while diagnosing PR 292993 and some attaching / detaching issues. Also some drivers have comments about that. I think normally users do not generate concurrent ioctls, so the issue is merely noticed. For parallel tests that is easy to spot.

In D56761#1300243, @glebius wrote:

Thanks for working on this, Zhenlei!

IMHO, adding atf_check to purely configuration lines is not really important. In "a near bright future", I hope, we will be able to delete the configuration lines from the test bodies completely and declare test jails in some declarative manner instead of scripting.

In D49158#1121383, @dave_freedave.net wrote:

I vastly under appreciated that folks rely upon the ng_eiface not moving with the struct ifnet. Probably because I've been using them in jails for over a decade and only recently noticed myself.

I encountered that situation because I was exploring the ordering of ng_eiface creation, move to jail, and rename. The idea being I could have the same interface names in jails if I waited until after the move to rename them and therefore use the same config for dhcpcd. I was then surprised at what happened when an interface was renamed in a jail and then the jail was shut down. I then went off with the wrong assumption that it was missed, moving ng_eiface to the new vnet.

It requires little effort on a single testcase. Let's keep moving.

1. Added atf_check to config / setup steps.
2. Minor style fix, if ... ; then
3. Shortened the jail names to ${j}[one|two|three] to reduce the line width, and focus only on the different part of the jail name, should improve readability a bit.
4. Prefer ifconfig -j , route -j , sysctl -j over jexec ifconfig / route / sysctl.

Removed redundant -s exit:0 to make the code easier to read.

In D56691#1298953, @sarah.walker2_arm.com wrote:

In D56691#1298952, @zlei wrote:

How can alignment lead locking issues ? Not an expert on this, just out of curious.

The standard behaviour for busdma_bounce implementations is that bus_dmamap with alignment > 1 is marked as COULD_BOUNCE. Among other things this can cause a bounce zone to be allocated at bus_dmamap creation time. This causes further allocations without M_NOWAIT which triggers the locking issue.

In D56614#1298893, @zlei wrote:

It appears some tests, for example tests/sys/mac/ipacl has side effect. The mac_ipacl module affects some tests running vnet jails when security.mac.ipacl.ipv4 is turned on.

Probably extra efforts is needed ( run tests those have side effects at last order and sequentially, or disable them temporally ) before change the default of kyua parallelism parameter.

How can alignment lead locking issues ? Not an expert on this, just out of curious.

It appears some tests, for example tests/sys/mac/ipacl has side effect. The mac_ipacl module affects some tests running vnet jails when security.mac.ipacl.ipv4 is turned on.

Looks good to me.

Tested with init_path=/bin/init set in the boot loader, the change to sh(1) looks good to me.

Tested with dynamically linked init(8). The change to init(8) looks good to me.

In D54752#1296854, @des wrote:

The commit message says “no functional change intended” but there is one: strncpy() zero-fills the unused portion of the buffer, strlcpy() does not, so this leaves ifr partly uninitialized. It probably doesn't matter in this direction, but I don't see this addressed anywhere in the review.

I'd suggest to add links to the discussing about defer calculating the checksum. I believe I saw that before, but I can not find it now.

Looks good to me.

Not familiar with the ucred part, but the concept looks good to me. I'm adding @olce ( I think he is familiar with ucred) for this.

Hi @kp @glebius , I added two tests , see D56606 and D56609 .