search

Igalia / snabb

Snabb Switch: Fast open source packet processing
Apache License 2.0
48 stars 5 forks source link

Fix intel_mp RRD counter collection #1178

Closed wingo closed 6 years ago

wingo commented 6 years ago

When we made the 2018.06.01 release, we had some issues with intel_mp counters that had just been resolved prior to the release. Unfortunately that refactoring broke RRD recording for intel_mp counters.

The fundamental issue is that the ptree manager uses inotify (or periodic polling) to see what counters are present for its set of workers. However the symlink mechanism meant that the intel counters are no longer under the workers, and instead in /var/run/snabb/intel-mp. As that directory wasn't being inotified, we didn't see its counters, and thusly the ptree manager failed to create RRD files.

Now, we could monitor /var/run/snabb/intel-mp. However that has a problem, in that it could be there are other instances there; we only want to monitor the general statistics and the queue-specific statistics of the stats queue for the ptree workers.

I propose to fix this by making the symlink tree a bit deeper -- instead of symlinking directories, we only symlink files (in practice, .counter files). If we need a directory, we create it under the app instead of symlinking. That way inotify will see things automatically.

This will need some refactors in "snabb top" as well.

eugeneia commented 6 years ago

Not a huge fan of /var/run/snabb/intel-mp, why is this not under .../<pid>/group?

wingo commented 6 years ago

@eugeneia The issue is that you can have multiple independent users of, say, PCI device 02:00.0 -- via RSS or VMDq -- but only one of those users should collect statistics from the card. So the statistics are a kind of shared resource. But that sharing doesn't necessarily correspond to the group mechanism, so instead we drop it into the intel-mp directory.

We could rename /var/run/snabb/intel-mp to /var/run/snabb/pci, if that's more useful. WDYT?

takikawa commented 6 years ago

Symlinking the individual counters sounds fine to me. I tried this at one point in PR #1148 in this commit (https://github.com/Igalia/snabb/pull/1148/commits/373d1951a22eb434943410ad306b02b14068fa52) which I subsequently undid. I think I only undid the change because it was more code and I thought it was unnecessary (and hadn't realized it broke the RRD recording of course). So I think the approach should work and having RRD recording back sounds like a win!

Re: the intel-mp shm folder, it's used for a few other pieces of state related to VMDq too in the intel-mp driver but having a different name sounds fine and shouldn't be a problem for the driver (presumably the driver can still assume no other drivers will touch the contents of its pci folder while running).