Re: [RFC PATCH] net: Implement read-only protection and COW'ing of metrics.

[Eric Dumazet <eric.dumazet@gmail.com>]

Le mercredi 15 décembre 2010 à 13:21 -0800, David Miller a écrit :
> Routing metrics are now copy-on-write.
> 
> Initially a route entry points it's metrics at a read-only location.
> If a routing table entry exists, it will point there.  Else it will
> point at the all zero metric place- holder 'dst_default_metrics'.
> 
> The writeability state of the metrics is stored in the low bits of the
> metrics pointer, we have two bits left to spare if we want to store
> more states.
> 
> For the initial implementation, COW is implemented simply via kmalloc.
> However future enhancements will change this to place the writable
> metrics somewhere else, in order to increase sharing.  Very likely
> this "somewhere else" will be the inetpeer cache.
> 
> Note also that this means that metrics updates may transiently fail
> if we cannot COW the metrics successfully.
> 
> But even by itself, this patch should decrease memory usage and
> increase cache locality especially for routing workloads.  In those
> cases the read-only metric copies stay in place and never get written
> to.
> 
> TCP workloads where metrics get updated, and those rare cases where
> PMTU triggers occur, will take a very slight performance hit.  But
> that hit will be alleviated when the long-term writable metrics
> move to a more sharable location.
> 
> Since the metrics storage went from a u32 array of RTAX_MAX entries to
> what is essentially a pointer, some retooling of the dst_entry layout
> was necessary.
> 
> Most importantly, we need to preserve the alignment of the reference
> count so that it doesn't share cache lines with the read-mostly state,
> as per Eric Dumazet's alignment assertion checks.
> 
> The only non-trivial bit here is the move of the 'flags' member into
> the writeable cacheline.  This is OK since we are always accessing the
> flags around the same moment when we made a modification to the
> reference count.
> 
> Signed-off-by: David S. Miller <davem@davemloft.net>
> ---

Hi David

>  
> 
> @@ -1840,7 +1843,7 @@ static void rt_set_nexthop(struct rtable *rt, struct fib_result *res, u32 itag)
>  		if (FIB_RES_GW(*res) &&
>  		    FIB_RES_NH(*res).nh_scope == RT_SCOPE_LINK)
>  			rt->rt_gateway = FIB_RES_GW(*res);
> -		dst_import_metrics(dst, fi->fib_metrics);
> +		dst_attach_metrics(dst, fi->fib_metrics, true);

I am a bit concerned about this part.

What prevents fi->fib_metrics to disappear,  if fib is destroyed, since
we dont take a reference ?