From patchwork Sat Apr 30 04:47:03 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Cheng Li X-Patchwork-Id: 1624605 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=140.211.166.133; helo=smtp2.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Received: from smtp2.osuosl.org (smtp2.osuosl.org [140.211.166.133]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by bilbo.ozlabs.org (Postfix) with ESMTPS id 4Kqxft2MrNz9s0r for ; Sat, 30 Apr 2022 14:47:34 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by smtp2.osuosl.org (Postfix) with ESMTP id 22C2840D7C; Sat, 30 Apr 2022 04:47:32 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp2.osuosl.org ([127.0.0.1]) by localhost (smtp2.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id o3JGxpVct32l; Sat, 30 Apr 2022 04:47:31 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [IPv6:2605:bc80:3010:104::8cd3:938]) by smtp2.osuosl.org (Postfix) with ESMTPS id 0EEFC400CB; Sat, 30 Apr 2022 04:47:29 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id E07CFC0039; Sat, 30 Apr 2022 04:47:29 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp1.osuosl.org (smtp1.osuosl.org [140.211.166.138]) by lists.linuxfoundation.org (Postfix) with ESMTP id 6E69DC002D for ; Sat, 30 Apr 2022 04:47:28 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp1.osuosl.org (Postfix) with ESMTP id 47F8A84093 for ; Sat, 30 Apr 2022 04:47:28 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp1.osuosl.org ([127.0.0.1]) by localhost (smtp1.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id nHmokeBAJhX8 for ; Sat, 30 Apr 2022 04:47:27 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 Received: from chinatelecom.cn (prt-mail.chinatelecom.cn [42.123.76.219]) by smtp1.osuosl.org (Postfix) with ESMTP id E035B84038 for ; Sat, 30 Apr 2022 04:47:26 +0000 (UTC) HMM_SOURCE_IP: 172.18.0.48:50234.2098657081 HMM_ATTACHE_NUM: 0000 HMM_SOURCE_TYPE: SMTP Received: from clientip-171.217.50.170 (unknown [172.18.0.48]) by chinatelecom.cn (HERMES) with SMTP id 3C6A02800A5; Sat, 30 Apr 2022 12:47:21 +0800 (CST) X-189-SAVE-TO-SEND: +lic121@chinatelecom.cn Received: from ([172.18.0.48]) by app0024 with ESMTP id 7ad90a58f6ec49ce9456e494f98a5ab6 for dev@openvswitch.org; Sat, 30 Apr 2022 12:47:22 CST X-Transaction-ID: 7ad90a58f6ec49ce9456e494f98a5ab6 X-Real-From: lic121@chinatelecom.cn X-Receive-IP: 172.18.0.48 X-MEDUSA-Status: 0 From: lic121 To: dev@openvswitch.org, echaudro@redhat.com Date: Sat, 30 Apr 2022 04:47:03 +0000 Message-Id: <1651294023-31652-1-git-send-email-lic121@chinatelecom.cn> X-Mailer: git-send-email 1.8.3.1 Subject: [ovs-dev] [PATCH v2] ipfix: trigger revalidation if ipfix options changes X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" ipfix cfg creation/deleting triggers revalidation. But this does not cover the case where ipfix options changes, which also suppose to trigger revalidation. Fixes: a9f5ee1199e1 ("ofproto-dpif: Trigger revalidation when ipfix config set.") Signed-off-by: lic121 Acked-by: Eelco Chaudron --- ofproto/ofproto-dpif-ipfix.c | 51 +++++++++++++++++++++++++++++--------------- ofproto/ofproto-dpif-ipfix.h | 2 +- ofproto/ofproto-dpif.c | 7 +++--- tests/ofproto-dpif.at | 17 ++++++++++++++- 4 files changed, 54 insertions(+), 23 deletions(-) diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c index fc927fe..742eed3 100644 --- a/ofproto/ofproto-dpif-ipfix.c +++ b/ofproto/ofproto-dpif-ipfix.c @@ -926,17 +926,21 @@ dpif_ipfix_bridge_exporter_destroy(struct dpif_ipfix_bridge_exporter *exporter) static void dpif_ipfix_bridge_exporter_set_options( struct dpif_ipfix_bridge_exporter *exporter, - const struct ofproto_ipfix_bridge_exporter_options *options) + const struct ofproto_ipfix_bridge_exporter_options *options, + bool *options_changed) { - bool options_changed; - if (!options || sset_is_empty(&options->targets)) { /* No point in doing any work if there are no targets. */ - dpif_ipfix_bridge_exporter_clear(exporter); + if (exporter->options) { + dpif_ipfix_bridge_exporter_clear(exporter); + *options_changed = true; + } else { + *options_changed = false; + } return; } - options_changed = ( + *options_changed = ( !exporter->options || !ofproto_ipfix_bridge_exporter_options_equal( options, exporter->options)); @@ -945,7 +949,7 @@ dpif_ipfix_bridge_exporter_set_options( * shortchanged in collectors (which indicates that opening one or * more of the configured collectors failed, so that we should * retry). */ - if (options_changed + if (*options_changed || collectors_count(exporter->exporter.collectors) < sset_count(&options->targets)) { if (!dpif_ipfix_exporter_set_options( @@ -957,7 +961,7 @@ dpif_ipfix_bridge_exporter_set_options( } /* Avoid reconfiguring if options didn't change. */ - if (!options_changed) { + if (!*options_changed) { return; } @@ -1015,17 +1019,21 @@ dpif_ipfix_flow_exporter_destroy(struct dpif_ipfix_flow_exporter *exporter) static bool dpif_ipfix_flow_exporter_set_options( struct dpif_ipfix_flow_exporter *exporter, - const struct ofproto_ipfix_flow_exporter_options *options) + const struct ofproto_ipfix_flow_exporter_options *options, + bool *options_changed) { - bool options_changed; - if (sset_is_empty(&options->targets)) { /* No point in doing any work if there are no targets. */ - dpif_ipfix_flow_exporter_clear(exporter); + if (exporter->options) { + dpif_ipfix_flow_exporter_clear(exporter); + *options_changed = true; + } else { + *options_changed = false; + } return true; } - options_changed = ( + *options_changed = ( !exporter->options || !ofproto_ipfix_flow_exporter_options_equal( options, exporter->options)); @@ -1034,7 +1042,7 @@ dpif_ipfix_flow_exporter_set_options( * shortchanged in collectors (which indicates that opening one or * more of the configured collectors failed, so that we should * retry). */ - if (options_changed + if (*options_changed || collectors_count(exporter->exporter.collectors) < sset_count(&options->targets)) { if (!dpif_ipfix_exporter_set_options( @@ -1046,7 +1054,7 @@ dpif_ipfix_flow_exporter_set_options( } /* Avoid reconfiguring if options didn't change. */ - if (!options_changed) { + if (!*options_changed) { return true; } @@ -1069,7 +1077,7 @@ remove_flow_exporter(struct dpif_ipfix *di, free(node); } -void +bool dpif_ipfix_set_options( struct dpif_ipfix *di, const struct ofproto_ipfix_bridge_exporter_options *bridge_exporter_options, @@ -1077,16 +1085,19 @@ dpif_ipfix_set_options( size_t n_flow_exporters_options) OVS_EXCLUDED(mutex) { int i; + bool beo_changed, feo_changed, entry_changed; struct ofproto_ipfix_flow_exporter_options *options; struct dpif_ipfix_flow_exporter_map_node *node; ovs_mutex_lock(&mutex); dpif_ipfix_bridge_exporter_set_options(&di->bridge_exporter, - bridge_exporter_options); + bridge_exporter_options, + &beo_changed); /* Add new flow exporters and update current flow exporters. */ options = (struct ofproto_ipfix_flow_exporter_options *) flow_exporters_options; + feo_changed = false; for (i = 0; i < n_flow_exporters_options; i++) { node = dpif_ipfix_find_flow_exporter_map_node( di, options->collector_set_id); @@ -1095,10 +1106,14 @@ dpif_ipfix_set_options( dpif_ipfix_flow_exporter_init(&node->exporter); hmap_insert(&di->flow_exporter_map, &node->node, hash_int(options->collector_set_id, 0)); + feo_changed = true; } - if (!dpif_ipfix_flow_exporter_set_options(&node->exporter, options)) { + if (!dpif_ipfix_flow_exporter_set_options(&node->exporter, + options, + &entry_changed)) { remove_flow_exporter(di, node); } + feo_changed = entry_changed ? true : feo_changed; options++; } @@ -1117,10 +1132,12 @@ dpif_ipfix_set_options( } if (i == n_flow_exporters_options) { /* Not found. */ remove_flow_exporter(di, node); + feo_changed = true; } } ovs_mutex_unlock(&mutex); + return beo_changed || feo_changed; } struct dpif_ipfix * diff --git a/ofproto/ofproto-dpif-ipfix.h b/ofproto/ofproto-dpif-ipfix.h index 1f42cd5..75c0ab8 100644 --- a/ofproto/ofproto-dpif-ipfix.h +++ b/ofproto/ofproto-dpif-ipfix.h @@ -48,7 +48,7 @@ bool dpif_ipfix_get_bridge_exporter_output_sampling(const struct dpif_ipfix *); bool dpif_ipfix_get_flow_exporter_tunnel_sampling(const struct dpif_ipfix *, const uint32_t); bool dpif_ipfix_is_tunnel_port(const struct dpif_ipfix *, odp_port_t); -void dpif_ipfix_set_options( +bool dpif_ipfix_set_options( struct dpif_ipfix *, const struct ofproto_ipfix_bridge_exporter_options *, const struct ofproto_ipfix_flow_exporter_options *, size_t); diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 6601f23..6e8c483 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -2337,6 +2337,7 @@ set_ipfix( struct dpif_ipfix *di = ofproto->ipfix; bool has_options = bridge_exporter_options || flow_exporters_options; bool new_di = false; + bool options_changed = false; if (has_options && !di) { di = ofproto->ipfix = dpif_ipfix_create(); @@ -2346,7 +2347,7 @@ set_ipfix( if (di) { /* Call set_options in any case to cleanly flush the flow * caches in the last exporters that are to be destroyed. */ - dpif_ipfix_set_options( + options_changed = dpif_ipfix_set_options( di, bridge_exporter_options, flow_exporters_options, n_flow_exporters_options); @@ -2363,9 +2364,7 @@ set_ipfix( ofproto->ipfix = NULL; } - /* TODO: need to consider ipfix option changes more than - * enable/disable */ - if (new_di || !ofproto->ipfix) { + if (new_di || options_changed) { ofproto->backer->need_revalidate = REV_RECONFIGURE; } } diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index dbb3b6d..8504c21 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -7629,13 +7629,28 @@ dnl configure bridge IPFIX and ensure that sample action generation works at the dnl datapath level. AT_SETUP([ofproto-dpif - Bridge IPFIX sanity check]) OVS_VSWITCHD_START +dnl first revalidation triggered by add interface +AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl +1 +]) + add_of_ports br0 1 2 3 +AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl +2 +]) dnl Sample every packet using bridge-based sampling. AT_CHECK([ovs-vsctl -- set bridge br0 ipfix=@fix -- \ --id=@fix create ipfix targets=\"127.0.0.1:4739\" \ - sampling=1], [0], [ignore]) + sampling=2], [0], [ignore]) +AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl +3 +]) +AT_CHECK([ovs-vsctl set ipfix `ovs-vsctl get bridge br0 ipfix` sampling=1], [0]) +AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl +4 +]) dnl Send some packets that should be sampled. for i in `seq 1 3`; do AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800)'])