diff mbox

[ovs-dev,v2,17/21] meta-flow: New functions mf_subfield_copy() and mf_subfield_swap().

Message ID 1470672872-19450-18-git-send-email-blp@ovn.org
State Superseded
Headers show

Commit Message

Ben Pfaff Aug. 8, 2016, 4:14 p.m. UTC 
The function nxm_execute_reg_move() was almost a general-purpose function
for manipulating subfields, except for its awkward interface that took a
struct ofpact_reg_move instead of a plain source and destination.  This
commit introduces a general-purpose function in meta-flow that corrects
this flaw, and updates the callers.  An upcoming commit will introduce a
new user of the function.

This commit also introduces a related function mf_subfield_swap() to swap
the contents of subfields.  An upcoming commit will introduce the first
user.

Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 include/openvswitch/meta-flow.h |  6 ++++
 lib/meta-flow.c                 | 74 +++++++++++++++++++++++++++++++++++++++++
 lib/nx-match.c                  | 31 -----------------
 lib/nx-match.h                  |  2 --
 ofproto/ofproto-dpif-xlate.c    |  3 +-
 5 files changed, 82 insertions(+), 34 deletions(-)

Comments

Ryan Moats Aug. 8, 2016, 5:29 p.m. UTC | #1 
"dev" <dev-bounces@openvswitch.org> wrote on 08/08/2016 11:14:28 AM:

> From: Ben Pfaff <blp@ovn.org>
> To: dev@openvswitch.org
> Cc: Ben Pfaff <blp@ovn.org>
> Date: 08/08/2016 11:17 AM
> Subject: [ovs-dev] [PATCH v2 17/21] meta-flow: New functions
> mf_subfield_copy() and mf_subfield_swap().
> Sent by: "dev" <dev-bounces@openvswitch.org>
>
> The function nxm_execute_reg_move() was almost a general-purpose function
> for manipulating subfields, except for its awkward interface that took a
> struct ofpact_reg_move instead of a plain source and destination.  This
> commit introduces a general-purpose function in meta-flow that corrects
> this flaw, and updates the callers.  An upcoming commit will introduce a
> new user of the function.
>
> This commit also introduces a related function mf_subfield_swap() to swap
> the contents of subfields.  An upcoming commit will introduce the first
> user.
>
> Signed-off-by: Ben Pfaff <blp@ovn.org>
> ---

I've acked up to patch set 12, but patch sets 13 through 16 didn't
make it to my mailbox...

After applying patch set 14, I'm getting the following error when trying
to build:

ovn/lib/actions.c: In function 'encode_CT_LB':
ovn/lib/actions.c:1043:28: error: format '%u' expects argument of type
  'unsigned int', but argument 3 has type 'size_t {aka long unsigned int}'
  [-Werror=format=/b]

... I'm thinking the arg in the string needs ao change
Ben Pfaff Aug. 8, 2016, 6:23 p.m. UTC | #2 
On Mon, Aug 08, 2016 at 12:29:42PM -0500, Ryan Moats wrote:
> "dev" <dev-bounces@openvswitch.org> wrote on 08/08/2016 11:14:28 AM:
> 
> > From: Ben Pfaff <blp@ovn.org>
> > To: dev@openvswitch.org
> > Cc: Ben Pfaff <blp@ovn.org>
> > Date: 08/08/2016 11:17 AM
> > Subject: [ovs-dev] [PATCH v2 17/21] meta-flow: New functions
> > mf_subfield_copy() and mf_subfield_swap().
> > Sent by: "dev" <dev-bounces@openvswitch.org>
> >
> > The function nxm_execute_reg_move() was almost a general-purpose function
> > for manipulating subfields, except for its awkward interface that took a
> > struct ofpact_reg_move instead of a plain source and destination.  This
> > commit introduces a general-purpose function in meta-flow that corrects
> > this flaw, and updates the callers.  An upcoming commit will introduce a
> > new user of the function.
> >
> > This commit also introduces a related function mf_subfield_swap() to swap
> > the contents of subfields.  An upcoming commit will introduce the first
> > user.
> >
> > Signed-off-by: Ben Pfaff <blp@ovn.org>
> > ---
> 
> I've acked up to patch set 12, but patch sets 13 through 16 didn't
> make it to my mailbox...
> 
> After applying patch set 14, I'm getting the following error when trying
> to build:
> 
> ovn/lib/actions.c: In function 'encode_CT_LB':
> ovn/lib/actions.c:1043:28: error: format '%u' expects argument of type
>   'unsigned int', but argument 3 has type 'size_t {aka long unsigned int}'
>   [-Werror=format=/b]
> 
> ... I'm thinking the arg in the string needs ao change

