[ovs-dev] [PATCH v6 5/5] userspace: add vxlan gpe support to vport

Yang, Yi Y yi.y.yang at intel.com
Fri May 19 05:18:03 UTC 2017


Ben, vxlangpe.h is from Linux kernel header file, so style is Linux but not ovs, keeping these intact is to make sure there are same pieces in both userspace and kernel, so code is also almost same. We'll change it to ovs style per your comments. Zoltan, please let me know if you need any help from me.

-----Original Message-----
From: ovs-dev-bounces at openvswitch.org [mailto:ovs-dev-bounces at openvswitch.org] On Behalf Of Ben Pfaff
Sent: Friday, May 19, 2017 12:54 PM
To: Zoltán Balogh <zoltan.balogh at ericsson.com>
Cc: 'dev at openvswitch.org' <dev at openvswitch.org>
Subject: Re: [ovs-dev] [PATCH v6 5/5] userspace: add vxlan gpe support to vport

On Fri, May 12, 2017 at 11:07:46AM +0000, Zoltán Balogh wrote:
> From: Georg Schmuecking <georg.schmuecking at ericsson.com>
> 
> This patch is based on the "datapath: enable vxlangpe creation in compat mode"
> from Yi Yang. It introduces an extension option "gpe" to the vxlan 
> port in the netdev-dpdk datapath. Furthermore it introduces a new 
> protocol header file vxlangpe.h in which the vxlan gpe protocoll is 
> described. In the vxlan specific methods the different packet are introduced and handled.
> 
> Added VXLAN GPE tunnel push test.
> 
> Signed-off-by: Yi Yang <yi.y.yang at intel.com>
> Signed-off-by: Georg Schmuecking <georg.schmuecking at ericsson.com>

checkpatch says:

    ERROR: Inappropriate bracing around statement
    #138 FILE: lib/netdev-native-tnl.c:519:
            if (gpe->oam_flag)

    WARNING: Line has non-spaces leading whitespace
    #153 FILE: lib/netdev-native-tnl.c:534:
            }

    WARNING: Line length is >79-characters long
    #188 FILE: lib/netdev-native-tnl.c:578:
            put_16aligned_be32(&vxh->vx_vni, htonl(ntohll(params->flow->tunnel.tun_id) << 8));

    WARNING: Line length is >79-characters long
    #206 FILE: lib/netdev-native-tnl.c:596:
            put_16aligned_be32(&vxh->vx_vni, htonl(ntohll(params->flow->tunnel.tun_id) << 8));

In netdev_vxlan_pop_header(), I don't see anything that verifies that the packet is long enough when GPE is present.

The formatting of the switch statements look different from the usual OVS style, which is like this:

        switch (gpe->next_protocol) {
        case VXLAN_GPE_NP_IPV4:
            next_pt = PT_IPV4;
            break;
        case VXLAN_GPE_NP_IPV6:
            next_pt = PT_IPV6;
            break;
        case VXLAN_GPE_NP_ETHERNET:
            next_pt = PT_ETH;
            break;
        default:
            goto err;
        }

I suspect that struct vxlanhdr_gpe should use ovs_16aligned_be32 for the vx_vni member.

I doubt that these #defines are a good idea, in vxlangpe.h:

    #define u8 uint8_t
    #define u32 uint8_t
    #define __be32 ovs_be32

struct vxlanhdr in lib/packets.h is pretty different in philosophy from struct vxlanhdr_gpe in vxlangpe.h.  I'd suggest making the new structure more like the existing one; OVS doesn't really use bit-fields much and in my experience they do not make code much easier to deal with.

The new struct vxlan_metadata doesn't seem to be used anywhere and I wonder whether it should be included.

Thanks,

Ben.
_______________________________________________
dev mailing list
dev at openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list