[ovs-dev] netdev_reopen() review

Ben Pfaff 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 mailing list