svn commit: r228969 - head/sys/netinet

[Marko Zec]

On Friday 18 October 2013 12:09:16 Gleb Smirnoff wrote:
> On Fri, Oct 18, 2013 at 12:07:35AM +0200, Marko Zec wrote:
> M> > Concerning this more general solution from Gleb, with storing the
> vnet M> > pointer in the task, I wander how it is safe when the vnet is
> being M> > removed?
> M> >
> M> > In vnet_destroy() the vnet interfaces are moved back to their parent
> M> > vnets, so setting the vnet context from the interface looks safer.
> M>
> M> +1
> M>
> M> Gleb,
> M>
> M> what exactly was the justification for embedding a vnet context inside
> M> struct task, when it could have been easily embedded, directly or
> M> indirectly, inside the object pointed to by ta_context?
> M>
> M> What is / was your plan to garbage collect such stale pointers?
> M>
> M> At least in the early days of VIMAGE / VNET hacking, all the people
> involved M> strived very hard to store backpointers to vnets in as few
> structs as M> possible, and for quite a while we managed to constrain
> this to only ifnets M> and sockets, and both of those references are
> refcounted in struct vnet.
>
> And my change is in the same direction, isn't it?

No, it is not.

1) you're not refcounting tasks in struct vnet, so when a vnet is freed you 
can't be sure (in a generic way) that there are no pending tasks referencig 
that vnet;

2) taskqueue is a general-purpose facility, not network-stack specialized, 
so you can't afford a luxury to keep non-refcounted references to vnets in 
struct task, since there's no guarantee a task will go away before the 
referenced vnet is destroyed.  In your particular example you're probably 
doing the right thing in pf cleanup code, but then the reference to a vnet 
should be kept in a pf-related object, not struct task.

> We will store vnet 
> pointer only in the struct task, not in all structs that ta_context may
> point at.
>
> M> Now, more (unreferenced) backpointers to struct vnet seem to be
> popping up M> here and there, but in most cases they are embedded in
> objects which are M> guaranteed to persist only as long as the referenced
> vnet is alive.  One M> notable exception to this rule is struct tcpcb
> which apparently can outlive M> vnet teardowns under certain conditions,
> but this is a known issue which M> needs to be fixed, not a precedent
> which calls for more timebombs to be M> scattered around the source tree.
> M>
> M> So pls. reconsider solving the issue at hand using a different
> approach, and M> back out r256587.
>
> I don't see problem here. When a vnet is destroyed, every facility is
> notified to tear down its vnet related data. In pf(4) that would mean,
> that the task is freed, and would never be put again on taskqueue. So I
> don't see any leaks or dereferences of dead vnet here.

Just as you described here, it is the pf(4) code which gets notified on vnet 
teardowns, not some generic taskqueue subroutine, so the right place to 
hold a vnet pointer is in a pf-related struct, not struct task.

I'm also surprised noone else chimed in to object to this obviously 
unnecessary layering violation, since up to this point there were no 
inherent dependencies between taskqueues and vnets, and now you are 
introducing one for no good reason.

Again, please remove vnet references from struct task.

Thanks,

Marko


> I have just looked into multicast code and see that it takes another
> approach. The task is single and global, and it sets vnet context
> per-option. Well, my patch won't hurt its operation. Just orthogonal
> approach.