diff mbox series

[ovs-dev,v8,1/2] ofproto-dpif-mirror: Reduce number of function parameters.

Message ID 20240429190358.869793-1-mkp@redhat.com
State Superseded, archived
Headers show
Series [ovs-dev,v8,1/2] ofproto-dpif-mirror: Reduce number of function parameters. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation fail test: fail

Commit Message

Mike Pattrick April 29, 2024, 7:03 p.m. UTC
Previously the mirror_set() and mirror_get() functions took a large
number of parameters, which was inefficient and difficult to read and
extend. This patch moves most of the parameters into a struct.

Signed-off-by: Mike Pattrick <mkp@redhat.com>
Acked-by: Simon Horman <horms@ovn.org>
Acked-by: Eelco Chaudron <echaudro@redhat.com>
Signed-off-by: Mike Pattrick <mkp@redhat.com>
---
 ofproto/ofproto-dpif-mirror.c | 60 ++++++++++++++++++-----------------
 ofproto/ofproto-dpif-mirror.h | 40 ++++++++++++++++++-----
 ofproto/ofproto-dpif-xlate.c  | 29 ++++++++---------
 ofproto/ofproto-dpif.c        | 23 +++++++-------
 4 files changed, 88 insertions(+), 64 deletions(-)
diff mbox series

Patch

diff --git a/ofproto/ofproto-dpif-mirror.c b/ofproto/ofproto-dpif-mirror.c
index 343b75f0e..4967ecc9a 100644
--- a/ofproto/ofproto-dpif-mirror.c
+++ b/ofproto/ofproto-dpif-mirror.c
@@ -207,19 +207,22 @@  mirror_bundle_dst(struct mbridge *mbridge, struct ofbundle *ofbundle)
 }
 
 int
