On Wed, Jul 13, 2016 at 7:56 PM, Yang, Yi <yi.y.y...@intel.com> wrote:
> On Wed, Jul 13, 2016 at 07:22:39PM -0700, Jesse Gross wrote:
>> >>
>> >> In any case, I don't think this is a fundamental issue, just a matter
>> >> of timing. Since the premise of the original question was that MD type
>> >> 2 shouldn't be too much additional work and the series is currently
>> >> dependent on Simon's patches, it seems like now might be a good time
>> >> for the authors to look into implementing this.
>> >
>> > Jesse, MD type 2 support needs a lot of changes, you know
>> > tunnel_metadata GENEVE is part of tunnel, but it should be part of NSH in
>> > NSH,
>> > you suggested we are to leverage GENEVE tunnel_metadata, but how to
>> > leverage it better is a big question mark for us. they should be called
>> > as nsh_metadata in NSH, so we can't directly use GENEVE's match fields
>> > and struct flow_tnl in struct flow, we find a better way to reuse Geneve
>> > tunnel_metadata code, that needs to generalize Geneve tunnel_metadata
>> > code in order that NSH and Geneve can share them but we can have
>> > different match fields name but can use a struct metadata in struct
>> > flow, what do you think about it?
>> >
>> > If you agree this, then I can confirm we will have a lot of changes,
>> > especially for GENEVE.
>>
>> It's possible to have aliases for field names in OVS, so that might be
>> one possible solution. However, you had the opportunity (and did!) to
>> rename these fields or otherwise make them more amenable to your needs
>> so I'm not really all that receptive to complaints in this area. In
>> any case, I don't think that it is really a good idea to have 2x 256
>> bytes in struct flow (which is an internal structure and can still be
>> changed) if that is what you are proposing.
>
> Currently, struct tun_metadata in struct flow_tnl.
>
> /* Tunnel information used in flow key and metadata. */
> struct flow_tnl {
> ovs_be32 ip_dst;
> struct in6_addr ipv6_dst;
> ovs_be32 ip_src;
> struct in6_addr ipv6_src;
> ovs_be64 tun_id;
> uint16_t flags;
> uint8_t ip_tos;
> uint8_t ip_ttl;
> ovs_be16 tp_src;
> ovs_be16 tp_dst;
> ovs_be16 gbp_id;
> uint8_t gbp_flags;
> uint8_t pad1[5]; /* Pad to 64 bits. */
> struct tun_metadata metadata;
> };
>
> But NSH isn't a tunnel, so NSH metadata shouldn't be here, does it make
> sense?
>
> We propose to move "struct tun_metadata metadata;" to struct flow, this
> won't increase size of struct flow. In this way, we can define a union
> in struct flow
>
> union {
> struct metadata tun_metadata;
> struct metadata nsh_metadata;
> } u;
>
> Geneve should operate flow.u.tun_metadata, NSH should operate
> flow.u.nsh_metadata, make sense?
I think that sounds fine as long as you are careful - one particular
thing to pay attention will be DPDK performance. DPDK tends to be very
sensitive to the size of the flow key and storing the metadata in the
tunnel area is part of an optimization to avoid initializing memory
when not necessary. We'll need to make sure that these changes don't
cause any regression in that area.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
