On 02/16/13 00:45, Jesse Gross wrote:
> On Fri, Feb 15, 2013 at 10:34 AM, Lori Jakab <[email protected]> wrote:
>> On 02/15/13 01:32, Jesse Gross wrote:
>>> On Wed, Feb 13, 2013 at 6:44 AM, Lorand Jakab <[email protected]> wrote:
>>>> diff --git a/datapath/vport-lisp.c b/datapath/vport-lisp.c
>>>> new file mode 100644
>>>> index 0000000..9ca851c
>>>> --- /dev/null
>>>> +++ b/datapath/vport-lisp.c
>>> [...]
>>>> +#define LISP_DST_PORT 4341 /* Well known UDP port for LISP data packets.
>>>> */
>>>
>>> Does it make sense to make this configurable as it is for VXLAN? I
>>> realize that unlike VXLAN, IANA has allocated a port for LISP.
>>> However, perhaps an analogous situation is whether one would want to
>>> bake the port number into an ASIC.
>>
>> Configurability would only be useful between OVS implementations, since
>> all other implementations to date that I know of have this IANA
>> allocated port number hardcoded. It helps network monitoring as well,
>> if LISP traffic is using the well known port only. The only benefit of
>> configurable UDP ports I can think of would be to avoid restrictive
>> firewalls (set it to port 53?). So personally I don't see value in
>> making this configurable, but I'm open to be convinced. ;)
>
> We don't have to make it configurable all the way to the user but it
> seems like exposing it to userspace makes sense given that we already
> have the infrastructure from VXLAN. In many ways, I view the kernel
> module as the equivalent of a forwarding ASIC in a physical switch,
> which is why I asked the question about whether it would be hardcoded
> in the ASIC. My feeling is that usually it would not be since L4
> ports definitely tend to be more fluid for all kinds of reasons (it
> the case that you mentioned, it's likely that your DNS server allows
> you to specify the port to listen on even though it is very well
> known).
OK, I will add support for this a-la-VXLAN.
>
>>>> +/* Compute source port for outgoing packet.
>>>> + * Currently we use the flow hash.
>>>> + */
>>>> +static u16 get_src_port(struct sk_buff *skb)
>>>> +{
>>>> + int low;
>>>> + int high;
>>>> + unsigned int range;
>>>> + u32 hash = OVS_CB(skb)->flow->hash;
>>>> +
>>>> + inet_get_local_port_range(&low, &high);
>>>> + range = (high - low) + 1;
>>>> + return (((u64) hash * range) >> 32) + low;
>>>> +}
>>>
>>> Since this is the same as VXLAN, it might be worth factoring it out to
>>> some common location.
>>
>> What would be the correct place for this? datapath/tunnel.c ? Or maybe
>> as a 'static inline' function in datapath/tunnel.h ? I will move it
>> there and remove it from the VXLAN vport code as well.
>
> Probably datapath/tunnel.h makes the most sense.
OK. There is one issue with this though. datapath/datapath.h includes
datapath/tunnel.h, but the OVS_CB macro, required by the the new
function, is defined in the former. I can either expand the macro in
place of OVS_CB in tunnel.h (a bit ugly) or move the #define from
datapath.h to tunnel.h. Which solution would you prefer?
>
>>>> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
>>>> index 7c6e3ab..edccf11 100644
>>>> --- a/include/linux/openvswitch.h
>>>> +++ b/include/linux/openvswitch.h
>>>> @@ -184,6 +184,7 @@ enum ovs_vport_type {
>>>> OVS_VPORT_TYPE_INTERNAL, /* network device implemented by datapath
>>>> */
>>>> OVS_VPORT_TYPE_GRE, /* GRE tunnel. */
>>>> OVS_VPORT_TYPE_VXLAN, /* VXLAN tunnel */
>>>> + OVS_VPORT_TYPE_LISP, /* LISP tunnel */
>>>
>>> Assuming that this continues to evolve to move natively support L3
>>> tunnels, I think it's going to be necessary to make some incompatible
>>> changes in the future. Given that, I would probably put the
>>> identifier at the top of the non-upstream range (i.e. 105).
>>
>> When you say incompatible changes, do you mean breaking existing user
>> scripts for setting up flows, etc.? I can only think of the MAC address
>> rewrite that we have to do now as hack, since we don't do ARP
>> resolution. That rule however would not break a future implementation,
>> and is really simple to remove by users. Are there other things you
>> foresee that would change?
>
> Well, currently OVS userspace will reject anything without an Ethernet
> header. My assumption is that having native L3 support would involve
> supporting this between userspace/kernel and then the LISP vport would
> generate that type of packet. This change would break compatibility
> since an old userspace would no longer be able to work with a new
> kernel even though it is requesting the same type of port.
Thanks for explaining, I see now.
>
> Mostly though, it just seems that there are too many pieces not yet
> designed to be feel confident that it won't break compatibility.
> Also, putting it in the non-upstream range doesn't preclude it from
> being upstreamed - it just means that when we are ready we'll move it,
> which will be a break in compatibility for mismatched userspace/kernel
> versions (which we only support for features that are upstreamed).
OK, good too know! ;)
>
>> If possible, we would like to see LISP as a candidate for upstreaming,
>> and will try our best to avoid backwards compatible changes.
>>
>> BTW, how does upstreaming work? Is there a branch, which gets pulled by
>> the netdev maintainers, when the kernel merge window opens? Or do you
>> sned a pull request from latest master during the window? Or they pull
>> point releases? How often do they pull changes? Every merge window?
>> Or upstreaming is not tied to the kernel merge window, because it goes
>> to the network subsystem tree first? I'm asking this to see if we can
>> at least do the design of the L3 work before the next upstream code
>> pull, and see how backward compatibility would be affected by the
>> planned changes.
>
> There's an upstream OVS tree here:
> http://git.kernel.org/?p=linux/kernel/git/jesse/openvswitch.git;a=summary
>
> It has to be maintained separately from the main OVS tree since the
> paths are different and the openvswitch.org version also has things
> like compatibility with different kernel versions that don't make
> sense in the context of the mainline kernel. Usually I just
> accumulate things going into master there and then send a pull request
> once per cycle.
>
> Tunneling is a little more complicated though because none of the
> infrastructure is there yet (Pravin is working on it right now).
> Until that happens, I can't send anything that depends on it upstream
> so it will just stay in the OVS repository. In any case though, I
> don't send things upstream unless there is a pretty solid plan for how
> to evolve them in a compatible way (which is why the tunneling stuff
> is going so much later - the previous way of doing things just wasn't
> extensible enough). So while I would also like to get this upstream,
> there isn't any danger of it being upstreamed before it's ready.
Thanks again for the detailed explanation.
I will submit v3 once I know how to best handle the OVS_CB macro.
-Lori
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
