Allow changing lagg(4) MTU

[freqlabs]

Summary

Previously, changing the MTU would require destroying the lagg and creating a new one. Now it is allowed to change the MTU of the lagg interface. The MTU of the ports will be set to match. If any port cannot set the new MTU, all ports are reverted to the original MTU of the lagg.

Additionally, when adding ports, the MTU of a port will be automatically set to the MTU of the lagg. As always, the MTU of the lagg is initially determined by the MTU of the first port added. If adding an interface as a port for some reason fails, that interface is reverted to its original MTU.

Test Plan

I have tested this change over the weekend on one of my systems configured for LACP, and under light use nothing seems amiss so far. I don't see any Kyua tests for net code, so some guidance with testing will be appreciated!

I hope someone familiar with the locking can sanity-check that particular aspect.

I have tested changing MTU several times, and I have tested the following error paths:

• Attempting to set an invalid MTU (15000) on the lagg to ensure no change is attempted
• Attempting to set a valid MTU on the lagg (16384) that is unsupported by a port (em) to ensure all ports are reverted and the error message is correctly displayed.

I tested adding a lagg to itself as a port (which did not end well, so I added a check for that and now it ends with a harmless error message).

I tested adding and removing ports several times with different MTUs and checked that the MTU gets changed to correctly match the lagg.

[araujo]

LGTM! I have spoke with Ryan to get a better scenario about this issue and here there is a reference for that: https://redmine.ixsystems.com/issues/25092

Also I have checked the lock!! I will wait somebody from @manpages to double check the documentation, then I will do the process to include it on 12-RELEASE if possible.

[bcr]

You need to bump the .Dd at the beginning of the document to the date of the commit since this is a content change.

[freqlabs]

Bumped the date in the man page.

[0mp]

sys/net/if_lagg.c

1462 ↗

(On Diff #49208)

What is this 576?

1474 ↗

(On Diff #49208)

I'm not sure if that's important but this is not compliant with style(9). It should be indented with 4 spaces, right?

1481 ↗

(On Diff #49208)

Shouldn't it be if (error != 0) as in line 1675?

[0mp]

Also, the manpage patch looks good to me.

sys/net/if_lagg.c

1474 ↗

(On Diff #49208)

Whoops, I've linked style(9) incorrectly in the previous comment. Sorry about that!

[araujo]

sys/net/if_lagg.c

1462 ↗

(On Diff #49208)

It is the minimum accepted datagram, it cannot be less than 576 octets.

1474 ↗

(On Diff #49208)

That is correct!!! I can fix that when I commit it.

1481 ↗

(On Diff #49208)

It has the same result!

[0mp]

sys/net/if_lagg.c

1462 ↗

(On Diff #49208)

Oh, cool.

I just thought that we should use a constant for values like this one but I guess that it's not really necessary in this case, right?

1481 ↗

(On Diff #49208)

True. It's just that style(9) says:

Do not use ! for tests unless it is a boolean, e.g., use:

if (*p == '\0')

not:

if (!*p)

but I'm pretty new here so I'm happy with whatever you find reasonable :)

[pi]

The value seems to come from RFC791, quote:

[...]
All hosts must be prepared to accept datagrams of up to 576 octets (whether they arrive whole or in fragments)
[...]

but I'm not sure if this really means that the *MTU* has to 576 or larger than that. I think the MTU
*can* be smaller, as the ethernet layer does accept smaller packets just fine.

There's IP_MSS with value 576, in netinet/ip.h -- maybe this is related ?

[freqlabs]

For additional context, here is the code from if_bridge.c this is based on:

case SIOCSIFMTU:
        if (ifr->ifr_mtu < 576) {
                error = EINVAL;
                break;
        }
        if (LIST_EMPTY(&sc->sc_iflist)) {
                sc->sc_ifp->if_mtu = ifr->ifr_mtu;
                break;
        }
        BRIDGE_LOCK(sc);
        LIST_FOREACH(bif, &sc->sc_iflist, bif_next) {
                if (bif->bif_ifp->if_mtu != ifr->ifr_mtu) {
                        log(LOG_NOTICE, "%s: invalid MTU: %u(%s)"
                            " != %d\n", sc->sc_ifp->if_xname,
                            bif->bif_ifp->if_mtu,
                            bif->bif_ifp->if_xname, ifr->ifr_mtu);
                        error = EINVAL;
                        break;
                }
        }
        if (!error)
                sc->sc_ifp->if_mtu = ifr->ifr_mtu;
        BRIDGE_UNLOCK(sc);
        break;

It was not obvious to me why the minimum MTU should be limited by IPv4 but I included it here erring on the side of caution.

sys/net/if_lagg.c

1481 ↗

(On Diff #49208)

It should be if (error == 0) though. The MTU will only change if there is no error.

[freqlabs]

Addressed feedback I received here and elsewhere on the previous patch. Please consider this alternative approach.

Key changes:

• There doesn't seem to be a good reason to put a lower bound on the MTU as is done in bridge(4). That has been removed.
• The optimization of checking if there are no ports to avoid taking a lock seems not worth it given how infrequently MTU changes are likely to occur. That has been removed.
• While allowing and requiring the user to manually change port MTU brought lagg(4) in line with bridge(4), the eventual goal was to automatically handle changing the MTU of the ports. Now it does just that, avoiding needless churn:
  • Users cannot manually manipulate the MTU of ports on a lagg in LACP mode (this is the original behavior).
  • Setting the MTU on the lagg is allowed, and sets the MTU of all the ports automatically.
  • If setting the MTU on any port fails, all ports are reverted to the original MTU of the lagg.
• Because no manual MTU manipulation is enabled, no documentation change is necessary.

I didn't see any document stating that lagg(4) MTU specifically cannot be changed, I hope I didn't miss it hiding somewhere though.

[freqlabs]

Addressed feedback by adding handling of new ports, so creating a lagg or adding new ports to an existing lagg sets all the port interfaces to the correct MTU automatically, too.

I also added a check to prevent a recursive lagg being created, which would cause infinite loops and other awful things if attempted.

[mav]

I have no specific objections. Looks good to me. It just looks slightly odd, that MTU is handled different from other properties -- set before interface addition to LAGG, while multicast filters set after just before protocol initialization, and capabilities are set after the protocol initialization. As one of downsides this different order required addition of duplicate checks for LAGG nesting.