Re: [PATCH 2/6] vxlan: Group Policy extension

On Tue, Jan 13, 2015 at 3:32 AM, Thomas Graf [off-list ref] wrote:

On 01/12/15 at 06:28pm, Tom Herbert wrote:

quoted

On Mon, Jan 12, 2015 at 5:03 PM, Thomas Graf [off-list ref] wrote:

quoted

quoted

Creating a level of indirection for extensions seems overly
complicated to me. Why not just define IFLA_VXLAN_GBP as just another
enum above?

I think it's cleaner to group them in a nested attribute.
It clearly separates the optional extensions from the base
attributes. RCO, GPE, GBP can all live in there.

This is inconsistent with similar things in GRE and GUE. For instance,
GRE keyid is set as its own attribute. It just seems like this adding
more code to the driver than is necessary for the functionality
needed.

The major difference here is that we have to consider backwards
compatibility specifically for VXLAN. Your initial feedback on GPE
actually led me to how I implemented GBP.

I think the axioms we want to establish are as follows:
 1. Extensions need to be explicitly enabled by the user. A previously
    dropped frame should only be processed if the user explitly asks
    for it.
 2. As a consequence: only share a VLXAN UDP port if the enabled
    extensions match (vxlan_sock_add), e.g. user A might want RCO
    but user B might be unaware. They cannot share the same UDP port.

The 2nd lead me to introduce the 'exts' member to vxlan_sock so we can
compare it in vxlan_find_sock() and only share a UDP port if the
enabled extensions match.

RCO is represented in the socket in VXLAN flags (VLXAN_F_*). My patch
also adds a flags to vxlan_sock which contains the VLXAN flags. For
shared port, I suspect all the receive features must match, including
receive checksum settings for instance, but we don't care about
transmit side. To facilitate this, I would suggest splitting flags
into o_flags and i_flags like ip_tunnel does, and then compare i_flags
in vxlan_find_sock.

Regardless of the internal implementation, I still don't see much
value in exposing these distinctions in netlink.

Tom

Your patch currently implements (1) but not (2).