>-----Original Message-----
>From: Eelco Chaudron <[email protected]>
>Sent: Thursday, 11 December 2025 15:22
>To: Eli Britstein <[email protected]>
>Cc: [email protected]
>Subject: Re: [ovs-dev] [PATCH v2 14/41] dpif-offload: Call meter netdev-offload
>APIs via dpif-offload.
>
>External email: Use caution opening links or attachments
>
>
>On 10 Dec 2025, at 18:44, Eli Britstein wrote:
>
>>> -----Original Message-----
>>> From: dev <[email protected]> On Behalf Of Eelco
>>> Chaudron
>>> Sent: Tuesday, 2 December 2025 16:05
>>> To: [email protected]
>>> Subject: [ovs-dev] [PATCH v2 14/41] dpif-offload: Call meter
>>> netdev-offload APIs via dpif-offload.
>>>
>>> Note that this change does not yet move meter handling to
>>> dpif-offload-tc, where it probably should eventually reside.
>>> This should be addressed in a follow-up patch.
>> There is this disclaimer but there is no description what this commit DOES 
>> do.
>Please add it.
>> Regarding this note - could you clarify?
>> 1. what is exactly the handling you mean?
>> 2. why doesn't it move yet in this patch?
>> 3. in what commit it is eventually moved?
>
>Good catch, removed the note and replaced it with a proper commit message.
>
>>>
>>> Signed-off-by: Eelco Chaudron <[email protected]>
>>> ---
>>>
>>> v2 changes:
>>>  - Fix opening brace on dpif_offload_tc_meter_init().
>>> ---
>>> lib/dpif-netlink.c            | 29 ++-----------
>>> lib/dpif-offload-provider.h   | 26 ++++++++++++
>>> lib/dpif-offload-tc.c         |  5 +++
>>> lib/dpif-offload.c            | 77 +++++++++++++++++++++++++++++++++++
>>> lib/dpif-offload.h            |  6 +++
>>> lib/dpif.c                    |  3 ++
>>> lib/netdev-offload-provider.h | 27 ------------
>>> lib/netdev-offload-tc.c       | 69 ++++++++++++++++++++++++-------
>>> lib/netdev-offload-tc.h       |  8 ++++
>>> lib/netdev-offload.c          | 61 ---------------------------
>>> lib/netdev-offload.h          |  4 --
>>> 11 files changed, 184 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 4439e9f71..b60b7f17e 100644
>>> --- a/lib/dpif-offload-provider.h
>>> +++ b/lib/dpif-offload-provider.h
>>> @@ -131,6 +131,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
>>> +     * 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 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();
>>> +
>>>     *dpif_offload = &offload_tc->offload;
>>>     return 0;
>>> }
>>> @@ -290,5 +292,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
>>> 5f2be95ee..a3089aa38
>>> 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) = \ @@ -735,6 +737,81 @@
>>> 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) {
>>> +        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);
>> Blank line, also below occurrences.
>
>Ack
>
>>> +            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) {
>>> +        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);
>> The stats should be accumulated
>
>Yes, this is described in the API definition for this callback.
OK, yes, I see it now. However, I think it would make more sense if this is 
taken care of by the infrastructure and not by each provider.
>
>>> +            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) {
>>> +        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 4e6f13255..c6a165605 100644
>>> --- a/lib/dpif.c
>>> +++ b/lib/dpif.c
>>> @@ -2005,6 +2005,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)); @@ -
>>> 2024,6 +2025,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", @@ -2047,6
>>> +2049,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..264f86381 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,15 +3347,24 @@ 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;
>>>     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;
>>> +    }
>>> +
>> It's not clear how do we get here if offload is disabled. Isn't dpif-offload 
>> not
>registered at all in this case?
>
>These callback are registered, so this call will be made even if hardware 
>offload
>is disabled. For now, I’m keeping the existing TC offload behavior to avoid
>adding too much code to cache all previously installed meters. This can be done
>in another patch, as we are not changing the existing behavior.
Why don't early bail out in the infrastructure level and not going down to the 
TC?
>
>Regarding the bug mentioned, I think the documentation already indicates that
>changing hardware offload requires a restart, but in practice, people often do
>not follow it. Moreover, enabling seemed supported; however, disabling
>explicitly requires a restart.
If it's documented, it's not a bug but a misuse. Anyway it might work or not, 
it doesn't matter. It is not guaranteed. For doca (at least our current 
implementation) for example it's a must either way.
>
>See:
>https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.
>com%2Fopenvswitch%2Fovs%2Fblob%2F5be77f1ecc521174af188ce9a9372c8
>9db0d22e8%2Fvswitchd%2Fvswitch.xml%23L238&data=05%7C02%7Celibr%4
>0nvidia.com%7C75bc24869e684842d9bc08de38b85a46%7C43083d15727340c
>1b7db39efd9ccc17a%7C0%7C0%7C639010561680838373%7CUnknown%7CT
>WFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXa
>W4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=SDszy
>fEtWTkLsb19fFD6yoylnJxPiX%2F11eb9gc%2BMxCU%3D&reserved=0
>
>>>     if (!config->bands || config->n_bands < 1 ||
>>>         config->bands[0].type != OFPMBT13_DROP) {
>>>         return 0;
>>> @@ -3384,13 +3410,22 @@ 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;
>>>
>>> +    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;
>>> +    }
>>> +
>>>     if (!meter_id_lookup(meter_id.uint32, &police_index)) {
>>>         err = tc_get_policer_action(police_index, stats);
>>>         if (err) {
>>> @@ -3403,13 +3438,22 @@ 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;
>>>
>>> +    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;
>>> +    }
>>> +
>>>     if (!meter_id_lookup(meter_id.uint32, &police_index)) {
>>>         err = tc_del_policer_action(police_index, stats);
>>>         if (err && err != ENOENT) {
>>> @@ -3434,8 +3478,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

Reply via email to