Use iflib_if_get_counter_default() to get output dropped packets

[zlei]

Summary

PR: 280386
MFC after: 1 week

Test Plan

Stress test with iperf3, monitor the output dropped packets with netstat -di .

Note: iperf3 -u can overflow the output queue excessively.

[zlei]

mgb(4) employs iflib(4) but does not have ifdi_get_counter method. Not sure how it report network statistics counters. CC @emaste .

[erj]

ixl(4) and ice(4) could integrate this function, too:

ixl(4) needs to remove the unused "vsi->oqdrops" variable and just not handle the IFCOUNTER_OQDROPS case in its switch statement in ixl_if_get_counter(), then call your new iflib_if_get_counter_default().

ice(4) is more complex because it does actually include a stat from the hardware to include in OQDROPS; I'm not sure how to get that added to the iflib stat. Maybe it should be added to the ifnet stat instead of being directly reported by ice_if_get_counter()?

[emaste]

In D46187#1052940, @zlei wrote:

mgb(4) employs iflib(4) but does not have ifdi_get_counter method. Not sure how it report network statistics counters. CC @emaste .

It is possible that it just doesn't, right now.

[zlei]

In D46187#1052954, @erj wrote:

ixl(4) and ice(4) could integrate this function, too:

ixl(4) needs to remove the unused "vsi->oqdrops" variable and just not handle the IFCOUNTER_OQDROPS case in its switch statement in ixl_if_get_counter(), then call your new iflib_if_get_counter_default().

That make senses.

ice(4) is more complex because it does actually include a stat from the hardware to include in OQDROPS; I'm not sure how to get that added to the iflib stat. Maybe it should be added to the ifnet stat instead of being directly reported by ice_if_get_counter()?

I think generally OQDROPS = ( hardware drops ) + ( iflib drops ) + ( ifnet drops ) . ALTQ drops can be integrated into ifnet drops, if ALTQ is enabled. I have not looked throughout but some drivers simply ignore ALTQ drops. I have no idea but I think fixing ALTQ drops is out of scope of this change.

Some driver has admin task that retrieve hardware drops into ifnet stats, then OQDROPS can be simplified to ( iflib drops ) + ( ifnet drops ) .

[zlei]

Update ixl(4), iavf(4), ice(4) to take iflib managed OQDROPS into account.

[zlei]

In D46187#1052954, @erj wrote:

ixl(4) and ice(4) could integrate this function, too:

ixl(4) needs to remove the unused "vsi->oqdrops" variable and just not handle the IFCOUNTER_OQDROPS case in its switch statement in ixl_if_get_counter(), then call your new iflib_if_get_counter_default().

"vsi->oqdrops" is actually in use. It is set via IXL_SET_OQDROPS in function ixl_update_vsi_stats().

ice(4) is more complex because it does actually include a stat from the hardware to include in OQDROPS; I'm not sure how to get that added to the iflib stat. Maybe it should be added to the ifnet stat instead of being directly reported by ice_if_get_counter()?

See the diff. It is quite simple but I do not have devices to do the test ;)

[erj]

In D46187#1053418, @zlei wrote:

In D46187#1052954, @erj wrote:

ixl(4) and ice(4) could integrate this function, too:

ixl(4) needs to remove the unused "vsi->oqdrops" variable and just not handle the IFCOUNTER_OQDROPS case in its switch statement in ixl_if_get_counter(), then call your new iflib_if_get_counter_default().

"vsi->oqdrops" is actually in use. It is set via IXL_SET_OQDROPS in function ixl_update_vsi_stats().

You're right; that's what I get for not looking at the code too closely...

ice(4) is more complex because it does actually include a stat from the hardware to include in OQDROPS; I'm not sure how to get that added to the iflib stat. Maybe it should be added to the ifnet stat instead of being directly reported by ice_if_get_counter()?

See the diff. It is quite simple but I do not have devices to do the test ;)

I think the changes are logically correct.

But now I'm thinking that these driver stats functions should change so that for each counter that counts errors, we return the sum of the hardware stats and the result of iflib_if_get_counter_default() so that we never ignore stats collected in either. But that should be something done in some other patch.

[kbowling]

Seems good.

I think, with these stats being combined, it would be very important to think about if they are individually exposed (in the sysctl tree?) whilst one may need to dive in and figure out where the drops are actually happening but that is just a general comment.

sys/dev/e1000/if_em.c

4366

igb htdpmc statistic register may be relevant to oqdrop @erj? can be done in later commit if so.

sys/dev/igc/if_igc.c

2387

htdpmc statistic register may be relevant to oqdrop @erj? can be done in later commit if so.

sys/dev/ixgbe/if_ix.c

1229

SSVPC register may be relevant to OQDROP @erj? Can be done in later commit if so.

[kbowling]

sys/dev/e1000/if_em.c

4366

and/or TQDPC per queue? DPDK seems to recognize that one

sys/dev/igc/if_igc.c

2387

and/or TQDPC per queue? DPDK seems to recognize that one

[zlei]

In D46187#1066462, @kbowling wrote:

Seems good.

I think, with these stats being combined, it would be very important to think about if they are individually exposed (in the sysctl tree?) whilst one may need to dive in and figure out where the drops are actually happening but that is just a general comment.

This drives me to think more about D46751. That looks having a nice effect, that iflib stats can persist and be easily retrieved separately, so no worrying about losing them or save them via more logic.