On Thu, Jul 27, 2023 at 9:17 PM Mark Michelson <mmich...@redhat.com> wrote:
> Hi Ales, I had a look through this patch finally :)
>
> From a high-level, I appreciate the design. Periodically requesting
> flow stats in a background thread makes good sense.
>
> I have a couple of suggestions to make to the design.
>
Hi Mark,
thank you for the review.
>
> On 7/10/23 07:05, Ales Musil wrote:
> > To achieve that add thread that will handle
> > statistics requests and delegate the processing
> > to defined functions. This allows the thread to
> > be flexible enough, so it could be extended in future
> > if needed.
> >
> > At the same time connected the thread with the MAC
> > cache I-P node to have the timestamp updates. The
> > updates should happen once per dump_period
> > (3/4 of the aging threshold) per chassis only if
> > the MAC binding is actively used.
> >
> > Signed-off-by: Ales Musil <amu...@redhat.com>
> > ---
> > controller/automake.mk | 4 +-
> > controller/mac_cache.c | 84 +++++++
> > controller/mac_cache.h | 7 +
> > controller/ovn-controller.c | 11 +
> > controller/statctrl.c | 434 ++++++++++++++++++++++++++++++++++++
> > controller/statctrl.h | 28 +++
> > tests/ovn.at | 23 +-
> > 7 files changed, 585 insertions(+), 6 deletions(-)
> > create mode 100644 controller/statctrl.c
> > create mode 100644 controller/statctrl.h
> >
> > diff --git a/controller/automake.mk b/controller/automake.mk
> > index 562290359..0dbbd5d26 100644
> > --- a/controller/automake.mk
> > +++ b/controller/automake.mk
> > @@ -45,7 +45,9 @@ controller_ovn_controller_SOURCES = \
> > controller/mirror.h \
> > controller/mirror.c \
> > controller/mac_cache.h \
> > - controller/mac_cache.c
> > + controller/mac_cache.c \
> > + controller/statctrl.h \
> > + controller/statctrl.c
> >
> > controller_ovn_controller_LDADD = lib/libovn.la $(OVS_LIBDIR)/
> libopenvswitch.la
> > man_MANS += controller/ovn-controller.8
> > diff --git a/controller/mac_cache.c b/controller/mac_cache.c
> > index 4663499a1..6f1d661d4 100644
> > --- a/controller/mac_cache.c
> > +++ b/controller/mac_cache.c
> > @@ -252,3 +252,87 @@ mac_cache_threshold_remove(struct hmap *thresholds,
> > hmap_remove(thresholds, &threshold->hmap_node);
> > free(threshold);
> > }
> > +
> > +struct mac_cache_mb_stats {
> > + struct ovs_list list_node;
> > +
> > + int64_t idle_age_ms;
> > + uint32_t cookie;
> > + /* Common data to identify MAC binding. */
> > + struct mac_cache_mb_data data;
> > +};
> > +
> > +void
> > +mac_cache_mb_stats_process_flow_stats(struct ovs_list *stats_list,
> > + struct ofputil_flow_stats
> *ofp_stats)
> > +{
> > + struct mac_cache_mb_stats *stats = xmalloc(sizeof *stats);
> > +
> > + stats->idle_age_ms = ofp_stats->idle_age * 1000;
> > + stats->cookie = ntohll(ofp_stats->cookie);
> > + stats->data.port_key =
> > + ofp_stats->match.flow.regs[MFF_LOG_INPORT - MFF_REG0];
> > + stats->data.dp_key = ntohll(ofp_stats->match.flow.metadata);
> > +
> > + if (ofp_stats->match.flow.dl_type == htons(ETH_TYPE_IP)) {
> > + stats->data.ip =
> in6_addr_mapped_ipv4(ofp_stats->match.flow.nw_src);
> > + } else {
> > + stats->data.ip = ofp_stats->match.flow.ipv6_src;
> > + }
> > +
> > + stats->data.mac = ofp_stats->match.flow.dl_src;
> > +
> > + ovs_list_push_back(stats_list, &stats->list_node);
> > +}
> > +
> > +void
> > +mac_cache_mb_stats_destroy(struct ovs_list *stats_list)
> > +{
> > + struct mac_cache_mb_stats *stats;
> > + LIST_FOR_EACH_POP (stats, list_node, stats_list) {
> > + free(stats);
> > + }
> > +}
> > +
> > +void
> > +mac_cache_mb_stats_run(struct ovs_list *stats_list, uint64_t *req_delay,
> > + void *data)
> > +{
> > + struct mac_cache_data *cache_data = data;
> > + long long timewall_now = time_wall_msec();
> > +
> > + struct mac_cache_threshold *threshold;
> > + struct mac_cache_mb_stats *stats;
> > + struct mac_cache_mac_binding *mc_mb;
> > + LIST_FOR_EACH_POP (stats, list_node, stats_list) {
> > + mc_mb = mac_cache_mac_binding_find_by_mb_data(cache_data,
> > + &stats->data);
> > +
> > + if (!mc_mb) {
> > + free(stats);
> > + continue;
> > + }
> > +
> > + struct uuid *dp_uuid = &mc_mb->sbrec_mb->datapath->header_.uuid;
> > + threshold = mac_cache_threshold_find(&cache_data->mb_thresholds,
> > + dp_uuid);
> > +
> > + uint64_t dump_period = (3 * threshold->value) / 4;
>
> The dump period correlates directly with the configured threshold.
> Perhaps the dump period could be stored on the mac_cache_threshold and
> updated whenever the threshold->value is changed. This way, you would
> not have to calculate it twice every time this function is run.
>
> I also suggest offloading the dump_period calculation to a function so
> that it is easy to change if desired.
>
Yeah that makes sense. This way the calculation will in single place and we
don't have
to probably do a separate function for that.
>
> > + /* If "idle_age" is under threshold it means that the mac
> binding is
> > + * used on this chassis. Also make sure that we don't update the
> > + * timestamp more than once during the dump period. */
> > + if (stats->idle_age_ms < threshold->value &&
> > + (timewall_now - mc_mb->sbrec_mb->timestamp) >= dump_period)
> {
> > + sbrec_mac_binding_set_timestamp(mc_mb->sbrec_mb,
> timewall_now);
> > + }
> > +
> > + free(stats);
> > + }
> > +
> > + uint64_t dump_period = UINT64_MAX;
> > + HMAP_FOR_EACH (threshold, hmap_node, &cache_data->mb_thresholds) {
> > + dump_period = MIN(dump_period, (3 * threshold->value) / 4);
> > + }
> > +
> > + *req_delay = dump_period < UINT64_MAX ? dump_period : 0;
> > +}
> > diff --git a/controller/mac_cache.h b/controller/mac_cache.h
> > index f1f1772c8..a29713908 100644
> > --- a/controller/mac_cache.h
> > +++ b/controller/mac_cache.h
> > @@ -71,4 +71,11 @@ void mac_cache_mac_binding_remove(struct
> mac_cache_data *data,
> > void mac_cache_mac_bindings_destroy(struct mac_cache_data *data);
> > bool mac_cache_sb_mac_binding_updated(const struct sbrec_mac_binding
> *mb);
> >
> > +void
> > +mac_cache_mb_stats_process_flow_stats(struct ovs_list *stats_list,
> > + struct ofputil_flow_stats
> *ofp_stats);
> > +void mac_cache_mb_stats_destroy(struct ovs_list *stats_list);
> > +void mac_cache_mb_stats_run(struct ovs_list *stats_list, uint64_t
> *req_delay,
> > + void *data);
> > +
> > #endif /* controller/mac_cache.h */
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index abb18647a..bdf7368b8 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -84,6 +84,7 @@
> > #include "hmapx.h"
> > #include "mirror.h"
> > #include "mac_cache.h"
> > +#include "statctrl.h"
> >
> > VLOG_DEFINE_THIS_MODULE(main);
> >
> > @@ -4804,6 +4805,7 @@ main(int argc, char *argv[])
> > lflow_init();
> > mirror_init();
> > vif_plug_provider_initialize();
> > + statctrl_init();
> >
> > /* Connect to OVS OVSDB instance. */
> > struct ovsdb_idl_loop ovs_idl_loop = OVSDB_IDL_LOOP_INITIALIZER(
> > @@ -5205,6 +5207,8 @@ main(int argc, char *argv[])
> > engine_get_internal_data(&en_template_vars);
> > struct ed_type_lb_data *lb_data =
> > engine_get_internal_data(&en_lb_data);
> > + struct mac_cache_data *mac_cache_data =
> > + engine_get_internal_data(&en_mac_cache);
> >
> > ofctrl_init(&lflow_output_data->group_table,
> > &lflow_output_data->meter_table,
> > @@ -5593,6 +5597,11 @@ main(int argc, char *argv[])
> > }
> > }
> >
> > + if (mac_cache_data) {
> > + statctrl_update(br_int->name);
> > + statctrl_run(ovnsb_idl_txn, mac_cache_data);
> > + }
> > +
> > ofctrl_seqno_update_create(
> > ofctrl_seq_type_nb_cfg,
> > get_nb_cfg(sbrec_sb_global_table_get(
> > @@ -5702,6 +5711,7 @@ main(int argc, char *argv[])
> > if (br_int) {
> > ofctrl_wait();
> > pinctrl_wait(ovnsb_idl_txn);
> > + statctrl_wait(ovnsb_idl_txn);
> > }
> >
> > binding_wait();
> > @@ -5836,6 +5846,7 @@ loop_done:
> > patch_destroy();
> > mirror_destroy();
> > encaps_destroy();
> > + statctrl_destroy();
> > if_status_mgr_destroy(if_mgr);
> > shash_destroy(&vif_plug_deleted_iface_ids);
> > shash_destroy(&vif_plug_changed_iface_ids);
> > diff --git a/controller/statctrl.c b/controller/statctrl.c
> > new file mode 100644
> > index 000000000..9bef827fc
> > --- /dev/null
> > +++ b/controller/statctrl.c
> > @@ -0,0 +1,434 @@
> > +/* Copyright (c) 2023, Red Hat, Inc.
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at:
> > + *
> > + * http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +
> > +#include <config.h>
> > +
> > +#include "byte-order.h"
> > +#include "dirs.h"
> > +#include "latch.h"
> > +#include "lflow.h"
> > +#include "mac_cache.h"
> > +#include "openvswitch/ofp-errors.h"
> > +#include "openvswitch/ofp-flow.h"
> > +#include "openvswitch/ofp-msgs.h"
> > +#include "openvswitch/ofp-print.h"
> > +#include "openvswitch/ofp-util.h"
> > +#include "openvswitch/poll-loop.h"
> > +#include "openvswitch/rconn.h"
> > +#include "openvswitch/vlog.h"
> > +#include "ovn/logical-fields.h"
> > +#include "ovs-thread.h"
> > +#include "seq.h"
> > +#include "socket-util.h"
> > +#include "statctrl.h"
> > +
> > +VLOG_DEFINE_THIS_MODULE(statctrl);
> > +
> > +enum stat_type {
> > + STATS_MAC_BINDING = 0,
> > + STATS_MAX,
> > +};
> > +
> > +struct stats_node {
> > + /* Table ID for the statistics request. */
> > + uint8_t table_id;
>
> Instead of using a table_id here, would it make sense to store a struct
> ofputil_flow_stats_request instead? This way, if stats nodes care about
> group IDs or cookies, they can store them here.
>
That is a good idea, thanks.
>
> > + /* xid of the last statistics request. */
> > + ovs_be32 xid;
> > + /* Timestamp when the next request should happen. */
> > + int64_t next_request_timestamp;
> > + /* Request delay in ms. */
> > + uint64_t request_delay;
> > + /* List of processed statistics. */
> > + struct ovs_list stats_list;
> > + /* Function to clean up the node.
> > + * This function runs in main thread. */
> > + void (*destroy)(struct ovs_list *stats_list);
> > + /* Function to process the response and store it in the list.
> > + * This function runs in statctrl thread locked behind mutex. */
> > + void (*process_flow_stats)(struct ovs_list *stats_list,
> > + struct ofputil_flow_stats *ofp_stats);
> > + /* Function to process the parsed stats.
> > + * This function runs in main thread locked behind mutex. */
> > + void (*run)(struct ovs_list *stats_list, uint64_t *req_delay, void
> *data);
> > +};
> > +
> > +#define STATS_NODE(NAME, TABLE_ID, DESTROY, PROCESS, RUN)
> \
> > + statctrl_ctx.nodes[STATS_##NAME] = (struct stats_node) {
> \
> > + .table_id = TABLE_ID,
> \
> > + .xid = 0,
> \
> > + .next_request_timestamp = INT64_MAX,
> \
> > + .request_delay = 0,
> \
> > + .stats_list =
> \
> > + OVS_LIST_INITIALIZER(
> \
> > + &statctrl_ctx.nodes[STATS_##NAME].stats_list),
> \
> > + .destroy = DESTROY,
> \
> > + .process_flow_stats = PROCESS,
> \
> > + .run = RUN
> \
> > + };
> > +
> > +struct statctrl_ctx {
> > + char *br_int;
> > +
> > + pthread_t thread;
> > + struct latch exit_latch;
> > +
> > + struct seq *thread_seq;
> > + struct seq *main_seq;
> > +
> > + struct stats_node nodes[STATS_MAX];
> > +};
> > +
> > +static struct statctrl_ctx statctrl_ctx;
> > +static struct ovs_mutex mutex;
> > +
> > +static void *statctrl_thread_handler(void *arg);
> > +static void statctrl_rconn_setup(struct rconn *swconn, char
> *conn_target)
> > + OVS_REQUIRES(mutex);
> > +static void statctrl_handle_rconn_msg(struct rconn *swconn,
> > + struct statctrl_ctx *ctx,
> > + struct ofpbuf *msg);
> > +static enum stat_type statctrl_get_stat_type(struct statctrl_ctx *ctx,
> > + const struct ofp_header
> *oh);
> > +static void statctrl_decode_statistics_reply(struct stats_node *node,
> > + struct ofpbuf *msg)
> > + OVS_REQUIRES(mutex);
> > +static void statctrl_send_request(struct rconn *swconn,
> > + struct statctrl_ctx *ctx)
> > + OVS_REQUIRES(mutex);
> > +static void statctrl_notify_main_thread(struct statctrl_ctx *ctx);
> > +static void statctrl_set_conn_target(const char *br_int_name)
> > + OVS_REQUIRES(mutex);
> > +static void statctrl_wait_next_request(struct statctrl_ctx *ctx)
> > + OVS_REQUIRES(mutex);
> > +static bool statctrl_update_next_request_timestamp(struct stats_node
> *node,
> > + long long now,
> > + uint64_t prev_delay)
> > + OVS_REQUIRES(mutex);
> > +
> > +void
> > +statctrl_init(void)
> > +{
> > + statctrl_ctx.br_int = NULL;
> > + latch_init(&statctrl_ctx.exit_latch);
> > + ovs_mutex_init(&mutex);
> > + statctrl_ctx.thread_seq = seq_create();
> > + statctrl_ctx.main_seq = seq_create();
> > +
> > + /* Definition of all stat nodes. */
> > + STATS_NODE(MAC_BINDING, OFTABLE_MAC_CACHE_USE,
> mac_cache_mb_stats_destroy,
> > + mac_cache_mb_stats_process_flow_stats,
> mac_cache_mb_stats_run);
> > +
> > +
> > + statctrl_ctx.thread = ovs_thread_create("ovn_statctrl",
> > + statctrl_thread_handler,
> > + &statctrl_ctx);
> > +}
> > +
> > +void
> > +statctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
> > + struct mac_cache_data *mac_cache_data)
> > +{
> > + if (!ovnsb_idl_txn) {
> > + return;
> > + }
> > +
> > + void *node_data[STATS_MAX] = {mac_cache_data};
>
> I was thinking about what happens when more than just the MAC cache
> needs to collect flow statistics. Based on this initial design, my
> assumption is that the function signature for statctrl_run() would grow
> as each new stats node type is defined:
>
> void
> statctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
> struct mac_cache_data *mac_cache_data,
> struct foo_data *foo_data,
> struct bar_data *bar_data)
>
> Is that the intention? I suppose this will be OK as long as the number
> of stats nodes stays low. But if we suspect that we will end up with a
> lot more, then we may want to switch to passing in, say, a shash of data
> instead of each individual stats node data type. My biggest concern here
> is merge conflicts when trying to backport patches.
>
> Or did you have a different idea in mind for how new stats data types
> would be handled?
>
Yeah that is the original intention as I don't anticipate that it will grow
too much.
So for example initially there will be only one argument as the
mac_cache_data
is planned to be used for both MAC binding and FDB.
>
> > +
> > + bool schedule_updated = false;
> > + long long now = time_msec();
> > +
> > + ovs_mutex_lock(&mutex);
> > + for (size_t i = 0; i < STATS_MAX; i++) {
> > + struct stats_node *node = &statctrl_ctx.nodes[i];
> > + uint64_t prev_delay = node->request_delay;
> > +
> > + node->run(&node->stats_list, &node->request_delay,
> node_data[i]);
> > +
> > + schedule_updated |=
> > + statctrl_update_next_request_timestamp(node, now,
> prev_delay);
> > + }
> > + ovs_mutex_unlock(&mutex);
> > +
> > + if (schedule_updated) {
> > + seq_change(statctrl_ctx.thread_seq);
> > + }
> > +}
> > +
> > +void
> > +statctrl_update(const char *br_int_name)
> > +{
> > + ovs_mutex_lock(&mutex);
> > + statctrl_set_conn_target(br_int_name);
> > + ovs_mutex_unlock(&mutex);
> > +}
> > +
> > +void
> > +statctrl_wait(struct ovsdb_idl_txn *ovnsb_idl_txn)
> > +{
> > + if (!ovnsb_idl_txn) {
> > + return;
> > + }
> > +
> > + ovs_mutex_lock(&mutex);
> > + for (size_t i = 0; i < STATS_MAX; i++) {
> > + struct stats_node *node = &statctrl_ctx.nodes[i];
> > + if (!ovs_list_is_empty(&node->stats_list)) {
> > + poll_immediate_wake();
> > + }
> > + }
> > + int64_t new_seq = seq_read(statctrl_ctx.main_seq);
> > + seq_wait(statctrl_ctx.main_seq, new_seq);
> > + ovs_mutex_unlock(&mutex);
> > +}
> > +
> > +void
> > +statctrl_destroy(void)
> > +{
> > + latch_set(&statctrl_ctx.exit_latch);
> > + pthread_join(statctrl_ctx.thread, NULL);
> > + latch_destroy(&statctrl_ctx.exit_latch);
> > + free(statctrl_ctx.br_int);
> > + seq_destroy(statctrl_ctx.thread_seq);
> > + seq_destroy(statctrl_ctx.main_seq);
> > +
> > + for (size_t i = 0; i < STATS_MAX; i++) {
> > + struct stats_node *node = &statctrl_ctx.nodes[i];
> > + node->destroy(&node->stats_list);
> > + }
> > +}
> > +
> > +static void *
> > +statctrl_thread_handler(void *arg)
> > +{
> > + struct statctrl_ctx *ctx = arg;
> > +
> > + /* OpenFlow connection to the switch. */
> > + struct rconn *swconn = rconn_create(5, 0, DSCP_DEFAULT,
> > + 1 << OFP15_VERSION);
> > +
> > + while (!latch_is_set(&ctx->exit_latch)) {
> > + ovs_mutex_lock(&mutex);
> > + statctrl_rconn_setup(swconn, ctx->br_int);
> > + ovs_mutex_unlock(&mutex);
> > +
> > + rconn_run(swconn);
> > + uint64_t new_seq = seq_read(ctx->thread_seq);
> > +
> > + if (rconn_is_connected(swconn)) {
> > + for (int i = 0; i < 100; i++) {
> > + struct ofpbuf *msg = rconn_recv(swconn);
> > +
> > + if (!msg) {
> > + break;
> > + }
> > +
> > + statctrl_handle_rconn_msg(swconn, ctx, msg);
> > + ofpbuf_delete(msg);
> > + }
> > +
> > + ovs_mutex_lock(&mutex);
> > + statctrl_send_request(swconn, ctx);
> > + ovs_mutex_unlock(&mutex);
> > + }
> > +
> > + statctrl_notify_main_thread(ctx);
> > + rconn_run_wait(swconn);
> > + rconn_recv_wait(swconn);
> > + ovs_mutex_lock(&mutex);
> > + statctrl_wait_next_request(ctx);
> > + ovs_mutex_unlock(&mutex);
> > + seq_wait(ctx->thread_seq, new_seq);
> > + latch_wait(&ctx->exit_latch);
> > +
> > + poll_block();
> > + }
> > +
> > + rconn_destroy(swconn);
> > + return NULL;
> > +}
> > +
> > +static void
> > +statctrl_rconn_setup(struct rconn *swconn, char *br_int)
> > + OVS_REQUIRES(mutex)
> > +{
> > + if (!br_int) {
> > + rconn_disconnect(swconn);
> > + return;
> > + }
> > +
> > + char *conn_target = xasprintf("unix:%s/%s.mgmt", ovs_rundir(),
> br_int);
> > +
> > + if (strcmp(conn_target, rconn_get_target(swconn))) {
> > + VLOG_INFO("%s: connecting to switch", conn_target);
> > + rconn_connect(swconn, conn_target, conn_target);
> > + }
> > +
> > + free(conn_target);
> > +}
> > +
> > +static void
> > +statctrl_handle_rconn_msg(struct rconn *swconn, struct statctrl_ctx
> *ctx,
> > + struct ofpbuf *msg)
> > +{
> > + enum ofptype type;
> > + const struct ofp_header *oh = msg->data;
> > +
> > + ofptype_decode(&type, oh);
> > +
> > + if (type == OFPTYPE_ECHO_REQUEST) {
> > + rconn_send(swconn, ofputil_encode_echo_reply(oh), NULL);
> > + } else if (type == OFPTYPE_FLOW_STATS_REPLY) {
> > + enum stat_type stype = statctrl_get_stat_type(ctx, oh);
> > + if (stype == STATS_MAX) {
> > + return;
> > + }
> > +
> > + ovs_mutex_lock(&mutex);
> > + statctrl_decode_statistics_reply(&ctx->nodes[stype], msg);
> > + ovs_mutex_unlock(&mutex);
> > + } else {
> > + if (VLOG_IS_DBG_ENABLED()) {
> > +
> > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(30,
> 300);
> > +
> > + char *s = ofp_to_string(oh, ntohs(oh->length), NULL, NULL,
> 2);
> > +
> > + VLOG_DBG_RL(&rl, "OpenFlow packet ignored: %s", s);
> > + free(s);
> > + }
> > + }
> > +}
> > +
> > +static enum stat_type
> > +statctrl_get_stat_type(struct statctrl_ctx *ctx, const struct
> ofp_header *oh)
> > +{
> > + for (size_t i = 0; i < STATS_MAX; i++) {
> > + if (ctx->nodes[i].xid == oh->xid) {
> > + return i;
> > + }
> > + }
> > + return STATS_MAX;
> > +}
> > +
> > +static void
> > +statctrl_decode_statistics_reply(struct stats_node *node, struct ofpbuf
> *msg)
> > + OVS_REQUIRES(mutex)
> > +{
> > + struct ofpbuf ofpacts;
> > + ofpbuf_init(&ofpacts, 0);
> > +
> > + while (true) {
> > + struct ofputil_flow_stats fs;
> > +
> > + int error = ofputil_decode_flow_stats_reply(&fs, msg, true,
> &ofpacts);
> > + if (error == EOF) {
> > + break;
> > + } else if (error) {
> > + VLOG_DBG("Couldn't parse stat reply: %s",
> ofperr_to_string(error));
> > + break;
> > + }
> > +
> > + node->process_flow_stats(&node->stats_list, &fs);
> > + }
> > +
> > + ofpbuf_uninit(&ofpacts);
> > +}
> > +
> > +static void
> > +statctrl_send_request(struct rconn *swconn, struct statctrl_ctx *ctx)
> > + OVS_REQUIRES(mutex)
> > +{
> > + long long now = time_msec();
> > + enum ofp_version version = rconn_get_version(swconn);
> > + enum ofputil_protocol proto =
> ofputil_protocol_from_ofp_version(version);
> > +
> > + for (size_t i = 0; i < STATS_MAX; i++) {
> > + struct stats_node *node = &ctx->nodes[i];
> > +
> > + if (now < node->next_request_timestamp) {
> > + continue;
> > + }
> > +
> > + struct ofputil_flow_stats_request fsr = {
> > + .cookie = htonll(0),
> > + .cookie_mask = htonll(0),
> > + .out_port = OFPP_ANY,
> > + .out_group = OFPG_ANY,
> > + .table_id = node->table_id,
> > + }; > + struct ofpbuf *msg =
> ofputil_encode_flow_stats_request(&fsr,
> proto);
> > + node->xid = ((struct ofp_header *) msg->data)->xid;
> > +
> > + statctrl_update_next_request_timestamp(node, now, 0);
> > +
> > + rconn_send(swconn, msg, NULL);
> > + }
> > +}
> > +
> > +static void
> > +statctrl_notify_main_thread(struct statctrl_ctx *ctx)
> > +{
> > + for (size_t i = 0; i < STATS_MAX; i++) {
> > + if (!ovs_list_is_empty(&ctx->nodes[i].stats_list)) {
> > + seq_change(ctx->main_seq);
> > + return;
> > + }
> > + }
> > +}
> > +
> > +static void
> > +statctrl_set_conn_target(const char *br_int_name)
> > + OVS_REQUIRES(mutex)
> > +{
> > + if (!br_int_name) {
> > + return;
> > + }
> > +
> > +
> > + if (!statctrl_ctx.br_int || strcmp(statctrl_ctx.br_int,
> br_int_name)) {
> > + free(statctrl_ctx.br_int);
> > + statctrl_ctx.br_int = xstrdup(br_int_name);
> > + /* Notify statctrl thread that integration bridge is
> set/changed. */
> > + seq_change(statctrl_ctx.thread_seq);
> > + }
> > +}
> > +
> > +static void
> > +statctrl_wait_next_request(struct statctrl_ctx *ctx)
> > + OVS_REQUIRES(mutex)
> > +{
> > + for (size_t i = 0; i < STATS_MAX; i++) {
> > + int64_t timestamp = ctx->nodes[i].next_request_timestamp;
> > + if (timestamp < INT64_MAX) {
> > + poll_timer_wait_until(timestamp);
> > + }
> > + }
> > +}
> > +
> > +static bool
> > +statctrl_update_next_request_timestamp(struct stats_node *node,
> > + long long now, uint64_t
> prev_delay)
> > +{
> > + if (!node->request_delay) {
> > + node->next_request_timestamp = INT64_MAX;
> > + return false;
> > + }
> > +
> > + int64_t timestamp = prev_delay ? node->next_request_timestamp : now;
> > + node->next_request_timestamp =
> > + timestamp + node->request_delay - prev_delay;
> > +
> > + return timestamp != node->next_request_timestamp;
> > +}
> > diff --git a/controller/statctrl.h b/controller/statctrl.h
> > new file mode 100644
> > index 000000000..c5cede353
> > --- /dev/null
> > +++ b/controller/statctrl.h
> > @@ -0,0 +1,28 @@
> > +/* Copyright (c) 2023, Red Hat, Inc.
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at:
> > + *
> > + * http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +
> > +#ifndef STATCTRL_H
> > +#define STATCTRL_H
> > +
> > +#include "mac_cache.h"
> > +
> > +void statctrl_init(void);
> > +void statctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
> > + struct mac_cache_data *mac_cache_data);
> > +void statctrl_update(const char *br_int_name);
> > +void statctrl_wait(struct ovsdb_idl_txn *ovnsb_idl_txn);
> > +void statctrl_destroy(void);
> > +
> > +#endif /* controller/statctrl.h */
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 7cee8a175..f4c28c57b 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -34556,8 +34556,10 @@ AT_CHECK([fetch_column nb:logical_router
> options name="gw" | grep -q mac_binding
> > send_garp hv1 ext1 10
> > send_garp hv2 ext2 20
> >
> > -OVS_WAIT_UNTIL([ovn-sbctl list mac_binding | grep -q "192.168.10.10"])
> > -OVS_WAIT_UNTIL([ovn-sbctl list mac_binding | grep -q "192.168.10.20"])
> > +wait_row_count mac_binding 1 ip="192.168.10.10"
> > +wait_row_count mac_binding 1 ip="192.168.10.20"
> > +
> > +timestamp=$(fetch_column mac_binding timestamp ip="192.168.10.20")
> >
> > send_udp hv1 ext1 10
> > send_udp hv2 ext2 20
> > @@ -34566,16 +34568,27 @@ OVS_WAIT_UNTIL([as hv1 ovs-ofctl dump-flows
> br-int table=79 | grep "192.168.10.1
> > OVS_WAIT_UNTIL([as hv2 ovs-ofctl dump-flows br-int table=79 | grep
> "192.168.10.20" | grep -q "n_packets=1"])
> >
> > # Set the MAC binding aging threshold
> > -AT_CHECK([ovn-nbctl set logical_router gw
> options:mac_binding_age_threshold=1])
> > -AT_CHECK([fetch_column nb:logical_router options | grep -q
> mac_binding_age_threshold=1])
> > +AT_CHECK([ovn-nbctl set logical_router gw
> options:mac_binding_age_threshold=5])
> > +AT_CHECK([fetch_column nb:logical_router options | grep -q
> mac_binding_age_threshold=5])
> > AT_CHECK([ovn-nbctl --wait=sb sync])
> >
> > +# Wait send few packets for "192.168.10.20" to indicate that it is
> still in use
> > +send_udp hv2 ext2 20
> > +sleep 1
> > +send_udp hv2 ext2 20
> > +
> > # Set the timeout for OVS_WAIT* functions to 5 seconds
> > OVS_CTL_TIMEOUT=5
> > +OVS_WAIT_UNTIL([
> > + test "$timestamp" != "$(fetch_column mac_binding timestamp
> ip='192.168.10.20')"
> > +])
> > +check $(test "$(fetch_column mac_binding timestamp ip='192.168.10.20')"
> != "")
> > +
> > # Check if the records are removed after some inactivity
> > OVS_WAIT_UNTIL([
> > test "0" = "$(ovn-sbctl list mac_binding | grep -c
> '192.168.10.10')"
> > ])
> > +# The second one takes longer because it got refreshed
> > OVS_WAIT_UNTIL([
> > test "0" = "$(ovn-sbctl list mac_binding | grep -c
> '192.168.10.20')"
> > ])
> > @@ -35257,7 +35270,7 @@ check ovs-vsctl add-br br-phys
> > ovn_attach n1 br-phys 192.168.0.1
> >
> > dnl Ensure that there are at least 3 openflow connections.
>
> Nit: The comment is already incorrect since it checks for an exact
> number of connections instead of "at least" some number. But it also
> needs to be updated to say 4 instead of 3.
>
>
Ah right, it will be fixed in v2.
>
> > -OVS_WAIT_UNTIL([test "$(grep -c 'negotiated OpenFlow version'
> hv1/ovs-vswitchd.log)" -eq "3"])
> > +OVS_WAIT_UNTIL([test "$(grep -c 'negotiated OpenFlow version'
> hv1/ovs-vswitchd.log)" -eq "4"])
> >
> > dnl "Wait" 3 times 60 seconds and ensure ovn-controller writes to the
> > dnl openflow connections in the meantime. This should allow
> ovs-vswitchd
>
>
Thanks,
Ales
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
amu...@redhat.com IM: amusil
<https://red.ht/sig>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
