On 15 Dec 2022, at 2:01, wangchuanlei wrote:

> Add support to count upall packets, when kmod of openvswitch upcall to
> count the number of packets for upcall succeed and failed, which is a
> better way to see how many packets upcalled on every interfaces.
>
> Signed-off-by: wangchuanlei <wangchuan...@inspur.com>
> ---

Hi,

Thanks for this patch, see comments below.

//Eelco

>
> ovs-kmod already support count statistic of interfaces, the link is
> below, and this commit is the part of userspace.
>
> https://git.kernel.org/netdev/net-next/c/1933ea365aa7
>
> note: this commit is compatible with old version of ovs-kmod, that is,
> even the kernel is older, and do not support count statistic of
> interfaces(do not have the code in upper link), this part of code is
>  still stable!
>
>  include/linux/openvswitch.h  | 19 +++++++++++++++++++
>  include/openvswitch/netdev.h |  3 +++
>  lib/dpctl.c                  |  2 ++
>  lib/dpif-netlink.c           | 13 +++++++++++++
>  lib/dpif-netlink.h           |  2 ++
>  lib/netdev-linux.c           |  8 ++++++++
>  6 files changed, 47 insertions(+)
>
> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
> index 8bb5abdc8..ff2dc58c9 100644
> --- a/include/linux/openvswitch.h
> +++ b/include/linux/openvswitch.h
> @@ -141,6 +141,11 @@ struct ovs_vport_stats {
>       __u64   tx_dropped;             /* no space available in linux  */
>  };
>
> +struct ovs_vport_upcall_stats {
> +     uint64_t   tx_success;          /* total packets upcall succeed */
> +     uint64_t   tx_fail;             /* total packets upcall failed  */
> +};
> +

This is a Linux include file, so it should be aligned with the Linux include.
This structure is not in the Linux UAPI, so please move it to a different 
include file.

Also if you move it to an OVS include, make sure comments start with a capital 
letter and end with a dot.

