On Feb 20, 2013, at 1:56 PM, Kyle Mestery (kmestery) <kmest...@cisco.com> wrote:
> On Feb 20, 2013, at 12:44 PM, Jesse Gross <je...@nicira.com> wrote:
>> Here are a couple of small comments that I'd already written. I
>> haven't gone through the main part of the patch yet but I figured that
>> I might as well send them if you are going to respin the patch.
>>
>> On Tue, Feb 19, 2013 at 5:35 PM, Pravin B Shelar <pshe...@nicira.com> wrote:
>>> diff --git a/datapath/actions.c b/datapath/actions.c
>>> index f638ffc..cf44de3 100644
>>> --- a/datapath/actions.c
>>> +++ b/datapath/actions.c
>>> @@ -439,22 +439,6 @@ static int execute_set_action(struct sk_buff *skb,
>>> skb_set_mark(skb, nla_get_u32(nested_attr));
>>> break;
>>>
>>> - case OVS_KEY_ATTR_TUN_ID:
>>> - /* If we're only using the TUN_ID action, store the value
>>> in a
>>> - * temporary instance of struct ovs_key_ipv4_tunnel on the
>>> stack.
>>> - * If both IPV4_TUNNEL and TUN_ID are being used together we
>>> - * can't write into the IPV4_TUNNEL action, so make a copy
>>> and
>>> - * write into that version.
>>> - */
>>> - if (!OVS_CB(skb)->tun_key)
>>> - memset(tun_key, 0, sizeof(*tun_key));
>>> - else if (OVS_CB(skb)->tun_key != tun_key)
>>> - memcpy(tun_key, OVS_CB(skb)->tun_key,
>>> sizeof(*tun_key));
>>> - OVS_CB(skb)->tun_key = tun_key;
>>> -
>>> - OVS_CB(skb)->tun_key->tun_id = nla_get_be64(nested_attr);
>>> - break;
>>
>> I think we can further simplify this by removing the temporary tun_key
>> that we are keeping on the stack in ovs_execute_actions() and passing
>> around.
>>
>>> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
>>> index 7ee31a2..12e85b2 100644
>>> --- a/include/linux/openvswitch.h
>>> +++ b/include/linux/openvswitch.h
>>> @@ -242,6 +242,18 @@ enum {
>>>
>>> #define OVS_PATCH_ATTR_MAX (__OVS_PATCH_ATTR_MAX - 1)
>>>
>>> +/* OVS_VPORT_ATTR_OPTIONS attributes for tunnels.
>>> + *
>>> + * OVS_TUNNEL_ATTR_DST_PORT is useful for vxlan.
>>
>> I'm not sure that we need this comment about VXLAN here since it's
>> probably easier to just describe each element next to it.
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>
> Another thing I just noticed:
>
> I don't see where tnl_vport->dst_port is set when creating a VXLAN
> port. I see it set when setting tunnel options, but not during create.
> I think this explains why when I tested this patch with VXLAN I see
> the destination port being zero during transmit.
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
Something like this fixes this particular problem:
diff --git a/datapath/vport-vxlan.c b/datapath/vport-vxlan.c
index 598feee..4a3726c 100644
--- a/datapath/vport-vxlan.c
+++ b/datapath/vport-vxlan.c
@@ -336,6 +336,7 @@ static struct vport *vxlan_tnl_create(const struct
vport_parms *parms)
int err;
struct vport *vport;
struct vxlan_port *vxlan_port = NULL;
+ struct tnl_vport *tnl_vport;
struct ovs_net *ovs_net;
ovs_net = net_generic(ovs_dp_get_net(parms->dp), ovs_net_id);
@@ -347,8 +348,12 @@ static struct vport *vxlan_tnl_create(const struct
vport_parms *parms)
vport = ovs_tnl_create(parms, &ovs_vxlan_vport_ops, &ovs_vxlan_tnl_ops);
- if (IS_ERR(vport))
+ if (IS_ERR(vport)) {
vxlan_tunnel_release(vxlan_port);
+ } else {
+ tnl_vport = tnl_vport_priv(vport);
+ tnl_vport->dst_port = vxlan_port->port;
+ }
ovs_net->vport_net.vxlan_vport = vport;
return vport;
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
