Message-ID: <20100322035201.GA14457@gondor.apana.org.au>
Date:	Mon, 22 Mar 2010 11:52:01 +0800
From:	Herbert Xu <herbert@...dor.apana.org.au>
To:	Timo Teräs <timo.teras@....fi>
Cc:	netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH] xfrm: cache bundle lookup results in flow cache

On Sun, Mar 21, 2010 at 10:31:23AM +0200, Timo Teräs wrote:
>
>> Ok, we can do that to skip 2. But I think 1 would be still useful.
>> It'd probably be good to actually have flow_cache_ops pointer in
>> each entry instead of the atomic_t pointer.
>>
>> The reasoning:
>> - we can then have type-based checks that the reference count
>>  is valid (e.g. policy's refcount must not go to zero, it's bug,
>>  and we can call dst_release which warns if refcount goes to
>>  negative); imho it's hack to call atomic_dec instead of the
>>  real type's xxx_put
>> - the flow cache needs to somehow know if the entry is stale so
>>  it'll try to refresh it atomically; e.g. if there's no
>>  check for 'stale', the lookup returns stale xfrm_dst. we'd
>>  then need new api to update the stale entry, or flush it out
>>  and repeat the lookup. the virtual get could check for it being
>>  stale (if so release the entry) and then return null for the
>>  generic code to call the resolver atomically
>> - for paranoia we can actually check the type of the object in
>>  cache via the ops (if needed)

The reason I'd prefer to keep the current scheme is to avoid
an additional indirect function call on each packet.

The way it would work is (we need flow_cache_lookup to return
fle instead of the object):

	fle = flow_cache_lookup
	xdst = fle->object
	if (xdst is stale) {
		flow_cache_mark_obsolete(fle)
		fle = flow_cache_lookup
		xdst = fle->object
		if (xdst is stale)
			return error
	}

Where flow_cache_mark_obsolete would set a flag in the fle that's
checked by flow_cache_lookup.  To prevent the very rare case
where we mark an entry obsolete incorrectly, the resolver function
should double-check that the existing entry is indeed obsolete
before making a new one.

This way we give the overhead over to the slow path where the
bundle is stale.

You were saying that our bundles are going stale very frequently,
that would sound like a bug that we should look into.  The whole
caching scheme is pointless if the bundle is going stale every
other packet.

> - could cache bundle OR policy for outgoing stuff. it's useful
>  to cache the policy in case we need to sleep, or if it's a
>  policy forbidding traffic. in those cases there's no bundle
>  to cache at all. alternatively we can make dummy bundles that
>  are marked invalid and are just used to keep a reference to
>  the policy.

My instinct is to go with dummy bundles.  That way given the
direction we know exactly what object type it is.  Having mixed
object types is just too much of a pain.

> Oh, this also implies that the resolver function should be
> changed to get the old stale object so it can re-use it to
> get the policy object instead of searching it all over again.

That should be easy to implement.  Just prefill the obj argument
to the resolver with either NULL or the stale object.

For the bundle resolver, it should also remove the stale bundle
from the policy bundle list and drop its reference.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@...dor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists