Erik Nordmark wrote:
Rao Shoaib wrote:
Why would a lock be needed? The loop over ills need to ensure that
an ill doesn't disappear while it is being accessed, but that
doesn't require preventing other concerns. Thus I don't understand
what you think is hard here.
>
To walk the ill's one needs to hold the global ill_g_lock.
That is merely how current code does it. If we want current mib code
to be more efficient in this respect, it can restrict itself to hold
ill_g_lock while skipping from one ill to the next, and then call
ill_refhold and then drop the lock. Thus it would acquire ill_g_lock
once around the loop.
But I'm far from convinced that is necessary if the goal for this
project is merely to not make performance any worse.
I did these Q&A session with myself to try to understand the impact
even if we do not do a refhold and drop the lock each time around the
loop.
Actually dropping the lock is an issue as the ill's are held in an avl
tree and if another ill is added or deleted the walk will have problems.
Q: When I do a netstat -s today, how many times does the kernel do an
rw_enter(&ill_g_lock, RW_READER)?
A: at least 6 times. And it calls allocb (i.e. potentially all the way
down into the slab allocator) while holding the lock.
Q: if we want ip_snmp_get_mib2_ip return both per interface statistics
and global ones, how many more walks over the ill are needed? (We want
this so that netstat -sa can display the per interface statistics for
IPv4 and IPv6.)
A: One more. But this is the case whether ip_snmp_get_mib2_ip needs to
add per interface ones together to get the global number or not; in
order to produce the per-interface ones it needs to walk the ills and
while doing that it can also produce the sum.
Correct. My concern was not about how many times but how long. Doing a
bcopy is fast compared to adding each individual field. However your
analysis highlighted something that I missed, i.e the lock is not a
mutex but a reader/writer lock (Even though I made the change). Since we
are holding the lock only as a READER it should be OK. So we will go
ahead and implement your suggestion.
We do have to do some more work in ip_kstat_update to walk the ills. I
haven't looked at the potential performance impact of that in detail.
But ip_kstat seems woefully incomplete in that it doesn't export the
IPv6 stats.
Yes if we don't have a global ip_mib we will have to update this
function. We will also update it for V6.
Rao.
Yes, you should address this now. Postponing things to RFEs means
they are likely to never get done.
>
But what you are suggesting is really an RFE as we are not
introducing this behavior. This is what Solaris does today. If we
were to make this change than we will have to go and test all cases
to make sure there is no degradation.
My point is that you should not introduce the reduction of code
quality by replacing every BUMP_MIB(&ip_mib,...) with two BUMP_MIBs.
Today the code doesn't have two BUMP_MIBs next to each other for the
same stat.
And the result should be uniform across the IPv4 and IPv6 code paths.
>
Can you elaborate on this please. If V4 is to return aggregate stats
and V6 does not have to than how can the result be uniform or are you
suggesting that we deprecate returning aggregates for V4.
What I was referring to was the above dual BUMP_MIBs. IPv6 also shows
where it is necessary to use the global instance (in the output path
when there is no ire hence no ill.)
Erik
_______________________________________________
networking-discuss mailing list
[email protected]
