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.
> 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.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
