[ovs-dev] [PATCH 2/5] lib/ofp-actions: make ofpact_reg_load.value mf_value from uint64_t
Simon Horman
horms at verge.net.au
Thu Sep 6 07:14:30 UTC 2012
On Tue, Sep 04, 2012 at 02:12:49PM -0700, Ben Pfaff wrote:
> On Tue, Sep 04, 2012 at 10:39:19AM +0900, Simon Horman wrote:
> > On Thu, Aug 30, 2012 at 09:29:27PM -0700, Ben Pfaff wrote:
> > > On Thu, Aug 30, 2012 at 10:40:24AM +0900, Simon Horman wrote:
> > > > From: Isaku Yamahata <yamahata at valinux.co.jp>
> > > >
> > > > set_field action needs more than 64 bits value unlike nx_reg_load action.
> > > > This is preparation for set_field action. mf_value will be used later.
> > > >
> > > > Signed-off-by: Isaku Yamahata <yamahata at valinux.co.jp>
> > > > Signed-off-by: Simon Horman <horms at verge.net.au>
> > >
> > > Presumably the goal is to make ofpact_reg_load able to set any subset of
> > > a field, not just a 64-bit subset. For this, it would seem more natural
> > > to use union mf_subvalue instead of union mf_value. Then
> > > nxm_execute_reg_load() would become a single call to
> > > mf_write_subfield(), rather than (as seen in patch 3/5) a conditional.
> >
> > I spoke with Yamahata-san and clarified the motivation for using mf_value.
> >
> > It was:
> > * To allow the storage of IPv6 addresses which require more than 64bits and;
> > * To allow use of portions of meta-flow.c
> >
> > Having looked over the code I'm not convinced that it aims to set
> > field subsets. Do you believe that should be an aim?
>
> Certainly NXAST_REG_LOAD can modify part of a field rather than having
> to set an entire field (which is what I mean by a "subset", in case it
> wasn't clear). That's what mf_subvalue represents: some part of a
> field. (Naturally, it can represent a whole field, too, since that's
> just a special case of "part of a field".)
Hi Ben,
I took a look at using union mf_subvalue instead of union mf_value
but the result wasn't pretty and I don't think it resolves the
problem that Yamahata-san was trying to address. That is having
sufficient space for ipv6 addresses or in other words entities
wider than 64bits.
Instead I would like to propose leaving this patch as is and rolling the
following incremental change to the subsequent patch which adds the
conditional to mf_write_subfield() ("lib/ofp-actions: helper functions for
of12 set-field action").
It adds mf_write_subfield_flow() which differs from mf_write_subfield()
in that:
* It acts on a struct flow rather than a struct rule.
* It acts on a union mf_value rather than a union mf_subfield as input.
It is constructed mostly of code moved from nxm_reg_load().
And it is used in nxm_execute_reg_load() to:
* Avoid the conditional you noticed and;
* Thus allow the possibility for OF set-field to work with sub-fields
index 77ee83c..a9b9e79 100644
--- a/lib/meta-flow.c
+++ b/lib/meta-flow.c
@@ -2156,6 +2156,21 @@ mf_format(const struct mf_field *mf,
}
}
+/* Makes subfield 'sf' within 'flow' exactly match the 'sf->n_bits'
+ * least-significant bits in 'x'.
+ */
+void
+mf_write_subfield_flow(const struct mf_subfield *sf,
+ const union mf_value *x, struct flow *flow)
+{
+ const struct mf_field *field = sf->field;
+ union mf_value value;
+
+ mf_get_value(field, flow, &value);
+ bitwise_copy(x, sizeof *x, 0, &value, field->n_bytes, sf->ofs, sf->n_bits);
+ mf_set_flow_value(sf->field, &value, flow);
+}
+
/* Makes subfield 'sf' within 'rule' exactly match the 'sf->n_bits'
* least-significant bits in 'x'.
*/
diff --git a/lib/meta-flow.h b/lib/meta-flow.h
index c5e31af..9d9150c 100644
--- a/lib/meta-flow.h
+++ b/lib/meta-flow.h
@@ -320,6 +320,8 @@ void mf_set_wild(const struct mf_field *, struct cls_rule *);
void mf_random_value(const struct mf_field *, union mf_value *value);
/* Subfields. */
+void mf_write_subfield_flow(const struct mf_subfield *sf,
+ const union mf_value *x, struct flow *flow);
void mf_write_subfield(const struct mf_subfield *, const union mf_subvalue *,
struct cls_rule *);
diff --git a/lib/nx-match.c b/lib/nx-match.c
index 10fa712..2113365 100644
--- a/lib/nx-match.c
+++ b/lib/nx-match.c
@@ -1218,24 +1218,15 @@ nxm_execute_reg_move(const struct ofpact_reg_move *move,
void
nxm_execute_reg_load(const struct ofpact_reg_load *load, struct flow *flow)
{
- if (load->ofpact.compat == OFPUTIL_OFPAT12_SET_FIELD) {
- mf_set_flow_value(load->dst.field, &load->value, flow);
- return;
- }
- nxm_reg_load(&load->dst, ntohll(load->value.be64), flow);
+ mf_write_subfield_flow(&load->dst, &load->value, flow);
}
void
nxm_reg_load(const struct mf_subfield *dst, uint64_t src_data,
struct flow *flow)
{
- union mf_value dst_value;
union mf_value src_value;
- mf_get_value(dst->field, flow, &dst_value);
src_value.be64 = htonll(src_data);
- bitwise_copy(&src_value, sizeof src_value.be64, 0,
- &dst_value, dst->field->n_bytes, dst->ofs,
- dst->n_bits);
- mf_set_flow_value(dst->field, &dst_value, flow);
+ mf_write_subfield_flow(dst, &src_value, flow);
}
More information about the dev
mailing list