From: "Timo Teräs" <timo.teras@iki.fi>
To: Herbert Xu <herbert@gondor.apana.org.au>
Cc: netdev@vger.kernel.org
Subject: Re: [PATCH] xfrm: cache bundle lookup results in flow cache
Date: Fri, 19 Mar 2010 11:53:44 +0200 [thread overview]
Message-ID: <4BA349A8.9050105@iki.fi> (raw)
In-Reply-To: <20100319093210.GA23895@gondor.apana.org.au>
Herbert Xu wrote:
> On Fri, Mar 19, 2010 at 11:12:21AM +0200, Timo Teräs wrote:
>>> That would be better but it's still hacky. Proper reference
>>> counting like we had before would be my preference.
>> Well, the cache entry is still referenced only very shortly,
>> I don't see why keeping bh disabled why doing it is considered
>> a hack. Refcounting the cache entries is trickier. Though,
>> it could be used to optimize the update process: we could safely
>> update it instead of doing now lookup later.
>
> Well we had a nicely type-agnostic cache which is self-contained,
> but your patch is bleeding generic code into xfrm_policy.c, that's
> why I felt it to be hacky :)
>
> Anyway I see how your scheme works now as far as object life
> is concerned, and I agree that it is safe.
>
> However, I wonder if we could do it while still leaving all the
> object life-cycle management stuff (and the BH disabling bits)
> in flow.c
>
> The crux of the issue is that you now have two objects to track
> instead of one. As the direction is a key in the lookup, we're
> really only worried about the outbound case here.
>
> So how about going back to what I suggested earlier, and keeping
> a back-pointer from xfrm_dst to the policy? Of course xfrm_dst
> would also hold a ref count on the policy. You'd only have to
> do it for the top-level xfrm_dst.
>
> It does mean that you'll need to write a different resolver for
> outbound vs. inbound/forward, but that makes sense because we
> only use bundles for outbound policies.
>
> What do you think?
When I first started reading the code, I got confused slightly
on how the garbage collection is happening. What I did not like
in current is the atomic_dec() in flow.c that does not check if
it was turned to zero. Because on policy objects it means you
need to delete it (which would a bug if it happened in flow.c;
the policy gc calls flush before releasing it's own reference),
but on xfrm_dst it's perfectly ok to do atomic_dec() and the
dst core will garbage collect items.
But now, thinking more, it would probably make more sense to
just cache xfrm_dst's and keep ref to the policy on them. So
in general I agree with your recommendation. The only immediate
problem I can think now is that the resolved would need to
atomically check if xfrm_dst is valid, if not, resolve new one.
But creating new xfrm_dst involves taking locks and can sleep
so it cannot be inside the main resolver.
Alternatively, we'd need to:
a) still expose flow cache entry structs and do locking or
refcounting on them
b) have two version of flow lookup: one that calls resolver
with bh disabled and can atomically lookup and update entry
and a second version that lookups, calls validation, if
not-valid it calls resolver with bh enabled and does a new
flow cache lookup to update cache
Also, relatedly. Is there way to release xfrm_dst's child and
route refs when xfrm_bundle_ok fails? This would improve GC
collection of the ipv4 rtable entries they referenced.
- Timo
next prev parent reply other threads:[~2010-03-19 9:53 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-03-15 12:20 [PATCH] xfrm: cache bundle lookup results in flow cache Timo Teras
2010-03-17 13:07 ` Herbert Xu
2010-03-17 14:16 ` Timo Teräs
2010-03-17 14:58 ` Herbert Xu
2010-03-17 15:56 ` Timo Teräs
2010-03-17 16:32 ` Timo Teräs
2010-03-18 19:30 ` Timo Teräs
2010-03-19 0:31 ` Herbert Xu
2010-03-19 5:48 ` Timo Teräs
2010-03-19 6:03 ` Herbert Xu
2010-03-19 6:21 ` Timo Teräs
2010-03-19 7:17 ` Herbert Xu
2010-03-19 7:27 ` Timo Teräs
2010-03-19 0:32 ` Herbert Xu
2010-03-19 7:20 ` Herbert Xu
2010-03-19 7:48 ` Timo Teräs
2010-03-19 8:29 ` Herbert Xu
2010-03-19 8:37 ` Timo Teräs
2010-03-19 8:47 ` Herbert Xu
2010-03-19 9:12 ` Timo Teräs
2010-03-19 9:32 ` Herbert Xu
2010-03-19 9:53 ` Timo Teräs [this message]
2010-03-20 15:17 ` Herbert Xu
2010-03-20 16:26 ` Timo Teräs
2010-03-21 0:46 ` Herbert Xu
2010-03-21 7:34 ` Timo Teräs
2010-03-21 8:31 ` Timo Teräs
2010-03-22 3:52 ` Herbert Xu
2010-03-22 18:03 ` Timo Teräs
2010-03-23 7:28 ` Timo Teräs
2010-03-23 7:42 ` Herbert Xu
2010-03-23 9:19 ` Timo Teräs
2010-03-23 9:41 ` Herbert Xu
2010-03-22 1:26 ` David Miller
2010-03-22 1:28 ` David Miller
2010-03-22 1:32 ` Herbert Xu
2010-03-22 1:36 ` David Miller
2010-03-22 1:40 ` Herbert Xu
2010-03-22 3:12 ` David Miller
2010-03-22 3:52 ` Herbert Xu
2010-03-22 18:31 ` Timo Teräs
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4BA349A8.9050105@iki.fi \
--to=timo.teras@iki.fi \
--cc=herbert@gondor.apana.org.au \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.