Oops.  I applied patches 1 through 12, fixed that up in #14, and posted
v3.

Thanks a lot for all the reviews!
diff mbox

Patch

diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h
index 76d915c..f209fc2 100644
--- a/include/openvswitch/meta-flow.h
+++ b/include/openvswitch/meta-flow.h
@@ -2116,6 +2116,12 @@  void mf_read_subfield(const struct mf_subfield *, const struct flow *,
                       union mf_subvalue *);
 uint64_t mf_get_subfield(const struct mf_subfield *, const struct flow *);
 
+void mf_subfield_copy(const struct mf_subfield *src,
+                      const struct mf_subfield *dst,
+                      struct flow *, struct flow_wildcards *);
+void mf_subfield_swap(const struct mf_subfield *,
+                      const struct mf_subfield *,
+                      struct flow *flow, struct flow_wildcards *);
 
 enum ofperr mf_check_src(const struct mf_subfield *, const struct flow *);
 enum ofperr mf_check_dst(const struct mf_subfield *, const struct flow *);
diff --git a/lib/meta-flow.c b/lib/meta-flow.c
index 2c89613..a7d3131 100644
--- a/lib/meta-flow.c
+++ b/lib/meta-flow.c
@@ -1962,6 +1962,80 @@  mf_check__(const struct mf_subfield *sf, const struct flow *flow,
     }
 }
 
+/* Sets all the bits in 'sf' to 1 within 'wc', if 'wc' is nonnull. */
+static void
+unwildcard_subfield(const struct mf_subfield *sf, struct flow_wildcards *wc)
+{
+    if (wc) {
+        union mf_value mask;
+
+        memset(&mask, 0, sizeof mask);
+        bitwise_one(&mask, sf->field->n_bytes, sf->ofs, sf->n_bits);
+        mf_mask_field_masked(sf->field, &mask, wc);
+    }
+}
+
+/* Copies 'src' into 'dst' within 'flow', and sets all the bits in 'src' and
+ * 'dst to 1s in 'wc', if 'wc' is nonnull.
+ *
+ * 'src' and 'dst may overlap. */
+void
+mf_subfield_copy(const struct mf_subfield *src,
+                 const struct mf_subfield *dst,
+                 struct flow *flow, struct flow_wildcards *wc)
+{
+    ovs_assert(src->n_bits == dst->n_bits);
+    if (mf_are_prereqs_ok(dst->field, flow, wc)
+        && mf_are_prereqs_ok(src->field, flow, wc)) {
+        unwildcard_subfield(src, wc);
+        unwildcard_subfield(dst, wc);
+
+        union mf_value src_value;
+        union mf_value dst_value;
+        mf_get_value(dst->field, flow, &dst_value);
+        mf_get_value(src->field, flow, &src_value);
+        bitwise_copy(&src_value, src->field->n_bytes, src->ofs,
+                     &dst_value, dst->field->n_bytes, dst->ofs,
+                     src->n_bits);
+        mf_set_flow_value(dst->field, &dst_value, flow);
+    }
+}
+
+/* Swaps the bits in 'src' and 'dst' within 'flow', and sets all the bits in
+ * 'src' and 'dst to 1s in 'wc', if 'wc' is nonnull.
+ *
+ * 'src' and 'dst may overlap. */
+void
+mf_subfield_swap(const struct mf_subfield *a,
+                 const struct mf_subfield *b,
+                 struct flow *flow, struct flow_wildcards *wc)
+{
+    ovs_assert(a->n_bits == b->n_bits);
+    if (mf_are_prereqs_ok(a->field, flow, wc)
+        && mf_are_prereqs_ok(b->field, flow, wc)) {
+        unwildcard_subfield(a, wc);
+        unwildcard_subfield(b, wc);
+
+        union mf_value a_value;
+        union mf_value b_value;
+        mf_get_value(a->field, flow, &a_value);
+        mf_get_value(b->field, flow, &b_value);
+        union mf_value b2_value = b_value;
+
+        /* Copy 'a' into 'b'. */
+        bitwise_copy(&a_value, a->field->n_bytes, a->ofs,
+                     &b_value, b->field->n_bytes, b->ofs,
+                     a->n_bits);
+        mf_set_flow_value(b->field, &b_value, flow);
+
+        /* Copy original 'b' into 'a'. */
+        bitwise_copy(&b2_value, b->field->n_bytes, b->ofs,
+                     &a_value, a->field->n_bytes, a->ofs,
+                     b->n_bits);
+        mf_set_flow_value(a->field, &a_value, flow);
+    }
+}
+
 /* Checks whether 'sf' is valid for reading a subfield out of 'flow'.  Returns
  * 0 if so, otherwise an OpenFlow error code (e.g. as returned by
  * ofp_mkerr()).  */
