lagg: Avoid dropping locks when starting the interface

[zlei]

Summary

The init routine of a lagg(4) interface will not change during the whole
lifecycle. So we can call lagg_init() directly instead of through the
function pointer. Well, that requires a drop and pickup lock, which
unnecessarily expose a small race window. Refactor lagg_init() into
lagg_init_locked() and call the later one to avoid that.

Meanwhile, delay updating the driver managed status until after the
interface is really ready.

MFC after: 5 days

[markj]

The init routine of a lagg(4) interface will not change during the whole lifecycle. So we can call lagg_init() directly instead of through the function pointer.

I just want to note that this logic does not generalize everywhere. e.g., netmap will sneakily override the I/O routines in an ifnet in some cases. It does not touch if_init, so this particular case is ok. If you wanted to keep the indirection, you could make lagg_init() check whether it already holds the lock, and behave accordingly.

[zlei]

In D55198#1262845, @markj wrote:

The init routine of a lagg(4) interface will not change during the whole lifecycle. So we can call lagg_init() directly instead of through the function pointer.

I just want to note that this logic does not generalize everywhere. e.g., netmap will sneakily override the I/O routines in an ifnet in some cases. It does not touch if_init, so this particular case is ok.

I also noticed the changing of some I/O routines when adding lagg(4) members.

If you wanted to keep the indirection, you could make lagg_init() check whether it already holds the lock, and behave accordingly.

I think the current approach is proper. The lock LAGG_XLOCK is to serialize the io control, and lagg_init() is assigned to if_init and exposed only to ether_ioctl(), if I do not miss anything.

int
ether_ioctl(struct ifnet *ifp, u_long command, caddr_t data)
{
        struct ifaddr *ifa = (struct ifaddr *) data;
        struct ifreq *ifr = (struct ifreq *) data;
        int error = 0;

        switch (command) {
        case SIOCSIFADDR:
                ifp->if_flags |= IFF_UP;

                switch (ifa->ifa_addr->sa_family) {
#ifdef INET
                case AF_INET:
                        ifp->if_init(ifp->if_softc);    /* before arpwhohas */
                        arp_ifinit(ifp, ifa);
                        break;
#endif
                default:
                        ifp->if_init(ifp->if_softc);
                        break;
                }
                break;
...

The handling of SIOCSIFADDR in ether_ioctl() is not ideally IMO. I have plan to improve this. That is out of this scope.

[markj]

In D55198#1263467, @zlei wrote:

In D55198#1262845, @markj wrote:

The init routine of a lagg(4) interface will not change during the whole lifecycle. So we can call lagg_init() directly instead of through the function pointer.

I just want to note that this logic does not generalize everywhere. e.g., netmap will sneakily override the I/O routines in an ifnet in some cases. It does not touch if_init, so this particular case is ok.

I also noticed the changing of some I/O routines when adding lagg(4) members.

If you wanted to keep the indirection, you could make lagg_init() check whether it already holds the lock, and behave accordingly.

I think the current approach is proper. The lock LAGG_XLOCK is to serialize the io control, and lagg_init() is assigned to if_init and exposed only to ether_ioctl(), if I do not miss anything.

Yes, I think it's ok. If you want to be paranoid you could assert that ifp->if_init == lagg_init.