On 11 Dec 2025, at 15:28, Eli Britstein wrote:
>> -----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. I tried to prototype this, and there’s a fair amount of overhead in doing the accumulation in the infrastructure layer; allocating and initializing temporary structures, merging values, and so on. By comparison, it’s much simpler and cheaper for each provider to directly accumulate into the stats structure (e.g., stats->packet_in_count += xx instead of stats->packet_in_count = xx). Unless there’s a strong benefit to centralizing this logic, my preference would be to keep the accumulation done by the 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. Having discussed this quickly with others and looking at the historical behavior, we should keep the existing “you must restart” requirement. I’ll move these checks up into the dpif-offload layer so we bail out earlier and avoid going down to the TC provider when offload is disabled. >> >> 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
