[ovs-dev] [PATCH v2 2/2] Set datapath mask bits when setting a flow field.

[Ben Pfaff]

On Tue, Oct 15, 2013 at 10:23:02AM -0700, Jarno Rajahalme wrote:
> 
> On Oct 15, 2013, at 9:52 AM, Ben Pfaff <blp at nicira.com> wrote:
> 
> > On Tue, Oct 15, 2013 at 09:19:52AM -0700, Jarno Rajahalme wrote:
> >> Since at the datapath interface we do not have set actions for
> >> individual fields, but larger sets of fields for a given protocol
> >> layer, the set action will in practice only ever apply to exactly
> >> matched flows for the given protocol layer.  For example, if the
> >> reg_load changes the IP TTL, the corresponding datapath action will
> >> rewrite also the IP addresses and TOS byte.  Since these other field
> >> values may not be explicity set, they depend on the incoming flow field
> > 
> > "explicitly"
> > 
> 
> Thanks.
> 
> >> values, and are hence all of them are set in the wildcards masks, when
> >> the action is committed to the datapath.  For the rare case, where the
> >> reg_load action does not actually change the value, and no other flow
> >> field values are set (or loaded), the datapath action is skipped, and
> >> no mask bits are set.  Such a datapath flow should, however, be
> >> dependent on the specific field value, so the corresponding wildcard
> >> mask bits must be set, lest the datapath flow be applied to packets
> >> containing some other value in the field and the field value remain
> >> unchanged regardless of the incoming value.
> >> 
> >> Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
> > 
> > I understand why this adds mf_force_prereqs(), but I don't understand
> > why it removes the mf_write_subfield_flow() call from
> > nxm_execute_reg_move().  Can you explain?
> > 
> 
> I placed the equivalent mf_set_flow_value() call in the
> mf_force_prereqs(), which I now see should be renamed.

Oh.  I'm really not paying attention here, am I?

I keep thinking that mf_force_prereqs() has the effect that I originally
meant it to have, which is different from the one that you defined it to
have.  For my own sanity, renaming it might be a good idea.

> Note that it is equivalent in the sense that masking only some bits
> vs. all the bits of the field usually makes no difference, as the ODP
> commit will anyway set them all if there is an actual change, and for
> the (rare?) case where the set/load did not change anything, it is
> better not to fragment the datapath mega-flow masks into too many
> different kinds of masks by masking the sub-field bits only.
> 
> Maybe I got too carried away noticing that there was a prototype for
> mf_force_prereqs() that was actually needed but not implemented.. How
> about mf_mask_field_and_prereqs() for a better name?

Yes, that's better.

We should probably remove the unimplemented prototype.