Re: [ovs-dev] [RFC 1/3] ofproto: change type of n_handlers and n_revalidators
On Fri, Apr 30, 2021 at 11:31:27AM -0400, Mark Gray wrote:
> 'n_handlers' and 'n_revalidators' are declared as type 'size_t'.
> However, dpif_handlers_set() requires parameter 'n_handlers' as
> type 'uint32_t'. This patch fixes this type mismatch.
The change looks good, but I didn't understand the criteria used
to do the change. For example, at udpif_stop_threads() you changed
from 'size_t' to 'uint32_t', but variable 'i' is not required
to be of the same type (marked in line below). However, I could
find other similar cases left unchanged.
fbl
>
> Signed-off-by: Mark Gray
> ---
> ofproto/ofproto-dpif-upcall.c | 24
> ofproto/ofproto-dpif-upcall.h | 5 +++--
> ofproto/ofproto-provider.h| 2 +-
> ofproto/ofproto.c | 2 +-
> 4 files changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index ccf97266c0b9..88406fea1391 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -129,10 +129,10 @@ struct udpif {
> struct dpif_backer *backer;/* Opaque dpif_backer pointer. */
>
> struct handler *handlers; /* Upcall handlers. */
> -size_t n_handlers;
> +uint32_t n_handlers;
>
> struct revalidator *revalidators; /* Flow revalidators. */
> -size_t n_revalidators;
> +uint32_t n_revalidators;
>
> struct latch exit_latch; /* Tells child threads to exit. */
>
> @@ -335,8 +335,8 @@ static int process_upcall(struct udpif *, struct upcall *,
>struct ofpbuf *odp_actions, struct flow_wildcards
> *);
> static void handle_upcalls(struct udpif *, struct upcall *, size_t
> n_upcalls);
> static void udpif_stop_threads(struct udpif *, bool delete_flows);
> -static void udpif_start_threads(struct udpif *, size_t n_handlers,
> -size_t n_revalidators);
> +static void udpif_start_threads(struct udpif *, uint32_t n_handlers,
> +uint32_t n_revalidators);
> static void udpif_pause_revalidators(struct udpif *);
> static void udpif_resume_revalidators(struct udpif *);
> static void *udpif_upcall_handler(void *);
> @@ -522,7 +522,7 @@ static void
> udpif_stop_threads(struct udpif *udpif, bool delete_flows)
> {
> if (udpif && (udpif->n_handlers != 0 || udpif->n_revalidators != 0)) {
> -size_t i;
> +uint32_t i;
>
> /* Tell the threads to exit. */
> latch_set(&udpif->exit_latch);
> @@ -562,8 +562,8 @@ udpif_stop_threads(struct udpif *udpif, bool delete_flows)
>
> /* Starts the handler and revalidator threads. */
> static void
> -udpif_start_threads(struct udpif *udpif, size_t n_handlers_,
> -size_t n_revalidators_)
> +udpif_start_threads(struct udpif *udpif, uint32_t n_handlers_,
> +uint32_t n_revalidators_)
> {
> if (udpif && n_handlers_ && n_revalidators_) {
> /* Creating a thread can take a significant amount of time on some
> @@ -574,7 +574,7 @@ udpif_start_threads(struct udpif *udpif, size_t
> n_handlers_,
> udpif->n_revalidators = n_revalidators_;
>
> udpif->handlers = xzalloc(udpif->n_handlers * sizeof
> *udpif->handlers);
> -for (size_t i = 0; i < udpif->n_handlers; i++) {
> +for (uint32_t i = 0; i < udpif->n_handlers; i++) {
> struct handler *handler = &udpif->handlers[i];
>
> handler->udpif = udpif;
> @@ -632,8 +632,8 @@ udpif_resume_revalidators(struct udpif *udpif)
> * datapath handle must have packet reception enabled before starting
> * threads. */
> void
> -udpif_set_threads(struct udpif *udpif, size_t n_handlers_,
> - size_t n_revalidators_)
> +udpif_set_threads(struct udpif *udpif, uint32_t n_handlers_,
> + uint32_t n_revalidators_)
> {
> ovs_assert(udpif);
> ovs_assert(n_handlers_ && n_revalidators_);
> @@ -691,8 +691,8 @@ udpif_get_memory_usage(struct udpif *udpif, struct simap
> *usage)
> void
> udpif_flush(struct udpif *udpif)
> {
> -size_t n_handlers_ = udpif->n_handlers;
> -size_t n_revalidators_ = udpif->n_revalidators;
> +uint32_t n_handlers_ = udpif->n_handlers;
> +uint32_t n_revalidators_ = udpif->n_revalidators;
>
> udpif_stop_threads(udpif, true);
> dpif_flow_flush(udpif->dpif);
> diff --git a/ofproto/ofproto-dpif-upcall.h b/ofproto/ofproto-dpif-upcall.h
> index 693107ae56c1..b4dfed32046e 100644
> --- a/ofproto/ofproto-dpif-upcall.h
> +++ b/ofproto/ofproto-dpif-upcall.h
> @@ -16,6 +16,7 @@
> #define OFPROTO_DPIF_UPCALL_H
>
> #include
> +#include
>
> struct dpif;
> struct dpif_backer;
> @@ -31,8 +32,8 @@ struct simap;
> void udpif_init(void);
> struct udpif *udpif_create(struct dpif_backer *, struct dpif *);
> void udpif_run(struct udpif *udpif);
> -void udpif_set_threads(struct udpif *, size_t n_handlers,
> -
Re: [ovs-dev] [RFC 2/3] dpif-netlink: fix report_loss() message
On Fri, Apr 30, 2021 at 11:31:28AM -0400, Mark Gray wrote:
> Signed-off-by: Mark Gray
This looks like a bug fix for this commit:
1579cf677fcb dpif-linux: Implement the API functions to allow multiple ...
If you agree, please add the Fixes: tag.
fbl
> ---
> lib/dpif-netlink.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 50520f8c0687..2ded5fdd01b3 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -4662,7 +4662,7 @@ report_loss(struct dpif_netlink *dpif, struct
> dpif_channel *ch, uint32_t ch_idx,
>time_msec() - ch->last_poll);
> }
>
> -VLOG_WARN("%s: lost packet on port channel %u of handler %u",
> - dpif_name(&dpif->dpif), ch_idx, handler_id);
> +VLOG_WARN("%s: lost packet on port channel %u of handler %u%s",
> + dpif_name(&dpif->dpif), ch_idx, handler_id, ds_cstr(&s));
> ds_destroy(&s);
> }
> --
> 2.27.0
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
--
fbl
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC net-next] openvswitch: Introduce per-cpu upcall dispatch
Hi Mark,
I think this patch is going in the right direction but there
are some points that I think we should address. See below.
On Fri, Apr 30, 2021 at 11:33:25AM -0400, Mark Gray wrote:
> The Open vSwitch kernel module uses the upcall mechanism to send
> packets from kernel space to user space when it misses in the kernel
> space flow table. The upcall sends packets via a Netlink socket.
> Currently, a Netlink socket is created for every vport. In this way,
> there is a 1:1 mapping between a vport and a Netlink socket.
> When a packet is received by a vport, if it needs to be sent to
> user space, it is sent via the corresponding Netlink socket.
>
> This mechanism, with various iterations of the corresponding user
> space code, has seen some limitations and issues:
>
> * On systems with a large number of vports, there is a correspondingly
> large number of Netlink sockets which can limit scaling.
> (https://bugzilla.redhat.com/show_bug.cgi?id=1526306)
> * Packet reordering on upcalls.
> (https://bugzilla.redhat.com/show_bug.cgi?id=1844576)
> * A thundering herd issue.
> (https://bugzilla.redhat.com/show_bug.cgi?id=183)
>
> This patch introduces an alternative, feature-negotiated, upcall
> mode using a per-cpu dispatch rather than a per-vport dispatch.
>
> In this mode, the Netlink socket to be used for the upcall is
> selected based on the CPU of the thread that is executing the upcall.
> In this way, it resolves the issues above as:
>
> a) The number of Netlink sockets scales with the number of CPUs
> rather than the number of vports.
> b) Ordering per-flow is maintained as packets are distributed to
> CPUs based on mechanisms such as RSS and flows are distributed
> to a single user space thread.
> c) Packets from a flow can only wake up one user space thread.
>
> The corresponding user space code can be found at:
> https://mail.openvswitch.org/pipermail/ovs-dev/2021-April/382618.html
Thanks for writing a nice commit description.
>
> Bugzilla: https://bugzilla.redhat.com/1844576
> Signed-off-by: Mark Gray
> ---
> include/uapi/linux/openvswitch.h | 8
> net/openvswitch/datapath.c | 70 +++-
> net/openvswitch/datapath.h | 18
> net/openvswitch/flow_netlink.c | 4 --
> 4 files changed, 94 insertions(+), 6 deletions(-)
>
> diff --git a/include/uapi/linux/openvswitch.h
> b/include/uapi/linux/openvswitch.h
> index 8d16744edc31..6571b57b2268 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -70,6 +70,8 @@ enum ovs_datapath_cmd {
> * set on the datapath port (for OVS_ACTION_ATTR_MISS). Only valid on
> * %OVS_DP_CMD_NEW requests. A value of zero indicates that upcalls should
> * not be sent.
> + * OVS_DP_ATTR_PER_CPU_PIDS: Per-cpu array of PIDs for upcalls when
> + * OVS_DP_F_DISPATCH_UPCALL_PER_CPU feature is set.
> * @OVS_DP_ATTR_STATS: Statistics about packets that have passed through the
> * datapath. Always present in notifications.
> * @OVS_DP_ATTR_MEGAFLOW_STATS: Statistics about mega flow masks usage for
> the
> @@ -87,6 +89,9 @@ enum ovs_datapath_attr {
> OVS_DP_ATTR_USER_FEATURES, /* OVS_DP_F_* */
> OVS_DP_ATTR_PAD,
> OVS_DP_ATTR_MASKS_CACHE_SIZE,
> + OVS_DP_ATTR_PER_CPU_PIDS, /* Netlink PIDS to receive upcalls in
> per-cpu
> + * dispatch mode
> + */
> __OVS_DP_ATTR_MAX
> };
>
> @@ -127,6 +132,9 @@ struct ovs_vport_stats {
> /* Allow tc offload recirc sharing */
> #define OVS_DP_F_TC_RECIRC_SHARING (1 << 2)
>
> +/* Allow per-cpu dispatch of upcalls */
> +#define OVS_DP_F_DISPATCH_UPCALL_PER_CPU (1 << 3)
> +
> /* Fixed logical ports. */
> #define OVSP_LOCAL ((__u32)0)
>
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index 9d6ef6cb9b26..98d54f41fdaa 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -121,6 +121,8 @@ int lockdep_ovsl_is_held(void)
> #endif
>
> static struct vport *new_vport(const struct vport_parms *);
> +static u32 ovs_dp_get_upcall_portid(const struct datapath *, uint32_t);
> +static int ovs_dp_set_upcall_portids(struct datapath *, const struct nlattr
> *);
> static int queue_gso_packets(struct datapath *dp, struct sk_buff *,
>const struct sw_flow_key *,
>const struct dp_upcall_info *,
> @@ -238,7 +240,12 @@ void ovs_dp_process_packet(struct sk_buff *skb, struct
> sw_flow_key *key)
>
> memset(&upcall, 0, sizeof(upcall));
> upcall.cmd = OVS_PACKET_CMD_MISS;
> - upcall.portid = ovs_vport_find_upcall_portid(p, skb);
> +
> + if (dp->user_features & OVS_DP_F_DISPATCH_UPCALL_PER_CPU)
> + upcall.portid = ovs_dp_get_upcall_portid(dp,
> smp_processor_id());
> + else
> + upcall.portid = ovs_vport_find_upcall_portid(
[ovs-dev] [PATCH ovn 4/4] ovn-controller: Fix incremental processing for logical port references.
If a lflow has an lport name in the match, but when the lflow is
processed the port-binding is not seen by ovn-controller, the
corresponding openflow will not be created. Later if the port-binding is
created/monitored by ovn-controller, the lflow is not reprocessed
because the lflow didn't change and ovn-controller doesn't know that the
port-binding affects the lflow. This patch fixes the problem by tracking
the references when parsing the lflow, even if the port-binding is not
found when the lflow is firstly parsed. A test case is also added to
cover the scenario.
Signed-off-by: Han Zhou
---
controller/lflow.c | 66 +++--
controller/lflow.h | 3 ++
controller/ovn-controller.c | 17 --
tests/ovn.at| 48 +++
4 files changed, 114 insertions(+), 20 deletions(-)
diff --git a/controller/lflow.c b/controller/lflow.c
index 0abb6eb96..fa102e8d0 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -61,6 +61,7 @@ struct lookup_port_aux {
struct condition_aux {
struct ovsdb_idl_index *sbrec_port_binding_by_name;
+const struct sbrec_datapath_binding *dp;
const struct sbrec_chassis *chassis;
const struct sset *active_tunnels;
const struct sbrec_logical_flow *lflow;
@@ -98,6 +99,12 @@ lookup_port_cb(const void *aux_, const char *port_name,
unsigned int *portp)
const struct lookup_port_aux *aux = aux_;
+/* Store the name that used to lookup the lport to lflow reference, so that
+ * in the future when the lport's port binding changes, the logical flow
+ * that references this lport can be reprocessed. */
+lflow_resource_add(aux->lfrr, REF_TYPE_PORTBINDING, port_name,
+ &aux->lflow->header_.uuid);
+
const struct sbrec_port_binding *pb
= lport_lookup_by_name(aux->sbrec_port_binding_by_name, port_name);
if (pb && pb->datapath == aux->dp) {
@@ -149,19 +156,18 @@ is_chassis_resident_cb(const void *c_aux_, const char
*port_name)
{
const struct condition_aux *c_aux = c_aux_;
+/* Store the port name that used to lookup the lport to lflow reference, so
+ * that in the future when the lport's port-binding changes the logical
+ * flow that references this lport can be reprocessed. */
+lflow_resource_add(c_aux->lfrr, REF_TYPE_PORTBINDING, port_name,
+ &c_aux->lflow->header_.uuid);
+
const struct sbrec_port_binding *pb
= lport_lookup_by_name(c_aux->sbrec_port_binding_by_name, port_name);
if (!pb) {
return false;
}
-/* Store the port_name to lflow reference. */
-int64_t dp_id = pb->datapath->tunnel_key;
-char buf[16];
-get_unique_lport_key(dp_id, pb->tunnel_key, buf, sizeof(buf));
-lflow_resource_add(c_aux->lfrr, REF_TYPE_PORTBINDING, buf,
- &c_aux->lflow->header_.uuid);
-
if (strcmp(pb->type, "chassisredirect")) {
/* for non-chassisredirect ports */
return pb->chassis && pb->chassis == c_aux->chassis;
@@ -623,8 +629,6 @@ add_matches_to_flow_table(const struct sbrec_logical_flow
*lflow,
int64_t dp_id = dp->tunnel_key;
char buf[16];
get_unique_lport_key(dp_id, port_id, buf, sizeof(buf));
-lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_PORTBINDING, buf,
- &lflow->header_.uuid);
if (!sset_contains(l_ctx_in->local_lport_ids, buf)) {
VLOG_DBG("lflow "UUID_FMT
" port %s in match is not local, skip",
@@ -788,6 +792,7 @@ consider_logical_flow__(const struct sbrec_logical_flow
*lflow,
};
struct condition_aux cond_aux = {
.sbrec_port_binding_by_name = l_ctx_in->sbrec_port_binding_by_name,
+.dp = dp,
.chassis = l_ctx_in->chassis,
.active_tunnels = l_ctx_in->active_tunnels,
.lflow = lflow,
@@ -883,7 +888,9 @@ consider_logical_flow__(const struct sbrec_logical_flow
*lflow,
/* Cache new entry if caching is enabled. */
if (lflow_cache_is_enabled(l_ctx_out->lflow_cache)) {
-if (cached_expr && !is_cr_cond_present) {
+if (cached_expr
+&& !lflow_ref_lookup(&l_ctx_out->lfrr->lflow_ref_table,
+ &lflow->header_.uuid)) {
lflow_cache_add_matches(l_ctx_out->lflow_cache,
&lflow->header_.uuid, matches,
matches_size);
@@ -1746,19 +1753,42 @@ lflow_processing_end:
return handled;
}
+/* Handles a port-binding change that is possibly related to a lport's
+ * residence status on this chassis. */
bool
lflow_handle_flows_for_lport(const struct sbrec_port_binding *pb,
struct lflow_ctx_in *l_ctx_in,
struct lflow_ctx_out *l_ctx_out)
{
-bool ch
[ovs-dev] [PATCH ovn 2/4] ovn-northd: Remove lflow_add_unique.
This patch removes the workaround when adding multicast group related
lflows, because the multicast group dependency problem is fixed in
ovn-controller in the previous commit.
This patch also removes the UniqueFlow/AnnotatedFlow usage in northd
DDlog implementation for the same reason.
Signed-off-by: Han Zhou
---
northd/ovn-northd.c | 24 ++---
northd/ovn_northd.dl | 220 +--
tests/ovn-northd.at | 2 +-
3 files changed, 118 insertions(+), 128 deletions(-)
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index ca56a6efb..89d86596b 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -6443,7 +6443,7 @@ build_lswitch_rport_arp_req_self_orig_flow(struct
ovn_port *op,
ds_put_format(&match, "eth.src == %s && (arp.op == 1 || nd_ns)",
ds_cstr(ð_src));
-ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_LKUP, priority,
+ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, priority,
ds_cstr(&match),
"outport = \""MC_FLOOD_L2"\"; output;");
@@ -6498,7 +6498,7 @@ build_lswitch_rport_arp_req_flow_for_ip(struct sset *ips,
ds_put_format(&actions, "clone {outport = %s; output; }; "
"outport = \""MC_FLOOD_L2"\"; output;",
patch_op->json_key);
-ovn_lflow_add_unique_with_hint(lflows, od, S_SWITCH_IN_L2_LKUP,
+ovn_lflow_add_with_hint(lflows, od, S_SWITCH_IN_L2_LKUP,
priority, ds_cstr(&match),
ds_cstr(&actions), stage_hint);
} else {
@@ -6854,7 +6854,7 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap
*lflows)
"outport = get_fdb(eth.dst); next;");
if (od->has_unknown) {
-ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_UNKNOWN, 50,
+ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_UNKNOWN, 50,
"outport == \"none\"",
"outport = \""MC_UNKNOWN "\"; output;");
} else {
@@ -7300,24 +7300,24 @@ build_lswitch_destination_lookup_bmcast(struct
ovn_datapath *od,
}
ds_put_cstr(actions, "igmp;");
/* Punt IGMP traffic to controller. */
-ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_LKUP, 100,
+ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 100,
"ip4 && ip.proto == 2", ds_cstr(actions));
/* Punt MLD traffic to controller. */
-ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_LKUP, 100,
+ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 100,
"mldv1 || mldv2", ds_cstr(actions));
/* Flood all IP multicast traffic destined to 224.0.0.X to all
* ports - RFC 4541, section 2.1.2, item 2.
*/
-ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_LKUP, 85,
+ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 85,
"ip4.mcast && ip4.dst == 224.0.0.0/24",
"outport = \""MC_FLOOD"\"; output;");
/* Flood all IPv6 multicast traffic destined to reserved
* multicast IPs (RFC 4291, 2.7.1).
*/
-ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_LKUP, 85,
+ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 85,
"ip6.mcast_flood",
"outport = \""MC_FLOOD"\"; output;");
@@ -7349,13 +7349,13 @@ build_lswitch_destination_lookup_bmcast(struct
ovn_datapath *od,
ds_put_cstr(actions, "drop;");
}
-ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_LKUP, 80,
+ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 80,
"ip4.mcast || ip6.mcast",
ds_cstr(actions));
}
}
-ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_LKUP, 70, "eth.mcast",
+ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 70, "eth.mcast",
"outport = \""MC_FLOOD"\"; output;");
}
}
@@ -7434,7 +7434,7 @@ build_lswitch_ip_mcast_igmp_mld(struct ovn_igmp_group
*igmp_group,
ds_put_format(actions, "outport = \"%s\"; output; ",
igmp_group->mcgroup.name);
-ovn_lflow_add_unique(lflows, igmp_group->datapath, S_SWITCH_IN_L2_LKUP,
+ovn_lflow_add(lflows, igmp_group->datapath, S_SWITCH_IN_L2_LKUP,
90, ds_cstr(match), ds_cstr(actions));
}
}
@@ -9976,7 +9976,7 @@ build_mcast_lookup_flows_for_lrouter(
}
ds_put_format(actions, "outport = \"%s\"; ip.ttl--; next;",
igmp_group->mcgroup.name);
-ovn_lflow_add_unique(lflows, od
[ovs-dev] [PATCH ovn 3/4] ovn-controller: Always monitor all logical datapath groups.
Always monitor all logical datapath groups. Otherwise, DPG updates may
be received *after* the lflows using it are seen by ovn-controller.
Since the number of DPGs are relatively small, we monitor all DPGs to
avoid the unnecessarily extra control plane round trip for the lflows to
be processed.
Signed-off-by: Han Zhou
---
controller/ovn-controller.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index bf29d..949137c0a 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -190,10 +190,15 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
struct ovsdb_idl_condition igmp = OVSDB_IDL_CONDITION_INIT(&igmp);
struct ovsdb_idl_condition chprv = OVSDB_IDL_CONDITION_INIT(&chprv);
+/* Always monitor all logical datapath groups. Otherwise, DPG updates may
+ * be received *after* the lflows using it are seen by ovn-controller.
+ * Since the number of DPGs are relatively small, we monitor all DPGs to
+ * avoid the unnecessarily extra wake-ups of ovn-controller. */
+ovsdb_idl_condition_add_clause_true(&ldpg);
+
if (monitor_all) {
ovsdb_idl_condition_add_clause_true(&pb);
ovsdb_idl_condition_add_clause_true(&lf);
-ovsdb_idl_condition_add_clause_true(&ldpg);
ovsdb_idl_condition_add_clause_true(&mb);
ovsdb_idl_condition_add_clause_true(&mg);
ovsdb_idl_condition_add_clause_true(&dns);
@@ -257,8 +262,6 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
sbrec_port_binding_add_clause_datapath(&pb, OVSDB_F_EQ, uuid);
sbrec_logical_flow_add_clause_logical_datapath(&lf, OVSDB_F_EQ,
uuid);
-sbrec_logical_dp_group_add_clause_datapaths(
-&ldpg, OVSDB_F_INCLUDES, &uuid, 1);
sbrec_mac_binding_add_clause_datapath(&mb, OVSDB_F_EQ, uuid);
sbrec_multicast_group_add_clause_datapath(&mg, OVSDB_F_EQ, uuid);
sbrec_dns_add_clause_datapaths(&dns, OVSDB_F_INCLUDES, &uuid, 1);
--
2.30.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn 1/4] ovn-controller: Fix incremental processing for multicast group dependency.
Multicast group changes are handled for physical flows in I-P, but not
handled for logical flows. Although logical flow doesn't care about the
content of a multicast group, the existance of it matters for logical
flow processing. If the multicast group is not found when the logical
flow is processed at first but laster it is created/monitored by
ovn-controller, the logical flow that has been processed before should
be reprocessed. This would avoid the workaound of ovn_lflow_add_unique()
used in northd when adding multicast group related logical flows.
Signed-off-by: Han Zhou
---
controller/lflow.c | 40 +
controller/lflow.h | 5 -
controller/ovn-controller.c | 13 +++-
lib/ovn-util.h | 8
4 files changed, 64 insertions(+), 2 deletions(-)
diff --git a/controller/lflow.c b/controller/lflow.c
index 680b8cca1..0abb6eb96 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -55,6 +55,8 @@ struct lookup_port_aux {
struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath;
struct ovsdb_idl_index *sbrec_port_binding_by_name;
const struct sbrec_datapath_binding *dp;
+const struct sbrec_logical_flow *lflow;
+struct lflow_resource_ref *lfrr;
};
struct condition_aux {
@@ -103,6 +105,16 @@ lookup_port_cb(const void *aux_, const char *port_name,
unsigned int *portp)
return true;
}
+/* Store the key (DP + name) that used to lookup the multicast group to
+ * lflow reference, so that in the future when the multicast group's
+ * existance (found/not found) changes, the logical flow that references
+ * this multicast group can be reprocessed. */
+struct ds mg_key = DS_EMPTY_INITIALIZER;
+get_mc_group_key(port_name, aux->dp->tunnel_key, &mg_key);
+lflow_resource_add(aux->lfrr, REF_TYPE_MC_GROUP, ds_cstr(&mg_key),
+ &aux->lflow->header_.uuid);
+ds_destroy(&mg_key);
+
const struct sbrec_multicast_group *mg = mcgroup_lookup_by_dp_name(
aux->sbrec_multicast_group_by_name_datapath, aux->dp, port_name);
if (mg) {
@@ -570,6 +582,8 @@ add_matches_to_flow_table(const struct sbrec_logical_flow
*lflow,
= l_ctx_in->sbrec_multicast_group_by_name_datapath,
.sbrec_port_binding_by_name = l_ctx_in->sbrec_port_binding_by_name,
.dp = dp,
+.lflow = lflow,
+.lfrr = l_ctx_out->lfrr,
};
/* Encode OVN logical actions into OpenFlow. */
@@ -769,6 +783,8 @@ consider_logical_flow__(const struct sbrec_logical_flow
*lflow,
= l_ctx_in->sbrec_multicast_group_by_name_datapath,
.sbrec_port_binding_by_name = l_ctx_in->sbrec_port_binding_by_name,
.dp = dp,
+.lflow = lflow,
+.lfrr = l_ctx_out->lfrr,
};
struct condition_aux cond_aux = {
.sbrec_port_binding_by_name = l_ctx_in->sbrec_port_binding_by_name,
@@ -1745,6 +1761,30 @@ lflow_handle_flows_for_lport(const struct
sbrec_port_binding *pb,
l_ctx_in, l_ctx_out, &changed);
}
+bool
+lflow_handle_mc_group_changes(struct lflow_ctx_in *l_ctx_in,
+ struct lflow_ctx_out *l_ctx_out)
+{
+bool changed, ret = true;
+struct ds mg_key = DS_EMPTY_INITIALIZER;
+const struct sbrec_multicast_group *mg;
+SBREC_MULTICAST_GROUP_TABLE_FOR_EACH_TRACKED (mg,
+ l_ctx_in->mc_group_table) {
+get_mc_group_key(mg->name, mg->datapath->tunnel_key, &mg_key);
+if (!sbrec_multicast_group_is_new(mg)
+&& !sbrec_multicast_group_is_deleted(mg)) {
+continue;
+}
+if (!lflow_handle_changed_ref(REF_TYPE_MC_GROUP, ds_cstr(&mg_key),
+ l_ctx_in, l_ctx_out, &changed)) {
+ret = false;
+break;
+}
+}
+ds_destroy(&mg_key);
+return ret;
+}
+
bool
lflow_handle_changed_lbs(struct lflow_ctx_in *l_ctx_in,
struct lflow_ctx_out *l_ctx_out)
diff --git a/controller/lflow.h b/controller/lflow.h
index 3c929d8a6..07263ff47 100644
--- a/controller/lflow.h
+++ b/controller/lflow.h
@@ -78,7 +78,8 @@ struct uuid;
enum ref_type {
REF_TYPE_ADDRSET,
REF_TYPE_PORTGROUP,
-REF_TYPE_PORTBINDING
+REF_TYPE_PORTBINDING,
+REF_TYPE_MC_GROUP
};
struct ref_lflow_node {
@@ -179,4 +180,6 @@ bool lflow_add_flows_for_datapath(const struct
sbrec_datapath_binding *,
bool lflow_handle_flows_for_lport(const struct sbrec_port_binding *,
struct lflow_ctx_in *,
struct lflow_ctx_out *);
+bool lflow_handle_mc_group_changes(struct lflow_ctx_in *,
+ struct lflow_ctx_out *);
#endif /* controller/lflow.h */
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index d48ddc7a2..bf29d 100644
--- a/controller/ovn-controll
[ovs-dev] [PATCH ovn 0/4] Fix ovn-controller I-P for multicast groups and lport changes
The series fixes incremental processing for missing dependency handling for multicast group and logical port binding changes when computing logical flows. It also removes the workaround in northd that was required due to the missing dependency handling. In addition, the fix also allows us to monitor all DPGs as an optimization, so it is also included in the series. Han Zhou (4): ovn-controller: Fix incremental processing for multicast group dependency. ovn-northd: Remove lflow_add_unique. ovn-controller: Always monitor all logical datapath groups. ovn-controller: Fix incremental processing for logical port references. controller/lflow.c | 106 ++--- controller/lflow.h | 8 +- controller/ovn-controller.c | 35 +- lib/ovn-util.h | 8 ++ northd/ovn-northd.c | 24 ++-- northd/ovn_northd.dl| 220 +--- tests/ovn-northd.at | 2 +- tests/ovn.at| 48 8 files changed, 300 insertions(+), 151 deletions(-) -- 2.30.2 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/2] ofproto: Fix potential NULL dereference in ofproto_ct_*_zone_timeout_policy().
Dumitru Ceara writes:
> Spotted during code inspection.
>
> Fixes: 993cae678bca ("ofproto-dpif: Consume CT_Zone, and CT_Timeout_Policy
> tables")
> Signed-off-by: Dumitru Ceara
> ---
Acked-by: Paolo Valerio
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/2] ofproto: Fix potential NULL dereference in ofproto_get_datapath_cap().
Dumitru Ceara writes:
> Reproducer:
> ovs-vsctl \
> -- add-br br \
> -- set bridge br datapath-type=foo \
> -- --id=@m create Datapath datapath_version=0 'capabilities={}' \
> -- set Open_vSwitch . datapaths:"foo"=@m
>
> Fixes: 27501802d09f ("ofproto-dpif: Expose datapath capability to ovsdb.")
> Signed-off-by: Dumitru Ceara
> ---
LGTM,
Acked-by: Paolo Valerio
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn] northd: Add support for DHCP Option 12 (Hostname)
DHCP Option Hostname is a per-Logical_Switch_Port property, configured in
Logical_Switch_Port's options:hostname field. It is used if DHCPv4 is
enabled for this LSP.
Signed-off-by: Vladislav Odintsov
---
The implementation for ovn-northd-ddlog is absent, it needs help from
somebody, who's familiar with ddlog.
lib/ovn-l7.h| 1 +
northd/ovn-northd.c | 9 +
ovn-nb.xml | 9 +
tests/ovn.at| 3 +++
tests/test-ovn.c| 1 +
5 files changed, 23 insertions(+)
diff --git a/lib/ovn-l7.h b/lib/ovn-l7.h
index 5e33d619c..9a33f5cda 100644
--- a/lib/ovn-l7.h
+++ b/lib/ovn-l7.h
@@ -81,6 +81,7 @@ struct gen_opts_map {
#define DHCP_OPT_DNS_SERVER DHCP_OPTION("dns_server", 6, "ipv4")
#define DHCP_OPT_LOG_SERVER DHCP_OPTION("log_server", 7, "ipv4")
#define DHCP_OPT_LPR_SERVER DHCP_OPTION("lpr_server", 9, "ipv4")
+#define DHCP_OPT_HOSTNAMEDHCP_OPTION("hostname", 12, "str")
#define DHCP_OPT_DOMAIN_NAME DHCP_OPTION("domain_name", 15, "str")
#define DHCP_OPT_SWAP_SERVER DHCP_OPTION("swap_server", 16, "ipv4")
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index c39d451ec..e43c6d91a 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -4574,6 +4574,14 @@ build_dhcpv4_action(struct ovn_port *op, ovs_be32
offer_ip,
REGBIT_DHCP_OPTS_RESULT" = put_dhcp_opts(offerip = "
IP_FMT", ", IP_ARGS(offer_ip));
+/* hostname is a per Logical_Switch_Port dhcp option,
+ * so try to get it from ovn_port and put it to options_action
+ * if it exists. */
+const char *hostname = smap_get(&op->nbsp->options, "hostname");
+if (hostname) {
+ds_put_format(options_action, "%s = %s, ", "hostname", hostname);
+}
+
/* We're not using SMAP_FOR_EACH because we want a consistent order of the
* options on different architectures (big or little endian, SSE4.2) */
const struct smap_node **sorted_opts = smap_sort(&dhcpv4_options);
@@ -13561,6 +13569,7 @@ static struct gen_opts_map supported_dhcp_opts[] = {
DHCP_OPT_BOOTFILE,
DHCP_OPT_PATH_PREFIX,
DHCP_OPT_TFTP_SERVER_ADDRESS,
+DHCP_OPT_HOSTNAME,
DHCP_OPT_DOMAIN_NAME,
DHCP_OPT_ARP_CACHE_TIMEOUT,
DHCP_OPT_TCP_KEEPALIVE_INTERVAL,
diff --git a/ovn-nb.xml b/ovn-nb.xml
index cf1e3aac4..404b15608 100644
--- a/ovn-nb.xml
+++ b/ovn-nb.xml
@@ -929,6 +929,15 @@
If set, indicates the maximum burst size for data sent from this
interface, in bits.
+
+
+
+If set, indicates the DHCPv4 option "Hostname" (option code 12)
+associated for this Logical Switch Port. If DHCPv4 is enabled for
+this Logical Switch Port, hostname dhcp option will be included in
+DHCP reply.
+
+
diff --git a/tests/ovn.at b/tests/ovn.at
index 71d2bab4d..757e15972 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -1345,6 +1345,9 @@ reg2[5] =
put_dhcp_opts(offerip=10.0.0.4,router=10.0.0.1,netmask=255.255.254.0,m
reg2[5] =
put_dhcp_opts(offerip=10.0.0.4,router=10.0.0.1,netmask=255.255.254.0,mtu=1400,domain_name="ovn.org",wpad="https://example.org",bootfile_name="https://127.0.0.1/boot.ipxe",path_prefix="/tftpboot",domain_search_list="ovn.org,abc.ovn.org,def.ovn.org,ovn.test,def.ovn.test,test.org,abc.com";);
formats as reg2[5] = put_dhcp_opts(offerip = 10.0.0.4, router = 10.0.0.1,
netmask = 255.255.254.0, mtu = 1400, domain_name = "ovn.org", wpad =
"https://example.org";, bootfile_name = "https://127.0.0.1/boot.ipxe";,
path_prefix = "/tftpboot", domain_search_list =
"ovn.org,abc.ovn.org,def.ovn.org,ovn.test,def.ovn.test,test.org,abc.com");
encodes as
controller(userdata=00.00.00.02.00.00.00.00.00.01.de.10.00.00.00.25.0a.00.00.04.43.1b.68.74.74.70.73.3a.2f.2f.31.32.37.2e.30.2e.30.2e.31.2f.62.6f.6f.74.2e.69.70.78.65.03.04.0a.00.00.01.01.04.ff.ff.fe.00.1a.02.05.78.0f.07.6f.76.6e.2e.6f.72.67.fc.13.68.74.74.70.73.3a.2f.2f.65.78.61.6d.70.6c.65.2e.6f.72.67.d2.09.2f.74.66.74.70.62.6f.6f.74.77.35.03.6f.76.6e.03.6f.72.67.00.03.61.62.63.c0.00.03.64.65.66.c0.00.03.6f.76.6e.04.74.65.73.74.00.03.64.65.66.c0.15.04.74.65.73.74.c0.04.03.61.62.63.03.63.6f.6d.00,pause)
+reg2[5] =
put_dhcp_opts(offerip=10.0.0.4,router=10.0.0.1,netmask=255.255.254.0,mtu=1400,domain_name="ovn.org",hostname="ip-10-0-0-4");
+formats as reg2[5] = put_dhcp_opts(offerip = 10.0.0.4, router = 10.0.0.1,
netmask = 255.255.254.0, mtu = 1400, domain_name = "ovn.org", hostname =
"ip-10-0-0-4");
+encodes as
controller(userdata=00.00.00.02.00.00.00.00.00.01.de.10.00.00.00.25.0a.00.00.04.03.04.0a.00.00.01.01.04.ff.ff.fe.00.1a.02.05.78.0f.07.6f.76.6e.2e.6f.72.67.0c.0b.69.70.2d.31.30.2d.30.2d.30.2d.34,pause)
reg1[0..1] = put_dhcp_opts(offerip = 1.2.3.4, router = 10.0.0.1);
Cannot use 2-bit field reg1[0..1] where 1-bit field is required.
diff --git a/tests/test-ovn.c b/tests/test-ovn.c
index 98cc2c503..a4701b4cb 100644
--- a/tests/test-ovn.c
+++ b/tests/test
Re: [ovs-dev] [PATCH ovn] controller-vtep: fix mmr and physical locators create/update
I've posted a new version of this patch with requested changes: https://patchwork.ozlabs.org/project/ovn/patch/20210528082105.70086-1-odiv...@gmail.com/ Regards, Vladislav Odintsov On 27.05.2021, 23:00, "dev on behalf of 0-day Robot" wrote: Bleep bloop. Greetings Vladislav Odintsov, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Line is 80 characters long (recommended limit is 79) #88 FILE: controller-vtep/vtep.c:178: vtep_create_mmr(vtep_idl_txn, "unknown-dst", vtep_ls, ploc_set); WARNING: Line is 82 characters long (recommended limit is 79) #90 FILE: controller-vtep/vtep.c:180: vteprec_mcast_macs_remote_set_locator_set(mmr_ext->mmr, ploc_set); WARNING: Line is 84 characters long (recommended limit is 79) #118 FILE: controller-vtep/vtep.c:249: vteprec_logical_switch_set_replication_mode(vtep_ls, "source_node"); Lines checked: 248, Warnings: 3, Errors: 0 Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v2 ovn] ovn-controller-vtep: Fix MMR create/update
Before this patch ovn-controller-vtep created Mcast_Macs_Remote
record for each Port Binding in the ovn Logical Switch, to which
vtep Logical Switch was attached.
With this patch there is only one Mcast_Macs_Remote record per datapath.
Physical Locator set is created every time when physical locators for
associated datapath are changed. Next, this newly-created physical
locator set is updated in the MMR record.
Also, update logical switch's tunnel key and replication method only
if needed.
Signed-off-by: Vladislav Odintsov
---
controller-vtep/vtep.c | 66 +-
tests/ovn-controller-vtep.at | 70
2 files changed, 111 insertions(+), 25 deletions(-)
diff --git a/controller-vtep/vtep.c b/controller-vtep/vtep.c
index f3b02f63f..92e6bec92 100644
--- a/controller-vtep/vtep.c
+++ b/controller-vtep/vtep.c
@@ -94,7 +94,7 @@ create_pl(struct ovsdb_idl_txn *vtep_idl_txn, const char
*chassis_ip)
return new_pl;
}
-
+
/* Creates a new 'Mcast_Macs_Remote'. */
static void
vtep_create_mmr(struct ovsdb_idl_txn *vtep_idl_txn, const char *mac,
@@ -104,6 +104,7 @@ vtep_create_mmr(struct ovsdb_idl_txn *vtep_idl_txn, const
char *mac,
struct vteprec_mcast_macs_remote *new_mmr =
vteprec_mcast_macs_remote_insert(vtep_idl_txn);
+VLOG_DBG("Inserting MMR for LS '%s'", vtep_ls->name);
vteprec_mcast_macs_remote_set_MAC(new_mmr, mac);
vteprec_mcast_macs_remote_set_logical_switch(new_mmr, vtep_ls);
vteprec_mcast_macs_remote_set_locator_set(new_mmr, ploc_set);
@@ -140,7 +141,7 @@ vtep_process_pls(const struct ovs_list *locators_list,
ploc_entry->vteprec_ploc;
if (mmr_ext && !shash_find_data(&mmr_ext->physical_locators,
locators[i]->dst_ip)) {
-locator_lists_differ = true;
+locator_lists_differ = true;
}
i++;
}
@@ -149,8 +150,8 @@ vtep_process_pls(const struct ovs_list *locators_list,
return locator_lists_differ;
}
-/* Creates a new 'Mcast_Macs_Remote' entry if needed and also cleans up
- * out-dated remote mcast mac entries as needed. */
+/* Creates a new 'Mcast_Macs_Remote' entry or modifies existing if needed
+ * and also cleans up out-dated remote mcast mac entries as needed. */
static void
vtep_update_mmr(struct ovsdb_idl_txn *vtep_idl_txn,
struct ovs_list *locators_list,
@@ -162,22 +163,29 @@ vtep_update_mmr(struct ovsdb_idl_txn *vtep_idl_txn,
bool mmr_changed;
locators = xmalloc(n_locators_new * sizeof *locators);
-
mmr_changed = vtep_process_pls(locators_list, mmr_ext, locators);
-if (mmr_ext && !n_locators_new) {
-vteprec_mcast_macs_remote_delete(mmr_ext->mmr);
-} else if ((mmr_ext && mmr_changed) ||
- (!mmr_ext && n_locators_new)) {
+if (mmr_changed) {
+if (n_locators_new) {
+const struct vteprec_physical_locator_set *ploc_set =
+vteprec_physical_locator_set_insert(vtep_idl_txn);
-const struct vteprec_physical_locator_set *ploc_set =
-vteprec_physical_locator_set_insert(vtep_idl_txn);
+vteprec_physical_locator_set_set_locators(ploc_set, locators,
+ n_locators_new);
-vtep_create_mmr(vtep_idl_txn, "unknown-dst", vtep_ls, ploc_set);
+if (!mmr_ext) { /* create new mmr */
+vtep_create_mmr(vtep_idl_txn, "unknown-dst", vtep_ls,
+ploc_set);
+} else { /* update existing mmr */
+vteprec_mcast_macs_remote_set_locator_set(mmr_ext->mmr,
+ ploc_set);
+}
-vteprec_physical_locator_set_set_locators(ploc_set, locators,
- n_locators_new);
+} else if (mmr_ext) { /* remove old mmr */
+vteprec_mcast_macs_remote_delete(mmr_ext->mmr);
+}
}
+
free(locators);
}
@@ -231,13 +239,19 @@ vtep_lswitch_run(struct shash *vtep_pbs, struct sset
*vtep_pswitches,
VLOG_DBG("set vtep logical switch (%s) tunnel key from "
"(%"PRId64") to (%"PRId64")", vtep_ls->name,
vtep_ls->tunnel_key[0], tnl_key);
+vteprec_logical_switch_set_tunnel_key(vtep_ls, &tnl_key, 1);
}
-vteprec_logical_switch_set_tunnel_key(vtep_ls, &tnl_key, 1);
/* OVN is expected to always use source node replication mode,
* hence the replication mode is hard-coded for each logical
* switch in the context of ovn-controller-vtep. */
-vteprec_logical_switch_set_replication_mode(vtep_ls,
"source_node");
+if (!vtep_ls->replication_mode
+|| strcmp(vtep_ls->replication_mode, "source_n
