Dan Smith wrote:
OL> What about leak detection ?
OL> Aren't we missing {netns,netdev}_users()?
This is something I need to give more thought to, but it's not as easy
as it sounds. Network devices aren't released at the last put() like
a lot of other things, and my initial attempts to reconcile the
refcount after a checkpoint operation have not been successful.
However, I'm not sure that it's as important here, because AFAIK, a
network device can only exist in one network namespace at a time. If
we're checkpointing a netdev, it's because we are checkpointing the
namespace that it lives in. Making sure the netns isn't leaked out of
the process tree would be much easier and just as effective, no?
We should guarantee that neither netns nor netdev leaks outside
the container; currently none is. If a netdev can only belong to
a single netns, then it suffices to only care about netns.
quoted
quoted
+config CHECKPOINT_NETNS
+ bool
+ default y if NET && NET_NS && CHECKPOINT
+
OL> Did you mean this to be visible (settable) by the user ?
No, it was specifically supposed to enable itself when those other
items are enabled, but not be a user adjustable toggle. I had a
discussion with Serge about it and we came to this as a solution,
although I don't remember what the problem we started with was. I'll
dig through my IRC logs to see if I can figure it out.
Duh.. my bad, I misinterpreted the code. That's fine.
BTW, there is a similar SYSVIPC_CHECKPOINT - we should decide
if we do X_CHECKPOINT or CHECKPOINT_X for a subsystem X, and
stick to that convention. I prefer the latter - what you did...
quoted
quoted
+ retry:
+ if (++pages > 4) {
+ addrs = -E2BIG;
+ goto out;
+ }
OL> Why 4 ?
It's just a sanity limit.
Hmm... let me be more explicit: why not keep trying until it
realloc fails ? or switch to vmalloc() at some point ?
OL> Do we really need this special case ? I'd be happy with a ckpt_err()
OL> for any error - and the actual error number would be useful to tell
OL> which case it was.
Unless I'm missing something, you asked for this specifically:
https://lists.linux-foundation.org/pipermail/containers/2010-February/022844.html
Lol .. that was me :o But looking at the code it feels wrong,
because the errno already reveals the type of the problem.
I'm thinking - wouldn't it make sense to do error reporting
in checkpoint_netdev() if the call to ->ndo_checkpoint() fails ?
OL> Isn't this check redundant ? I expect it to fail promptly in
OL> checkpoint_netdev() above.
No, as I've said a couple of times previously, this isn't the only way
we can arrive at a netdev for checkpointing. This case is the one
where we're marching through the netns and find a netdev that is not
supported. The other is where we arrive at a device as a peer of
another device, so the other check may come into play at times where
this one doesn't and vice versa.
I'm confused: in checkpoint_ns() inside the for_each_netdev()
loop you first test for dev->netdev_ops->ndo_checkpoint and
then call checkpoint_obj(... CKPT_OBJ_NETDEV) - which in turn
will call checkpoint_netdev(), which will again test for
dev->netdev_ops->ndo_checkpoint ... am I reading it wrongly ?
OL> This may be a bit simpler if you move the first deferqueue_add()
OL> forward to just before the other one. Or better: change dq_netdev
OL> to have two pointers, dev and peer (if any is null, the cleanup
OL> function will skip).
The reason it is this messy is because of the way network devices are
deallocated. Since they don't destroy themselves on the final put(),
we have to explicitly call unregister_netdev() on them when we know
they're no longer used (else we block). Once we've added them to the
deferqueue, we can no longer destroy them here because a reference is
held and the deferqueue will run afterwards.
The ordering of this is a result of me injecting failures at each step
and working it out until I got it to not block on unregistering either
of the devices in all of the error paths. That's not to say it's the
best way, but there is a reason it's ordered the way it is.
How about this - to me it feels simpler:
dev = rtnl_newlink(veth_new_link_msg, &veth, this_name);
if (IS_ERR(dev))
return dev;
peer = dev_get_by_name(current->nsproxy->net_ns, peer_name);
if (!peer) {
ret = -EINVAL;
goto err_dev;
}
ret = ckpt_obj_insert(ctx, peer, h->veth.peer_ref,
CKPT_OBJ_NETDEV);
if (ret < 0)
goto err_peer;
dev_put(peer);
dq.dev = dev;
dq.peer = peer;
ret = deferqueue_add(ctx->deferqueue, &dq, sizeof(dq),
netdev_noop, netdev_cleanup);
if (ret)
goto err_peer;
(yes, you need to adjust struct dq_netdev and netdev_cleanup).
BTW, the variable "didreg" should disappear from restore_veth().
Oren.
OL> I'm confused: in checkpoint_ns() inside the for_each_netdev() loop
OL> you first test for dev->netdev_ops->ndo_checkpoint and then call
OL> checkpoint_obj(... CKPT_OBJ_NETDEV) - which in turn will call
OL> checkpoint_netdev(), which will again test for
dev-> netdev_ops->ndo_checkpoint ... am I reading it wrongly ?
In the case of veth, yes. It goes something like this:
checkpoint_netns() {
foreach netdev in netns {
checkpoint_netdev {
if netdev is veth {
checkpoint_peer(); // Will call checkpoint_netdev again
}
}
}
}
It shouldn't happen, but it seems like since we could potentially add
another checkpoint_obj(mydev) somewhere other than in
checkpoint_netdev(), it is reasonable to check that there is actually
something to call before we call it.
Would you prefer a BUG()?
OL> How about this - to me it feels simpler:
OL> dev = rtnl_newlink(veth_new_link_msg, &veth, this_name);
OL> if (IS_ERR(dev))
OL> return dev;
OL> peer = dev_get_by_name(current->nsproxy->net_ns, peer_name);
OL> if (!peer) {
OL> ret = -EINVAL;
OL> goto err_dev;
OL> }
OL> ret = ckpt_obj_insert(ctx, peer, h->veth.peer_ref,
OL> CKPT_OBJ_NETDEV);
OL> if (ret < 0)
OL> goto err_peer;
OL> dev_put(peer);
OL> dq.dev = dev;
OL> dq.peer = peer;
OL> ret = deferqueue_add(ctx->deferqueue, &dq, sizeof(dq),
OL> netdev_noop, netdev_cleanup);
OL> if (ret)
OL> goto err_peer;
If you fail here you need to unregister_netdev() because the dev_put()
that the objhash will not cause it to happen. Unless we add something
to allow you to remove your object from the hash, you can't prevent
that final put, so you have to have it in the deferqueue for
later. You can't check the refcount in the objhash function because it
will differ depending on the number of addresses and protocols the
device has, and those don't get released until unregister_netdev()
which will block if you call it before you've released all of your
references. If the objhash put function could examine ctx->errno,
then it could drop its reference and then call unregister_netdev(),
but that would involve changing all the drop functions. What am I
missing?
--
Dan Smith
IBM Linux Technology Center
email: danms@us.ibm.com
Dan Smith [off-list ref] writes:
OL> I'm confused: in checkpoint_ns() inside the for_each_netdev() loop
OL> you first test for dev->netdev_ops->ndo_checkpoint and then call
OL> checkpoint_obj(... CKPT_OBJ_NETDEV) - which in turn will call
OL> checkpoint_netdev(), which will again test for
dev-> netdev_ops->ndo_checkpoint ... am I reading it wrongly ?
In the case of veth, yes. It goes something like this:
checkpoint_netns() {
foreach netdev in netns {
checkpoint_netdev {
if netdev is veth {
checkpoint_peer(); // Will call checkpoint_netdev again
}
}
}
}
It shouldn't happen, but it seems like since we could potentially add
another checkpoint_obj(mydev) somewhere other than in
checkpoint_netdev(), it is reasonable to check that there is actually
something to call before we call it.
Would you prefer a BUG()?
OL> How about this - to me it feels simpler:
OL> dev = rtnl_newlink(veth_new_link_msg, &veth, this_name);
OL> if (IS_ERR(dev))
OL> return dev;
OL> peer = dev_get_by_name(current->nsproxy->net_ns, peer_name);
OL> if (!peer) {
OL> ret = -EINVAL;
OL> goto err_dev;
OL> }
OL> ret = ckpt_obj_insert(ctx, peer, h->veth.peer_ref,
OL> CKPT_OBJ_NETDEV);
OL> if (ret < 0)
OL> goto err_peer;
OL> dev_put(peer);
OL> dq.dev = dev;
OL> dq.peer = peer;
OL> ret = deferqueue_add(ctx->deferqueue, &dq, sizeof(dq),
OL> netdev_noop, netdev_cleanup);
OL> if (ret)
OL> goto err_peer;
If you fail here you need to unregister_netdev() because the dev_put()
that the objhash will not cause it to happen. Unless we add something
to allow you to remove your object from the hash, you can't prevent
that final put, so you have to have it in the deferqueue for
later. You can't check the refcount in the objhash function because it
will differ depending on the number of addresses and protocols the
device has, and those don't get released until unregister_netdev()
which will block if you call it before you've released all of your
references. If the objhash put function could examine ctx->errno,
then it could drop its reference and then call unregister_netdev(),
but that would involve changing all the drop functions. What am I
missing?
Can we take advantage of the fact that when you destroy a network
namespace the virtual devices in that network namespace are also destroyed?
Eric
Dan Smith wrote:
OL> I'm confused: in checkpoint_ns() inside the for_each_netdev() loop
OL> you first test for dev->netdev_ops->ndo_checkpoint and then call
OL> checkpoint_obj(... CKPT_OBJ_NETDEV) - which in turn will call
OL> checkpoint_netdev(), which will again test for
dev-> netdev_ops->ndo_checkpoint ... am I reading it wrongly ?
In the case of veth, yes. It goes something like this:
checkpoint_netns() {
foreach netdev in netns {
checkpoint_netdev {
if netdev is veth {
checkpoint_peer(); // Will call checkpoint_netdev again
}
}
}
}
It shouldn't happen, but it seems like since we could potentially add
another checkpoint_obj(mydev) somewhere other than in
checkpoint_netdev(), it is reasonable to check that there is actually
something to call before we call it.
Would you prefer a BUG()?
Ok.. so this is solved over IRC - the test was redundant :)
OL> How about this - to me it feels simpler:
OL> dev = rtnl_newlink(veth_new_link_msg, &veth, this_name);
OL> if (IS_ERR(dev))
OL> return dev;
OL> peer = dev_get_by_name(current->nsproxy->net_ns, peer_name);
OL> if (!peer) {
OL> ret = -EINVAL;
OL> goto err_dev;
OL> }
OL> ret = ckpt_obj_insert(ctx, peer, h->veth.peer_ref,
OL> CKPT_OBJ_NETDEV);
OL> if (ret < 0)
OL> goto err_peer;
OL> dev_put(peer);
OL> dq.dev = dev;
OL> dq.peer = peer;
OL> ret = deferqueue_add(ctx->deferqueue, &dq, sizeof(dq),
OL> netdev_noop, netdev_cleanup);
OL> if (ret)
OL> goto err_peer;
If you fail here you need to unregister_netdev() because the dev_put()
that the objhash will not cause it to happen. Unless we add something
to allow you to remove your object from the hash, you can't prevent
that final put, so you have to have it in the deferqueue for
later. You can't check the refcount in the objhash function because it
will differ depending on the number of addresses and protocols the
device has, and those don't get released until unregister_netdev()
which will block if you call it before you've released all of your
references. If the objhash put function could examine ctx->errno,
then it could drop its reference and then call unregister_netdev(),
but that would involve changing all the drop functions. What am I
missing?
Oh .. I see - I missed that point that a ref is taken once it's
inserted to the objhash, so insert must be preceeded by the call
to deferqueue. Thanks for the explanation.
It still makes sense to have a single call to deferqueue that
relates to both the veth and the peer, instead of two separate
calls, no ?
Oren.