On 27 Jun 2022, at 11:32, Jianbo Liu wrote:
> On Mon, 2022-06-27 at 11:03 +0200, Eelco Chaudron wrote: >> >> >> 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. > > How about adding one more check? > if (!config->bands || config->n_bands < 1 > || config->bands[0].type != OFPMBT13_DROP) { > return 0; > } That will work, make sure you also update your get stats function with a similar check. >> >>>>> + 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? > > No, I didn't. > >> >>>> 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
