ofproto-dpif-mirror: Fix insane waste of memory and time in checking VLANs.

When a mirror was restricted to particular VLANs, this code was allocating,
copying, and freeing a 512-byte block of memory just to check the value of
a single bit in the block.  This fixes the problem.

Found by inspection.

Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Andy Zhou <azhou@nicira.com>

diff --git a/ofproto/ofproto-dpif-mirror.c b/ofproto/ofproto-dpif-mirror.c
index e0f3dcd..f3ff578 100644 (file)
--- a/ofproto/ofproto-dpif-mirror.c
+++ b/ofproto/ofproto-dpif-mirror.c
@@ -1,4 +1,4 @@
-/* Copyright (c) 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
+/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2015 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -375,11 +375,17 @@ mirror_update_stats(struct mbridge *mbridge, mirror_mask_t mirrors,
     }
 }
 
-/* Retrieves the mirror in 'mbridge' represented by the first bet set of
- * 'mirrors'.  Returns true if such a mirror exists, false otherwise.
- * The caller takes ownership of, and is expected to deallocate, 'vlans' */
+/* 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,
+ * 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). */
 bool
-mirror_get(struct mbridge *mbridge, int index, unsigned long **vlans,
+mirror_get(struct mbridge *mbridge, int index, const unsigned long **vlans,
            mirror_mask_t *dup_mirrors, struct ofbundle **out, int *out_vlan)
 {
     struct mirror *mirror;
@@ -393,7 +399,7 @@ mirror_get(struct mbridge *mbridge, int index, unsigned long **vlans,
         return false;
     }
 
-    *vlans = vlan_bitmap_clone(mirror->vlans);
+    *vlans = mirror->vlans;
     *dup_mirrors = mirror->dup_mirrors;
     *out = mirror->out ? mirror->out->ofbundle : NULL;
     *out_vlan = mirror->out_vlan;

diff --git a/ofproto/ofproto-dpif-mirror.h b/ofproto/ofproto-dpif-mirror.h
index 64c4561..6e0dc88 100644 (file)
--- a/ofproto/ofproto-dpif-mirror.h
+++ b/ofproto/ofproto-dpif-mirror.h
@@ -1,4 +1,4 @@
-/* Copyright (c) 2013 Nicira, Inc.
+/* Copyright (c) 2013, 2015 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -12,8 +12,8 @@
  * See the License for the specific language governing permissions and
  * limitations under the License. */
 
-#ifndef OFPROT_DPIF_MIRROR_H
-#define OFPROT_DPIF_MIRROR_H 1
+#ifndef OFPROTO_DPIF_MIRROR_H
+#define OFPROTO_DPIF_MIRROR_H 1
 
 #include <stdint.h>
 
@@ -48,7 +48,7 @@ int mirror_get_stats(struct mbridge *, void *aux, uint64_t *packets,
                      uint64_t *bytes);
 void mirror_update_stats(struct mbridge*, mirror_mask_t, uint64_t packets,
                          uint64_t bytes);
-bool mirror_get(struct mbridge *, int index, unsigned long **vlans,
+bool mirror_get(struct mbridge *, int index, const unsigned long **vlans,
                 mirror_mask_t *dup_mirrors, struct ofbundle **out,
                 int *out_vlan);
 

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index a002bda..2390ab2 100644 (file)
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -1559,7 +1559,7 @@ add_mirror_actions(struct xlate_ctx *ctx, const struct flow *orig_flow)
     while (mirrors) {
         mirror_mask_t dup_mirrors;
         struct ofbundle *out;
-        unsigned long *vlans;
+        const unsigned long *vlans;
         bool vlan_mirrored;
         bool has_mirror;
         int out_vlan;
@@ -1572,7 +1572,6 @@ add_mirror_actions(struct xlate_ctx *ctx, const struct flow *orig_flow)
             ctx->xout->wc.masks.vlan_tci |= htons(VLAN_CFI | VLAN_VID_MASK);
         }
         vlan_mirrored = !vlans || bitmap_is_set(vlans, vlan);
-        free(vlans);
 
         if (!vlan_mirrored) {
             mirrors = zero_rightmost_1bit(mirrors);