[ovs-dev] [PATCH v3 3/3] openvswitch: Userspace tunneling.

[Pravin Shelar]

On Tue, Nov 11, 2014 at 4:35 PM, Ben Pfaff <blp at nicira.com> wrote:
> On Mon, Nov 10, 2014 at 12:46:15PM -0800, Pravin B Shelar wrote:
>> Following patch adds support for userspace tunneling. Tunneling
>> needs three more component first is routing table which is configured by
>> caching kernel routes and second is ARP cache which build automatically
>> by snooping arp. And third is tunnel protocol table which list all
>> listening protocols which is populated by vswitchd as tunnel ports
>> are added. GRE and VXLAN protocol support is added in this patch.
>>
>> Tunneling works as follows:
>> On packet receive vswitchd check if this packet is targeted to tunnel
>> port. If it is then vswitchd inserts tunnel pop action which pops
>> header and sends packet to tunnel port.
>> On packet xmit rather than generating Set tunnel action it generate
>> tunnel push action which has tunnel header data. datapath can use
>> tunnel-push action data to generate header for each packet and
>> forward this packet to output port. Since tunnel-push action
>> contains most of packet header vswitchd needs to lookup routing
>> table and arp table to build this action.
>>
>> Signed-off-by: Pravin B Shelar <pshelar at nicira.com>
>> Acked-by: Jarno Rajahalme <jrajahalme at nicira.com>
>> -
>> Fixed according to comments from Jarno and Ben.
>> Added test cases.
>> Added documentation.
>
> I spotted a few more nits.  Thanks again for writing all this code.
>
> In parse_gre_header, this:
>     if (greh->flags & ~(htons(GRE_CSUM) | htons(GRE_KEY) | htons(GRE_SEQ))) {
> would be easier to read as:
>     if (greh->flags & ~htons(GRE_CSUM | GRE_KEY | GRE_SEQ)) {
>
ok.

>
> In vxlan_extract_md(), this log message should probably ntohl() the
> log arguments:
>         VLOG_WARN_RL(&err_rl, "invalid vxlan flags=%#x vni=%#x\n",
>                      get_16aligned_be32(&vxh->vx_flags),
>                      get_16aligned_be32(&vxh->vx_vni));
> Also similarly here in format_odp_tnl_push_header():
>         ds_put_format(ds, "vxlan(flags=0x%"PRIx32",vni=0x%"PRIx32")",
>                       get_16aligned_be32(&vxh->vx_flags),
>                       get_16aligned_be32(&vxh->vx_vni));
> and two other places later in the same function:
>             ds_put_format(ds, ",csum=0x%"PRIx32, get_16aligned_be32(options));
> ...
>             ds_put_format(ds, ",seq=0x%"PRIx32, get_16aligned_be32(options));
>
ok.

> In format_odp_action(), these calls can be combined into one:
>     case OVS_ACTION_ATTR_TUNNEL_POP:
>         ds_put_format(ds, "tnl_pop(%"PRIu32, nl_attr_get_u32(a));
>         ds_put_cstr(ds, ")");
>
ok.

> and actually it looks the first four ovs_scan_len() calls in
> ovs_parse_tnl_push() could be just one, but maybe you think it looks
> better as-is, which is fine with me.
>
I think it is easier to read code this way.

> Here in ovs_parse_tnl_push(), the doubled ( looks wrong:
>     } else if (ovs_scan_len(s, &n, "gre((flags=0x%"SCNx16",proto=0x%"SCNx16")",
> although I see other doubled parentheses around so maybe it's
> intentional?
>

Yes, the first patch is header part, following header part is optional.

> There's an extra blank line here in ofproto/ofproto-dpif.c:
>     bool
>     ovs_native_tunneling_is_on(struct ofproto_dpif *ofproto)
>     {
>
>         return ofproto_use_tnl_push_pop && ofproto->backer->enable_tnl_push_pop &&
>
ok. I will send patch with fixes.