On Thu, May 04, 2017 at 04:12:17PM -0700, Yi-Hung Wei wrote:
> This patch series add support to OpenFlow 1.5 packet-out message that
> enables setting pipeline fields. While there are six pipeline match
> fields as listed in OpenFlow spec 1.5.1, this patch series implements
> three of them (OXM_OF_IN_PORT, OXM_OF_METADATA, and OXM_OF_TUNNEL_ID),
> since the rest of them (OXM_OF_IN_PHY_PORT, OXM_OF_ACTSET_OUTPUT, and
> OXM_OF_PACKET_TYPE) does not fit into the ovs.
>
> Yi-Hung Wei (3):
> ofp-util: Add flow metadata to ofputil_packet_out
> ofproto: Add OpenFlow 1.5 packet-out support
> ofp-parse: Parse pipeline fields in OF1.5 packet-out
Thanks for working on this feature. I'm looking forward to being able
to say that OVS supports all the features that OpenFlow 1.5 requires. I
think that with this and extensible flow entry statistics (which we've
already seen a draft of from, I believe, a TCS developer) we'll be
there.
This is a high quality series. Thank you! I have only a few comments.
First, I see that this series represents metadata as a "struct match".
Certainly, this works, but I wonder about the alternatives. The most
obvious one is "struct flow", which has all the same features as struct
match, without the masks. To me, it looks like the masks in struct
match aren't even used, at least not consistently; for example, this
code in parse_ofp_packet_out_str__() initializes in_port without its
mask:
*po = (struct ofputil_packet_out) {
.buffer_id = UINT32_MAX,
.flow_metadata.flow.in_port.ofp_port = OFPP_CONTROLLER,
};
The other possible data structure for metadata is struct pkt_metadata,
which is designed specifically for metadata. However it's currently
used mostly in packet handling rather than in OpenFlow, so it would
probably be a second choice.
Second, this series seems to take the literal definition of "pipeline
fields" from OpenFlow, only allowing those fields actually mentioned in
OpenFlow to be used in "packet-out"s. But I think that we should
include OVS extension pipeline fields, too, such as the various fields
with tunnel information. I also think that the implementation misses
some fields that OpenFlow itself defines as pipeline fields, such as the
OpenFlow packet register pipeline fields.
Can you think through these issues and write up a v2?
Thanks,
Ben.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
