Re: [PATCH 0/4] Fix routing metrics

From: Steffen Klassert <steffen.klassert@secunet.com>
Date: Tue, 21 Feb 2012 09:18:27 +0100

I need the dst->ops->metrics() method because I removed the direct
reference to the inetpeer metrics from the dst_entry. I had to
remove this direct reference to be able to free the old metrics
safely. A dst_entry with a direct reference to old metrics
could leave the rcu protected region and might then try to access
already freed metrics (i.e. if a dst_entry with old metrics is already
attached to a skb when the routing cache is flushed and the skb is queued
for asynchronous processing). With this patchset we access the interpeer
metrics via the inetpeer itself on every metrics access, so we ensure
the metrics are not freed in the meantime.

Then a callback still seems like extreme overkill just to ensure the
RCU safety of metric pointer accesses.

It seems much simpler to me to just kill the inetpeer when we find out
we actually do need to change the metrics, instead of trying to change
the metric memory from underneath it.  Just make a new inetpeer and let
the old one with the old outdated metrics simply die off as the stray
references disappear.

Remove the old inetpeer from the tree (so it cannot be found in a
lookup), and then any dangling old, invalid, routing cache entries
referring to it will hold a reference count.  And once that final
reference drops, we'll know we can safely free the inetpeer up.  So
we'll use stale metrics for a while from these old invalid routing
cache entries, but that's perfectly fine.

The whole thing is about not doing something non-trivial in the fast
path, which is what your patch does.  Your approach causes us to incur
a cost every single metric access just because we "might" access stale
metrics from a old invalid routing cache entry.  This is not the
common case at all.