Prior to this commit, an lflow_ref_node would be created the first time
that an lflow was added to an lflow_ref. If this lflow was added for a
single datapath, then the lflow_ref_node would reference that single
datapath. If the lflow was added for a datapath_group, then the
lflow_ref_node would reference all datapaths in the group.
If the same lflow were added to the same lflow_ref a second time, but
specified a different datapath than the first time the lflow was added,
then the lflow_ref_node remained unchanged, even though the lflow now
applied to one or more new datapaths.
In most cases, this is fine, because lflows are only added once per
lflow_ref. The IGMP lflow_ref, on the other hand, works differently.
There is only a single lflow_ref for all of IGMP. Therefore, it is
possible (probable, even) for the same lflow to be added multiple times
to the same lflow_ref, each time for a different datapath.
Most lflow input node handlers start by calling lflow_ref_unlink_lflows().
This removes all of the datapath references from the lflow_ref and sets all
lflow_ref_nodes within the lflow_ref to be unlinked. Then the handler re-builds
the relevant lflows, causing any corresponding lflow_ref_nodes to be re-linked,
based on the datapath the lflow was added for. Finally, the handler calls
lflow_ref_sync_lflows(). This causes all lflows in the lflow_ref to be
synced with the southbound Logical_Flow table. Any lflow_ref_nodes that were
unlinked and not re-linked will either have their lflow's corresponding
Logical_Flow datapath-related columns updated, or they will have their
record deleted entirely if the lflow no longer has any datapath references.
This particular workflow breaks in the case that the lflow was added
multiple times to the lflow_ref, each with a different datapath. The
lflow_ref_node is only aware of the first datapath for which the lflow
was added. So when the lflow_ref_node is unlinked, only the reference to
that particular datapath is removed from the lflow. Therefore, the lflow
may still have references to datapaths in its dpg_bitmap field that
should have been removed when the lflow_ref_node was unlinked. After
rebuilding the flows and re-syncing, those leftover datapaths will still
be in the lflow, resulting in the southbound Logical_Flow referencing
datapaths for which the flow should have been removed. This translates
to the datapath in question having the lflow installed when it should
have been removed.
The IGMP lflow handler gets around this by calling
lflow_ref_resync_flows() instead of lflow_ref_unlink_lflows() at the
beginning. This causes the lflow_ref_nodes to be unlinked and synced
immediately before attempting to rebuild the lflows. Since no
lflow_ref_nodes had a chance to be re-linked, it results in all of the
lflows in the IGMP lflow_ref being deleted (both in memory and in the
Southbound Logical_Flow table), then the lflows are rebuilt from scratch.
Since the lflows are re-created entirely, there is no possibility of stray
datapaths being referenced.
The next change in this series is going to separate lflow_ref
unlinking/rebuilding into one engine node and lflow_ref syncing into a
separate one. Because of that, the en-lflow IGMP input handler can no longer
call lflow_ref_resync_flows(). It needs to use lflow_ref_unlink_lflows() like
the other lflow input handlers do. But as stated in an earlier paragraph, this
causes stray datapaths to be referenced by the southbound Logical_Flow table.
This change modifies how lflow_ref_nodes handle their datapath
references. lflow_ref_nodes can now have their datapath references updated
when an lflow is added to an lflow_ref multiple times. To accomplish this, the
lflow_ref_node dgrp_bitmap and dp_index fields have been combined into a
bitmap field, dp_bitmap. Whether a single datapath or a datapath group is
referenced by the added lflow, we handle it the same in the lflow_ref_node, by
using the dp_bitmap.
Signed-off-by: Mark Michelson <mmich...@redhat.com>
---
northd/lflow-mgr.c | 79 +++++++++++++++++++---------------------------
1 file changed, 33 insertions(+), 46 deletions(-)
diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c
index 0f7b989cf..aa8e60478 100644
--- a/northd/lflow-mgr.c
+++ b/northd/lflow-mgr.c
@@ -558,13 +558,11 @@ struct lflow_ref_node {
/* Indicates whether the lflow was added with a dp_group using the
* ovn_lflow_add_with_dp_group() macro. */
bool dpgrp_lflow;
- /* dpgrp bitmap and bitmap length. Valid only of dpgrp_lflow is true. */
- unsigned long *dpgrp_bitmap;
- size_t dpgrp_bitmap_len;
-
- /* Index id of the datapath this lflow_ref_node belongs to.
- * Valid only if dpgrp_lflow is false. */
- size_t dp_index;
+ /* dp bitmap and bitmap length. Used to track all datapaths that
+ * the lflow references
+ */
+ unsigned long *dp_bitmap;
+ size_t dp_bitmap_len;
/* Indicates if the lflow_ref_node for an lflow - L(M, A) is linked
* to datapath(s) or not.
@@ -600,10 +598,11 @@ lflow_ref_destroy(struct lflow_ref *lflow_ref)
}
/* Unlinks the lflows referenced by the 'lflow_ref'.
- * For each lflow_ref_node (lrn) in the lflow_ref, it basically clears
- * the datapath id (lrn->dp_index) or all the datapath id bits in the
- * dp group bitmap (set when ovn_lflow_add_with_dp_group macro was used)
- * from the lrn->lflow's dpg bitmap
+ * For each lflow_ref_node (lrn) in the lflow_ref, it removes the
+ * datapath references in the lrn. If all references are removed
+ * for a particular datapath index, then the bit is cleared from
+ * the lflow's dpg_bitmap, indicating the lflow no longer applies
+ * to the corresponding datapath.
*/
void
lflow_ref_unlink_lflows(struct lflow_ref *lflow_ref)
@@ -611,21 +610,12 @@ lflow_ref_unlink_lflows(struct lflow_ref *lflow_ref)
struct lflow_ref_node *lrn;
HMAP_FOR_EACH (lrn, ref_node, &lflow_ref->lflow_ref_nodes) {
- if (lrn->dpgrp_lflow) {
- size_t index;
- BITMAP_FOR_EACH_1 (index, lrn->dpgrp_bitmap_len,
- lrn->dpgrp_bitmap) {
- if (dp_refcnt_release(&lrn->lflow->dp_refcnts_map, index)) {
- bitmap_set0(lrn->lflow->dpg_bitmap, index);
- }
- }
- } else {
- if (dp_refcnt_release(&lrn->lflow->dp_refcnts_map,
- lrn->dp_index)) {
- bitmap_set0(lrn->lflow->dpg_bitmap, lrn->dp_index);
+ size_t index;
+ BITMAP_FOR_EACH_1 (index, lrn->dp_bitmap_len, lrn->dp_bitmap) {
+ if (dp_refcnt_release(&lrn->lflow->dp_refcnts_map, index)) {
+ bitmap_set0(lrn->lflow->dpg_bitmap, index);
}
}
-
lrn->linked = false;
}
}
@@ -673,9 +663,10 @@ lflow_ref_sync_lflows(struct lflow_ref *lflow_ref,
*
* If 'lflow_ref' is not NULL then
* - it first checks if the lflow is present in the lflow_ref or not
- * - if present, then it does nothing
* - if not present, then it creates an lflow_ref_node object for
* the [L(M, A), dp index] and adds ito the lflow_ref hmap.
+ * - if present, then the lflow_ref_node object's dp_bitmap is
+ * updated to include the new datapath.
*
* Note that this function is not thread safe for 'lflow_ref'.
* If 2 or more threads calls this function for the same 'lflow_ref',
@@ -719,31 +710,27 @@ lflow_table_add_lflow(struct lflow_table *lflow_table,
lrn = xzalloc(sizeof *lrn);
lrn->lflow = lflow;
lrn->lflow_ref = lflow_ref;
- lrn->dpgrp_lflow = !od;
- if (lrn->dpgrp_lflow) {
- lrn->dpgrp_bitmap = bitmap_clone(dp_bitmap, dp_bitmap_len);
- lrn->dpgrp_bitmap_len = dp_bitmap_len;
- } else {
- lrn->dp_index = od->index;
- }
+ lrn->dp_bitmap_len = od ? ods_size(od->datapaths) : dp_bitmap_len;
+ lrn->dp_bitmap = bitmap_allocate(lrn->dp_bitmap_len);
ovs_list_insert(&lflow->referenced_by, &lrn->ref_list_node);
hmap_insert(&lflow_ref->lflow_ref_nodes, &lrn->ref_node, hash);
}
- if (!lrn->linked) {
- if (lrn->dpgrp_lflow) {
- ovs_assert(lrn->dpgrp_bitmap_len == dp_bitmap_len);
- size_t index;
- BITMAP_FOR_EACH_1 (index, dp_bitmap_len, dp_bitmap) {
- /* Allocate a reference counter only if already used. */
- if (bitmap_is_set(lflow->dpg_bitmap, index)) {
- dp_refcnt_use(&lflow->dp_refcnts_map, index);
- }
- }
- } else {
+ /* Add refcounts for any datapaths already being used by this
+ * lflow.
+ */
+ if (od) {
+ bitmap_set1(lrn->dp_bitmap, od->index);
+ if (bitmap_is_set(lflow->dpg_bitmap, od->index)) {
+ dp_refcnt_use(&lflow->dp_refcnts_map, od->index);
+ }
+ } else {
+ size_t index;
+ BITMAP_FOR_EACH_1 (index, dp_bitmap_len, dp_bitmap) {
+ bitmap_set1(lrn->dp_bitmap, index);
/* Allocate a reference counter only if already used. */
- if (bitmap_is_set(lflow->dpg_bitmap, lrn->dp_index)) {
- dp_refcnt_use(&lflow->dp_refcnts_map, lrn->dp_index);
+ if (bitmap_is_set(lflow->dpg_bitmap, index)) {
+ dp_refcnt_use(&lflow->dp_refcnts_map, index);
}
}
}
@@ -1434,7 +1421,7 @@ lflow_ref_node_destroy(struct lflow_ref_node *lrn)
hmap_remove(&lrn->lflow_ref->lflow_ref_nodes, &lrn->ref_node);
ovs_list_remove(&lrn->ref_list_node);
if (lrn->dpgrp_lflow) {
- bitmap_free(lrn->dpgrp_bitmap);
+ bitmap_free(lrn->dp_bitmap);
}
free(lrn);
}
--
2.47.0
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
