diff mbox series

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

Message ID 20231004194050.387760-1-mkp@redhat.com
State Changes Requested
Headers show
Series [ovs-dev,v5,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 success test: success

Commit Message

Mike Pattrick Oct. 4, 2023, 7:40 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>
---
 ofproto/ofproto-dpif-mirror.c | 61 ++++++++++++++++++-----------------
 ofproto/ofproto-dpif-mirror.h | 30 ++++++++++++-----
 ofproto/ofproto-dpif-xlate.c  | 25 ++++++--------
 ofproto/ofproto-dpif.c        |  6 ++--
 4 files changed, 66 insertions(+), 56 deletions(-)

Comments

Simon Horman Oct. 5, 2023, 7:50 a.m. UTC | #1
On Wed, Oct 04, 2023 at 03:40:49PM -0400, Mike Pattrick wrote:
> 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>

Thanks for addressing this Mike, much appreciated.

Acked-by: Simon Horman <horms@ovn.org>
Eelco Chaudron Oct. 5, 2023, 2:04 p.m. UTC | #2
On 4 Oct 2023, at 21:40, Mike Pattrick wrote:

> 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>

Hi Mike,

Small nit below, if fixed, please add my ‘Acked-by: Eelco Chaudron <echaudro@redhat.com>’ in the next rev.

//Eelco

> ---
>  ofproto/ofproto-dpif-mirror.c | 61 ++++++++++++++++++-----------------
>  ofproto/ofproto-dpif-mirror.h | 30 ++++++++++++-----
>  ofproto/ofproto-dpif-xlate.c  | 25 ++++++--------
>  ofproto/ofproto-dpif.c        |  6 ++--
>  4 files changed, 66 insertions(+), 56 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-mirror.c b/ofproto/ofproto-dpif-mirror.c
> index 343b75f0e..17ba6f799 100644
> --- a/ofproto/ofproto-dpif-mirror.c
> +++ b/ofproto/ofproto-dpif-mirror.c
> @@ -207,19 +207,23 @@ 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, struct ofbundle **srcs,
> +           struct ofbundle **dsts, struct ofbundle *bundle)
> +
>  {
>      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 +231,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 +246,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 (bundle) {
> +        out = mbundle_lookup(mbridge, bundle);
>          if (!out) {
>              mirror_destroy(mbridge, mirror->aux);
>              return EINVAL;
> @@ -252,16 +256,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, srcs, ms->n_srcs, &srcs_map);
> +    mbundle_lookup_multiple(mbridge, dsts, ms->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 +279,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 +410,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 +436,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->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..b19b9e69f 100644
> --- a/ofproto/ofproto-dpif-mirror.h
> +++ b/ofproto/ofproto-dpif-mirror.h
> @@ -22,9 +22,25 @@
>  #define MAX_MIRRORS 32
>  typedef uint32_t mirror_mask_t;
>
> +struct ofproto_mirror_settings;
>  struct ofproto_dpif;
>  struct ofbundle;
>
> +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 *bundle;    /* A registered ofbundle handle or NULL. */
> +    uint16_t out_vlan;          /* Output VLAN, only if out_bundle is NULL. */
> +    uint16_t snaplen;           /* Max packet size of a mirrored packet
> +                                   in byte, set to 0 equals 65535. */

Missing s in byte(s).

In addition is the Set 0 equals 65535 true? Does it not mean the original size of the packet? Not sure if we support more than 64K in TSO or the datapath in general.

> +};
> +
> +
>  /* The following functions are used by handler threads without any locking,
>   * assuming RCU protection. */
>
> @@ -38,9 +54,8 @@ 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 *mbridge, int index,
> +                struct mirror_config *mc);
>
>  /* The remaining functions are assumed to be called by the main thread only. */
>
> @@ -50,11 +65,10 @@ 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 *mbridge, void *aux,
> +               const struct ofproto_mirror_settings *ms,
> +               struct ofbundle **srcs, struct ofbundle **dsts,
> +               struct ofbundle *bundle);
>  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 be4bd6657..3746edf06 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -2247,16 +2247,11 @@ 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))) {
> +                                     &mc))) {
>              /* The mirror got reconfigured before we got to read it's
>               * configuration. */
>              mirrors = zero_rightmost_1bit(mirrors);
> @@ -2266,10 +2261,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;
>          }
> @@ -2281,21 +2276,21 @@ 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.bundle) {
> +            struct xbundle *out_xbundle = xbundle_lookup(ctx->xcfg, mc.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 ba5706f6a..5a5a8ea86 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -3663,10 +3663,8 @@ mirror_set__(struct ofproto *ofproto_, void *aux,
>          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);
> +    error = mirror_set(ofproto->mbridge, aux, s, srcs, dsts,
> +                       bundle_lookup(ofproto, s->out_bundle));
>      free(srcs);
>      free(dsts);
>      return error;
> -- 
> 2.39.3
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff mbox series

Patch

diff --git a/ofproto/ofproto-dpif-mirror.c b/ofproto/ofproto-dpif-mirror.c
index 343b75f0e..17ba6f799 100644
--- a/ofproto/ofproto-dpif-mirror.c
+++ b/ofproto/ofproto-dpif-mirror.c
@@ -207,19 +207,23 @@  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, struct ofbundle **srcs,
+           struct ofbundle **dsts, struct ofbundle *bundle)
+
 {
     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 +231,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 +246,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 (bundle) {
+        out = mbundle_lookup(mbridge, bundle);
         if (!out) {
             mirror_destroy(mbridge, mirror->aux);
             return EINVAL;
@@ -252,16 +256,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, srcs, ms->n_srcs, &srcs_map);
+    mbundle_lookup_multiple(mbridge, dsts, ms->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 +279,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 +410,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 +436,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->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..b19b9e69f 100644
--- a/ofproto/ofproto-dpif-mirror.h
+++ b/ofproto/ofproto-dpif-mirror.h
@@ -22,9 +22,25 @@ 
 #define MAX_MIRRORS 32
 typedef uint32_t mirror_mask_t;
 
+struct ofproto_mirror_settings;
 struct ofproto_dpif;
 struct ofbundle;
 
+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 *bundle;    /* A registered ofbundle handle or NULL. */
+    uint16_t out_vlan;          /* Output VLAN, only if out_bundle is NULL. */
+    uint16_t snaplen;           /* Max packet size of a mirrored packet
+                                   in byte, set to 0 equals 65535. */
+};
+
+
 /* The following functions are used by handler threads without any locking,
  * assuming RCU protection. */
 
@@ -38,9 +54,8 @@  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 *mbridge, int index,
+                struct mirror_config *mc);
 
 /* The remaining functions are assumed to be called by the main thread only. */
 
@@ -50,11 +65,10 @@  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 *mbridge, void *aux,
+               const struct ofproto_mirror_settings *ms,
+               struct ofbundle **srcs, struct ofbundle **dsts,
+               struct ofbundle *bundle);
 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 be4bd6657..3746edf06 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2247,16 +2247,11 @@  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))) {
+                                     &mc))) {
             /* The mirror got reconfigured before we got to read it's
              * configuration. */
             mirrors = zero_rightmost_1bit(mirrors);
@@ -2266,10 +2261,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;
         }
@@ -2281,21 +2276,21 @@  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.bundle) {
+            struct xbundle *out_xbundle = xbundle_lookup(ctx->xcfg, mc.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 ba5706f6a..5a5a8ea86 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -3663,10 +3663,8 @@  mirror_set__(struct ofproto *ofproto_, void *aux,
         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);
+    error = mirror_set(ofproto->mbridge, aux, s, srcs, dsts,
+                       bundle_lookup(ofproto, s->out_bundle));
     free(srcs);
     free(dsts);
     return error;