git sha: c25c49e3971b8c048af047dde1674e5be12d8852
Author: Eelco Chaudron <[email protected]>
Subject: [PATCH 14/41] dpif-offload: Call meter netdev-offload APIs via
dpif-offload.
This patch refactors meter handling by moving meter operations from the
netdev-offload layer to the dpif-offload layer. The change introduces
new meter APIs in dpif-offload-provider.h and implements them in
dpif-offload-tc.c, while removing the corresponding functions from
netdev-offload.
> diff --git a/lib/dpif-offload-tc.c b/lib/dpif-offload-tc.c
> index 63ce789c6..f258c7147 100644
> --- a/lib/dpif-offload-tc.c
> +++ b/lib/dpif-offload-tc.c
> @@ -113,6 +113,8 @@ dpif_offload_tc_open(const struct dpif_offload_class
> *offload_class,
> offload_tc->once_enable = (struct ovsthread_once) \
> OVSTHREAD_ONCE_INITIALIZER;
>
> + dpif_offload_tc_meter_init();
> +
Does this call to dpif_offload_tc_meter_init() within dpif_offload_tc_open
potentially initialize the meter subsystem multiple times if multiple
offload instances are created?
> diff --git a/lib/dpif-offload.c b/lib/dpif-offload.c
> index 537939d2d..5a13e0e44 100644
> --- a/lib/dpif-offload.c
> +++ b/lib/dpif-offload.c
> @@ -31,6 +31,8 @@
>
> VLOG_DEFINE_THIS_MODULE(dpif_offload);
>
> +static struct vlog_rate_limit rl_dbg = VLOG_RATE_LIMIT_INIT(1, 5);
> +
> [ ... ]
> +void
> +dpif_offload_meter_set(const struct dpif *dpif, ofproto_meter_id meter_id,
> + struct ofputil_meter_config *config)
> +{
> + struct dp_offload *dp_offload = dpif_offload_get_dp_offload(dpif);
> + const struct dpif_offload *offload;
> +
> + if (!dp_offload) {
> + return;
> + }
> +
> + LIST_FOR_EACH (offload, dpif_list_node, &dp_offload->offload_providers) {
> + if (offload->class->meter_set) {
> + int err = offload->class->meter_set(offload, meter_id, config);
> + if (err) {
> + /* Offload APIs could fail, for example, because the offload
> + * is not supported. This is fine, as the offload API should
> + * take care of this. */
> + VLOG_DBG_RL(&rl_dbg,
> + "Failed setting meter %u on dpif-offload
> provider"
> + " %s, error %s", meter_id.uint32,
> + dpif_offload_name(offload), ovs_strerror(err));
> + }
> + }
> + }
> +}
Should this function return an error code to indicate if all offload
providers failed, rather than ignoring all failures?
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index 1c554416c..8dc6b0c91 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -3149,6 +3149,21 @@ tc_cleanup_policer_actions(struct id_pool *police_ids,
> hmap_destroy(&map);
> }
>
> +void
> +dpif_offload_tc_meter_init(void) {
Does this opening brace follow the coding style guidelines? Should it be
on the next line?
> + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
> +
> + if (ovsthread_once_start(&once)) {
> + ovs_mutex_lock(&meter_police_ids_mutex);
> + meter_police_ids = id_pool_create(
> + METER_POLICE_IDS_BASE,
> + METER_POLICE_IDS_MAX - METER_POLICE_IDS_BASE + 1);
> + ovs_mutex_unlock(&meter_police_ids_mutex);
> +
> + ovsthread_once_done(&once);
> + }
> +}
> @@ -3202,9 +3218,9 @@ netdev_tc_init_flow_api(struct netdev *netdev)
> probe_vxlan_gbp_support(ifindex);
> probe_enc_flags_support(ifindex);
>
> + dpif_offload_tc_meter_init();
> +
> ovs_mutex_lock(&meter_police_ids_mutex);
> - meter_police_ids = id_pool_create(METER_POLICE_IDS_BASE,
> - METER_POLICE_IDS_MAX - METER_POLICE_IDS_BASE +
> 1);
> tc_cleanup_policer_actions(meter_police_ids, METER_POLICE_IDS_BASE,
> METER_POLICE_IDS_MAX);
> ovs_mutex_unlock(&meter_police_ids_mutex);
With both dpif_offload_tc_open() and netdev_tc_init_flow_api() calling
dpif_offload_tc_meter_init(), does this create a race condition where
tc_cleanup_policer_actions() might access meter_police_ids before it's
initialized?
> +int
> +dpif_offload_tc_meter_set(const struct dpif_offload *offload OVS_UNUSED,
> + ofproto_meter_id meter_id,
> + struct ofputil_meter_config *config)
> +{
> + uint32_t police_index;
> + uint32_t rate, burst;
> + bool add_policer;
> + int err;
> +
> + if (!dpif_offload_is_offload_enabled()) {
> + /* FIXME: If offload is disabled, ignore this call. This preserves
> the
> + * behavior from before the dpif-offload implementation. However, it
> + * also retains the same bug where the meter_id is not offloaded if
> it
> + * was configured before offload was enabled. */
> + return 0;
> + }
Should this FIXME comment be addressed before merging, or is it acceptable
to preserve the existing bug temporarily?
> diff --git a/lib/netdev-offload-tc.h b/lib/netdev-offload-tc.h
> index 74dd33b8b..d0485b4ad 100644
> --- a/lib/netdev-offload-tc.h
> +++ b/lib/netdev-offload-tc.h
> [ ... ]
> +int dpif_offload_tc_meter_del(const struct dpif_offload *, ofproto_meter_id,
> + struct ofputil_meter_stats *);
>
> #endif /* NETDEV_OFFLOAD_TC_H */
> \ No newline at end of file
Does this file lack a trailing newline character as required by the
coding style?
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
