[ovs-dev] [PATCH 4/6] internal-vport: Convert internal devices to vport stats.
Jesse Gross
jesse at nicira.com
Thu Jun 10 21:17:07 UTC 2010
On Thu, Jun 10, 2010 at 9:56 AM, Ben Pfaff <blp at nicira.com> wrote:
> Before I get started, while reviewing code I noticed that vport.c has
> some "extern" declarations in it. Those are a no-no in the kernel that
> I see people getting called out on from time to time (mostly by Andrew
> Morton). We should move them into a header.
>
That's a good point. I moved them into vport.h.
>
> On Wed, Jun 09, 2010 at 07:33:19PM -0700, Jesse Gross wrote:
> > Internal devices currently keep track of stats themselves. However,
> > we now have stats tracking in the vport layer, so convert to use
> > that instead to avoid code duplication and take advantage of
> > additional features such as 64-bit counters.
>
> At first I thought there must be recursion in internal_dev_get_stats(),
> but I convinced myself it's OK:
>
> vport_user_stats_get() calls
> vport_get_stats(), which implements generic stats itself.
>
> something in the core kernel network stack calls
> internal_dev_get_stats(), which calls
> vport_get_stats(), which implements generic stats itself.
>
That's right.
>
> It's too bad both network devices and vports have a member called
> 'get_stats'. That's probably the source of my confusion. Maybe we
> should have added a 'v_' or 'vp_' prefix to struct vport_ops member
> names.
>
I'm not sure that renaming the vport functions helps all that much. There
is a potential here for infinite recursion if you're not careful and define
a get_stats() function for the internal dev vport. To me when you call
vport->ops->get_stats() it is pretty clear that you are calling a function
in the vport implementation and renaming it to vport->ops->v_get_stats()
doesn't really improve things.
I renamed internal_dev_get_stats() to internal_dev_sys_stats() and added a
comment so hopefully it should be clear that this is not a function in the
vport implementation. Does that seem sufficient to you?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20100610/9c9f867f/attachment-0003.html>
More information about the dev
mailing list