Re: [ovs-dev] [PATCH ovn v4 2/3] controller: Add I-P node for recent MAC bindings
On 3/28/23 10:12, Ales Musil wrote:
> Add node that will hold data about recently added
> MAC bindings. This node will be used later on to prevent
> race condition in packet buffering. the MAC binding is
> kept in the node longer than the timeout of the buffered
> packet to make sure that we don't miss it.
>
> Signed-off-by: Ales Musil
> ---
> v4: Rebase on top of current main.
> Split into three patches.
> ---
> controller/ovn-controller.c | 201
> 1 file changed, 201 insertions(+)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 76be2426e..d465f00e9 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -45,6 +45,7 @@
> #include "lib/vswitch-idl.h"
> #include "local_data.h"
> #include "lport.h"
> +#include "mac-learn.h"
> #include "memory.h"
> #include "ofctrl.h"
> #include "ofctrl-seqno.h"
> @@ -4303,6 +4304,185 @@ pflow_output_debug_handler(struct engine_node *node,
> void *data)
> return true;
> }
>
> +static void
> +recent_mac_bindings_add(struct hmap *recent_mbs,
> +const struct sbrec_mac_binding *smb,
> +const struct hmap *local_datapaths,
> +struct ovsdb_idl_index *sbrec_port_binding_by_name)
> +{
This function is extremely similar with the body of the loop in
run_buffered_binding() in pinctrl. Can we find a way to avoid the code
duplication?
After reading patch 3/3 I realized you actually do something like that
there. That's indication to me that patches 2/3 and 3/3 should actually
be squashed. It was also unclear to me from the current patch 2/3 log
why we need these recent mac bindings.
Also, should we add an API to the mac_binding_map structure I suggested
for patch 1/3 to insert an entry that's built from a sbrec_mac_binding?
> +ovs_be32 ip4;
> +struct in6_addr ip;
> +struct eth_addr mac;
> +const struct sbrec_port_binding *pb;
> +
> +if (!get_local_datapath(local_datapaths, smb->datapath->tunnel_key)) {
> +return;
> +}
> +
> +pb = lport_lookup_by_name(sbrec_port_binding_by_name, smb->logical_port);
> +if (!pb) {
> +return;
> +}
> +
> +if (!eth_addr_from_string(smb->mac, )) {
> +return;
> +}
> +
> +if (strchr(smb->ip, '.')) {
> +if (!ip_parse(smb->ip, )) {
> +return;
> +}
> +ip = in6_addr_mapped_ipv4(ip4);
> +} else {
> +if (!ipv6_parse(smb->ip, )) {
> +return;
> +}
> +}
Given that you refactor so much in patch 1/3 already, can you please
also add a patch that adds a new ovn-util.c function to parse IPs and
hides the strchr(.., '.')? We do that in a few places already and it's
a bit annoying to see the same code everywhere.
> +
> +/* Give the recent mac_binding entry bigger timeout than the buffered
> + * packet in case the MAC binding is created earlier. */
> +ovn_mac_binding_add(recent_mbs, smb->datapath->tunnel_key,
> pb->tunnel_key,
> +, mac, 2 * OVN_BUFFERED_PACKETS_TIMEOUT, false);
> +
> +const char *redirect_port =
> +smap_get(>options, "chassis-redirect-port");
> +if (!redirect_port) {
> +return;
> +}
> +
> +pb = lport_lookup_by_name(sbrec_port_binding_by_name, redirect_port);
> +if (!pb || pb->datapath->tunnel_key != smb->datapath->tunnel_key ||
> +strcmp(pb->type, "chassisredirect")) {
> +return;
> +}
> +
> +/* Add the same entry also for chassisredirect port as the buffered
> traffic
> + * might be buffered on the cr port. */
> +ovn_mac_binding_add(recent_mbs, smb->datapath->tunnel_key,
> pb->tunnel_key,
> +, mac, 2 * OVN_BUFFERED_PACKETS_TIMEOUT, false);
> +}
> +
> +static void *
> +en_recent_mac_bindings_init(struct engine_node *node OVS_UNUSED,
> +struct engine_arg *arg OVS_UNUSED)
> +{
> +struct hmap *recent_mbs = xmalloc(sizeof *recent_mbs);
> +hmap_init(recent_mbs);
> +
> +return recent_mbs;
> +}
> +
> +static void
> +en_recent_mac_bindings_cleanup(void *data)
> +{
> +struct hmap *recent_mbs = data;
> +ovn_mac_bindings_destroy(recent_mbs);
> +}
> +
> +static void
> +en_recent_mac_bindings_clear_tracked_data(void *data)
> +{
> +long long now = time_msec();
> +struct hmap *recent_mbs = data;
> +
> +struct mac_binding *mb;
> +HMAP_FOR_EACH_SAFE (mb, hmap_node, recent_mbs) {
> +if (ovn_mac_binding_is_expired(mb, now)) {
> +ovn_mac_binding_remove(mb, recent_mbs);
> +}
> +}
> +}
> +
> +static void
> +en_recent_mac_bindings_run(struct engine_node *node, void *data)
> +{
> +struct hmap *recent_mbs = data;
> +struct ed_type_runtime_data *rt_data =
> +engine_get_input_data("runtime_data", node);
> +const struct sbrec_mac_binding_table *mac_binding_table =
> +
[ovs-dev] [PATCH ovn v4 2/3] controller: Add I-P node for recent MAC bindings
Add node that will hold data about recently added
MAC bindings. This node will be used later on to prevent
race condition in packet buffering. the MAC binding is
kept in the node longer than the timeout of the buffered
packet to make sure that we don't miss it.
Signed-off-by: Ales Musil
---
v4: Rebase on top of current main.
Split into three patches.
---
controller/ovn-controller.c | 201
1 file changed, 201 insertions(+)
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 76be2426e..d465f00e9 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -45,6 +45,7 @@
#include "lib/vswitch-idl.h"
#include "local_data.h"
#include "lport.h"
+#include "mac-learn.h"
#include "memory.h"
#include "ofctrl.h"
#include "ofctrl-seqno.h"
@@ -4303,6 +4304,185 @@ pflow_output_debug_handler(struct engine_node *node,
void *data)
return true;
}
+static void
+recent_mac_bindings_add(struct hmap *recent_mbs,
+const struct sbrec_mac_binding *smb,
+const struct hmap *local_datapaths,
+struct ovsdb_idl_index *sbrec_port_binding_by_name)
+{
+ovs_be32 ip4;
+struct in6_addr ip;
+struct eth_addr mac;
+const struct sbrec_port_binding *pb;
+
+if (!get_local_datapath(local_datapaths, smb->datapath->tunnel_key)) {
+return;
+}
+
+pb = lport_lookup_by_name(sbrec_port_binding_by_name, smb->logical_port);
+if (!pb) {
+return;
+}
+
+if (!eth_addr_from_string(smb->mac, )) {
+return;
+}
+
+if (strchr(smb->ip, '.')) {
+if (!ip_parse(smb->ip, )) {
+return;
+}
+ip = in6_addr_mapped_ipv4(ip4);
+} else {
+if (!ipv6_parse(smb->ip, )) {
+return;
+}
+}
+
+/* Give the recent mac_binding entry bigger timeout than the buffered
+ * packet in case the MAC binding is created earlier. */
+ovn_mac_binding_add(recent_mbs, smb->datapath->tunnel_key, pb->tunnel_key,
+, mac, 2 * OVN_BUFFERED_PACKETS_TIMEOUT, false);
+
+const char *redirect_port =
+smap_get(>options, "chassis-redirect-port");
+if (!redirect_port) {
+return;
+}
+
+pb = lport_lookup_by_name(sbrec_port_binding_by_name, redirect_port);
+if (!pb || pb->datapath->tunnel_key != smb->datapath->tunnel_key ||
+strcmp(pb->type, "chassisredirect")) {
+return;
+}
+
+/* Add the same entry also for chassisredirect port as the buffered traffic
+ * might be buffered on the cr port. */
+ovn_mac_binding_add(recent_mbs, smb->datapath->tunnel_key, pb->tunnel_key,
+, mac, 2 * OVN_BUFFERED_PACKETS_TIMEOUT, false);
+}
+
+static void *
+en_recent_mac_bindings_init(struct engine_node *node OVS_UNUSED,
+struct engine_arg *arg OVS_UNUSED)
+{
+struct hmap *recent_mbs = xmalloc(sizeof *recent_mbs);
+hmap_init(recent_mbs);
+
+return recent_mbs;
+}
+
+static void
+en_recent_mac_bindings_cleanup(void *data)
+{
+struct hmap *recent_mbs = data;
+ovn_mac_bindings_destroy(recent_mbs);
+}
+
+static void
+en_recent_mac_bindings_clear_tracked_data(void *data)
+{
+long long now = time_msec();
+struct hmap *recent_mbs = data;
+
+struct mac_binding *mb;
+HMAP_FOR_EACH_SAFE (mb, hmap_node, recent_mbs) {
+if (ovn_mac_binding_is_expired(mb, now)) {
+ovn_mac_binding_remove(mb, recent_mbs);
+}
+}
+}
+
+static void
+en_recent_mac_bindings_run(struct engine_node *node, void *data)
+{
+struct hmap *recent_mbs = data;
+struct ed_type_runtime_data *rt_data =
+engine_get_input_data("runtime_data", node);
+const struct sbrec_mac_binding_table *mac_binding_table =
+EN_OVSDB_GET(engine_get_input("SB_mac_binding", node));
+struct ovsdb_idl_index *sbrec_port_binding_by_name =
+engine_ovsdb_node_get_index(
+engine_get_input("SB_port_binding", node),
+"name");
+
+const struct sbrec_mac_binding *smb;
+SBREC_MAC_BINDING_TABLE_FOR_EACH (smb, mac_binding_table) {
+recent_mac_bindings_add(recent_mbs, smb, _data->local_datapaths,
+sbrec_port_binding_by_name);
+}
+}
+
+static bool
+recent_mac_bindings_mac_binding_handler(struct engine_node *node, void *data)
+{
+struct hmap *recent_mbs = data;
+struct ed_type_runtime_data *rt_data =
+engine_get_input_data("runtime_data", node);
+const struct sbrec_mac_binding_table *mac_binding_table =
+EN_OVSDB_GET(engine_get_input("SB_mac_binding", node));
+struct ovsdb_idl_index *sbrec_port_binding_by_name =
+engine_ovsdb_node_get_index(
+engine_get_input("SB_port_binding", node),
+"name");
+
+const struct sbrec_mac_binding *smb;
+SBREC_MAC_BINDING_TABLE_FOR_EACH_TRACKED (smb, mac_binding_table) {
+/*