>  /* Allow last Netlink attribute to be unaligned */
>  #define OVS_DP_F_UNALIGNED   (1 << 0)
>
> @@ -301,11 +306,25 @@ enum ovs_vport_attr {
>       OVS_VPORT_ATTR_PAD,
>       OVS_VPORT_ATTR_IFINDEX,
>       OVS_VPORT_ATTR_NETNSID,
> +     OVS_VPORT_ATTR_UPCALL_STATS,
>       __OVS_VPORT_ATTR_MAX
>  };
>
>  #define OVS_VPORT_ATTR_MAX (__OVS_VPORT_ATTR_MAX - 1)
>
> +/**
> +* enum OVS_VPORT_UPCALL_ATTR -- attributes for %OVS_VPORT_UPCALL* commands
> +* @OVS_VPORT_UPCALL_ATTR_SUCCESS: 64-bit upcall success packets.
> +* @OVS_VPORT_UPCALL_ATTR_FAIL: 64-bit upcall fail packets.
> +*/
> +enum OVS_VPORT_UPCALL_ATTR {

In the Linux include ovs_vport_upcall_attr is lower case, can we make sure we 
copy the exact content from the Linux include?

> +     OVS_VPORT_UPCALL_ATTR_SUCCESS,
> +     OVS_VPORT_UPCALL_ATTR_FAIL,
> +     __OVS_VPORT_UPCALL_ATTR_MAX,
> +};
> +
> +#define OVS_VPORT_UPCALL_ATTR_MAX (__OVS_VPORT_UPCALL_ATTR_MAX - 1)
> +
>  enum {
>       OVS_VXLAN_EXT_UNSPEC,
>       OVS_VXLAN_EXT_GBP,
> diff --git a/include/openvswitch/netdev.h b/include/openvswitch/netdev.h
> index 0c10f7b48..ed1bf73dc 100644
> --- a/include/openvswitch/netdev.h
> +++ b/include/openvswitch/netdev.h
> @@ -87,6 +87,9 @@ struct netdev_stats {
>      uint64_t rx_oversize_errors;
>      uint64_t rx_fragmented_errors;
>      uint64_t rx_jabber_errors;

Can we add a comment here explaining what these stats are? Especially as tx 
sounds like we are sending them out, maybe we should rename them to rx from an 
OVS point of view.

> +
> +    uint64_t tx_upcall_success;
> +    uint64_t tx_upcall_fail;
>  };
>
>  /* Structure representation of custom statistics counter */
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index 29041fa3e..d03d84fe6 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -742,6 +742,8 @@ show_dpif(struct dpif *dpif, struct dpctl_params *dpctl_p)
>                  dpctl_print(dpctl_p, "\n");
>
>                  print_stat(dpctl_p, "    collisions:", s.collisions);
> +                print_stat(dpctl_p, " upcall success:", s.tx_upcall_success);
> +                print_stat(dpctl_p, " upcall fail:", s.tx_upcall_fail);

As mentioned above, we should maybe move it to the RX section?

>                  dpctl_print(dpctl_p, "\n");
>
>                  print_stat(dpctl_p, "    RX bytes:", s.rx_bytes);
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 026b0daa8..492f0ee72 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -4685,6 +4685,8 @@ dpif_netlink_vport_from_ofpbuf(struct 
> dpif_netlink_vport *vport,
>                                     .optional = true },
>          [OVS_VPORT_ATTR_OPTIONS] = { .type = NL_A_NESTED, .optional = true },
>          [OVS_VPORT_ATTR_NETNSID] = { .type = NL_A_U32, .optional = true },
> +        [OVS_VPORT_ATTR_UPCALL_STATS] = { .type = NL_A_NESTED,
> +                                   .optional = true },

Alignment is off.

        [OVS_VPORT_ATTR_UPCALL_STATS] = { .type = NL_A_NESTED,
                                          .optional = true },


>      };
>
>      dpif_netlink_vport_init(vport);
> @@ -4716,6 +4718,17 @@ dpif_netlink_vport_from_ofpbuf(struct 
> dpif_netlink_vport *vport,
>      if (a[OVS_VPORT_ATTR_STATS]) {
>          vport->stats = nl_attr_get(a[OVS_VPORT_ATTR_STATS]);
>      }

We should initialize the upcall_stats to all 1’s (UINT64_MAX) to make sure we 
display them as not supported if they are not read from the kernel.

> +    if (a[OVS_VPORT_ATTR_UPCALL_STATS]) {
> +        const struct nlattr *nla;
> +        size_t left;

Line feed after variable declaration.

> +        NL_NESTED_FOR_EACH (nla, left, a[OVS_VPORT_ATTR_UPCALL_STATS]) {
> +            if (nl_attr_type(nla) == OVS_VPORT_UPCALL_ATTR_SUCCESS) {
> +                vport->upcall_stats.tx_success = nl_attr_get_u64(nla);
> +            } else if (nl_attr_type(nla) == OVS_VPORT_UPCALL_ATTR_FAIL) {
> +                vport->upcall_stats.tx_fail = nl_attr_get_u64(nla);
> +            }
> +        }
> +    }
>      if (a[OVS_VPORT_ATTR_OPTIONS]) {
>          vport->options = nl_attr_get(a[OVS_VPORT_ATTR_OPTIONS]);
>          vport->options_len = nl_attr_get_size(a[OVS_VPORT_ATTR_OPTIONS]);
> diff --git a/lib/dpif-netlink.h b/lib/dpif-netlink.h
> index 24294bc42..1dae371a0 100644
> --- a/lib/dpif-netlink.h
> +++ b/lib/dpif-netlink.h
> @@ -44,6 +44,8 @@ struct dpif_netlink_vport {
>      uint32_t n_upcall_pids;
>      const uint32_t *upcall_pids;           /* OVS_VPORT_ATTR_UPCALL_PID. */
>      const struct ovs_vport_stats *stats;   /* OVS_VPORT_ATTR_STATS. */
> +    struct ovs_vport_upcall_stats upcall_stats;
> +                                           /* OVS_VPORT_ATTR_UPCALL_STATS. */

As we got rid of this structure on the kernel side, we should probably also get 
rid of it here, and do something like this:

    uint64_t upcall_success;               /* OVS_VPORT_UPCALL_ATTR_SUCCESS */
    uint64_t upcall_fail;                  /* OVS_VPORT_UPCALL_ATTR_FAIL */

>      const struct nlattr *options;          /* OVS_VPORT_ATTR_OPTIONS. */
>      size_t options_len;
>  };
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 59e8dc0ae..dc2865466 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -2181,6 +2181,13 @@ netdev_stats_from_ovs_vport_stats(struct netdev_stats 
> *dst,
>      dst->tx_window_errors = 0;
>  }
>
> +static void netdev_stats_from_ovs_vport_upcall_stats(struct netdev_stats 
> *dst,
> +                                  struct dpif_netlink_vport *vport)
> +{
> +    dst->tx_upcall_success = vport->upcall_stats.tx_success;
> +    dst->tx_upcall_fail = vport->upcall_stats.tx_fail;
> +}
> +
>  static int
>  get_stats_via_vport__(const struct netdev *netdev, struct netdev_stats 
> *stats)
>  {
> @@ -2197,6 +2204,7 @@ get_stats_via_vport__(const struct netdev *netdev, 
> struct netdev_stats *stats)
>      }
>
>      netdev_stats_from_ovs_vport_stats(stats, reply.stats);
> +    netdev_stats_from_ovs_vport_upcall_stats(stats, &reply);

I do not think we need a new function for this. Can we not update 
netdev_stats_from_ovs_vport_stats() to take reply as an argument, and do all 
the stats handling there?

>
>      ofpbuf_delete(buf);
>
> -- 
> 2.27.0

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to