diff mbox series

[ovs-dev,v2,12/13] ofproto-dpif-xlate: Avoid allocating mf_subfield.

Message ID 20240711233238.1038670-13-amorenoz@redhat.com
State Superseded
Headers show
Series Introduce local sampling with NXAST_SAMPLE action. | expand

Checks

Context Check Description
ovsrobot/apply-robot warning apply and check: warning

Commit Message

Adrián Moreno July 11, 2024, 11:32 p.m. UTC 
"enum mf_subfield" (a 128byte object) is dynamically allocated a few
times just to set it to an all-ones mask.

Avoid dynamically allocating them by creating a static all-ones mask
similar to what was done with "exact_match_mask".

Suggested-by: Eelco Chaudron <echaudro@redhat.com>
Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 include/openvswitch/meta-flow.h |  3 +++
 lib/meta-flow.c                 |  2 ++
 ofproto/ofproto-dpif-xlate.c    | 16 +++++-----------
 3 files changed, 10 insertions(+), 11 deletions(-)

Comments

Eelco Chaudron July 12, 2024, 2:35 p.m. UTC | #1 
On 12 Jul 2024, at 1:32, Adrian Moreno wrote:

> "enum mf_subfield" (a 128byte object) is dynamically allocated a few
> times just to set it to an all-ones mask.
>
> Avoid dynamically allocating them by creating a static all-ones mask
> similar to what was done with "exact_match_mask".
>
> Suggested-by: Eelco Chaudron <echaudro@redhat.com>
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>

Changes look good to me! Thanks for making this change.

Acked-by: Eelco Chaudron <echaudro@redhat.com>

//Eelco
diff mbox series

Patch

diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h
index 3b0220aaa..ca422e697 100644
--- a/include/openvswitch/meta-flow.h
+++ b/include/openvswitch/meta-flow.h
@@ -2233,6 +2233,9 @@  union mf_subvalue {
 };
 BUILD_ASSERT_DECL(sizeof(union mf_value) == sizeof (union mf_subvalue));
 
+/* A const mf_subvalue with all bits initialized to ones. */
+extern const union mf_subvalue exact_sub_match_mask ;
+
 bool mf_subvalue_intersect(const union mf_subvalue *a_value,
                            const union mf_subvalue *a_mask,
                            const union mf_subvalue *b_value,
diff --git a/lib/meta-flow.c b/lib/meta-flow.c
index aa7cf1fcb..499be04b6 100644
--- a/lib/meta-flow.c
+++ b/lib/meta-flow.c
@@ -71,8 +71,10 @@  static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
 #define MF_VALUE_EXACT_64 MF_VALUE_EXACT_32, MF_VALUE_EXACT_32
 #define MF_VALUE_EXACT_128 MF_VALUE_EXACT_64, MF_VALUE_EXACT_64
 #define MF_VALUE_EXACT_INITIALIZER { .tun_metadata = { MF_VALUE_EXACT_128 } }
+#define MF_SUBVALUE_EXACT_INITIALIZER { .u8 = { MF_VALUE_EXACT_128 } }
 
 const union mf_value exact_match_mask = MF_VALUE_EXACT_INITIALIZER;
+const union mf_subvalue exact_sub_match_mask = MF_SUBVALUE_EXACT_INITIALIZER;
 
 static void nxm_init(void);
 
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 15e89e6a8..843ea2f79 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -5598,15 +5598,12 @@  xlate_output_reg_action(struct xlate_ctx *ctx,
 {
     uint64_t port = mf_get_subfield(&or->src, &ctx->xin->flow);
     if (port <= UINT16_MAX) {
-        union mf_subvalue *value = xmalloc(sizeof *value);
-
         xlate_report(ctx, OFT_DETAIL, "output port is %"PRIu64, port);
-        memset(value, 0xff, sizeof *value);
-        mf_write_subfield_flow(&or->src, value, &ctx->wc->masks);
+        mf_write_subfield_flow(&or->src, &exact_sub_match_mask,
+                               &ctx->wc->masks);
         xlate_output_action(ctx, u16_to_ofp(port), or->max_len,
                             false, is_last_action, false,
                             group_bucket_action);
-        free(value);
     } else {
         xlate_report(ctx, OFT_WARN, "output port %"PRIu64" is out of range",
                      port);
@@ -6574,9 +6571,6 @@  compose_conntrack_action(struct xlate_ctx *ctx, struct ofpact_conntrack *ofc,
 {
     uint16_t zone;
     if (ofc->zone_src.field) {
-        union mf_subvalue *value = xmalloc(sizeof *value);
-        memset(value, 0xff, sizeof *value);
-
         zone = mf_get_subfield(&ofc->zone_src, &ctx->xin->flow);
         if (ctx->xin->frozen_state) {
             /* If the upcall is a resume of a recirculation, we only need to
@@ -6585,13 +6579,13 @@  compose_conntrack_action(struct xlate_ctx *ctx, struct ofpact_conntrack *ofc,
              * which will invalidate the megaflow with old the recirc_id.
              */
             if (!mf_is_frozen_metadata(ofc->zone_src.field)) {
-                mf_write_subfield_flow(&ofc->zone_src, value,
+                mf_write_subfield_flow(&ofc->zone_src, &exact_sub_match_mask,
                                        &ctx->wc->masks);
             }
         } else {
-            mf_write_subfield_flow(&ofc->zone_src, value, &ctx->wc->masks);
+            mf_write_subfield_flow(&ofc->zone_src, &exact_sub_match_mask,
+                                   &ctx->wc->masks);
         }
-        free(value);
     } else {
         zone = ofc->zone_imm;
     }