On 1/12/26 12:20 PM, Eelco Chaudron wrote:
> Migrate the meter offload operations (set, get, del, and stats) to use
> the dpif-offload infrastructure instead of calling netdev-offload
> functions directly from dpif-netlink.
>
> This change moves meter_set, meter_get, meter_del, and meter_get_stats
> callbacks to the dpif-offload-provider interface and implements them
> in dpif-offload-tc by calling the existing netdev-offload-tc functions.
>
> Acked-by: Eli Britstein <elibr.nvidia.com>
> Signed-off-by: Eelco Chaudron <[email protected]>
> ---
>
> v2 changes:
> - Fix opening brace on dpif_offload_tc_meter_init().
>
> v3 changes:
> - Fixed some newline issues.
> - Updated commit message.
> - Moved offload enabled checks to dpif-offload from
> dpif-offload-tc.
> ---
> lib/dpif-netlink.c | 29 ++-----------
> lib/dpif-offload-provider.h | 26 ++++++++++++
> lib/dpif-offload-tc.c | 5 +++
> lib/dpif-offload.c | 78 +++++++++++++++++++++++++++++++++++
> lib/dpif-offload.h | 6 +++
> lib/dpif.c | 3 ++
> lib/netdev-offload-provider.h | 27 ------------
> lib/netdev-offload-tc.c | 45 +++++++++++++-------
> lib/netdev-offload-tc.h | 8 ++++
> lib/netdev-offload.c | 61 ---------------------------
> lib/netdev-offload.h | 4 --
> 11 files changed, 161 insertions(+), 131 deletions(-)
>
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 6241ba8f7..aff90976c 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -4229,18 +4229,11 @@ static int
> dpif_netlink_meter_set(struct dpif *dpif_, ofproto_meter_id meter_id,
> struct ofputil_meter_config *config)
> {
> - int err;
> -
> if (probe_broken_meters(dpif_)) {
> return ENOMEM;
> }
>
> - err = dpif_netlink_meter_set__(dpif_, meter_id, config);
> - if (!err && dpif_offload_is_offload_enabled()) {
> - meter_offload_set(meter_id, config);
> - }
> -
> - return err;
> + return dpif_netlink_meter_set__(dpif_, meter_id, config);
> }
>
> /* Retrieve statistics and/or delete meter 'meter_id'. Statistics are
> @@ -4339,30 +4332,16 @@ static int
> dpif_netlink_meter_get(const struct dpif *dpif, ofproto_meter_id meter_id,
> struct ofputil_meter_stats *stats, uint16_t max_bands)
> {
> - int err;
> -
> - err = dpif_netlink_meter_get_stats(dpif, meter_id, stats, max_bands,
> - OVS_METER_CMD_GET);
> - if (!err && dpif_offload_is_offload_enabled()) {
> - meter_offload_get(meter_id, stats);
> - }
> -
> - return err;
> + return dpif_netlink_meter_get_stats(dpif, meter_id, stats, max_bands,
> + OVS_METER_CMD_GET);
> }
>
> static int
> dpif_netlink_meter_del(struct dpif *dpif, ofproto_meter_id meter_id,
> struct ofputil_meter_stats *stats, uint16_t max_bands)
> {
> - int err;
> -
> - err = dpif_netlink_meter_get_stats(dpif, meter_id, stats,
> + return dpif_netlink_meter_get_stats(dpif, meter_id, stats,
> max_bands, OVS_METER_CMD_DEL);
> - if (!err && dpif_offload_is_offload_enabled()) {
> - meter_offload_del(meter_id, stats);
> - }
> -
> - return err;
> }
>
> static bool
> diff --git a/lib/dpif-offload-provider.h b/lib/dpif-offload-provider.h
> index 3855add8f..f49443758 100644
> --- a/lib/dpif-offload-provider.h
> +++ b/lib/dpif-offload-provider.h
> @@ -132,6 +132,32 @@ struct dpif_offload_class {
> * successful, otherwise returns a positive errno value. */
> int (*flow_flush)(const struct dpif_offload *);
>
> + /* Adds or modifies the meter in 'dpif_offload' with the given 'meter_id'
> + * and the configuration in 'config'.
> + *
> + * The meter id specified through 'config->meter_id' is ignored. */
> + int (*meter_set)(const struct dpif_offload *, ofproto_meter_id meter_id,
> + struct ofputil_meter_config *);
> +
> + /* Queries HW for meter stats with the given 'meter_id'. Store the stats
> + * of dropped packets to band 0. On failure, a non-zero error code is
nit: double space.
> + * returned.
> + *
> + * Note that the 'stats' structure is already initialized, and only the
> + * available statistics should be incremented, not replaced. Those
> fields
> + * are packet_in_count, byte_in_count and band[]->byte_count and
> + * band[]->packet_count. */
> + int (*meter_get)(const struct dpif_offload *, ofproto_meter_id meter_id,
> + struct ofputil_meter_stats *);
> +
> + /* Removes meter 'meter_id' from HW. Store the stats of dropped packets
> to
> + * band 0. On failure, a non-zero error code is returned.
> + *
> + * 'stats' may be passed in as NULL if no stats are needed. See the
> above
> + * function for additional details on the 'stats' usage. */
> + int (*meter_del)(const struct dpif_offload *, ofproto_meter_id meter_id,
> + struct ofputil_meter_stats *);
> +
>
> /* These APIs operate directly on the provided netdev for performance
> * reasons. They are intended for use in fast path processing and should
> diff --git a/lib/dpif-offload-tc.c b/lib/dpif-offload-tc.c
> index 200685fa8..70f4f3c97 100644
> --- a/lib/dpif-offload-tc.c
> +++ b/lib/dpif-offload-tc.c
> @@ -119,6 +119,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();
> +
> *dpif_offload = &offload_tc->offload;
> return 0;
> }
> @@ -292,5 +294,8 @@ struct dpif_offload_class dpif_offload_tc_class = {
> .port_add = dpif_offload_tc_port_add,
> .port_del = dpif_offload_tc_port_del,
> .flow_flush = dpif_offload_tc_flow_flush,
> + .meter_set = dpif_offload_tc_meter_set,
> + .meter_get = dpif_offload_tc_meter_get,
> + .meter_del = dpif_offload_tc_meter_del,
> .netdev_flow_flush = dpif_offload_tc_netdev_flow_flush,
> };
> diff --git a/lib/dpif-offload.c b/lib/dpif-offload.c
> index bab811554..357a7fe62 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);
> +
> static struct ovs_mutex dpif_offload_mutex = OVS_MUTEX_INITIALIZER;
> static struct shash dpif_offload_classes \
> OVS_GUARDED_BY(dpif_offload_mutex) = \
> @@ -740,6 +742,82 @@ dpif_offload_flow_flush(struct dpif *dpif)
> }
> }
>
> +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 || !dpif_offload_is_offload_enabled()) {
> + 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));
> + }
> + }
> + }
> +}
> +
> +void
> +dpif_offload_meter_get(const struct dpif *dpif, ofproto_meter_id meter_id,
> + struct ofputil_meter_stats *stats)
> +{
> + struct dp_offload *dp_offload = dpif_offload_get_dp_offload(dpif);
> + const struct dpif_offload *offload;
> +
> + if (!dp_offload || !dpif_offload_is_offload_enabled()) {
> + return;
> + }
> +
> + LIST_FOR_EACH (offload, dpif_list_node, &dp_offload->offload_providers) {
> + if (offload->class->meter_get) {
> + int err = offload->class->meter_get(offload, meter_id, stats);
> + if (err) {
> + VLOG_DBG_RL(&rl_dbg,
> + "Failed getting meter %u on dpif-offload
> provider"
> + " %s, error %s", meter_id.uint32,
> + dpif_offload_name(offload), ovs_strerror(err));
> + }
> + }
> + }
> +}
> +
> +void
> +dpif_offload_meter_del(const struct dpif *dpif, ofproto_meter_id meter_id,
> + struct ofputil_meter_stats *stats)
> +{
> + struct dp_offload *dp_offload = dpif_offload_get_dp_offload(dpif);
> + const struct dpif_offload *offload;
> +
> + if (!dp_offload || !dpif_offload_is_offload_enabled()) {
> + return;
> + }
> +
> + LIST_FOR_EACH (offload, dpif_list_node, &dp_offload->offload_providers) {
> + if (offload->class->meter_del) {
> + int err = offload->class->meter_del(offload, meter_id, stats);
> + if (err) {
> + VLOG_DBG_RL(&rl_dbg,
> + "Failed deleting meter %u on dpif-offload
> provider"
> + " %s, error %s", meter_id.uint32,
> + dpif_offload_name(offload), ovs_strerror(err));
> + }
> + }
> + }
> +}
> +
>
> int
> dpif_offload_netdev_flush_flows(struct netdev *netdev)
> diff --git a/lib/dpif-offload.h b/lib/dpif-offload.h
> index 2d8778f7b..0485b7e98 100644
> --- a/lib/dpif-offload.h
> +++ b/lib/dpif-offload.h
> @@ -51,6 +51,12 @@ void dpif_offload_dump_start(struct dpif_offload_dump *,
> const struct dpif *);
> bool dpif_offload_dump_next(struct dpif_offload_dump *,
> struct dpif_offload **);
> int dpif_offload_dump_done(struct dpif_offload_dump *);
> +void dpif_offload_meter_set(const struct dpif *dpif, ofproto_meter_id
> meter_id,
> + struct ofputil_meter_config *);
> +void dpif_offload_meter_get(const struct dpif *dpif, ofproto_meter_id
> meter_id,
> + struct ofputil_meter_stats *);
> +void dpif_offload_meter_del(const struct dpif *dpif, ofproto_meter_id
> meter_id,
> + struct ofputil_meter_stats *);
>
> /* Iterates through each DPIF_OFFLOAD in DPIF, using DUMP as state.
> *
> diff --git a/lib/dpif.c b/lib/dpif.c
> index bac2faa64..12dbc9cd1 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -2004,6 +2004,7 @@ dpif_meter_set(struct dpif *dpif, ofproto_meter_id
> meter_id,
> if (!error) {
> VLOG_DBG_RL(&dpmsg_rl, "%s: DPIF meter %"PRIu32" set",
> dpif_name(dpif), meter_id.uint32);
> + dpif_offload_meter_set(dpif, meter_id, config);
> } else {
> VLOG_WARN_RL(&error_rl, "%s: failed to set DPIF meter %"PRIu32": %s",
> dpif_name(dpif), meter_id.uint32, ovs_strerror(error));
> @@ -2023,6 +2024,7 @@ dpif_meter_get(const struct dpif *dpif,
> ofproto_meter_id meter_id,
> if (!error) {
> VLOG_DBG_RL(&dpmsg_rl, "%s: DPIF meter %"PRIu32" get stats",
> dpif_name(dpif), meter_id.uint32);
> + dpif_offload_meter_get(dpif, meter_id, stats);
> } else {
> VLOG_WARN_RL(&error_rl,
> "%s: failed to get DPIF meter %"PRIu32" stats: %s",
> @@ -2046,6 +2048,7 @@ dpif_meter_del(struct dpif *dpif, ofproto_meter_id
> meter_id,
> if (!error) {
> VLOG_DBG_RL(&dpmsg_rl, "%s: DPIF meter %"PRIu32" deleted",
> dpif_name(dpif), meter_id.uint32);
> + dpif_offload_meter_del(dpif, meter_id, stats);
> } else {
> VLOG_WARN_RL(&error_rl,
> "%s: failed to delete DPIF meter %"PRIu32": %s",
> diff --git a/lib/netdev-offload-provider.h b/lib/netdev-offload-provider.h
> index 6e36ed4c8..07068bd82 100644
> --- a/lib/netdev-offload-provider.h
> +++ b/lib/netdev-offload-provider.h
> @@ -91,33 +91,6 @@ struct netdev_flow_api {
> * takes ownership of a packet if errno != EOPNOTSUPP. */
> int (*hw_miss_packet_recover)(struct netdev *, struct dp_packet *);
>
> - /* Offloads or modifies the offloaded meter in HW with the given
> 'meter_id'
> - * and the configuration in 'config'. On failure, a non-zero error code
> is
> - * returned.
> - *
> - * The meter id specified through 'config->meter_id' is ignored. */
> - int (*meter_set)(ofproto_meter_id meter_id,
> - struct ofputil_meter_config *config);
> -
> - /* Queries HW for meter stats with the given 'meter_id'. Store the stats
> - * of dropped packets to band 0. On failure, a non-zero error code is
> - * returned.
> - *
> - * Note that the 'stats' structure is already initialized, and only the
> - * available statistics should be incremented, not replaced. Those fields
> - * are packet_in_count, byte_in_count and band[]->byte_count and
> - * band[]->packet_count. */
> - int (*meter_get)(ofproto_meter_id meter_id,
> - struct ofputil_meter_stats *stats);
> -
> - /* Removes meter 'meter_id' from HW. Store the stats of dropped packets
> to
> - * band 0. On failure, a non-zero error code is returned.
> - *
> - * 'stats' may be passed in as NULL if no stats are needed, See the above
> - * function for additional details on the 'stats' usage. */
> - int (*meter_del)(ofproto_meter_id meter_id,
> - struct ofputil_meter_stats *stats);
> -
> /* 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-tc.c b/lib/netdev-offload-tc.c
> index 1c554416c..46de23014 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -42,6 +42,7 @@
> #include "tc.h"
> #include "unaligned.h"
> #include "util.h"
> +#include "dpif-offload.h"
> #include "dpif-provider.h"
>
> VLOG_DEFINE_THIS_MODULE(netdev_offload_tc);
> @@ -3148,6 +3149,22 @@ tc_cleanup_policer_actions(struct id_pool *police_ids,
> hmap_destroy(&map);
> }
>
> +void
> +dpif_offload_tc_meter_init(void)
> +{
> + 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);
> + }
> +}
> +
> static int
> netdev_tc_init_flow_api(struct netdev *netdev)
> {
> @@ -3202,9 +3219,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);
> @@ -3330,9 +3347,10 @@ meter_free_police_index(uint32_t police_index)
> ovs_mutex_unlock(&meter_police_ids_mutex);
> }
>
> -static int
> -meter_tc_set_policer(ofproto_meter_id meter_id,
> - struct ofputil_meter_config *config)
> +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;
> @@ -3384,9 +3402,10 @@ meter_tc_set_policer(ofproto_meter_id meter_id,
> return err;
> }
>
> -static int
> -meter_tc_get_policer(ofproto_meter_id meter_id,
> - struct ofputil_meter_stats *stats)
> +int
> +dpif_offload_tc_meter_get(const struct dpif_offload *offload OVS_UNUSED,
> + ofproto_meter_id meter_id,
> + struct ofputil_meter_stats *stats)
> {
> uint32_t police_index;
> int err = ENOENT;
> @@ -3403,9 +3422,10 @@ meter_tc_get_policer(ofproto_meter_id meter_id,
> return err;
> }
>
> -static int
> -meter_tc_del_policer(ofproto_meter_id meter_id,
> - struct ofputil_meter_stats *stats)
> +int
> +dpif_offload_tc_meter_del(const struct dpif_offload *offload OVS_UNUSED,
> + ofproto_meter_id meter_id,
> + struct ofputil_meter_stats *stats)
> {
> uint32_t police_index;
> int err = ENOENT;
> @@ -3434,8 +3454,5 @@ 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,
> };
> diff --git a/lib/netdev-offload-tc.h b/lib/netdev-offload-tc.h
> index 964559728..a6b000852 100644
> --- a/lib/netdev-offload-tc.h
> +++ b/lib/netdev-offload-tc.h
> @@ -18,10 +18,18 @@
> #define NETDEV_OFFLOAD_TC_H
>
> /* Forward declarations of private structures. */
> +struct dpif_offload;
> struct netdev;
>
> /* Netdev-specific offload functions. These should only be used by the
> * associated dpif offload provider. */
> int netdev_offload_tc_flow_flush(struct netdev *);
> +void dpif_offload_tc_meter_init(void);
> +int dpif_offload_tc_meter_set(const struct dpif_offload *, ofproto_meter_id,
> + struct ofputil_meter_config *);
> +int dpif_offload_tc_meter_get(const struct dpif_offload *, ofproto_meter_id,
> + struct ofputil_meter_stats *);
> +int dpif_offload_tc_meter_del(const struct dpif_offload *, ofproto_meter_id,
> + struct ofputil_meter_stats *);
>
> #endif /* NETDEV_OFFLOAD_TC_H */
> diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c
> index 2b1c99fb6..abbb930be 100644
> --- a/lib/netdev-offload.c
> +++ b/lib/netdev-offload.c
> @@ -58,9 +58,6 @@
>
> VLOG_DEFINE_THIS_MODULE(netdev_offload);
>
> -
> -static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> -
> /* XXX: Temporarily duplicates definition in dpif-offload-dpdk.c. */
> #define MAX_OFFLOAD_THREAD_NB 10
> #define DEFAULT_OFFLOAD_THREAD_NB 1
> @@ -204,64 +201,6 @@ netdev_assign_flow_api(struct netdev *netdev)
> return -1;
> }
>
> -void
> -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(&rl, "Failed setting meter %u for flow api %s, "
> - "error %d", meter_id.uint32, rfa->flow_api->type,
> - ret);
> - }
> - }
> - }
> - /* Offload APIs could fail, for example, because the offload is not
> - * supported. This is fine, as the offload API should take care of this.
> */
> -}
> -
> -int
> -meter_offload_get(ofproto_meter_id meter_id, struct ofputil_meter_stats
> *stats)
> -{
> - struct netdev_registered_flow_api *rfa;
> -
> - CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) {
> - if (rfa->flow_api->meter_get) {
> - int ret = rfa->flow_api->meter_get(meter_id, stats);
> - if (ret) {
> - VLOG_DBG_RL(&rl, "Failed getting meter %u for flow api %s, "
> - "error %d", meter_id.uint32, rfa->flow_api->type,
> - ret);
> - }
> - }
> - }
> -
> - return 0;
> -}
> -
> -int
> -meter_offload_del(ofproto_meter_id meter_id, struct ofputil_meter_stats
> *stats)
> -{
> - struct netdev_registered_flow_api *rfa;
> -
> - CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) {
> - if (rfa->flow_api->meter_del) {
> - int ret = rfa->flow_api->meter_del(meter_id, stats);
> - if (ret) {
> - VLOG_DBG_RL(&rl, "Failed deleting meter %u for flow api %s, "
> - "error %d", meter_id.uint32, rfa->flow_api->type,
> - ret);
> - }
> - }
> - }
> -
> - return 0;
> -}
> -
> int
> netdev_flow_dump_create(struct netdev *netdev, struct netdev_flow_dump
> **dump,
> bool terse)
> diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h
> index 0bee005fd..8552b6e0b 100644
> --- a/lib/netdev-offload.h
> +++ b/lib/netdev-offload.h
> @@ -154,10 +154,6 @@ 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);
>
> -void meter_offload_set(ofproto_meter_id, struct ofputil_meter_config *);
> -int meter_offload_get(ofproto_meter_id, struct ofputil_meter_stats *);
> -int meter_offload_del(ofproto_meter_id, struct ofputil_meter_stats *);
> -
> #ifdef __cplusplus
> }
> #endif
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
