On 27 May 2022, at 11:00, Jianbo Liu 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. > > Signed-off-by: Jianbo Liu <[email protected]> > --- > lib/netdev-offload-provider.h | 19 +++++++++++++ > lib/netdev-offload.c | 51 +++++++++++++++++++++++++++++++++++ > lib/netdev-offload.h | 8 ++++++ > 3 files changed, 78 insertions(+) > > diff --git a/lib/netdev-offload-provider.h b/lib/netdev-offload-provider.h > index 8ff2de983..c3028606b 100644 > --- a/lib/netdev-offload-provider.h > +++ b/lib/netdev-offload-provider.h > @@ -94,6 +94,25 @@ struct netdev_flow_api { > * takes ownership of a packet if errno != EOPNOTSUPP. */ > int (*hw_miss_packet_recover)(struct netdev *, struct dp_packet *); Your code seems to assume that the stats structure is already initialized, this includes setting up all the possible bands, the stats->n_bands is also set up and should be used in your code (see also my comments on patch 6). If this is true, you can remove the max_band variable from all your callback. If this is not the case, you should change your patch 6, to use the max_bands value and increase/update the n_bands if it's currently not set to 1. > + /* Offloads or modifies the offloaded meter in HW with the given > 'meter_id' > + * and the configuration in 'config'. > + * > + * The meter id specified through 'config->meter_id' is ignored. */ > + int (*meter_set)(ofproto_meter_id meter_id, > + struct ofputil_meter_config *config); > + > + /* Queries HW for meter stats with the given 'meter_id'. > + * Store the stats of dropped packets to band 0. */ > + int (*meter_get)(ofproto_meter_id meter_id, > + struct ofputil_meter_stats *stats, > + uint16_t max_bands); > + > + /* Removes meter 'meter_id' from HW. Stores meter statistics if > successful. > + * 'stats' may be passed in as NULL if no stats are needed. */ > + int (*meter_del)(ofproto_meter_id meter_id, > + struct ofputil_meter_stats *stats, > + uint16_t max_bands); > + I was hoping for some more details on the API, stats and return values. What about the following change? diff --git a/lib/netdev-offload-provider.h b/lib/netdev-offload-provider.h index c3028606b..9d78fd953 100644 --- a/lib/netdev-offload-provider.h +++ b/lib/netdev-offload-provider.h @@ -95,20 +95,31 @@ struct netdev_flow_api { int (*hw_miss_packet_recover)(struct netdev *, struct dp_packet *); /* Offloads or modifies the offloaded meter in HW with the given 'meter_id' - * and the configuration in 'config'. + * and the configuration in 'config'. On failure, a non-zero error code is + * returned. * * The meter id specified through 'config->meter_id' is ignored. */ int (*meter_set)(ofproto_meter_id meter_id, struct ofputil_meter_config *config); - /* Queries HW for meter stats with the given 'meter_id'. - * Store the stats of dropped packets to band 0. */ + /* Queries HW for meter stats with the given 'meter_id'. Store the stats + * of dropped packets to band 0. On failure, a non-zero error code is + * returned. + * + * Note that the 'stats' structure is already initialized, and only the + * available statistics should be incremented, not replaced. Those fields + * are packet_in_count, byte_in_count and band[]->byte_count and + * band[]->packet_count. */ int (*meter_get)(ofproto_meter_id meter_id, struct ofputil_meter_stats *stats, uint16_t max_bands); - /* Removes meter 'meter_id' from HW. Stores meter statistics if successful. - * 'stats' may be passed in as NULL if no stats are needed. */ + /* Removes meter 'meter_id' from HW. Store the stats of dropped packets to + * band 0. On failure, a non-zero error code is returned. + * + * 'stats' may be passed in as NULL if no stats are needed, See the above + * function for additional details on the 'stats' usage. */ 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..e853b4e2a 100644 > --- a/lib/netdev-offload.c > +++ b/lib/netdev-offload.c > @@ -58,6 +58,7 @@ > VLOG_DEFINE_THIS_MODULE(netdev_offload); > > > +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > static bool netdev_flow_api_enabled = false; > > #define DEFAULT_OFFLOAD_THREAD_NB 1 > @@ -195,6 +196,56 @@ 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) { > + int ret = rfa->flow_api->meter_set(meter_id, config); > + if (ret) { > + VLOG_DBG_RL(&rl, > + "Failed setting meter for flow api %s, error %d", > + rfa->flow_api->type, ret); Thanks for adding this, assumed you would be adding it to the other two functions also. In addition, I added the meter ID to the message. For example: diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c index e853b4e2a..104870383 100644 --- a/lib/netdev-offload.c +++ b/lib/netdev-offload.c @@ -206,9 +206,9 @@ meter_offload_set(ofproto_meter_id meter_id, if (rfa->flow_api->meter_set) { int ret = rfa->flow_api->meter_set(meter_id, config); if (ret) { - VLOG_DBG_RL(&rl, - "Failed setting meter for flow api %s, error %d", - rfa->flow_api->type, ret); + VLOG_DBG_RL(&rl, "Failed setting meter %u for flow api %s, " + "error %d", meter_id.uint32, rfa->flow_api->type, + ret); } } } @@ -224,7 +224,12 @@ meter_offload_get(ofproto_meter_id meter_id, 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); + int ret = rfa->flow_api->meter_get(meter_id, stats, max_bands); + if (ret) { + VLOG_DBG_RL(&rl, "Failed getting meter %u for flow api %s, " + "error %d", meter_id.uint32, rfa->flow_api->type, + ret); + } } } @@ -239,7 +244,12 @@ meter_offload_del(ofproto_meter_id meter_id, 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); + int ret = rfa->flow_api->meter_del(meter_id, stats, max_bands); + if (ret) { + VLOG_DBG_RL(&rl, "Failed deleting meter %u for flow api %s, " + "error %d", meter_id.uint32, rfa->flow_api->type, + ret); + } } } > + } > + } > + } > + > + return 0; > +} > + > +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); > + } > + } > + > + 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
