igmp: convert igmpstat to use PCPU counters

[mhorne]

Summary

Currently there is no locking done to protect this structure. It is
likely okay due to the low-volume nature of IGMP, but allows for
the possibility of underflow. This appears to be one of the only
holdouts of the conversion to counter(9) which was done for most
protocol stat structures around 2013.

This also updates the visibility of this stats structure so that it can
be consumed from elsewhere in the kernel, consistent with the vast
majority of VNET_PCPUSTAT structures.

[mhorne]

Remove the accidental newline.

[kp]

What else in the kernel wants to have access to this information?

Also, and somewhat unrelatedly, it looks like we don't lock to protect this struct. Its counters get incremented through IGMPSTAT_INC(), but I don't see any locking. That could lead to undercounting as two threads load, increment and store at the same time. That also produces bad caching effects. Ideally they should become counter(9)s instead.

It may not be a huge issue though. An occasional undercount isn't really a problem, and we likely don't see enough IGMP traffic for the cache effects to be problematic either.

[mhorne]

In D27023#602923, @kp wrote:

What else in the kernel wants to have access to this information?

Nothing yet. NetApp has a module that aggregates stats from various protocols, so that's where this patch is coming from. It would seem that a read-only copy is enough for this use-case, if that's your concern.

Also, and somewhat unrelatedly, it looks like we don't lock to protect this struct. Its counters get incremented through IGMPSTAT_INC(), but I don't see any locking. That could lead to undercounting as two threads load, increment and store at the same time. That also produces bad caching effects. Ideally they should become counter(9)s instead.

It may not be a huge issue though. An occasional undercount isn't really a problem, and we likely don't see enough IGMP traffic for the cache effects to be problematic either.

Yeah, seems like most of the high-traffic protocols use VNET_PCPUSTAT_*, which is built on top of counter(9). I don't know all the details of what the conversion would entail, so I'd probably avoid being the one to do it unless it seems necessary.

[kp]

In D27023#602993, @mhorne wrote:

In D27023#602923, @kp wrote:

What else in the kernel wants to have access to this information?

Nothing yet. NetApp has a module that aggregates stats from various protocols, so that's where this patch is coming from. It would seem that a read-only copy is enough for this use-case, if that's your concern.

I was mostly just wondering. If I'd noticed the inverse (i.e. V_igmpstat is in the header but used nowhere else) I'd be inclined to reverse that. Generally you want as little visibility for your data as possible.
It may be worth adding a comment here to explain why it's in the header.

Also, and somewhat unrelatedly, it looks like we don't lock to protect this struct. Its counters get incremented through IGMPSTAT_INC(), but I don't see any locking. That could lead to undercounting as two threads load, increment and store at the same time. That also produces bad caching effects. Ideally they should become counter(9)s instead.

It may not be a huge issue though. An occasional undercount isn't really a problem, and we likely don't see enough IGMP traffic for the cache effects to be problematic either.

Yeah, seems like most of the high-traffic protocols use VNET_PCPUSTAT_*, which is built on top of counter(9). I don't know all the details of what the conversion would entail, so I'd probably avoid being the one to do it unless it seems necessary.

I think it's not particularly pressing. It'd be a little extra complexity (mostly when reading them out, in the sysctl handler) for relatively little benefit.
It may be a good junior project though, although that would require changes in NetApp's module as well.

[bz]

Does that mean that we'll see another few changes like this making more stats public?

I'll add network to the review to see what others think about this change as there'll otherwise probably be public follow-up comments.

[donner]

If necessary, I'd like to change it to a read-only function call, which is exposed instead.

const struct igmpstat  * get_global_igmp_stat();

[donner]

OTOH even read-only access may fail without proper locking: There is no guaranty to read an unaligned multi-byte value while it's modified.
A separate function may keep the locking code locally, if necessary.
It may copy the struct into a (static) "read-only" buffer under the lock.

[mhorne]

In D27023#603075, @bz wrote:

Does that mean that we'll see another few changes like this making more stats public?

I think this will be the only one. As far as I can tell, igmp is the only protocol whose stats are not public to the rest of the kernel. Perhaps there is a reason for this that I'm not aware of, but otherwise it just looks like nobody has needed it before.

Edit: s/the only/one of few/ I found struct pimstat and struct mrtstat which are private stat structures as well.

[mhorne]

In D27023#603532, @lutz_donnerhacke.de wrote:

OTOH even read-only access may fail without proper locking: There is no guaranty to read an unaligned multi-byte value while it's modified.
A separate function may keep the locking code locally, if necessary.
It may copy the struct into a (static) "read-only" buffer under the lock.

Can you expand on what you mean by 'unaligned'?

[mhorne]

In D27023#602997, @kp wrote:

I think it's not particularly pressing. It'd be a little extra complexity (mostly when reading them out, in the sysctl handler) for relatively little benefit.
It may be a good junior project though, although that would require changes in NetApp's module as well.

Okay, it looks simpler than I initially thought. Since we're making changes here I might as well attempt it.

[donner]

In D27023#603682, @mhorne wrote:

In D27023#603532, @lutz_donnerhacke.de wrote:

OTOH even read-only access may fail without proper locking: There is no guaranty to read an unaligned multi-byte value while it's modified.
A separate function may keep the locking code locally, if necessary.
It may copy the struct into a (static) "read-only" buffer under the lock.

Can you expand on what you mean by 'unaligned'?

I.e. reading multiple fields at once. They are inconsistent. Even for single field values, I'm not sure, if every CPU provide an atomic update for every data type in question.

[mhorne]

Convert igmpstat to use counter(9).

The struct's ABI header fields will be zeroed at all times in the kernel, but
set properly for the net.inet.igmp.stats sysctl.

[kp]

sys/netinet/igmp.c

235 ↗

(On Diff #79132)

Do we need this #ifdef? It looks like VNET_PCPUSTAT_SYSUNINIT() should be just as safe to use as VNET_PCPUSTAT_SYSINIT() when VIMAGE is not set.

[mhorne]

sys/netinet/igmp.c

235 ↗

(On Diff #79132)

It would appear that we don't. I added it only because other uses of this macro have the #ifdef. Perhaps it was required at one point.

In any case, I will update this.

[mhorne]

Remove unneeded ifdef.