On 24 May 2022, at 4:05, Jianbo Liu wrote:
> On Mon, 2022-05-23 at 11:59 +0200, Eelco Chaudron wrote: >> >> >> On 16 May 2022, at 12:31, Jianbo Liu wrote: >> >>> On Fri, 2022-05-13 at 13:46 +0200, Eelco Chaudron wrote: >>>> >>>> >>>> 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 <[email protected]> >>>>> --- >>>>> 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. >>>> >>> >>> OK. >>> >>>>> + /* 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. >>>> >>> >>> If offload fails, ovs-meter should work as before. So, it's not >>> necessary to check the return value, right? >> >> If this is the design, maybe we should at least add a log message so >> we know the initialization has failed, so we know why any flow >> inserts would fail with ENOSUPP? > > Did you mean adding logging when return !ENOSUPP? This part is already there in netdev-offload-tc.c > Another issue is that, do you want to log every time user fails to add > or modify a meter parameters? Yes, that was what I was suggesting so in the above code something like: 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(“Failed setting meter for flow api %s, error %d”, rya->flow_api->type, ret); } } } >>>>> +} >>>>> + >>>>> +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. >>>> >>> >>> I don't know why the counters are replaced, could you please >>> explain? >> >> Because the API help not cleanly states that meters should add the >> value rather than replace, it could be wrongly interpreted. But I >> guess you are fixing this. >> >>>> 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. >>> >>> You can check in later patch that I added >>> packet_in_count/byte_in_count >>> by "+=", so the value is not replaced, but added. >> >> ACK, ignore this part. >>> >>>> >>>>> + } >>>>> + } >>>>> + >>>>> + 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
