Top-down VNET teardown
ClosedPublic
Actions

Authored by bz on Jun 7 2016, 7:02 PM.

Details

Reviewers

bms
emaste
markj
jhb
gnn

Commits

rS302054: Get closer to a VIMAGE network stack teardown from top to bottom rather

Summary

Get closer to a VIMAGE network stack teardown from top to bottom rather
than removing the network interfaces first. This change is rather
larger and convoluted as the ordering requirements cannot be separated.

Move the pfil(9) framework to SI_SUB_PROTO_PFIL, move Firewalls and
related modules to their own SI_SUB_PROTO_FIREWALL.
Move initialization of "physical" interfaces to SI_SUB_DRIVERS,
move virtual (cloned) interfaces to SI_SUB_PSEUDO.
Move Multicast to SI_SUB_PROTO_MC.

Re-work parts of multicast initialisation and teardown, not taking the
huge amount of memory into account if used as a module yet.

For interface teardown we try to do as many of them as we can on
SI_SUB_INIT_IF, but for some this makes no sense, e.g., when tunnelling
over a higher layer protocol such as IP. In that case the interface
has to go along (or before) the higher layer protocol is shutdown.

Kernel hhooks need to go last on teardown as they may be used at various
higher layers and we cannot remove them before we cleaned up the higher
layers.

For interface teardown there are multiple paths:
(a) a cloned interface is destroyed (inside a VIMAGE or in the base
system), (b) any interface is moved from a virtual network stack to
a different network stack ("vmove"), or (c) a virtual network stack
is being torn down. All code paths go through if_detach_internal()
where we depending on the vmove flag or the vnet state make a decision
on how much to shut down; in case we are destroying a VNET the
individual protocol layers will cleanup their own parts so we cannot
do so again, for each interface as we end up with, e.g., double-frees,
destroying locks twice or acquiring already destroyed locks.
When calling into certain protocol cleanups we equally have to tell them
whether they need to detach upper layer protocols ("ulp") or not.

Provide or enahnce helper functions to do proper cleanup at a protocol
rather than at an interface level.

Obtained from: projects/vnet
Reviewed by:
Sponsored by: The FreeBSD Foundation
Differential Revision:

Test Plan

Passes make universe, been running it for a few weeks under scripted
testing.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 4280
Build 4324: arc lint + arc unit

Event Timeline

bz updated this revision to Diff 17384.Jun 7 2016, 7:02 PM

bz retitled this revision from to Top-down VNET teardown.

bz updated this object.

bz edited the test plan for this revision. (Show Details)

bz added reviewers: gnn, emaste.

bz added a subscriber: network.

Herald added a subscriber: imp. · View Herald TranscriptJun 7 2016, 7:02 PM

bz added reviewers: markj, jhb.Jun 7 2016, 7:04 PM

It seems that in_ifdetach() is always called with '1'? (There is one place in this patch where in6_ifdetach() is called with 0)

sys/net/if.c
918	I would suggest "The vmove flag, if set, .." as I think that reads clearer. You did that below for the shutdown flag already.
922	s/progress/process/
sys/netinet6/in6_ifattach.c
772–773	I think it would be good to document what 'purgeulp' means for both this function and in_ifdetach() in the comment block above the function.
780–787	This should be a single comment. (Merge existing comment into this one somehow)

cy added a subscriber: cy.Jun 15 2016, 7:40 PM

Addressed @jhb 's comments on comments.

One minor nit shown above.

sys/net/if_bridge.c
1135	What does 1 mean in this case and can it be replaced by a nicer #define?

This revision is now accepted and ready to land.Jun 17 2016, 2:56 PM

One option instead of #define's for the flags is to use wrapper functions. For example:

void
_in6_ifdetach(struct ifnet *ifp, int purgeulp)
{
    ...
}

void
in6_ifdetach(struct ifnet *ifp)
{
    _in6_ifdetach(ifp, 1);
}

void
in6_ifvmove(struct ifnet *ifp)
{
    _in6_ifdetach(ifp, 0);
}

This would make the diff smaller (don't have to change existing ifdetach callers) and might be more readable in callers.

Remove the changes for in_ifdetach() as there is indeed only one
caller.

Use the wrapper functions for in6_ifdetach() as suggested by @jhb .
I went with in6_ifdetach_destroy() which is not exactly what happens
for in_ifattach_destroy() but it is called in the same context so
it makes sense to syntactically keep the name similar.

This revision now requires review to proceed.Jun 21 2016, 12:03 AM

gnn accepted this revision.Jun 21 2016, 12:05 AM

gnn edited edge metadata.

This revision is now accepted and ready to land.Jun 21 2016, 12:05 AM

Closed by commit rS302054: Get closer to a VIMAGE network stack teardown from top to bottom rather (authored by bz). · Explain WhyJun 21 2016, 1:49 PM

This revision was automatically updated to reflect the committed changes.