[ovs-dev] [PATCH v2] Always use generic stats for devices (vports)

Jesse Gross jesse at nicira.com
Thu Sep 15 23:18:06 UTC 2011


On Tue, Sep 13, 2011 at 6:11 PM, Pravin Shelar <pshelar at nicira.com> wrote:
> Fixed according to comments from Jesse.
>
> ------
> Currently ovs is using device stats for Linux devices and count them
> itself in other situations. This leads to overlap with hardware stats,
> inconsistencies, etc. It's much better to just always count the packets
> flowing through the switch and let userspace do any merging that it wants.
>
> Following patch removes vport->get_stats() interface. vport-stat is changed
> to use new `struct ovs_vport_stat` rather than rtnl_link_stats64. Definitions
> of rtnl_link_stats64 is removed from OVS.
>
> Signed-off-by: Pravin B Shelar <pshelar at nicira.com>

The normal way to format this commit message is have the lower portion
be the actual commit message and then any messages such as "Fixed..."
at the bottom separated by three dashes so that it won't go into the
permanent commit log.  Once this is pushed, nobody will care about the
previous iterations that didn't make it in.

> diff --git a/include/openvswitch/datapath-protocol.h b/include/openvswitch/datapath-protocol.h
> index 5687792..5d765b1 100644
> --- a/include/openvswitch/datapath-protocol.h
> +++ b/include/openvswitch/datapath-protocol.h
> +struct ovs_vport_stats {
> +    uint64_t   rx_packets;             /* total packets received       */
> +    uint64_t   tx_packets;             /* total packets transmitted    */
> +    uint64_t   rx_bytes;               /* total bytes received         */
> +    uint64_t   tx_bytes;               /* total bytes transmitted      */
> +    uint64_t   rx_errors;              /* bad packets received         */
> +    uint64_t   tx_errors;              /* packet transmit problems     */
> +    uint64_t   rx_dropped;             /* no space in linux buffers    */
> +    uint64_t   tx_dropped;             /* no space available in linux  */
> +};

This file is exported by the kernel, so we're nominally using kernel
style in it, so it should have tabs instead of spaces.

> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index fc61d45..70f6829 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> -/* Retrieves current device stats for 'netdev'. */
> -static int
> -netdev_linux_get_stats(const struct netdev *netdev_,
> -                       struct netdev_stats *stats)
> +static void
> +netdev_vport_stats(const struct netdev *netdev_,
> +                    struct netdev_stats *stats)

netdev_vport is actually a prefix to indicate code that is part of a
particular module, which this isn't part of.  How about
get_stats_via_vport()?

>         error = netdev_vport_get_stats(netdev_, stats);
> +        if (error) {
> +            VLOG_WARN_RL(&rl, "%s: ovs get stats failed %d",
> +                         netdev_get_name(netdev_), error);

This error message no longer matches up with the function name.

>  /* Copies 'src' into 'dst', performing format conversion in the process. */
>  void
> -netdev_stats_from_rtnl_link_stats64(struct netdev_stats *dst,
> -                                    const struct rtnl_link_stats64 *src)
> -{
> -    COPY_NETDEV_STATS;

Given that there is now only a single instance of COPY_NETDEV_STATS, I
think it's probably better to just put the code directly into
netdev_stats_from_rtnl_link_stats().

> +netdev_stats_from_ovs_vport_stats(struct netdev_stats *dst,
> +                                  const struct ovs_vport_stats *src)
> +{
> +    COPY_OVS_STATS
> +    dst->multicast = 0;
> +    dst->collisions = 0;
> +    dst->rx_length_errors = 0;
> +    dst->rx_over_errors = 0;
> +    dst->rx_crc_errors = 0;
> +    dst->rx_frame_errors = 0;
> +    dst->rx_fifo_errors = 0;
> +    dst->rx_missed_errors = 0;
> +    dst->tx_aborted_errors = 0;
> +    dst->tx_carrier_errors = 0;
> +    dst->tx_fifo_errors = 0;
> +    dst->tx_heartbeat_errors = 0;
> +    dst->tx_window_errors = 0;
>  }

This function is also called by dpif-linux.c to report the stats on
its ports and with this it won't get the benefit of the summing of
stats.  This isn't terribly important because the only user of these
stats is ovs-dpctl.  It might actually be better to remove reporting
of stats directly from dpif than to return incorrect numbers - it's
redundant anyways.  ovs-dpctl can then just call  netdev_get_stats()
instead (this might even be better for debugging since it's what
everything else uses).



More information about the dev mailing list