iflib: Add iflib managed counters support

[zlei]

Summary

Currently supports IFCOUNTER_OQDROPS and fallbacks to if_get_counter_default().

PR: 280386
MFC after: 1 week

[zlei]

sys/net/iflib.c

2642

I'm not convinced that we need to reset txq stats on every call iflib_stop(). The stats can persist IMO, then iflib_save_stats can be omitted. Can the author @mmacy comment on this ?

[zlei]

See the usage of iflib_if_get_counter_default() in D46187 .

[erj]

sys/net/iflib.c

2642

I agree that I don't think we should do this; maybe it was something that was added because it was useful for debugging when iflib was being developed?

4665

Ok, I think it makes sense that we're adding the IFCOUNTER_OQDROPS from the ifnet, too.

[erj]

PR: 280386

Is this supposed to help solve the problem in the PR? I'm not sure what's going on there, but I'm not sure it's clear in there except for your comment that Odrops isn't incrementing because they're not being retrieved properly.

[zlei]

In D46186#1052952, @erj wrote:

PR: 280386

Is this supposed to help solve the problem in the PR?

Yes. That PR lead this change. I mean when I hacking if_bridge(4), I realized some iflib drivers ( em and igb in that PR ) does not report Odrops correctly.

I'm not sure what's going on there, but I'm not sure it's clear in there except for your comment that Odrops isn't incrementing because they're not being retrieved properly.

So users ( and also me ) got confused what happens behind. Users are more familiar with netstat(8) than driver specific sysctl variables.

[zlei]

sys/net/iflib.c

2642

Long time no responses from @mmacy . I posted the removal of ifmp_ring_reset_stats(txq->ift_br) to D46751. If that is accepted, I'll update and remove iflib_save_stats() in this CR.

4672

With this change we will not have to save the drops stat on every iflib_stop(), then the code will be simpler.

CC @jhb

[kbowling]

sys/net/iflib.c

2642

I think it's just an opinion to retain or not retain the stats. I don't see any logical flaw in resetting them on interface resets, which toggling certain HW changes will accomplish but I am not against retaining them either.. it should just carry sufficient reasoning to make a change in behavior. What problem are you trying to solve by persisting the ring drop stats, and what doesn't work if you leave it as is? That justification should be in your change and eventual commit message.

The code in question comes from sys/dev/cxgbe/t4_mp_ring.c which I think was written by @np you may wish to confer with him.

[zlei]

sys/net/iflib.c

2642

I checked cxgbe code, the reset of stats is called from CHELSIO_T4_CLEAR_STATS ioctl. Currently iflib does not have such approach, so probably no need for ifmp_ring_reset_stats().

Friendly ping @np .

[np]

sys/net/iflib.c

2642

I'm not sure I get the full context here. I see PR 280386 in the summary of this review but we seem to be discussing resetting stats. In general it is useful to have some way of clearing stats so maybe keep the code around and wire it so some clear_stats type sysctl in iflib in the future?

In this specific case it appears that iflib has always cleared the stats during iflib_stop. In D46751 you propose that only the mp_ring stats not be reset while the other txq stats are reset. Why change existing behavior, and only for some txq stats? Like kbowling@ said, a more detailed rationale for these changes would be quite useful.

[shurd]

sys/net/iflib.h

510

I'm confused about what this function is being exported for... is the idea that every driver that uses iflib would need to set this in the ifp?

That sounds like exactly the sort of boilerplate iflib exists to avoid.

[zlei]

sys/net/iflib.c

2642

I'm not sure I get the full context here. I see PR 280386 in the summary of this review but we seem to be discussing resetting stats.

I shall elaborate the SUMMARY of this CR... To be quick and simple, the background of this CR, is a fault found in iflib while debugging PR 280386, that the out dropped packets stats from iflib is not reported correctly via netstat -d option.

My very first revision ( in my local workspace ) does not have iflib_save_stats(), so while I ifconfig up / down / txcsum / tso igc0 I noticed something abnormal, that the out dropped packets decrease. So as you see, I added iflib_save_stats() to workaround it.

In general it is useful to have some way of clearing stats so maybe keep the code around and wire it so some clear_stats type sysctl in iflib in the future?

If that is useful.

In this specific case it appears that iflib has always cleared the stats during iflib_stop. In D46751 you propose that only the mp_ring stats not be reset while the other txq stats are reset. Why change existing behavior, and only for some txq stats? Like kbowling@ said, a more detailed rationale for these changes would be quite useful.

I do not know the reason why to reset mp_ring stats on every iflib_stop(), but that seems not necessary. So I propose to not to reset. I'm not aware other stats so I can not speak why I only make mp_ring stats persistent. I have to admit that I'm not familiar enough with iflib.

sys/net/iflib.h

510

I'm confused about what this function is being exported for... is the idea that every driver that uses iflib would need to set this in the ifp?

That really depends. See D46187 for demonstration. Drivers are still free to set this or provide their own.

My quick search shows that all implementations that use iflib need this due to design of iflib ( it manages tx queues which originally managed by nic drivers themselves, correct me if wrong ) , otherwise the output dropped packets will not reported correctly.

That sounds like exactly the sort of boilerplate iflib exists to avoid.

I'm not getting this. Actually I have ever considered exposing a helper function named iflib_get_counter_tx_drops() instead, to report IFCOUNTER_OQDROPS managed by iflib. Do you mean you would prefer this one ?

[shurd]

sys/net/iflib.h

510

Without digging too deeply, my initial idea would be to just increment IFCOUNTER_OQDROPS whenever ifmp_ring_enqueue() returns ENOBUFS.

[shurd]

sys/net/iflib.h

510

Actually, I think any error it returns would indicate a drop (it's just that ENOBUFS is currently the only error it will return). Likely best to increment whenever it's != 0.