On Mon, Sep 12, 2011 at 5:39 PM, Pravin Shelar <[email protected]> wrote:
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index 79df5f8..082701c 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -1669,8 +1669,10 @@ static struct vport *lookup_vport(struct ovs_header
> *ovs_header,
> static int change_vport(struct vport *vport, struct nlattr
> *a[OVS_VPORT_ATTR_MAX + 1])
> {
> int err = 0;
> +
> if (a[OVS_VPORT_ATTR_STATS])
> - err = vport_set_stats(vport,
> nla_data(a[OVS_VPORT_ATTR_STATS]));
> + vport_set_stats(vport, nla_data(a[OVS_VPORT_ATTR_STATS]));
> +
> if (!err && a[OVS_VPORT_ATTR_ADDRESS])
> err = vport_set_addr(vport,
> nla_data(a[OVS_VPORT_ATTR_ADDRESS]));
No need to check for an error now that the previous step can't fail.
> diff --git a/datapath/vport.c b/datapath/vport.c
> index b1418a4..acf84ef 100644
> --- a/datapath/vport.c
> +++ b/datapath/vport.c
> -int vport_get_stats(struct vport *vport, struct rtnl_link_stats64 *stats)
> +void vport_get_stats(struct vport *vport, struct rtnl_link_stats64 *stats)
> {
> int i;
>
> - if (!(vport->ops->flags & VPORT_F_GEN_STATS))
> - return vport_call_get_stats(vport, stats);
> -
> /* We potentially have 3 sources of stats that need to be
> * combined: those we have collected (split into err_stats and
> * percpu_stats), offset_stats from set_stats(), and device
> * error stats from get_stats() (for errors that happen
> * downstream and therefore aren't reported through our
> - * vport_record_error() function). */
> + * vport_record_error() function). Merging with device error stats
> + * is moved to userspace. */
Can you update this comment a little more? We don't have get_stats()
any more and the comment about moving things to userspace won't make
much sense once this patch is in.
> diff --git a/datapath/vport.h b/datapath/vport.h
> index bbbb214..08e424e 100644
> --- a/datapath/vport.h
> +++ b/datapath/vport.h
> @@ -113,7 +111,6 @@ struct vport {
> };
>
> #define VPORT_F_REQUIRED (1 << 0) /* If init fails, module loading
> fails. */
> -#define VPORT_F_GEN_STATS (1 << 1) /* Track stats at the generic layer.
> */
> #define VPORT_F_FLOW (1 << 2) /* Sets OVS_CB(skb)->flow. */
> #define VPORT_F_TUN_ID (1 << 3) /* Sets OVS_CB(skb)->tun_id. */
Can you shift the bits down?
Now that we've removed most of the counters, there are only 8 fields
that will ever be non-zero. However, we're storing them in
rtnl_link_stats64 which has 23 separate fields. It seems like it
might make sense to define our own struct again at this point.
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index fc61d45..6553906 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -115,7 +115,6 @@ enum {
> VALID_IN6 = 1 << 3,
> VALID_MTU = 1 << 4,
> VALID_CARRIER = 1 << 5,
> - VALID_IS_PSEUDO = 1 << 6, /* Represents is_internal and is_tap.
> */
> VALID_POLICING = 1 << 7,
> VALID_HAVE_VPORT_STATS = 1 << 8
Can you shift these bits down as well?
> +static void
> +ovs_get_stats(const struct netdev *netdev_,
> + struct netdev_stats *stats)
Can you give this a more descriptive name? Seeing as all of this is
part of OVS, I don't think the ovs_ prefix is very helpful.
> +static int
> +netdev_linux_sys_get_stats(const struct netdev *netdev_,
> + struct netdev_stats *stats)
> +{
> + static int use_netlink_stats = -1;
>
> - error = get_ifindex(netdev_, &ifindex);
> - if (!error) {
> - error = get_stats_via_netlink(ifindex, stats);
> - }
> + if (use_netlink_stats < 0) {
> + use_netlink_stats = check_for_working_netlink_stats();
> + }
> +
> + if (use_netlink_stats) {
> + int error;
> + int ifindex;
> +
> + error = get_ifindex(netdev_, &ifindex);
> + if (!error) {
> + error = get_stats_via_netlink(ifindex, stats);
> } else {
> error = get_stats_via_proc(netdev_get_name(netdev_), stats);
> }
These conditions don't look right to me. If we don't have Netlink
stats then we always return an error. If we do then we check to see
whether we can get the ifindex and if not get stats from /proc.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
