Hi eelco,
thanks for review again , i will give a new version
base on your comments, here, i don't undestand what "Alignment is off"
mean, can you give a explanation? thank you!
> true },
> + [OVS_VPORT_ATTR_UPCALL_STATS] = { .type = NL_A_NESTED,
> + .optional = true },
See v2 ???Alignment is off.???
Best regards!
wangchuanlei
---------------------------------------------------------------------------------
On 28 Dec 2022, at 3:41, 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.
See my comments below. Also, let???s discuss if you are not implementing any of
this in v4.
//Eelco
> Signed-off-by: wangchuanlei <[email protected]>
> ---
>
> Since v1:
> - Patch of add support
> Since v2:
> - Modify format of code
>
> include/linux/openvswitch.h | 14 ++++++++++++++
> include/openvswitch/netdev.h | 4 ++++
> lib/dpctl.c | 2 ++
> lib/dpif-netlink.c | 17 +++++++++++++++++
> lib/dpif-netlink.h | 7 +++++++
> lib/netdev-linux.c | 24 +++++++++++++-----------
> 6 files changed, 57 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
> index 8bb5abdc8..26a6af9a3 100644
> --- a/include/linux/openvswitch.h
> +++ b/include/linux/openvswitch.h
> @@ -301,11 +301,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 {
> + 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..cd8d4b8d2 100644
> --- a/include/openvswitch/netdev.h
> +++ b/include/openvswitch/netdev.h
> @@ -87,6 +87,10 @@ struct netdev_stats {
> uint64_t rx_oversize_errors;
> uint64_t rx_fragmented_errors;
> uint64_t rx_jabber_errors;
> +
> + /* Upcall statistics of datapath in kernel */
> + uint64_t rx_upcall_success;
> + uint64_t rx_upcall_fail;
See the previous comment on v2, maybe something like this (note I also dropped
the rx_ prefix):
--Yes, i'll modify it.
/* Datapath upcall statistics. */
uint64_t upcall_success; /* Rx packets forwarded to userspace. */
uint64_t upcall_fail; /* Rx packets failed forwarding to userspace. */
};
> };
>
> /* Structure representation of custom statistics counter */ diff
> --git a/lib/dpctl.c b/lib/dpctl.c index 29041fa3e..52d857999 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.rx_upcall_success);
> + print_stat(dpctl_p, " upcall fail:",
> + s.rx_upcall_fail);
See comment on v2, ???we should maybe move it to the RX section????.
--I see !
port 1: vport0
RX packets:0 errors:0 dropped:0 overruns:0 frame:0
upcall success:? upcall fail:?
TX packets:0 errors:0 dropped:0 aborted:0 carrier:0
collisions:0
RX bytes:0 TX bytes:0
> 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..1e55b7244 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 },
See v2 ???Alignment is off.???
--
> };
>
> dpif_netlink_vport_init(vport);
> @@ -4716,6 +4718,21 @@ 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]);
> }
> + if (a[OVS_VPORT_ATTR_UPCALL_STATS]) {
> + const struct nlattr *nla;
> + size_t left;
> +
> + 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.upcall_success = nl_attr_get_u64(nla);
> + } else if (nl_attr_type(nla) == OVS_VPORT_UPCALL_ATTR_FAIL) {
> + vport->upcall_stats.upcall_fail = nl_attr_get_u64(nla);
> + }
> + }
> + } else {
> + vport->upcall_stats.upcall_success = UINT64_MAX;
> + vport->upcall_stats.upcall_fail = UINT64_MAX;
> + }
> 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..055696c1a 100644
> --- a/lib/dpif-netlink.h
> +++ b/lib/dpif-netlink.h
> @@ -25,6 +25,11 @@
>
> struct ofpbuf;
>
> +struct ovs_vport_upcall_stats {
> + uint64_t upcall_success; /* OVS_VPORT_UPCALL_ATTR_SUCCESS
> */
> + uint64_t upcall_fail; /* OVS_VPORT_UPCALL_ATTR_FAIL */
> +};
> +
> struct dpif_netlink_vport {
> /* Generic Netlink header. */
> uint8_t cmd;
> @@ -44,6 +49,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. */
In v2 I suggested the below, any reason why you decided not to do it and keep
the structure?
???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 */
???
--Yes??? i will modify it!
> 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..7ecc620ba 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -2156,16 +2156,16 @@ swap_uint64(uint64_t *a, uint64_t *b)
> * 'src' is allowed to be misaligned. */ static void
> netdev_stats_from_ovs_vport_stats(struct netdev_stats *dst,
> - const struct ovs_vport_stats *src)
> -{
> - dst->rx_packets = get_32aligned_u64(&src->rx_packets);
> - dst->tx_packets = get_32aligned_u64(&src->tx_packets);
> - dst->rx_bytes = get_32aligned_u64(&src->rx_bytes);
> - dst->tx_bytes = get_32aligned_u64(&src->tx_bytes);
> - dst->rx_errors = get_32aligned_u64(&src->rx_errors);
> - dst->tx_errors = get_32aligned_u64(&src->tx_errors);
> - dst->rx_dropped = get_32aligned_u64(&src->rx_dropped);
> - dst->tx_dropped = get_32aligned_u64(&src->tx_dropped);
> + struct dpif_netlink_vport *src)
Don???t keep is as source, as it???s a vport struct, I think it should be:
struct dpif_netlink_vport *vport)
--ok!
> +{
> + dst->rx_packets = get_32aligned_u64(&src->stats->rx_packets);
> + dst->tx_packets = get_32aligned_u64(&src->stats->tx_packets);
> + dst->rx_bytes = get_32aligned_u64(&src->stats->rx_bytes);
> + dst->tx_bytes = get_32aligned_u64(&src->stats->tx_bytes);
> + dst->rx_errors = get_32aligned_u64(&src->stats->rx_errors);
> + dst->tx_errors = get_32aligned_u64(&src->stats->tx_errors);
> + dst->rx_dropped = get_32aligned_u64(&src->stats->rx_dropped);
> + dst->tx_dropped = get_32aligned_u64(&src->stats->tx_dropped);
> dst->multicast = 0;
> dst->collisions = 0;
> dst->rx_length_errors = 0;
> @@ -2179,6 +2179,8 @@ netdev_stats_from_ovs_vport_stats(struct netdev_stats
> *dst,
> dst->tx_fifo_errors = 0;
> dst->tx_heartbeat_errors = 0;
> dst->tx_window_errors = 0;
> + dst->rx_upcall_success = src->upcall_stats.upcall_success;
> + dst->rx_upcall_fail = src->upcall_stats.upcall_fail;
> }
>
> static int
> @@ -2196,7 +2198,7 @@ get_stats_via_vport__(const struct netdev *netdev,
> struct netdev_stats *stats)
> return EOPNOTSUPP;
> }
>
> - netdev_stats_from_ovs_vport_stats(stats, reply.stats);
> + netdev_stats_from_ovs_vport_stats(stats, &reply);
>
> ofpbuf_delete(buf);
>
> --
> 2.27.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
