Re: [PATCH] xfrm: implement basic garbage collection for bundles

["Timo Teräs" <timo.teras@iki.fi>]

Herbert Xu wrote:
> On Sat, Mar 20, 2010 at 02:54:31PM +0200, Timo Teräs wrote:
>> Those are more or less timer based too. I guess it would be
>> better to put to dst core's function then. It can see hanging
>> refs, and call xfrm gc in that case.
> 
> The IPv4 one appears to be invoked from dst_alloc just like
> xfrm.

Yes. But under normal circumstances that's not used. It also
has a separate timer based gc that goes through the buckets
periodically. That e.g. how the expired pmtu routes are kicked
out and how routes with old genid are purged before we start
to reach the gc_thresh limit. This happens in 
rt_worker_func() which is called every ip_rt_gc_interval
(defaults to one minute).
 
>> To even minimize that it would help a lot if xfrm_bundle_ok
>> could release (or swap to dummies) the referenced dst->route
>> and dst->child entries. xfrm_bundle_ok is mostly called for
>> all bundles regularly by xfrm_find_bundle before new ones are
>> created.
> 
> Yes I agree that's what we should do once your other patch is
> applied.
> 
> So every top-level xfrm_dst that's not referenced by some user
> should sit in the flow cache.  When it's needed and we find it
> to be stale we kick it out of the cache and free it along with
> the rest of its constituents.

Right. Ok, just to get road map of what I should do and in which
order and how.

xfrm gc:
  - should dst core call it?
  - should xfrm do it periodically?
  - or do we rely that we get new bundles and let
    xfrm[46]_find_bundle do the clean up?
  - should xfrm_bundle_ok() release the inner dst's right away
    instead of waiting any of the above to happen?

caching of bundles instead of policies for outgoing traffic:
  - should flow cache be per-netns?
  - since it will now have two types of objects, would it make
    sense to have virtual put and get callbacks instead of
    atomic_t pointer. this way qw can BUG_ON() if the refcount
    goes to zero unexpectedly (or call the appropriate destructor)
    and call the real _put function anway (e.g. dst_release does
    WARN_ON stuff); the _get function can also do additional
    checking if the object is valid or not; this way the flow
    cache resolver output is always a valid object
  - resolver to have "resolve_fast" and "resolve_slow". if fast
    fails, the slow gets called with bh enabled so it can sleep,
    since bundle creation needs that.
  - it would probably be then useful to have dummy xfrm_dst for
    policy reference when the policy forbids traffic?

- Timo