On 6 Feb 2023, at 8:48, Tianyu Yuan wrote:
> On Fri 2/3/2023 8:35 PM, Eelco Chaudron wrote:
>>
>> On 30 Jan 2023, at 17:42, Simon Horman wrote:
>>
>>> From: Tianyu Yuan <tianyu.y...@corigine.com>
>>>
>>> Allow revalidator to continuously delete police in kernel tc datapath
>>> until it is deleted.
>>>
>>> In current implementation, polices in tc datapath will not deleted
>>> when they are being used and these remaining polices will be cleared
>>> when the openvswitch restarts.
>>>
>>> This patch supports revalidator to delete remaining polices in tc
>>> datapath by storing meter_id deleted unsuccessfully in a hmap and
>>> continuously deleting them in revalidator until success.
>>
>> Hi Tianyu,
>>
>> Thanks for the new revision of the patch...
>>
>> Were you able to figure out if we really need to do this during revalidating
>> flows?
>>
>> I was wondering if there would be a different way, i.e. on flow deletion see
>> if
>> it was the last flow using this meter and if so remove it then. I guess this
>> can
>> be done with the meter having a flag that it has been deleted.
>>
>> This would be way more elegant than doing this in the revalidator thread.
>>
>> See some inline comments below. This was a quick review/tests, as the
>> above might require a different approach.
>>
>> Cheers,
>>
>> Eelco
>>
>
> Hi Eelco,
>
> Thanks for your review and comments. I've got your point of view:
> 1. Another approach that trigger tc police deletion only when the last flow
> using this
> meter.
Not sure if this is possible, but if, it will be more clean and hidden in the
tc layers.
> 2. depend the deletion on fail deletion of police_index rather than that of
> meter_id,
> keep the operation in dpif layer and this should be invisible from ofproto
> layer.
Yes, if the above is not possible, we should keep all this handling in the
offload layer for TC.
> Please point me out if I misunderstood.
>
> The AT test of system-offload is attached at the end of this mail.
> The error "9: offloads - check_pkt_len action - offloads disabled" also
> happens on
> Upstream master branch so it is unrelated to this patch
I’m asking for a new test case that shows this problem. i.e. it will fail
without your patch applied but will pass with it.
Thanks,
Eelco
> Cheers
> Tianyu
>
>>> Signed-off-by: Tianyu Yuan <tianyu.y...@corigine.com>
>>> Signed-off-by: Simon Horman <simon.hor...@corigine.com>
>>>
>>> ---
>>> lib/dpif-netdev.c | 1 +
>>> lib/dpif-netlink.c | 26 +++++++++++++++++++++++++-
>>> lib/dpif-provider.h | 4 ++++
>>> lib/dpif.c | 7 +++++++
>>> lib/dpif.h | 2 ++
>>> lib/netdev-offload-tc.c | 5 ++++-
>>> lib/netdev-offload.c | 8 +++++---
>>> ofproto/ofproto-dpif-upcall.c | 6 ++++++
>>> ofproto/ofproto-dpif.c | 10 +++++++++-
>>> ofproto/ofproto-dpif.h | 3 +++
>>> 10 files changed, 66 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index
>>> c9f7179c3b4c..878fe5933d1a 100644
>>> --- a/lib/dpif-netdev.c
>>> +++ b/lib/dpif-netdev.c
>>> @@ -9689,6 +9689,7 @@ const struct dpif_class dpif_netdev_class = {
>>> dpif_netdev_meter_set,
>>> dpif_netdev_meter_get,
>>> dpif_netdev_meter_del,
>>> + NULL,
>>> dpif_netdev_bond_add,
>>> dpif_netdev_bond_del,
>>> dpif_netdev_bond_stats_get,
>>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index
>>> 026b0daa8d83..8b95912aea97 100644
>>> --- a/lib/dpif-netlink.c
>>> +++ b/lib/dpif-netlink.c
>>> @@ -25,6 +25,7 @@
>>> #include <net/if.h>
>>> #include <linux/types.h>
>>> #include <linux/pkt_sched.h>
>>> +#include <linux/rtnetlink.h>
>>> #include <poll.h>
>>> #include <stdlib.h>
>>> #include <strings.h>
>>> @@ -37,6 +38,7 @@
>>> #include "dpif-provider.h"
>>> #include "fat-rwlock.h"
>>> #include "flow.h"
>>> +#include "id-pool.h"
>>> #include "netdev-linux.h"
>>> #include "netdev-offload.h"
>>> #include "netdev-provider.h"
>>> @@ -61,6 +63,7 @@
>>> #include "packets.h"
>>> #include "random.h"
>>> #include "sset.h"
>>> +#include "tc.h"
>>> #include "timeval.h"
>>> #include "unaligned.h"
>>> #include "util.h"
>>> @@ -4363,12 +4366,32 @@ dpif_netlink_meter_del(struct dpif *dpif,
>> ofproto_meter_id meter_id,
>>> err = dpif_netlink_meter_get_stats(dpif, meter_id, stats,
>>> max_bands, OVS_METER_CMD_DEL);
>>> if (!err && netdev_is_flow_api_enabled()) {
>>> - meter_offload_del(meter_id, stats);
>>> + return meter_offload_del(meter_id, stats);
>>> }
>>>
>>> return err;
>>> }
>>>
>>> +static void
>>> +dpif_netlink_meter_revalidate(struct dpif *dpif_ OVS_UNUSED,
>>> + struct hmap *meter_map) {
>>> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>>> + ofproto_meter_id meter_id;
>>> + struct id_node *id_node;
>>> +
>>> + HMAP_FOR_EACH_POP (id_node, node, meter_map) {
>>> + meter_id.uint32 = id_node->id;
>>> +
>>> + if (meter_offload_del(meter_id, NULL) == EPERM) {
>>> + id_hmap_add(meter_map, meter_id.uint32);
>>> + } else {
>>> + VLOG_DBG_RL(&rl, "Delete meter %u in datapath success",
>>> + meter_id.uint32);
>>> + }
>>> + }
>>> +}
>>> +
>>> static bool
>>> probe_broken_meters__(struct dpif *dpif) { @@ -4588,6 +4611,7 @@
>>> const struct dpif_class dpif_netlink_class = {
>>> dpif_netlink_meter_set,
>>> dpif_netlink_meter_get,
>>> dpif_netlink_meter_del,
>>> + dpif_netlink_meter_revalidate,
>>> NULL, /* bond_add */
>>> NULL, /* bond_del */
>>> NULL, /* bond_stats_get */
>>> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h index
>>> 12477a24feee..794fbc21686a 100644
>>> --- a/lib/dpif-provider.h
>>> +++ b/lib/dpif-provider.h
>>> @@ -631,6 +631,10 @@ struct dpif_class {
>>> int (*meter_del)(struct dpif *, ofproto_meter_id meter_id,
>>> struct ofputil_meter_stats *, uint16_t n_bands);
>>>
>>> + /* Checks unneeded meters from 'dpif' and removes them. They may
>>> + * be caused by deleting in-use meters. */
>>> + void (*meter_revalidate)(struct dpif *, struct hmap *meter_map);
>>> +
>>
>> I guess this is not really a revalidate action, it's more of a cleanup.
>> So maybe also call it something like meter_cleanup(), or
>> meter_garbage_collection.
>>
>> Or maybe even better, why not use the existing periodic 'bool (*run)(struct
>> dpif *dpif)' callback and handle it there?
>>
>>> /* Adds a bond with 'bond_id' and the member-map to 'dpif'. */
>>> int (*bond_add)(struct dpif *dpif, uint32_t bond_id,
>>> odp_port_t *member_map); diff --git a/lib/dpif.c
>>> b/lib/dpif.c index fe4db83fbfee..c8718239117d 100644
>>> --- a/lib/dpif.c
>>> +++ b/lib/dpif.c
>>> @@ -2028,6 +2028,13 @@ dpif_meter_del(struct dpif *dpif,
>> ofproto_meter_id meter_id,
>>> return error;
>>> }
>>>
>>> +void
>>> +dpif_meter_revalidate(struct dpif *dpif, struct hmap *meter_map){
>>> + if (dpif->dpif_class->meter_revalidate) {
>>> + dpif->dpif_class->meter_revalidate(dpif, meter_map);
>>> + }
>>> +}
>>> +
>>> int
>>> dpif_bond_add(struct dpif *dpif, uint32_t bond_id, odp_port_t
>>> *member_map) { diff --git a/lib/dpif.h b/lib/dpif.h index
>>> 6cb4dae6d8d7..e181e4ee1d6c 100644
>>> --- a/lib/dpif.h
>>> +++ b/lib/dpif.h
>>> @@ -378,6 +378,7 @@
>>>
>>> #include "dpdk.h"
>>> #include "dp-packet.h"
>>> +#include "id-pool.h"
>>> #include "netdev.h"
>>> #include "openflow/openflow.h"
>>> #include "openvswitch/ofp-meter.h"
>>> @@ -905,6 +906,7 @@ int dpif_meter_get(const struct dpif *,
>> ofproto_meter_id meter_id,
>>> struct ofputil_meter_stats *, uint16_t n_bands);
>>> int dpif_meter_del(struct dpif *, ofproto_meter_id meter_id,
>>> struct ofputil_meter_stats *, uint16_t n_bands);
>>> +void dpif_meter_revalidate(struct dpif *dpif, struct hmap
>>> +*meter_map);
>>>
>>> /* Bonding. */
>>>
>>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c index
>>> 15d1c36aa04e..f87a5e05e202 100644
>>> --- a/lib/netdev-offload-tc.c
>>> +++ b/lib/netdev-offload-tc.c
>>> @@ -2980,8 +2980,11 @@ meter_tc_del_policer(ofproto_meter_id
>> meter_id,
>>> police_index, meter_id.uint32, ovs_strerror(err));
>>> } else {
>>> meter_free_police_index(police_index);
>>> + /* Do not remove the mapping between meter_id and police_index
>>> + * until this meter is deleted successfully in datapath. This
>>> + * mapping will be used in meter deletion in revalidator. */
>>> + meter_id_remove(meter_id.uint32);
>>
>> This approach might have problem, what if the policy gets deleted, and re-
>> added with a new configuration?
>> The datapath flows are not cleared, and the add will fail as the meter_id is
>> still in use.
>>
>> Guess we need to keep the police index as this is tc specific, the meter_id
>> is
>> global.
>>
>>> }
>>> - meter_id_remove(meter_id.uint32);
>>> }
>>>
>>> return err;
>>> diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c index
>>> 4592262bd34e..0769f796e5b7 100644
>>> --- a/lib/netdev-offload.c
>>> +++ b/lib/netdev-offload.c
>>> @@ -241,19 +241,21 @@ int
>>> meter_offload_del(ofproto_meter_id meter_id, struct
>>> ofputil_meter_stats *stats) {
>>> struct netdev_registered_flow_api *rfa;
>>> + int ret = 0;
>>>
>>> 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) {
>>> + int err = rfa->flow_api->meter_del(meter_id, stats);
>>> + if (err) {
>>> VLOG_DBG_RL(&rl, "Failed deleting meter %u for flow api
>>> %s, "
>>> "error %d", meter_id.uint32,
>>> rfa->flow_api->type,
>>> ret);
>>> + ret = err;
>>> }
>>> }
>>> }
>>>
>>> - return 0;
>>> + return ret;
>>> }
>>>
>>> int
>>> diff --git a/ofproto/ofproto-dpif-upcall.c
>>> b/ofproto/ofproto-dpif-upcall.c index 31ac02d116fc..b802c9c7f13a
>>> 100644
>>> --- a/ofproto/ofproto-dpif-upcall.c
>>> +++ b/ofproto/ofproto-dpif-upcall.c
>>> @@ -2828,6 +2828,12 @@ revalidate(struct revalidator *revalidator)
>>> }
>>> ovsrcu_quiesce();
>>> }
>>> + /* When fail_meter_hmap is not empty, revalidate to delete meters in
>> this
>>> + * hashmap. */
>>> + if (hmap_count(&udpif->backer->fail_meter_hmap)) {
>>> + dpif_meter_revalidate(udpif->dpif,
>>> + &udpif->backer->fail_meter_hmap);
>>
>> This could be removed with the general run() implementation. But in general
>> we should not have this hmap, just call the callback. See below.
>>
>>> + }
>>> +
>>> dpif_flow_dump_thread_destroy(dump_thread);
>>> ofpbuf_uninit(&odp_actions);
>>> }
>>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index
>>> f87e27a8cd7f..591af5503765 100644
>>> --- a/ofproto/ofproto-dpif.c
>>> +++ b/ofproto/ofproto-dpif.c
>>> @@ -835,6 +835,7 @@ open_dpif_backer(const char *type, struct
>> dpif_backer **backerp)
>>> dpif_meter_get_features(backer->dpif, &features);
>>> if (features.max_meters) {
>>> backer->meter_ids = id_pool_create(0, features.max_meters);
>>> + hmap_init(&backer->fail_meter_hmap);
>>> } else {
>>> backer->meter_ids = NULL;
>>> }
>>> @@ -6790,8 +6791,15 @@ static void
>>> free_meter_id(struct free_meter_id_args *args) {
>>> struct ofproto_dpif *ofproto = args->ofproto;
>>> + int err;
>>> +
>>> + err = dpif_meter_del(ofproto->backer->dpif, args->meter_id, NULL, 0);
>>> + /* Add fail deleted meter to fail_meter_hmap and waiting to be
>> deleted in
>>> + * revalidator. */
>>> + if (err == EPERM) {
>>> + id_hmap_add(&ofproto->backer->fail_meter_hmap, args-
>>> meter_id.uint32);
>>> + }
>>
>> I think we should not track any of this in the ofproto layer. As this is a
>> dpif
>> problem, it should be completely hidden in the dpif layer. This should be
>> easy
>> if you remove the dependency on meter_id as suggested above.
>>
>>>
>>> - dpif_meter_del(ofproto->backer->dpif, args->meter_id, NULL, 0);
>>> id_pool_free_id(ofproto->backer->meter_ids, args->meter_id.uint32);
>>> free(args);
>>> }
>>> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h index
>>> d8e0cd37ac5b..befc68d13baa 100644
>>> --- a/ofproto/ofproto-dpif.h
>>> +++ b/ofproto/ofproto-dpif.h
>>> @@ -61,6 +61,7 @@ struct ofproto_dpif; struct uuid; struct
>>> xlate_cache; struct xlate_ctx;
>>> +struct id_pool;
>>>
>>> /* Number of implemented OpenFlow tables. */ enum { N_TABLES = 255
>>> }; @@ -260,6 +261,8 @@ struct dpif_backer {
>>>
>>> /* Meter. */
>>> struct id_pool *meter_ids; /* Datapath meter allocation. */
>>> + struct hmap fail_meter_hmap; /* Hashmap for meter with failed
>> deletion
>>> + * in datapath */
>>>
>>> /* Connection tracking. */
>>> struct id_pool *tp_ids; /* Datapath timeout policy id
>>> --
>>> 2.30.2
>>
>> Please add a test to 'make check-offloads'.
>
> ``````
> ## ------------------------------ ##
> ## openvswitch 3.1.90 test suite. ##
> ## ------------------------------ ##
>
> datapath offloads
>
> 1: offloads - ping between two ports - offloads disabled ok
> 2: offloads - ping between two ports - offloads enabled ok
> 3: offloads - set ingress_policing_rate and ingress_policing_burst -
> offloads disabled ok
> 4: offloads - set ingress_policing_rate and ingress_policing_burst -
> offloads enabled ok
> 5: offloads - set ingress_policing_kpkts_rate and
> ingress_policing_kpkts_burst - offloads disabled ok
> 6: offloads - set ingress_policing_kpkts_rate and
> ingress_policing_kpkts_burst - offloads enabled ok
> 7: offloads - check interface meter offloading - offloads disabled ok
> 8: offloads - check interface meter offloading - offloads enabled ok
> 9: offloads - check_pkt_len action - offloads disabled FAILED
> (system-offloads-traffic.at:328)
> 10: offloads - check_pkt_len action - offloads enabled ok
>
> ## ------------- ##
> ## Test results. ##
> ## ------------- ##
>
> ERROR: All 10 tests were run,
> 1 failed unexpectedly.
> ## ------------------------------------------ ##
> ## system-offloads-testsuite.log was created. ##
> ## ------------------------------------------ ##
>
> Please send `tests/system-offloads-testsuite.log' and all information you
> think might help:
>
> To: <b...@openvswitch.org>
> Subject: [openvswitch 3.1.90] system-offloads-testsuite: 9 failed
>
> You may investigate any problem if you feel able to do so, in which
> case the test suite provides a good starting point. Its output may
> be found below `tests/system-offloads-testsuite.dir'.
>
> make: *** [Makefile:7034: check-offloads] Error 1
> ``````
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
