In D61320#1405672, @adrian wrote:

is m_len / m_pktlen ever supposed to be negative?

In D61173#1402811, @kib wrote:

Perhaps properly fixing it would change to use designated initializers, as a follow-up.

Looks sane to me.

Looks good to me.

Generally looks good to me.

Looks good to me.

Generally looks good to me. Well I'd recommend fixing existing bugs prior to committing this change.

I like this idea !

Hi @glebius , I made a mistake while doing the benchmark -:)

Looks good to me.

In D58742#1370766, @kbowling wrote:

In D58742#1370364, @zlei wrote:

@kbowling I have the hardware ( QLogic FastLinQ QL41000 ) which is supported by this driver. Do you want me to test with this new feature ?

Yes please. Create some VFs with iovctl and use 'ifconfig -v'

I think this is a nice feature !

Looks good to me.

In D59450#1370998, @cc wrote:

For the case of MTU=64KB, in FreeBSD you have to change the net.inet.tcp.sendspace value from default 32KB to 64KB or larger to make sense of using MSS=64KB. If so, you also have to increase the net.inet.tcp.recvspace value from default 64KB to a larger value.

Reference:
https://wiki.freebsd.org/chengcui/tcp_loopback_perf

I have not tested this change yet, but it looks reasonable!

@kbowling I have the hardware ( QLogic FastLinQ QL41000 ) which is supported by this driver. Do you want me to test with this new feature ?

@glebius I think this is not a breaking change, but it improves the performance significantly ( now with a larger default MTU ). Does it deserve a change log or release note ?

Some time ago I was wondering why the MTU of loopback interface was limited to 16K, I thought a larger one should have better performance so I did a simple test ( iperf3 on lo0 ). It was weird that I saw performance degrees when increasing the MTU to 64K.

Looks good to me. @markj Do you plan to MFC ?

Nice feature!

In D58132#1334119, @pouria wrote:

@adrian Hi, could you please MFC this one into stable/15?
I'm not sure how to apply this patch from phabricator while cherry-picking it from CURRENT..

In D58096#1333045, @pouria wrote:

It's really hard to MFC this change to stable/15.

Looks good to me.

Looks good to me.

Looks good to me.

Looks good to me.

I'd recommend to split this to multiple commits. For example the removal of GRE_RLOCK() / GRE_RUNLOCK() in gre_transmit() is an optimization and does not look like a FIX.

In D57233#1318072, @emaste wrote:

In D57233#1318071, @zlei wrote:

Any plans to MFC this to stable branches ?

Yes, in a couple of days.

Any plans to MFC this to stable branches ?

The part watchdog_clock of this change looks good to me.

Looks sane to me.

In D57459#1316924, @adrian wrote:

hm, good question, should we do it per frame or in batches?

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 @olivier , 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 ?