diff --git a/lib/nx-match.c b/lib/nx-match.c
index 89c6e3e..b03ccf2 100644
--- a/lib/nx-match.c
+++ b/lib/nx-match.c
@@ -1623,37 +1623,6 @@  nxm_reg_move_check(const struct ofpact_reg_move *move, const struct flow *flow)
 /* nxm_execute_reg_move(). */
 
 void
-nxm_execute_reg_move(const struct ofpact_reg_move *move,
-                     struct flow *flow, struct flow_wildcards *wc)
-{
-    /* Check that the fields exist. */
-    if (mf_are_prereqs_ok(move->dst.field, flow, wc)
-        && mf_are_prereqs_ok(move->src.field, flow, wc)) {
-        union mf_value src_value;
-        union mf_value dst_value;
-        union mf_value mask;
-
-        /* Should only mask the bits affected. */
-        memset(&mask, 0, sizeof mask);
-        bitwise_one(&mask, move->dst.field->n_bytes, move->dst.ofs,
-                    move->src.n_bits);
-        mf_mask_field_masked(move->dst.field, &mask, wc);
-
-        memset(&mask, 0, sizeof mask);
-        bitwise_one(&mask, move->src.field->n_bytes, move->src.ofs,
-                    move->src.n_bits);
-        mf_mask_field_masked(move->src.field, &mask, wc);
-
-        mf_get_value(move->dst.field, flow, &dst_value);
-        mf_get_value(move->src.field, flow, &src_value);
-        bitwise_copy(&src_value, move->src.field->n_bytes, move->src.ofs,
-                     &dst_value, move->dst.field->n_bytes, move->dst.ofs,
-                     move->src.n_bits);
-        mf_set_flow_value(move->dst.field, &dst_value, flow);
-    }
-}
-
-void
 nxm_reg_load(const struct mf_subfield *dst, uint64_t src_data,
              struct flow *flow, struct flow_wildcards *wc)
 {
diff --git a/lib/nx-match.h b/lib/nx-match.h
index 51d6414..c366a04 100644
--- a/lib/nx-match.h
+++ b/lib/nx-match.h
@@ -107,8 +107,6 @@  void nxm_format_reg_move(const struct ofpact_reg_move *, struct ds *);
 enum ofperr nxm_reg_move_check(const struct ofpact_reg_move *,
                                const struct flow *);
 
-void nxm_execute_reg_move(const struct ofpact_reg_move *, struct flow *,
-                          struct flow_wildcards *);
 void nxm_reg_load(const struct mf_subfield *, uint64_t src_data,
                   struct flow *, struct flow_wildcards *);
 
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 393854e..7b71c6a 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -4908,7 +4908,8 @@  do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
             break;
 
         case OFPACT_REG_MOVE:
-            nxm_execute_reg_move(ofpact_get_REG_MOVE(a), flow, wc);
+            mf_subfield_copy(&ofpact_get_REG_MOVE(a)->src,
+                             &ofpact_get_REG_MOVE(a)->dst, flow, wc);
             break;
 
         case OFPACT_SET_FIELD: