[ovs-dev] [PATCH 4/6] internal-vport: Convert internal devices to vport stats.

[Jesse Gross]

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>