On 3 Jan 2023, at 3:12, wangchuanlei wrote:
> 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 },
The two dots above are not aligned, it should be like:
+ [OVS_VPORT_ATTR_UPCALL_STATS] = { .type = NL_A_NESTED,
+ .optional = true },
You might need a fixed font to see this.
//Eelco
>
> See v2 “Alignment is off.<U+00E2><U+0080>?
>
> 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?<U+00E2><U+0080>?.
> --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.<U+00E2><U+0080>?
> --
>
>> };
>>
>> 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
