On 21 Jun 2022, at 4:40, Jianbo Liu wrote:
> On Mon, 2022-06-20 at 12:12 +0200, Eelco Chaudron wrote: >> >> 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. >> > > I think yes, because you can see my last patch. I check the return > value of dpif_netlink_meter_set__() before calling meter_offload_set(). > > static int > dpif_netlink_meter_set(struct dpif *dpif_, ofproto_meter_id meter_id, > struct ofputil_meter_config *config) > { > .... > > err = dpif_netlink_meter_set__(dpif_, meter_id, config); > if (!err && netdev_is_flow_api_enabled()) { > meter_offload_set(meter_id, config); > } > > return err; > } > > In dpif_netlink_meter_set__(), we will check the band type. > So, if no err is returned from dpif_netlink_meter_set__(), we can > asssume that there is a drop band in the meter. > > And you are right about the max_band. I will remove it. Thanks for making these (and other) changes in the next rev. >> 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? > > Yes, I will change as you suggested. Thanks. > >> >> 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. > > Sure, will do. Thanks! > >> 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
