On 21 Jun 2022, at 5:29, Jianbo Liu wrote:
> On Mon, 2022-06-20 at 12:15 +0200, Eelco Chaudron wrote: >> On 27 May 2022, at 11:00, Jianbo Liu wrote: >> >>> For dpif-netlink, meters are mapped by tc to police actions with >>> one-to-one relationship. Implement meter offload API to set/get/del >>> the police action, and a hmap is used to save the mappings. >>> An id-pool is used to manage all the available police indexes, >>> which >>> are 0x10000000-0x1fffffff, reserved only for OVS. >>> >>> Signed-off-by: Jianbo Liu <[email protected]> >>> --- <SNIP> >>> + >>> +static int >>> +meter_tc_set_policer(ofproto_meter_id meter_id, >>> + struct ofputil_meter_config *config) >>> +{ >>> + uint32_t police_index; >>> + uint32_t rate, burst; >>> + bool add_policer; >>> + int err; >>> + >>> + ovs_assert(config->bands != NULL); >> >> I would remove the assert and just return and error if bands == NULL >> or n_bands is < 1. >> So: >> >> if (!config->bands || config->n_bands < 1) { >> return EINVAL; >> } >> > > I think maybe not necessary, as dpif_netlink_meter_set__() is called > before, and it doesn't check if bands is not NULL and n_bands >= 1. You are right it’s assuming it’s non-null and walks through the n_bands. So maybe you should just ignore if the bands is not 1, so something like: if (!config->bands || config->n_bands < 1) { return 0; } The dpif_netlink_meter_set__() also check is the band type is OVS_METER_BAND_TYPE_DROP. Not sure if you need to check this also when setting the stats, as now your code assumes this. >>> + rate = config->bands[0].rate; >>> + if (config->flags & OFPMF13_BURST) { >>> + burst = config->bands[0].burst_size; >>> + } else { >>> + burst = config->bands[0].rate; >>> + } <SNIP> >>> +static int >>> +meter_tc_del_policer(ofproto_meter_id meter_id, >>> + struct ofputil_meter_stats *stats, >>> + uint16_t max_bands OVS_UNUSED) >>> +{ >>> + uint32_t police_index; >>> + int err = ENOENT; >>> + >>> + if (!meter_id_lookup(meter_id.uint32, &police_index)) { >>> + err = tc_del_policer_action(police_index, stats); >>> + if (err && err != ENOENT) { >>> + VLOG_WARN_RL(&error_rl, >>> + "Failed to del police %u for meter %u: >>> %s", >>> + police_index, meter_id.uint32, >>> ovs_strerror(err)); >> >> Why do we not cleanup the meter id from the pool on TC failure? Do we >> assume the entry exists and it was not cleaned up correctly? >> > > Just in case, I don't think it's possible becausue the reserved police > index is big enough, and no other application will use them. Yes, we should have enough IDs, was just curious why. Did you encounter this when testing? >> If this is the case, this should be an error log, not a warning, as >> something is wrong in this environment. > > OK, I will add an error log. Thanks, this will at least trigger some attention to a potential problem. >> >>> + } else { >>> + meter_free_police_index(police_index); >>> + } >>> + meter_id_remove(meter_id.uint32); >>> + } >>> + >>> + return err; >>> +} >>> + >>> const struct netdev_flow_api netdev_offload_tc = { >>> .type = "linux_tc", >>> .flow_flush = netdev_tc_flow_flush, >>> @@ -2312,5 +2512,8 @@ const struct netdev_flow_api >>> netdev_offload_tc = { >>> .flow_get = netdev_tc_flow_get, >>> .flow_del = netdev_tc_flow_del, >>> .flow_get_n_flows = netdev_tc_get_n_flows, >>> + .meter_set = meter_tc_set_policer, >>> + .meter_get = meter_tc_get_policer, >>> + .meter_del = meter_tc_del_policer, >>> .init_flow_api = netdev_tc_init_flow_api, >>> }; >>> -- >>> 2.26.2 >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
