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