On Thu, Sep 06, 2012 at 08:41:10AM -0700, Ben Pfaff wrote:
> On Thu, Sep 06, 2012 at 04:14:30PM +0900, Simon Horman wrote:
> > 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 <[email protected]>
> > > > > >
> > > > > > 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 <[email protected]>
> > > > > > Signed-off-by: Simon Horman <[email protected]>
> > > > >
> > > > > 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".)
> >
> > 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.
>
> mf_subvalue has enough room for an ipv6 address.
Ok, I think I am a bit confused about mf_subvalue.
Would a modification along the lines of the following be in
keeping with its design?
diff --git a/lib/meta-flow.h b/lib/meta-flow.h
index 4938d9e..69e92ba 100644
--- a/lib/meta-flow.h
+++ b/lib/meta-flow.h
@@ -278,6 +278,11 @@ union mf_subvalue {
ovs_be16 be16[8];
ovs_be32 be32[4];
ovs_be64 be64[2];
+ struct {
+ uint8_t pad[2];
+ uint8_t mac[ETH_ADDR_LEN]; /* 6 bytes */
+ };
+ struct in6_addr ipv6;
};
BUILD_ASSERT_DECL(sizeof(union mf_value) == sizeof (union mf_subvalue));
> > 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.
>
> That's important, I can see that we need a function that does that.
>
> > * 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
>
> Patch 2/5 shoves a 64-bit value into the beginning of the mf_value, in
> the be64 member. mf_write_subfield_flow() uses mf_get_value() to copy
> a field into the correct member of an mf_value. Then bitwise_copy()
> copies from the end of the full 128 bits of the mf_value whose be64
> was initialized. I don't see how this can work.
Is the problem that the 64 bits that are initialised are to the left
rather than to the right?
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
