On 3 May 2022, at 5:08, Jianbo Liu via dev wrote:
> Add API to offload meter to HW, and the corresponding functions to call
> the meter callbacks from all the registered flow API providers.
> The interfaces are like those related to meter in dpif_class, in order
> to pass necessary info to HW.
See my comments inline below.
> Signed-off-by: Jianbo Liu <jian...@nvidia.com>
> ---
> lib/netdev-offload-provider.h | 15 ++++++++++++
> lib/netdev-offload.c | 45 +++++++++++++++++++++++++++++++++++
> lib/netdev-offload.h | 8 +++++++
> 3 files changed, 68 insertions(+)
>
> diff --git a/lib/netdev-offload-provider.h b/lib/netdev-offload-provider.h
> index 8ff2de983..2441534c3 100644
> --- a/lib/netdev-offload-provider.h
> +++ b/lib/netdev-offload-provider.h
> @@ -94,6 +94,21 @@ struct netdev_flow_api {
> * takes ownership of a packet if errno != EOPNOTSUPP. */
> int (*hw_miss_packet_recover)(struct netdev *, struct dp_packet *);
>
> + /* Offloads the meter or modifies it if exists in HW
> + * with the given 'meter_id' and the configuration in 'config'. */
> + int (*meter_set)(ofproto_meter_id meter_id,
> + struct ofputil_meter_config *config);
> +
> + /* Queries HW for meter stats with the given 'meter_id'. */
> + int (*meter_get)(ofproto_meter_id meter_id,
> + struct ofputil_meter_stats *stats,
> + uint16_t max_bands);
> +
The comments on meter_del and meter_get need more details on how statistics are
handled. See below, and the comments in dpif-provider.h.
> + /* Removes meter 'meter_id' from HW. */
> + int (*meter_del)(ofproto_meter_id meter_id,
> + struct ofputil_meter_stats *stats,
> + uint16_t max_bands);
> +
> /* Initializies the netdev flow api.
> * Return 0 if successful, otherwise returns a positive errno value. */
> int (*init_flow_api)(struct netdev *);
> diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c
> index fb108c0d5..41f6e1723 100644
> --- a/lib/netdev-offload.c
> +++ b/lib/netdev-offload.c
> @@ -195,6 +195,51 @@ netdev_assign_flow_api(struct netdev *netdev)
> return -1;
> }
>
> +int
> +meter_offload_set(ofproto_meter_id meter_id,
> + struct ofputil_meter_config *config)
> +{
> + struct netdev_registered_flow_api *rfa;
> +
> + CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) {
> + if (rfa->flow_api->meter_set) {
> + rfa->flow_api->meter_set(meter_id, config);
> + }
> + }
> +
> + return 0;
We are not reporting any failures here, this does not seem right to me? Same
for the other three functions below.
> +}
> +
> +int
> +meter_offload_get(ofproto_meter_id meter_id,
> + struct ofputil_meter_stats *stats, uint16_t max_bands)
> +{
> + struct netdev_registered_flow_api *rfa;
> +
> + CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) {
> + if (rfa->flow_api->meter_get) {
> + rfa->flow_api->meter_get(meter_id, stats, max_bands);
The following comments also apply to meter_offload_del() below, and comments on
the first comment ;)
So here in theory multiple offload APIs can handle the same meter ID, so here
we should clear all statistics in the structure and the individual callers
should ADD their counter, not replace them.
Or we should pass in a tmp ofputil_meter_stats structure and do this.
I think this goes for the stats->packet_in_count, stats->byte_in_count and
band->byte_count and band->packet_count.
> + }
> + }
> +
> + return 0;
> +}
> +
> +int
> +meter_offload_del(ofproto_meter_id meter_id,
> + struct ofputil_meter_stats *stats, uint16_t max_bands)
> +{
> + struct netdev_registered_flow_api *rfa;
> +
> + CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) {
> + if (rfa->flow_api->meter_del) {
> + rfa->flow_api->meter_del(meter_id, stats, max_bands);
> + }
> + }
> +
> + return 0;
> +}
> +
> int
> netdev_flow_flush(struct netdev *netdev)
> {
> diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h
> index 8237a85dd..a6d4ba3cb 100644
> --- a/lib/netdev-offload.h
> +++ b/lib/netdev-offload.h
> @@ -22,6 +22,7 @@
> #include "openvswitch/types.h"
> #include "ovs-rcu.h"
> #include "ovs-thread.h"
> +#include "openvswitch/ofp-meter.h"
> #include "packets.h"
> #include "flow.h"
>
> @@ -158,6 +159,13 @@ int netdev_ports_flow_get(const char *dpif_type, struct
> match *match,
> int netdev_ports_get_n_flows(const char *dpif_type,
> odp_port_t port_no, uint64_t *n_flows);
>
> +int meter_offload_set(ofproto_meter_id,
> + struct ofputil_meter_config *);
> +int meter_offload_get(ofproto_meter_id,
> + struct ofputil_meter_stats *, uint16_t);
> +int meter_offload_del(ofproto_meter_id,
> + struct ofputil_meter_stats *, uint16_t);
> +
> #ifdef __cplusplus
> }
> #endif
> --
> 2.26.2
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
