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.

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.

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.

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.

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();

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.

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.

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'?

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.

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.