On Tue, Nov 11, 2014 at 4:35 PM, Ben Pfaff <b...@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 <pshe...@nicira.com>
>> Acked-by: Jarno Rajahalme <jrajaha...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
