[ovs-dev] [PATCH 7/9] tunneling: Add userspace tunnel support for Geneve.

[Jesse Gross]

On Tue, Apr 7, 2015 at 4:22 PM, Pravin Shelar <pshelar at nicira.com> wrote:
> On Mon, Mar 30, 2015 at 3:14 PM, Jesse Gross <jesse at nicira.com> wrote:
>> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
>> index 0c9f5a4..ef96862 100644
>> --- a/lib/netdev-vport.c
>> +++ b/lib/netdev-vport.c
>> @@ -61,6 +61,11 @@ static struct vlog_rate_limit err_rl = VLOG_RATE_LIMIT_INIT(60, 5);
>>                        sizeof(struct udp_header) +         \
>>                        sizeof(struct vxlanhdr))
>>
> ...
>
>> +
>> +    if (gnh->proto_type != htons(ETH_TYPE_TEB)) {
>> +        VLOG_WARN_RL(&err_rl, "unknown geneve encapsulated protocol: %#x\n",
>> +                     ntohs(gnh->proto_type));
>> +        reset_tnl_md(md);
>> +        return;
>> +    }
>> +
>> +    tnl->flags |= gnh->oam ? FLOW_TNL_F_OAM : 0;
>> +    tnl->tun_id = htonll(ntohl(get_16aligned_be32(&gnh->vni)) >> 8);
>> +
>
> We need to set FLOW_TNL_F_KEY. It is also missing for vxlan.

Fixed for both, thanks.

>> +    dp_packet_reset_packet(packet, hlen);
>> +}
>> +
>> +static int
>> +netdev_geneve_pop_header(struct netdev *netdev_ OVS_UNUSED,
> ....
>
>> +
>> +static int
>> +netdev_geneve_push_header(const struct netdev *netdev OVS_UNUSED,
>> +                          struct dp_packet **packets, int cnt,
>> +                          const struct ovs_action_push_tnl *data)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < cnt; i++) {
>> +        push_udp_header(packets[i], data->header, data->header_len);
>> +        packets[i]->md = PKT_METADATA_INITIALIZER(u32_to_odp(data->out_port));
>> +    }
>> +    return 0;
>> +}
>> +
> This looks like vxlan_push, Is there reason for having two different function?

Looking a little bigger, GRE also has pretty much the same function;
the only difference being that it calls a GRE function inside the
loop. Similarly all three pop functions are basically the same except
with a different inner function. I think we can avoid all of this by
making the netdev functions provide a loop for us, which is a little
less generic but is a little cleaner in practice. Any reason to not do
this?

>> +static void
>>  netdev_vport_range(struct unixctl_conn *conn, int argc,
>>                     const char *argv[], void *aux OVS_UNUSED)
>>  {
>> @@ -1331,7 +1441,9 @@ netdev_vport_tunnel_register(void)
>
> Otherwise looks good.
> Acked-by: Pravin B Shelar <pshelar at nicira.com>

Thanks for the reviews. I think I'll push this series with just the
FLOW_TNL_F_KEY change above and then send a follow up patch to
refactor the push/pop functions if you agree.