Hi Ben,
Thanks for your valuable review. Yes, it makes sense to use 'struct flow'
instead of 'struct match' to represent metadata.
As for the "pipeline fields", I briefly look at ovs-fields (7), and I think the
patch series should be update to include at least the following fields.
* Tunnel fields: (tun_src/dst, tun_ipv6_src/dst, tun_gbp_id,
tun_gbp_flags, tun_flags, tun_metadata0 - tun_metadata63)
* Register fields: (reg0 - reg15, xreg0 - xreg7, xxreg0 - xxreg3)
I will address these issues and send out a v2 soon.
Thanks,
-Yi-Hung
On Fri, May 5, 2017 at 9:39 PM, Ben Pfaff <b...@ovn.org> wrote:
> 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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
