iflib: Add iflib managed counters support
Needs ReviewPublic
Actions

Authored by zlei on Jul 30 2024, 4:40 PM.

Details

Reviewers

shurd
jhb

Group Reviewers

iflib
network

Summary

Currently supports IFCOUNTER_OQDROPS and fallbacks to if_get_counter_default().

PR: 280386
MFC after: 1 week

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

zlei created this revision.Jul 30 2024, 4:40 PM

Herald added a reviewer: shurd. · View Herald TranscriptJul 30 2024, 4:40 PM

Herald added a reviewer: iflib. · View Herald Transcript

Herald added subscribers: glebius, melifaro, ae, imp. · View Herald Transcript

zlei requested review of this revision.Jul 30 2024, 4:40 PM

zlei added a subscriber: mmacy.Jul 30 2024, 5:24 PM

zlei added inline comments.

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 edited the summary of this revision. (Show Details)Jul 30 2024, 5:33 PM

zlei added a reviewer: network.

See the usage of iflib_if_get_counter_default() in D46187 .

erj added a subscriber: erj.Jul 30 2024, 8:17 PM

erj added inline comments.

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.

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.

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 added a subscriber: jhb.Sun, Sep 22, 5:17 PM

zlei added inline comments.

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

zlei added a reviewer: jhb.Sun, Sep 22, 5:18 PM

zlei added a parent revision: D46751: iflib: Make txq mp ring stats persistent.

kbowling added subscribers: np, kbowling.Wed, Sep 25, 6:03 AM

kbowling added inline comments.

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.