Hi Numan, only one finding down below.
I know that you've added some end-to-end tests for MAC learning in later
patches, but I think adding some basic unit tests for mac-learn.c is
also useful.
On 2/5/21 1:59 AM, [email protected] wrote:
From: Numan Siddique <[email protected]>
This patch moves the 'struct put_mac_binding' from controller/pinctrl.c
to controller/mac-learn.c as a 'struct mac_binding' and adds the
relevant functions to access it.
Signed-off-by: Numan Siddique <[email protected]>
---
controller/automake.mk | 5 +-
controller/mac-learn.c | 101 ++++++++++++++++++++++++++++++++++++++++
controller/mac-learn.h | 46 +++++++++++++++++++
controller/pinctrl.c | 102 ++++++++++++-----------------------------
4 files changed, 181 insertions(+), 73 deletions(-)
create mode 100644 controller/mac-learn.c
create mode 100644 controller/mac-learn.h
diff --git a/controller/automake.mk b/controller/automake.mk
index 480578eda3..9b8debd2ff 100644
--- a/controller/automake.mk
+++ b/controller/automake.mk
@@ -27,7 +27,10 @@ controller_ovn_controller_SOURCES = \
controller/ovn-controller.c \
controller/ovn-controller.h \
controller/physical.c \
- controller/physical.h
+ controller/physical.h \
+ controller/mac-learn.c \
+ controller/mac-learn.h
+
controller_ovn_controller_LDADD = lib/libovn.la
$(OVS_LIBDIR)/libopenvswitch.la
man_MANS += controller/ovn-controller.8
EXTRA_DIST += controller/ovn-controller.8.xml
diff --git a/controller/mac-learn.c b/controller/mac-learn.c
new file mode 100644
index 0000000000..36a6d6e589
--- /dev/null
+++ b/controller/mac-learn.c
@@ -0,0 +1,101 @@
+/* Copyright (c) 2020, 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 "mac-learn.h"
+
+/* OpenvSwitch lib includes. */
+#include "openvswitch/vlog.h"
+#include "lib/smap.h"
+
+VLOG_DEFINE_THIS_MODULE(mac_learn);
+
+#define MAX_MAC_BINDINGS 1000
+
+static size_t mac_binding_hash(uint32_t dp_key, uint32_t port_key,
+ struct in6_addr *);
+static struct mac_binding *mac_binding_find(struct hmap *mac_bindings,
+ uint32_t dp_key,
+ uint32_t port_key,
+ struct in6_addr *ip, size_t hash);
+
+void
+ovn_mac_bindings_init(struct hmap *mac_bindings)
+{
+ hmap_init(mac_bindings);
+}
+
+void
+ovn_mac_bindings_flush(struct hmap *mac_bindings)
+{
+ struct mac_binding *mb;
+ HMAP_FOR_EACH_POP (mb, hmap_node, mac_bindings) {
+ free(mb);
+ }
+}
+
+void
+ovn_mac_bindings_destroy(struct hmap *mac_bindings)
+{
+ ovn_mac_bindings_flush(mac_bindings);
+ hmap_destroy(mac_bindings);
+}
+
+struct mac_binding *
+ovn_mac_binding_add(struct hmap *mac_bindings, uint32_t dp_key,
+ uint32_t port_key, struct in6_addr *ip,
+ struct eth_addr mac)
+{
+ uint32_t hash = mac_binding_hash(dp_key, port_key, ip);
+
+ struct mac_binding *mb =
+ mac_binding_find(mac_bindings, dp_key, port_key, ip, hash);
+ if (!mb) {
+ if (hmap_count(mac_bindings) >= MAX_MAC_BINDINGS) {
+ return NULL;
+ }
+
+ mb = xmalloc(sizeof *mb);
+ mb->dp_key = dp_key;
+ mb->port_key = port_key;
+ mb->ip = *ip;
+ hmap_insert(mac_bindings, &mb->hmap_node, hash);
+ }
+ mb->mac = mac;
+
+ return mb;
+}
+
+static size_t
+mac_binding_hash(uint32_t dp_key, uint32_t port_key, struct in6_addr *ip)
+{
+ return hash_bytes(ip, sizeof *ip, hash_2words(dp_key, port_key));
+}
+
+static struct mac_binding *
+mac_binding_find(struct hmap *mac_bindings, uint32_t dp_key,
+ uint32_t port_key, struct in6_addr *ip, size_t hash)
+{
+ struct mac_binding *mb;
+ HMAP_FOR_EACH_WITH_HASH (mb, hmap_node, hash, mac_bindings) {
+ if (mb->dp_key == dp_key && mb->port_key == port_key &&
+ IN6_ARE_ADDR_EQUAL(&mb->ip, ip)) {
+ return mb;
+ }
+ }
+
+ return NULL;
+}
diff --git a/controller/mac-learn.h b/controller/mac-learn.h
new file mode 100644
index 0000000000..3a8520c360
--- /dev/null
+++ b/controller/mac-learn.h
@@ -0,0 +1,46 @@
+/*
+ * Copyright (c) 2020 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 OVN_MAC_LEARN_H
+#define OVN_MAC_LEARN_H 1
+
+#include <sys/types.h>
+#include <netinet/in.h>
+#include "openvswitch/hmap.h"
+
+struct mac_binding {
+ struct hmap_node hmap_node; /* In a hmap. */
+
+ /* Key. */
+ uint32_t dp_key;
+ uint32_t port_key; /* Port from where this mac_binding is learnt. */
+ struct in6_addr ip;
+
+ /* Value. */
+ struct eth_addr mac;
+};
+
+void ovn_mac_bindings_init(struct hmap *mac_bindings);
+void ovn_mac_bindings_flush(struct hmap *mac_bindings);
+void ovn_mac_bindings_destroy(struct hmap *mac_bindings);
+
+struct mac_binding *ovn_mac_binding_add(struct hmap *mac_bindings,
+ uint32_t dp_key, uint32_t port_key,
+ struct in6_addr *ip,
+ struct eth_addr mac);
+
+
+#endif /* OVN_MAC_LEARN_H */
\ No newline at end of file
diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index dcf6b4af5c..723d2a6522 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -26,6 +26,7 @@
#include "flow.h"
#include "ha-chassis.h"
#include "lport.h"
+#include "mac-learn.h"
#include "nx-match.h"
#include "latch.h"
#include "lib/packets.h"
@@ -193,7 +194,6 @@ static void run_put_mac_bindings(
struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip)
OVS_REQUIRES(pinctrl_mutex);
static void wait_put_mac_bindings(struct ovsdb_idl_txn *ovnsb_idl_txn);
-static void flush_put_mac_bindings(void);
static void send_mac_binding_buffered_pkts(struct rconn *swconn)
OVS_REQUIRES(pinctrl_mutex);
@@ -3893,47 +3893,20 @@ pinctrl_destroy(void)
* available. */
/* Buffered "put_mac_binding" operation. */
-struct put_mac_binding {
- struct hmap_node hmap_node; /* In 'put_mac_bindings'. */
- /* Key. */
- uint32_t dp_key;
- uint32_t port_key;
- struct in6_addr ip_key;
-
- /* Value. */
- struct eth_addr mac;
-};
-
-/* Contains "struct put_mac_binding"s. */
+/* Contains "struct mac_binding"s. */
static struct hmap put_mac_bindings;
static void
init_put_mac_bindings(void)
{
- hmap_init(&put_mac_bindings);
+ ovn_mac_bindings_init(&put_mac_bindings);
}
static void
destroy_put_mac_bindings(void)
{
- flush_put_mac_bindings();
- hmap_destroy(&put_mac_bindings);
-}
-
-static struct put_mac_binding *
-pinctrl_find_put_mac_binding(uint32_t dp_key, uint32_t port_key,
- const struct in6_addr *ip_key, uint32_t hash)
-{
- struct put_mac_binding *pa;
- HMAP_FOR_EACH_WITH_HASH (pa, hmap_node, hash, &put_mac_bindings) {
- if (pa->dp_key == dp_key
- && pa->port_key == port_key
- && IN6_ARE_ADDR_EQUAL(&pa->ip_key, ip_key)) {
- return pa;
- }
- }
- return NULL;
+ ovn_mac_bindings_destroy(&put_mac_bindings);
}
/* Called with in the pinctrl_handler thread context. */
@@ -3953,23 +3926,16 @@ pinctrl_handle_put_mac_binding(const struct flow *md,
ovs_be128 ip6 = hton128(flow_get_xxreg(md, 0));
memcpy(&ip_key, &ip6, sizeof ip_key);
}
- uint32_t hash = hash_bytes(&ip_key, sizeof ip_key,
- hash_2words(dp_key, port_key));
- struct put_mac_binding *pmb
- = pinctrl_find_put_mac_binding(dp_key, port_key, &ip_key, hash);
- if (!pmb) {
+
+ struct mac_binding *mb = ovn_mac_binding_add(&put_mac_bindings, dp_key,
+ port_key, &ip_key,
+ headers->dl_src);
+ if (!mb) {
if (hmap_count(&put_mac_bindings) >= 1000) {
COVERAGE_INC(pinctrl_drop_put_mac_binding);
return;
}
Should this COVERAGE_INC() call be moved to mac_learn.c? Even if you
don't move the COVERAGE_INC() call there, you should remove this if
statement since:
1) It's unnecessary. We already know that a NULL return from
ovn_mac_binding_add() means that there are too many bindings.
2) It's comparing against a magic value of 1000 instead of the #defined
MAX_MAC_BINDINGS.
-
- pmb = xmalloc(sizeof *pmb);
- hmap_insert(&put_mac_bindings, &pmb->hmap_node, hash);
- pmb->dp_key = dp_key;
- pmb->port_key = port_key;
- pmb->ip_key = ip_key;
}
- pmb->mac = headers->dl_src;
/* We can send the buffered packet once the main ovn-controller
* thread calls pinctrl_run() and it writes the mac_bindings stored
@@ -4012,12 +3978,12 @@ mac_binding_lookup(struct ovsdb_idl_index
*sbrec_mac_binding_by_lport_ip,
/* Update or add an IP-MAC binding for 'logical_port'.
* Caller should make sure that 'ovnsb_idl_txn' is valid. */
static void
-mac_binding_add(struct ovsdb_idl_txn *ovnsb_idl_txn,
- struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
- const char *logical_port,
- const struct sbrec_datapath_binding *dp,
- struct eth_addr ea, const char *ip,
- bool update_only)
+mac_binding_add_to_sb(struct ovsdb_idl_txn *ovnsb_idl_txn,
+ struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
+ const char *logical_port,
+ const struct sbrec_datapath_binding *dp,
+ struct eth_addr ea, const char *ip,
+ bool update_only)
{
/* Convert ethernet argument to string form for database. */
char mac_string[ETH_ADDR_STRLEN + 1];
@@ -4073,9 +4039,9 @@ send_garp_locally(struct ovsdb_idl_txn *ovnsb_idl_txn,
struct ds ip_s = DS_EMPTY_INITIALIZER;
ip_format_masked(ip, OVS_BE32_MAX, &ip_s);
- mac_binding_add(ovnsb_idl_txn, sbrec_mac_binding_by_lport_ip,
- remote->logical_port, remote->datapath,
- ea, ds_cstr(&ip_s), update_only);
+ mac_binding_add_to_sb(ovnsb_idl_txn, sbrec_mac_binding_by_lport_ip,
+ remote->logical_port, remote->datapath,
+ ea, ds_cstr(&ip_s), update_only);
ds_destroy(&ip_s);
}
}
@@ -4085,30 +4051,30 @@ run_put_mac_binding(struct ovsdb_idl_txn *ovnsb_idl_txn,
struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
struct ovsdb_idl_index *sbrec_port_binding_by_key,
struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
- const struct put_mac_binding *pmb)
+ const struct mac_binding *mb)
{
/* Convert logical datapath and logical port key into lport. */
const struct sbrec_port_binding *pb = lport_lookup_by_key(
sbrec_datapath_binding_by_key, sbrec_port_binding_by_key,
- pmb->dp_key, pmb->port_key);
+ mb->dp_key, mb->port_key);
if (!pb) {
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
VLOG_WARN_RL(&rl, "unknown logical port with datapath %"PRIu32" "
- "and port %"PRIu32, pmb->dp_key, pmb->port_key);
+ "and port %"PRIu32, mb->dp_key, mb->port_key);
return;
}
/* Convert ethernet argument to string form for database. */
char mac_string[ETH_ADDR_STRLEN + 1];
snprintf(mac_string, sizeof mac_string,
- ETH_ADDR_FMT, ETH_ADDR_ARGS(pmb->mac));
+ ETH_ADDR_FMT, ETH_ADDR_ARGS(mb->mac));
struct ds ip_s = DS_EMPTY_INITIALIZER;
- ipv6_format_mapped(&pmb->ip_key, &ip_s);
- mac_binding_add(ovnsb_idl_txn, sbrec_mac_binding_by_lport_ip,
- pb->logical_port, pb->datapath, pmb->mac, ds_cstr(&ip_s),
- false);
+ ipv6_format_mapped(&mb->ip, &ip_s);
+ mac_binding_add_to_sb(ovnsb_idl_txn, sbrec_mac_binding_by_lport_ip,
+ pb->logical_port, pb->datapath, mb->mac,
+ ds_cstr(&ip_s), false);
ds_destroy(&ip_s);
}
@@ -4125,14 +4091,14 @@ run_put_mac_bindings(struct ovsdb_idl_txn *ovnsb_idl_txn,
return;
}
- const struct put_mac_binding *pmb;
- HMAP_FOR_EACH (pmb, hmap_node, &put_mac_bindings) {
+ const struct mac_binding *mb;
+ HMAP_FOR_EACH (mb, hmap_node, &put_mac_bindings) {
run_put_mac_binding(ovnsb_idl_txn, sbrec_datapath_binding_by_key,
sbrec_port_binding_by_key,
sbrec_mac_binding_by_lport_ip,
- pmb);
+ mb);
}
- flush_put_mac_bindings();
+ ovn_mac_bindings_flush(&put_mac_bindings);
}
static void
@@ -4188,14 +4154,6 @@ wait_put_mac_bindings(struct ovsdb_idl_txn
*ovnsb_idl_txn)
}
}
-static void
-flush_put_mac_bindings(void)
-{
- struct put_mac_binding *pmb;
- HMAP_FOR_EACH_POP (pmb, hmap_node, &put_mac_bindings) {
- free(pmb);
- }
-}
/*
* Send gratuitous/reverse ARP for vif on localnet.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
