On 3 May 2022, at 5:08, Jianbo Liu via dev 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. See inline comments… > Signed-off-by: Jianbo Liu <[email protected]> > --- > lib/netdev-offload-tc.c | 209 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 209 insertions(+) > > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c > index 83d57c63b..b3c60c125 100644 > --- a/lib/netdev-offload-tc.c > +++ b/lib/netdev-offload-tc.c > @@ -21,6 +21,7 @@ > > #include "dpif.h" > #include "hash.h" > +#include "id-pool.h" > #include "openvswitch/hmap.h" > #include "openvswitch/match.h" > #include "openvswitch/ofpbuf.h" > @@ -62,6 +63,20 @@ struct chain_node { > uint32_t chain; > }; > > +struct meter_id_to_police_idx_data { > + struct hmap_node meter_id_node; > + uint32_t meter_id; > + uint32_t police_idx; > +}; > + > +#define METER_POLICE_IDS_BASE 0x10000000 > +#define METER_POLICE_IDS_MAX 0x1FFFFFFF > +/* Protects below meter ids pool and hashmaps. */ > +static struct ovs_mutex meter_mutex = OVS_MUTEX_INITIALIZER; > +static struct id_pool *meter_police_ids; This also requires the OVS_GUARDED_BY(meter_mutex) (and taking the mutex in the init function). > +static struct hmap meter_id_to_police_idx OVS_GUARDED_BY(meter_mutex) > + = HMAP_INITIALIZER(&meter_id_to_police_idx); > + > static bool > is_internal_port(const char *type) > { > @@ -2286,6 +2301,10 @@ netdev_tc_init_flow_api(struct netdev *netdev) > > probe_multi_mask_per_prio(ifindex); > probe_ct_state_support(ifindex); > + > + meter_police_ids = id_pool_create(METER_POLICE_IDS_BASE, > + METER_POLICE_IDS_MAX - METER_POLICE_IDS_BASE + > 1); > + > ovsthread_once_done(&once); > } > > @@ -2302,6 +2321,193 @@ netdev_tc_init_flow_api(struct netdev *netdev) > return 0; > } > > +static struct meter_id_to_police_idx_data * > +meter_id_find_locked(uint32_t meter_id) > + OVS_REQUIRES(meter_mutex) > +{ > + struct meter_id_to_police_idx_data *data; > + size_t hash = hash_int(meter_id, 0); > + > + HMAP_FOR_EACH_WITH_HASH (data, meter_id_node, hash, > + &meter_id_to_police_idx) { > + if (data->meter_id == meter_id) { > + return data; > + } > + } > + > + return NULL; > +} > + > +static int > +meter_id_lookup(uint32_t meter_id, uint32_t *police_idx) > +{ > + struct meter_id_to_police_idx_data *data; > + int ret = 0; > + > + ovs_mutex_lock(&meter_mutex); > + data = meter_id_find_locked(meter_id); > + if (data) { > + *police_idx = data->police_idx; > + } else { > + ret = ENOENT; > + } > + ovs_mutex_unlock(&meter_mutex); > + > + return ret; > +} > + > +static void > +meter_id_insert(uint32_t meter_id, uint32_t police_idx) > +{ > + struct meter_id_to_police_idx_data *data; > + > + ovs_mutex_lock(&meter_mutex); > + data = meter_id_find_locked(meter_id); > + if (!data) { > + data = xzalloc(sizeof *data); > + data->meter_id = meter_id; > + data->police_idx = police_idx; > + hmap_insert(&meter_id_to_police_idx, &data->meter_id_node, > + hash_int(meter_id, 0)); > + } else { > + VLOG_WARN_RL(&error_rl, > + "try to insert meter %u (%u) with different police > (%u)", Captital T for try. > + meter_id, data->police_idx, police_idx); This error message might be more clear, as reading only the message it’s not clear we tried to add an already existing meter_id. If the data->policy_idx is equal to policy_idx this message is even more cryptic. > + } > + ovs_mutex_unlock(&meter_mutex); > +} > + > +static void > +meter_id_remove(uint32_t meter_id) > +{ > + struct meter_id_to_police_idx_data *data; > + > + ovs_mutex_lock(&meter_mutex); > + data = meter_id_find_locked(meter_id); > + if (data) { > + hmap_remove(&meter_id_to_police_idx, &data->meter_id_node); > + free(data); > + } > + ovs_mutex_unlock(&meter_mutex); > +} > + > +static bool > +meter_alloc_police_index(uint32_t *police_index) > +{ > + bool ret; > + > + ovs_mutex_lock(&meter_mutex); > + ret = id_pool_alloc_id(meter_police_ids, police_index); > + ovs_mutex_unlock(&meter_mutex); > + > + return ret; > +} > + > +static void > +meter_free_police_index(uint32_t police_index) > +{ > + ovs_mutex_lock(&meter_mutex); > + id_pool_free_id(meter_police_ids, police_index); > + ovs_mutex_unlock(&meter_mutex); > +} > + > +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); > + > + rate = config->bands[0].rate; > + if (config->flags & OFPMF13_BURST) { > + burst = config->bands[0].burst_size; > + } else { > + burst = config->bands[0].rate; > + } > + > + add_policer = (meter_id_lookup(meter_id.uint32, &police_index) == > ENOENT); > + if (add_policer) { > + if (!meter_alloc_police_index(&police_index)) { > + VLOG_WARN_RL(&error_rl, "no free police index for meter id %u", Capital N for no. > + meter_id.uint32); > + return ENOENT; > + } > + } > + > + err = tc_add_policer_action(police_index, > + (config->flags & OFPMF13_KBPS) ? rate : 0, > + (config->flags & OFPMF13_KBPS) ? burst : 0, > + (config->flags & OFPMF13_PKTPS) ? rate : 0, > + (config->flags & OFPMF13_PKTPS) ? burst : 0, > + !add_policer); > + if (err) { > + VLOG_WARN_RL(&error_rl, > + "failed to %s police %u for meter id %u: %s", Capital F for failed. > + add_policer ? "add" : "modify", > + police_index, meter_id.uint32, ovs_strerror(err)); > + goto err_add_policer; > + } > + > + if (add_policer) { > + meter_id_insert(meter_id.uint32, police_index); > + } > + > + return 0; > + > +err_add_policer: > + if (add_policer) { > + meter_free_police_index(police_index); > + } > + return err; > +} > + > +static int > +meter_tc_get_policer(ofproto_meter_id meter_id, > + struct ofputil_meter_stats *stats, > + uint16_t max_bands OVS_UNUSED) > +{ > + uint32_t police_index; > + int err = 0; > + > + if (!meter_id_lookup(meter_id.uint32, &police_index)) { > + err = tc_get_policer_action(police_index, stats); max_bands is not passed, but I think it should be implemented, see the first comment on patch 8/8. > + if (err) { > + VLOG_WARN_RL(&error_rl, > + "failed to get police %u stats for meter %u: %s", Capital F for failed. > + police_index, meter_id.uint32, ovs_strerror(err)); > + } > + } > + > + return err; If the meter is not found no error is returned is this ok? Or should it return NOENT? > +} > + > +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 = 0; > + > + if (!meter_id_lookup(meter_id.uint32, &police_index)) { > + err = tc_del_policer_action(police_index, stats); > + if (err) { > + VLOG_WARN_RL(&error_rl, > + "failed to del police %u for meter %u: %s", Capital F for failed. > + police_index, meter_id.uint32, ovs_strerror(err)); > + } else { > + meter_free_police_index(police_index); Why do we not cleanup the meter id from the pool on TC failure? This could potentially starve the pool? The main reason for this to fail is probably that the entry does not exist? > + } > + meter_id_remove(meter_id.uint32); > + } > + > + return err; If the meter is not found no error is returned is this ok? Or should it return NOENT? > +} > + > const struct netdev_flow_api netdev_offload_tc = { > .type = "linux_tc", > .flow_flush = netdev_tc_flow_flush, > @@ -2312,5 +2518,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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
