[ovs-dev] [ovs-dev v2] datapath-windows: Reset ct_mark/ct_label to support ALG

Alin Serdean aserdean at cloudbasesolutions.com
Wed Jul 22 19:47:34 UTC 2020


Thanks a lot for the patch.

In general please prefer to use RtlCopyMemory instead of memcpy: https://community.osr.com/discussion/242667/rtlcopymemory-vs-memcpy

Just a few more nits inlined.

Alin.

From: Jinjun Gao<mailto:jinjung at vmware.com>
Sent: Monday, July 20, 2020 6:00 PM
To: dev at openvswitch.org<mailto:dev at openvswitch.org>; Alin Serdean<mailto:aserdean at cloudbasesolutions.com>; kumaranand at vmware.com<mailto:kumaranand at vmware.com>
Cc: Jinjun Gao<mailto:jinjung at vmware.com>
Subject: [ovs-dev v2] datapath-windows: Reset ct_mark/ct_label to support ALG

The ct_mark/ct_label setting on related connection keep the same
behavior with Linux datapath. If one CT entry has parent/master entry,
its ct_mark and ct_label should inherit from the corresponding part
of parent/master entry at initialization.

Signed-off-by: Jinjun Gao <jinjung at vmware.com>
---
v2: Resolve Anand's comment to add braces for if-block.
---
 datapath-windows/ovsext/Conntrack.c | 88 +++++++++++++++++++++++--------------
 1 file changed, 56 insertions(+), 32 deletions(-)

diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c
index d065591..cda6d8b 100644
--- a/datapath-windows/ovsext/Conntrack.c
+++ b/datapath-windows/ovsext/Conntrack.c
@@ -789,60 +789,84 @@ OvsProcessConntrackEntry(OvsForwardingContext *fwdCtx,
 static __inline VOID
 OvsConntrackSetMark(OvsFlowKey *key,
                     POVS_CT_ENTRY entry,
-                    UINT32 value,
-                    UINT32 mask,
+                    MD_MARK *mark,
                     BOOLEAN *markChanged)
 {
-    UINT32 newMark;
-    newMark = value | (entry->mark & ~(mask));
-    if (entry->mark != newMark) {
+    POVS_CT_ENTRY parent = entry->parent;
+    BOOLEAN changed = FALSE;
+    UINT32 newMark = 0;
+
+    if (parent && parent->mark) {
+        newMark = parent->mark;
+        changed = TRUE;
+    } else if (mark) {
+        newMark = mark->value | (entry->mark & ~(mark->mask));
+        changed = TRUE;
+    }
+
+    if (changed && entry->mark != newMark) {
         entry->mark = newMark;
         key->ct.mark = newMark;
         *markChanged = TRUE;
     }
 }

+static __inline BOOLEAN
+OvsConntrackIsLabelsNonZero(const struct ovs_key_ct_labels *labels)
+{
+    UINT8 i;
+
+    for (i = 0; i < OVS_CT_LABELS_LEN_32; i++) {
+        if (labels->ct_labels_32[i]) {
+            return TRUE;
+        }
+    }
+
+    return FALSE;
+}
+
 static __inline void
 OvsConntrackSetLabels(OvsFlowKey *key,
                       POVS_CT_ENTRY entry,
-                      struct ovs_key_ct_labels *val,
-                      struct ovs_key_ct_labels *mask,
+                      MD_LABELS *labels,
                       BOOLEAN *labelChanged)
 {
-    ovs_u128 v, m, pktMdLabel = {0};
-    memcpy(&v, val, sizeof v);
-    memcpy(&m, mask, sizeof m);
-    memcpy(&pktMdLabel, &entry->labels, sizeof(struct ovs_key_ct_labels));
+    POVS_CT_ENTRY parent = entry->parent;

-    pktMdLabel.u64.lo = v.u64.lo | (pktMdLabel.u64.lo & ~(m.u64.lo));
-    pktMdLabel.u64.hi = v.u64.hi | (pktMdLabel.u64.hi & ~(m.u64.hi));
+    /* Inherit master's labels at labels initialization, if any. */
+    if (!OvsConntrackIsLabelsNonZero(&entry->labels) &&
+        parent && OvsConntrackIsLabelsNonZero(&parent->labels)) {
+        memcpy(&entry->labels, &parent->labels, OVS_CT_LABELS_LEN);
+        *labelChanged = TRUE;
+    }
+
+    /* Use the same computing method with Linux kernel datapath.
+     * It is more clean and easy understanding.
+     */
[Alin]Nit: You should drop the comment above. It is more useful to
say what you are actually computing.
+    if (labels && OvsConntrackIsLabelsNonZero(&labels->mask)) {
+        UINT8 i;
+        UINT32 * dst = entry->labels.ct_labels_32;
[Alin] Nit: Remove the ‘ ‘. UINT32 *dst
+        for (i = 0; i < OVS_CT_LABELS_LEN_32; i++) {
+            dst[i] = (dst[i] & ~(labels->mask.ct_labels_32[i])) |
+                     (labels->value.ct_labels_32[i] & labels->mask.ct_labels_32[i]);
+        }

-    if (!NdisEqualMemory(&entry->labels, &pktMdLabel,
-                         sizeof(struct ovs_key_ct_labels))) {
         *labelChanged = TRUE;
     }
-    NdisMoveMemory(&entry->labels, &pktMdLabel,
-                   sizeof(struct ovs_key_ct_labels));
-    NdisMoveMemory(&key->ct.labels, &pktMdLabel,
-                   sizeof(struct ovs_key_ct_labels));
+
+    /* Update flow key's ct labels */
+    NdisMoveMemory(&key->ct.labels, &entry->labels, OVS_CT_LABELS_LEN);
 }

 static void
 OvsCtSetMarkLabel(OvsFlowKey *key,
-                       POVS_CT_ENTRY entry,
-                       MD_MARK *mark,
-                       MD_LABELS *labels,
-                       BOOLEAN *triggerUpdateEvent)
+                  POVS_CT_ENTRY entry,
+                  MD_MARK *mark,
+                  MD_LABELS *labels,
+                  BOOLEAN *triggerUpdateEvent)
 {
-    if (mark) {
-        OvsConntrackSetMark(key, entry, mark->value, mark->mask,
-                            triggerUpdateEvent);
-    }
-
-    if (labels) {
-        OvsConntrackSetLabels(key, entry, &labels->value, &labels->mask,
-                              triggerUpdateEvent);
-    }
+    OvsConntrackSetMark(key, entry, mark, triggerUpdateEvent);
+    OvsConntrackSetLabels(key, entry, labels, triggerUpdateEvent);
 }

 /*
--
2.7.4



More information about the dev mailing list