From: Oren Laadan <orenl@cs•columbia.edu>
To: Dan Smith <danms@us•ibm.com>
Cc: containers@lists•osdl.org, den@openvz•org,
netdev@vger•kernel.org, davem@davemloft•net,
ebiederm@xmission•com, benjamin.thery@bull•net
Subject: Re: [PATCH 2/6] C/R: Basic support for network namespaces and devices (v5)
Date: Mon, 08 Mar 2010 13:36:52 -0500 [thread overview]
Message-ID: <4B9543C4.4000508@cs.columbia.edu> (raw)
In-Reply-To: <871vfuiul6.fsf@caffeine.danplanet.com>
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.
next prev parent reply other threads:[~2010-03-08 18:36 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-02-25 20:43 C/R: Checkpoint and restore network namespaces and devices Dan Smith
2010-02-25 20:43 ` [PATCH 1/6] C/R: Add checkpoint and collect hooks to net_device_ops Dan Smith
2010-02-26 12:08 ` David Miller
2010-02-25 20:43 ` [PATCH 2/6] C/R: Basic support for network namespaces and devices (v5) Dan Smith
2010-02-26 12:08 ` David Miller
2010-02-26 14:56 ` Dan Smith
2010-03-06 3:55 ` Oren Laadan
[not found] ` <4B91D234.2020003-eQaUEPhvms7ENvBUuze7eA@public.gmane.org>
2010-03-06 17:09 ` Dan Smith
2010-03-06 3:53 ` Oren Laadan
[not found] ` <4B91D1A3.9030404-eQaUEPhvms7ENvBUuze7eA@public.gmane.org>
2010-03-06 17:08 ` Dan Smith
[not found] ` <87bpf1idic.fsf-FLMGYpZoEPULwtHQx/6qkW3U47Q5hpJU@public.gmane.org>
2010-03-06 22:21 ` Oren Laadan
2010-03-08 17:36 ` Dan Smith
2010-03-08 17:53 ` Eric W. Biederman
2010-03-08 18:07 ` Dan Smith
2010-03-08 18:36 ` Oren Laadan [this message]
2010-02-25 20:43 ` [PATCH 3/6] C/R: Add checkpoint support for veth devices (v2) Dan Smith
2010-02-26 12:09 ` David Miller
2010-02-25 20:43 ` [PATCH 4/6] C/R: Add loopback checkpoint support (v2) Dan Smith
2010-02-26 12:09 ` David Miller
2010-02-25 20:43 ` [PATCH 5/6] C/R: Add a checkpoint handler to the 'sit' device Dan Smith
2010-02-26 12:09 ` David Miller
2010-02-25 20:43 ` [PATCH 6/6] C/R: Add checkpoint support to macvlan driver Dan Smith
2010-02-26 12:09 ` David Miller
2010-03-15 2:49 ` C/R: Checkpoint and restore network namespaces and devices Oren Laadan
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=4B9543C4.4000508@cs.columbia.edu \
--to=orenl@cs$(echo .)columbia.edu \
--cc=benjamin.thery@bull$(echo .)net \
--cc=containers@lists$(echo .)osdl.org \
--cc=danms@us$(echo .)ibm.com \
--cc=davem@davemloft$(echo .)net \
--cc=den@openvz$(echo .)org \
--cc=ebiederm@xmission$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox