Erik Nordmark wrote:
Anders Persson wrote:
The ill_ip_mib is allocated/freed where it was previously done for
ill_ip6_mib. I am not sure I follow the comment about naming, could
you elaborate?
Sorry, I answered the naming part as I read more of the diffs.
ip.c (and elsewhere)
6103 BUMP_MIB(&ip_mib,
ipIfStatsInUnknownProtos);
6104 BUMP_MIB(ill->ill_ip_mib,
6105 ipIfStatsInUnknownProtos);
I think you must switch to only incrementing one MIB in each place;
in the vast majority of cases this will be the ill mib. Thus there
should never be a need to update the ill mib and the global mb in
the same place; use the IPv6 code paths as the example.
Then when handling a mib_get (and kstat update if there are kstats
for these) you can add the per-ill mib and the global mib together
into a separate sum which is what is returned as the global stats.
If there is code that sometimes have an ill and others not, instead
of doing this:
7536 BUMP_MIB(&ip_mib, ipIfStatsOutDiscards);
7537 if (ill != NULL)
7538 BUMP_MIB(ill->ill_ip_mib,
ipIfStatsOutDiscards);
you should do
if (ill != NULL)
BUMP_MIB(ill->ill_ip_mib, ...);
else
BUMP_MIB(&ip_mib, ...);
if there are functions that do lots of those, it might make sense to
have a local 'mibptr' variable set up front based on ill != NULL.
I believe that Rao have some concerns on taking this path.
Rao, can you explain your concerns?
Doing an add loop in ip_snmp_get is in the noise compared to the total
cost of an application doing a mib_get.
Hi Erik,
Thanks for taking the time to do the code review.
My concern is based on the experience of the SolarMax project where we
had 2K tunnels plumbed (This was on specific HW - on a bigger system
there may be more) and every time snmpd ran the system hung because the
code was holding a lock and going through individual counters and
adding them up. We learned that it was best to amortize the time by
updating the aggregate counters and individual counters at the same time
and then just copy them out.
There is obviously a concern that this global structure will be hot and
would result in cache misses. On current non SMP CMT systems this is a
non issue. On SNMP systems we have already seen that this is a better
approach. I believe that would also be the case for NUMA and SMP CMT
systems. If not then we will have to come up with a more creative scheme
such as not holding the lock or having a per cpu global ip statistics
structure. We could also announce that we are deprecating returning
aggregate statistics for V4 and the application requesting the
statistics would have to add them up just like it does for V6.
If you feel we should address this issue now then we will or we could
file an RFE to visit this later. Current testing shows that there is no
degradation with our changes probably because current Solaris code
already does this.
Regards,
Rao.
_______________________________________________
networking-discuss mailing list
[email protected]
