[ovs-dev] netdev_reopen() review
blp at nicira.com
Mon Apr 12 18:30:14 UTC 2010
On Sun, Apr 11, 2010 at 02:09:18PM -0400, Jesse Gross wrote:
> On Thu, Apr 8, 2010 at 7:09 PM, Ben Pfaff <blp at nicira.com> wrote:
> > I pushed a small change to the netdev library to the "wdp" branch this
> > morning. Since this is not a "core" branch (yet) and already has a
> > bunch of badly broken stuff in it, I did not see a need to have it
> > reviewed in advance, but it seems like a good idea to get it reviewed
> > now. Here's the commit.
> The code looks correct to me, though what do you think about calling it
> something like netdev_clone() instead? netdev_reopen() isn't the most
> descriptive name to me.
What do you think of netdev_ref() or netdev_incref()? To me, "clone"
implies data copying.
> I'm assuming that these netdevs are just used to get properties of devices
> and won't be used to send and receive? Obviously since they are are just
> refcounted they share file descriptors.
Yes, that's correct.
> Finally, I haven't really looked at the wdp branch so I don't know the
> context but do you think the performance benefits are worth it? It took me
> a while to understand all the levels to which struct sk_buff in Linux can be
> shared or independent. This isn't nearly as bad but this is now the second
> refcount that we have on netdevs. How does this compare to struct wdp_port
> that contains the netdev?
netdev_(reopen/clone/ref) is used as one step as part of copying
"wdp_port" objects around. If we can't copy wdp_ports cheaply then we
have to copy them expensively or avoid copying them at all. We already
rejected not copying them (see the ovs-hw discussion). It probably
doesn't matter whether it is expensive to copy wdp_ports, because they
are not used anywhere performance critical.
What I do like about using a refcount:
* The netdev implementations themselves don't have to care.
It's transparent to them. If we add a "copy" operation
etc. then presumably they would have to implement it.
* It can't fail. I guess that a copy operation could return an
error if something goes wrong.
* There are very few contexts where even the netdev core code
has to care (i.e. the patch is very short).
I'm leaning toward using the refcounts, as I guess you can see. If you
think my arguments are bogus, please let me know.
More information about the dev