On Wed, May 16, 2012 at 01:16:17PM +1200, Joe Stringer wrote:
> Arbitrary ethernet mask support is one step on the way to support for OpenFlow
> 1.0+. This patch set seeks to add this capability without breaking current
> protocol support.
Thank you for doing this! I have some comments.
> This patch also includes some whitespace fixes -- ^L replaced with empty
> lines.
Oh, those are page breaks between major sections of code. I took the
idea from GNU coding style. (If you use Emacs, you can navigate
between these "pages" with C-x [ and C-x ].)
> Initial introduction of arbitrary ethernet masks to flow_wildcards. I've
> gone through the build asserts to increment FLOW_WC_SEQ, implementing
> arbitrary
> ethernet support in places I understand the code.
>
> Areas that I did not understand fully have /* ATTN */ markers with a relevant
> question. One overarching question I have is how ethernet matching should be
> handled, going forward. As I understand currently, FWW_DL_DST and
> FWW_ETH_MCAST
> bits are used in wildcards_t to indicate what kind of matching takes place.
> Will this continue to be the case?
You should delete FWW_DL_SRC, FWW_DL_DST, and FWW_ETH_MCAST, because
they are now redundant with dl_src_mask[] and dl_dst_mask[] and can
only cause confusion. The code that looked at these wildcards bits
should now examine dl_src_mask[] and dl_dst_mask[] directly.
The related functions flow_wildcards_to_dl_dst_mask(),
flow_wildcards_is_dl_dst_mask_valid(), and
flow_wildcards_set_dl_dst_mask() should presumably disappear.
Some other implications:
* flow_equal_except() should look at dl_dst_mask[] and
dl_src_mask[].
* flow_zero_wildcards() should use eth_addr_bitand().
* ofputil_wildcard_from_openflow() needs to set dl_dst_mask[]
instead of FWW_DL_DST.
* nx_put_match() needs to change.
* Multiple functions in lib/meta-flow.c will need to change.
The particular ones should become obvious when you delete
the FWW_* constants.
You need to update nx-match.h, changing
* NXM_OF_ETH_SRC 4 6 -- 10
to
* NXM_OF_ETH_SRC_W 4 6 6 16
and then updating the total too.
In ofputil_usable_protocols, you wrote:
+ /* ATTN: How do we express OFP11-supported features? Do we need to add
+ * anything here?
+ */
I don't think we need to do anything here yet, because we don't yet
have any code for OF1.1 flows. When we do, we'll have to consider it
(along with other issues).
This is the only ATTN marker I see.
> With OpenFlow 1.1, there is no equivalent ofp_flow_wildcards flags for
> ethernet
> masking; this is replaced by the eth_{src,dst}_mask. To support this fully,
> will we set the appropriate FWW_DL_DST/FWW_ETH_MCAST bits in the wildcards_t
> when translating from OF11+? Or do we change the OF10 code to set the
> appropriate masks, and migrate other code away from using these flags?
The latter.
> Although nx-match.c code uses the cls_rule struct (which now contains eth mask
> fields), I haven't modified the nxm_put_match() code to serialize these masks.
> I noted that it uses flow_wildcards_to_dl_dst_mask(wc) for the mask, so left
> it alone as I wasn't sure if it is meant to follow OF10 functionality?
NXM/OXM always writes out the full bitmap mask, even if only certain
combinations are allowed, so it needed to obtain the bitmap somehow,
and flow_wildcards_to_dl_dst_mask() was an appropriate way. Now that
the bitmap mask is in struct flow_wildcards, we can use that directly
in nx_put_match().
> All tests are satisfied, with no (known) changes to functionality, given that
> currently supported protocols lack support for arbitrary ethernet masks.
A few more things:
You should update the entries in mf_fields[] for dl_src and dl_dst in
lib/meta-flow.c, changing their maskability to MFM_FULLY. dl_dst was
the only use of MFM_MCAST, so you can now delete all references to
that throughout the tree.
It's likely to be useful in one or two places to add a function
cls_rule_set_dl_src_masked(), fitting the pattern we have elsewhere.
It would be customary to add some new partially wildcarded form of
NXM_OF_ETH_DST and NXM_OF_ETH_SRC to the "ovs-ofctl parse-nx-match"
test in tests/ovs-ofctl.at.
You should add a definition of NXM_OF_ETH_SRC_W to
include/openflow/nicira-ext.h, and expand the comment to explain that
support for arbitrary masking is new in OVS 1.8.
You should update the documentation for dl_src and dl_dst in
utilities/ovs-ofctl.8.in to explain that they are fully maskable, and
to explain that before OVS 1.8 they were not.
You should add an item to NEWS mentioning the new feature.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