-mirror_set(struct mbridge *mbridge, void *aux, const char *name,
-           struct ofbundle **srcs, size_t n_srcs,
-           struct ofbundle **dsts, size_t n_dsts,
-           unsigned long *src_vlans, struct ofbundle *out_bundle,
-           uint16_t snaplen,
-           uint16_t out_vlan)
+mirror_set(struct mbridge *mbridge, void *aux,
+           const struct ofproto_mirror_settings *ms,
+           const struct mirror_bundles *mb)
 {
     struct mbundle *mbundle, *out;
     mirror_mask_t mirror_bit;
     struct mirror *mirror;
     struct hmapx srcs_map;          /* Contains "struct ofbundle *"s. */
     struct hmapx dsts_map;          /* Contains "struct ofbundle *"s. */
+    uint16_t out_vlan;
 
+    if (!ms || !mbridge) {
+        return EINVAL;
+    }
+
+    out_vlan = ms->out_vlan;
     mirror = mirror_lookup(mbridge, aux);
     if (!mirror) {
         int idx;
@@ -227,7 +230,7 @@  mirror_set(struct mbridge *mbridge, void *aux, const char *name,
         idx = mirror_scan(mbridge);
         if (idx < 0) {
             VLOG_WARN("maximum of %d port mirrors reached, cannot create %s",
-                      MAX_MIRRORS, name);
+                      MAX_MIRRORS, ms->name);
             return EFBIG;
         }
 
@@ -242,8 +245,8 @@  mirror_set(struct mbridge *mbridge, void *aux, const char *name,
     unsigned long *vlans = ovsrcu_get(unsigned long *, &mirror->vlans);
 
     /* Get the new configuration. */
-    if (out_bundle) {
-        out = mbundle_lookup(mbridge, out_bundle);
+    if (mb->out_bundle) {
+        out = mbundle_lookup(mbridge, mb->out_bundle);
         if (!out) {
             mirror_destroy(mbridge, mirror->aux);
             return EINVAL;
@@ -252,16 +255,16 @@  mirror_set(struct mbridge *mbridge, void *aux, const char *name,
     } else {
         out = NULL;
     }
-    mbundle_lookup_multiple(mbridge, srcs, n_srcs, &srcs_map);
-    mbundle_lookup_multiple(mbridge, dsts, n_dsts, &dsts_map);
+    mbundle_lookup_multiple(mbridge, mb->srcs, mb->n_srcs, &srcs_map);
+    mbundle_lookup_multiple(mbridge, mb->dsts, mb->n_dsts, &dsts_map);
 
     /* If the configuration has not changed, do nothing. */
     if (hmapx_equals(&srcs_map, &mirror->srcs)
         && hmapx_equals(&dsts_map, &mirror->dsts)
-        && vlan_bitmap_equal(vlans, src_vlans)
+        && vlan_bitmap_equal(vlans, ms->src_vlans)
         && mirror->out == out
         && mirror->out_vlan == out_vlan
-        && mirror->snaplen == snaplen)
+        && mirror->snaplen == ms->snaplen)
     {
         hmapx_destroy(&srcs_map);
         hmapx_destroy(&dsts_map);
@@ -275,15 +278,15 @@  mirror_set(struct mbridge *mbridge, void *aux, const char *name,
     hmapx_swap(&dsts_map, &mirror->dsts);
     hmapx_destroy(&dsts_map);
 
-    if (vlans || src_vlans) {
+    if (vlans || ms->src_vlans) {
         ovsrcu_postpone(free, vlans);
-        vlans = vlan_bitmap_clone(src_vlans);
+        vlans = vlan_bitmap_clone(ms->src_vlans);
         ovsrcu_set(&mirror->vlans, vlans);
     }
 
     mirror->out = out;
     mirror->out_vlan = out_vlan;
-    mirror->snaplen = snaplen;
+    mirror->snaplen = ms->snaplen;
 
     /* Update mbundles. */
     mirror_bit = MIRROR_MASK_C(1) << mirror->idx;
@@ -406,23 +409,22 @@  mirror_update_stats(struct mbridge *mbridge, mirror_mask_t mirrors,
 /* Retrieves the mirror numbered 'index' in 'mbridge'.  Returns true if such a
  * mirror exists, false otherwise.
  *
- * If successful, '*vlans' receives the mirror's VLAN membership information,
+ * If successful 'mc->vlans' receives the mirror's VLAN membership information,
  * either a null pointer if the mirror includes all VLANs or a 4096-bit bitmap
  * in which a 1-bit indicates that the mirror includes a particular VLAN,
- * '*dup_mirrors' receives a bitmap of mirrors whose output duplicates mirror
- * 'index', '*out' receives the output ofbundle (if any), and '*out_vlan'
- * receives the output VLAN (if any).
+ * 'mc->dup_mirrors' receives a bitmap of mirrors whose output duplicates
+ * mirror 'index', 'mc->out' receives the output ofbundle (if any),
+ * and 'mc->out_vlan' receives the output VLAN (if any).
  *
  * Everything returned here is assumed to be RCU protected.
  */
 bool
-mirror_get(struct mbridge *mbridge, int index, const unsigned long **vlans,
-           mirror_mask_t *dup_mirrors, struct ofbundle **out,
-           int *snaplen, int *out_vlan)
+mirror_get(struct mbridge *mbridge, int index,
+           struct mirror_config *mc)
 {
     struct mirror *mirror;
 
-    if (!mbridge) {
+    if (!mc || !mbridge) {
         return false;
     }
 
@@ -433,11 +435,11 @@  mirror_get(struct mbridge *mbridge, int index, const unsigned long **vlans,
     /* Assume 'mirror' is RCU protected, i.e., it will not be freed until this
      * thread quiesces. */
 
-    *vlans = ovsrcu_get(unsigned long *, &mirror->vlans);
-    *dup_mirrors = mirror->dup_mirrors;
-    *out = mirror->out ? mirror->out->ofbundle : NULL;
-    *out_vlan = mirror->out_vlan;
-    *snaplen = mirror->snaplen;
+    mc->vlans = ovsrcu_get(unsigned long *, &mirror->vlans);
+    mc->dup_mirrors = mirror->dup_mirrors;
+    mc->out_bundle = mirror->out ? mirror->out->ofbundle : NULL;
+    mc->out_vlan = mirror->out_vlan;
+    mc->snaplen = mirror->snaplen;
     return true;
 }
 
diff --git a/ofproto/ofproto-dpif-mirror.h b/ofproto/ofproto-dpif-mirror.h
index eed63ec4a..37d57463c 100644
--- a/ofproto/ofproto-dpif-mirror.h
+++ b/ofproto/ofproto-dpif-mirror.h
@@ -22,9 +22,37 @@ 
 #define MAX_MIRRORS 32
 typedef uint32_t mirror_mask_t;
 
+struct ofproto_mirror_settings;
 struct ofproto_dpif;
 struct ofbundle;
 
+struct mirror_bundles {
+    struct ofbundle **srcs;
+    size_t n_srcs;
+
+    struct ofbundle **dsts;
+    size_t n_dsts;
+
+    struct ofbundle *out_bundle;
+};
+
+struct mirror_config {
+    /* A bitmap of mirrors that duplicate the current mirror. */
+    mirror_mask_t dup_mirrors;
+
+    /* VLANs of packets to select for mirroring. */
+    unsigned long *vlans;           /* vlan_bitmap, NULL selects all VLANs. */
+
+    /* Output (mutually exclusive). */
+    struct ofbundle *out_bundle;    /* A registered ofbundle handle or NULL. */
+    uint16_t out_vlan;              /* Output VLAN, not used if out_bundle is
+                                       set. */
+
+    /* Max size of a mirrored packet in bytes, if set to zero then no
+     * truncation will occur.  */
+    uint16_t snaplen;
+};
+
 /* The following functions are used by handler threads without any locking,
  * assuming RCU protection. */
 
@@ -38,9 +66,7 @@  mirror_mask_t mirror_bundle_dst(struct mbridge *, struct ofbundle *);
 
 void mirror_update_stats(struct mbridge*, mirror_mask_t, uint64_t packets,
                          uint64_t bytes);
-bool mirror_get(struct mbridge *, int index, const unsigned long **vlans,
-                mirror_mask_t *dup_mirrors, struct ofbundle **out,
-                int *snaplen, int *out_vlan);
+bool mirror_get(struct mbridge *, int index, struct mirror_config *);
 
 /* The remaining functions are assumed to be called by the main thread only. */
 
@@ -50,11 +76,9 @@  bool mbridge_need_revalidate(struct mbridge *);
 void mbridge_register_bundle(struct mbridge *, struct ofbundle *);
 void mbridge_unregister_bundle(struct mbridge *, struct ofbundle *);
 
-int mirror_set(struct mbridge *, void *aux, const char *name,
-               struct ofbundle **srcs, size_t n_srcs,
-               struct ofbundle **dsts, size_t n_dsts,
-               unsigned long *src_vlans, struct ofbundle *out_bundle,
-               uint16_t snaplen, uint16_t out_vlan);
+int mirror_set(struct mbridge *, void *aux,
+               const struct ofproto_mirror_settings *,
+               const struct mirror_bundles *);
 void mirror_destroy(struct mbridge *, void *aux);
 int mirror_get_stats(struct mbridge *, void *aux, uint64_t *packets,
                      uint64_t *bytes);
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 7c4950895..55846fe98 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2279,16 +2279,12 @@  mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle,
      * 'used_mirrors', as long as some candidates remain.  */
     mirror_mask_t used_mirrors = 0;
     while (mirrors) {
-        const unsigned long *vlans;
-        mirror_mask_t dup_mirrors;
-        struct ofbundle *out;
-        int out_vlan;
-        int snaplen;
+        struct mirror_config mc;
 
         /* Get the details of the mirror represented by the rightmost 1-bit. */
-        if (OVS_UNLIKELY(!mirror_get(xbridge->mbridge, raw_ctz(mirrors),
-                                     &vlans, &dup_mirrors,
-                                     &out, &snaplen, &out_vlan))) {
+        if (OVS_UNLIKELY(!mirror_get(xbridge->mbridge,
+                                     raw_ctz(mirrors),
+                                     &mc))) {
             /* The mirror got reconfigured before we got to read it's
              * configuration. */
             mirrors = zero_rightmost_1bit(mirrors);
@@ -2298,10 +2294,10 @@  mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle,
 
         /* If this mirror selects on the basis of VLAN, and it does not select
          * 'vlan', then discard this mirror and go on to the next one. */
-        if (vlans) {
+        if (mc.vlans) {
             ctx->wc->masks.vlans[0].tci |= htons(VLAN_CFI | VLAN_VID_MASK);
         }
-        if (vlans && !bitmap_is_set(vlans, xvlan.v[0].vid)) {
+        if (mc.vlans && !bitmap_is_set(mc.vlans, xvlan.v[0].vid)) {
             mirrors = zero_rightmost_1bit(mirrors);
             continue;
         }
@@ -2313,21 +2309,22 @@  mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle,
          * destination, so that we don't mirror to them again.  This must be
          * done now to ensure that output_normal(), below, doesn't recursively
          * output to the same mirrors. */
-        ctx->mirrors |= dup_mirrors;
-        ctx->mirror_snaplen = snaplen;
+        ctx->mirrors |= mc.dup_mirrors;
+        ctx->mirror_snaplen = mc.snaplen;
 
         /* Send the packet to the mirror. */
-        if (out) {
-            struct xbundle *out_xbundle = xbundle_lookup(ctx->xcfg, out);
+        if (mc.out_bundle) {
+            struct xbundle *out_xbundle = xbundle_lookup(ctx->xcfg,
+                                                         mc.out_bundle);
             if (out_xbundle) {
                 output_normal(ctx, out_xbundle, &xvlan);
             }
-        } else if (xvlan.v[0].vid != out_vlan
+        } else if (xvlan.v[0].vid != mc.out_vlan
                    && !eth_addr_is_reserved(ctx->xin->flow.dl_dst)) {
             struct xbundle *xb;
             uint16_t old_vid = xvlan.v[0].vid;
 
-            xvlan.v[0].vid = out_vlan;
+            xvlan.v[0].vid = mc.out_vlan;
             LIST_FOR_EACH (xb, list_node, &xbridge->xbundles) {
                 if (xbundle_includes_vlan(xb, &xvlan)
                     && !xbundle_mirror_out(xbridge, xb)) {
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 32d037be6..6f5830e40 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -3730,7 +3730,7 @@  mirror_set__(struct ofproto *ofproto_, void *aux,
              const struct ofproto_mirror_settings *s)
 {
     struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
-    struct ofbundle **srcs, **dsts;
+    struct mirror_bundles mb;
     int error;
     size_t i;
 
@@ -3739,23 +3739,24 @@  mirror_set__(struct ofproto *ofproto_, void *aux,
         return 0;
     }
 
-    srcs = xmalloc(s->n_srcs * sizeof *srcs);
-    dsts = xmalloc(s->n_dsts * sizeof *dsts);
+    mb.srcs = xmalloc(s->n_srcs * sizeof *mb.srcs);
+    mb.dsts = xmalloc(s->n_dsts * sizeof *mb.dsts);
 
     for (i = 0; i < s->n_srcs; i++) {
-        srcs[i] = bundle_lookup(ofproto, s->srcs[i]);
+        mb.srcs[i] = bundle_lookup(ofproto, s->srcs[i]);
     }
 
     for (i = 0; i < s->n_dsts; i++) {
-        dsts[i] = bundle_lookup(ofproto, s->dsts[i]);
+        mb.dsts[i] = bundle_lookup(ofproto, s->dsts[i]);
     }
 
-    error = mirror_set(ofproto->mbridge, aux, s->name, srcs, s->n_srcs, dsts,
-                       s->n_dsts, s->src_vlans,
-                       bundle_lookup(ofproto, s->out_bundle),
-                       s->snaplen, s->out_vlan);
-    free(srcs);
-    free(dsts);
+    mb.n_srcs = s->n_srcs;
+    mb.n_dsts = s->n_dsts;
+    mb.out_bundle = bundle_lookup(ofproto, s->out_bundle);
+
+    error = mirror_set(ofproto->mbridge, aux, s, &mb);
+    free(mb.srcs);
+    free(mb.dsts);
     return error;
 }