lagg: stop double-counting output errors and counting drops as errors

[gallatin]

Summary

Currently lagg double-counts errors from lagg members, and counts drops from members as errors. Eg, if lagg sends a packet, and the underlying hardware driver drops it, that will increment a counter in both the underlying driver, and in lagg. The problem is that lagg also adds errors / drops from underlying drivers to its counts, so the same packet will increment IFCOUNTER_OERRORS twice (once for the underlying driver, once for the error returned from the underlying driver via lagg_proto_start(), or will increment IFCOUNTER_OQDROP *and* IFCOUNTER_OERRORS for the same dropped packet.

Consider the following output from netstat -ndi (filtered for clarity):

Name    Mtu Network       Address              Ipkts Ierrs Idrop    Opkts Oerrs  Coll  Drop
mce0   1500 <Link#4>      98:03:9b:67:c3:d2 14981407225     0     0 100879042527     0     0 1839680
mce2   1500 <Link#6>      98:03:9b:67:c3:d2 15023021203     0     0 100963275792     0     0 1522150
lagg0  1500 <Link#8>      98:03:9b:67:c3:d2 30004428429     0     0 201842318320      3361846   0 3361830

In this case, the underlying ports have dropped 3361830 packets as output drops. Lagg has counted them towards its output errors (along with 16 output errors at the lagg level). The lagg0 line above should look like:

lagg0  1500 <Link#8>      98:03:9b:67:c3:d2 30004428429     0     0 201842318320    16     0 3361830

This change attempts to fix that by incrementing lagg's counters only for errors that do not come from underlying drivers.

[jhb]

sys/net/if_lagg.c

2127

Rather than using 'enc_errors' to count non-errors and using subtraction, perhaps instead only use 'errors' for OERRORS (ENOBUFS). That is, maybe something like:

CK_SLIST_FOREACH(...) {
    ...
    if (last != NULL) {
        m0 = ...;
        if (m0 == NULL) {
            ret = ENOBUFS;
            errors++;
            break;
        }

        ret = lagg_enqueue(...);
        if (ret != 0)
            enc_errors++;
    }
    last = lp;
}

if (last == NULL) {
     /* errors must be zero here since it only increments for last != NULL in the loop */
     if_inc_counter(..., 1);
     m_freem(m);
     return (ENOENT);
}

if (last = lagg_link_active(...) == NULL) {
     /* this is an additional error. */
     if_inc_counter(..., errors + 1);
     m_freem(m);
     return (ENETDOWN);
}

ret = lagg_enqueue(...);

if (errors != 0)
    if_inc_counter(..., errors);

return (ret);

The convoluted logic it does now to decide to just do 'return (ret)' always is kind of dumb, so I would at least simplify it, possibly even move the if_inc_counter before the final call to lagg_enqueue and just return the return value of lagg_enqueue directly without using 'ret'. As it is, the ENOBUFS is never returned as 'ret' is always overwritten.

[gallatin]

Update to reflect jhb's feedback. We can stop checking the ret value of lagg_enqueue() inside the loop entirely, as it will be overwritten the next time it is used outside the loop, so this simplifies things as well.

[• hselasky]

sys/net/if_lagg.c

2157

if_inc_counter(sc->sc_ifp, IFCOUNTER_OERRORS, ++errors);

Saves you one line of code.

[jhb]

sys/net/if_lagg.c

2151

s/errors/1/ here. errors will always be zero as last == NULL means all ports were inactive and we never tried to call m_copym.

[gallatin]

• Fix accounting issue that jhb pointed out in lagg_bcast_start()