[PATCH net-next] vxlan: Provide means for obtaining port information

[PATCH net-next] vxlan: Provide means for obtaining port information

[Alexander Duyck]

This patch provides a very simple means for device drivers to obtain access to
the VXLAN destination port number configuration.  This information is needed
in order to provide devices with a means of determining if a given UDP flow is
a VXLAN flow or if it is a standard flow for offload purposes.

The main disadvantage with this approach is that loading a driver that calls
this interface will mean that it will also load the vxlan module and any other
modules it depends on.  To that end I really hope at some point the value
becomes static as we could then avoid this extra overhead.  The alternative to
this though is significantly more complicated as it would likely require
registering notifiers to capture when the vxlan module is loaded and then
updating the value.

This patch adds a new header, if_vxlan.h which I suspect will slowly be
populated with more details such as the header layout as more devices end up
needing to figure out how to parse VXLAN for various offloads.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---

 drivers/net/vxlan.c      |    7 +++++++
 include/linux/if_vxlan.h |   28 ++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 0 deletions(-)
 create mode 100644 include/linux/if_vxlan.h

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 62a4438..5b3bb9a 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -898,6 +898,13 @@ static u16 vxlan_src_port(const struct vxlan_dev *vxlan, struct sk_buff *skb)
 	return (((u64) hash * range) >> 32) + vxlan->port_min;
 }
 
+/* simple accessor function for getting VXLAN destination port number */
+int vxlan_dst_port(void)
+{
+	return vxlan_port;
+}
+EXPORT_SYMBOL_GPL(vxlan_dst_port);
+
 static int handle_offloads(struct sk_buff *skb)
 {
 	if (skb_is_gso(skb)) {
diff --git a/include/linux/if_vxlan.h b/include/linux/if_vxlan.h
new file mode 100644
index 0000000..b997eac
--- /dev/null
+++ b/include/linux/if_vxlan.h
@@ -0,0 +1,28 @@
+/*
+ * VXLAN: Virtual eXtensible Local Area Network
+ *
+ * Copyright (c) 2013, Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc., 59 Temple
+ * Place - Suite 330, Boston, MA 02111-1307 USA.
+ *
+ */
+
+#if IS_ENABLED(CONFIG_VXLAN)
+extern int vxlan_dst_port(void);
+#else
+static inline int vxlan_dst_port(void)
+{
+	return 0;
+}
+#endif

Re: [PATCH net-next] vxlan: Provide means for obtaining port information

[Stephen Hemminger]

On Wed, 27 Mar 2013 14:33:11 -0700
Alexander Duyck <alexander.h.duyck@intel.com> wrote:

> This patch provides a very simple means for device drivers to obtain access to
> the VXLAN destination port number configuration.  This information is needed
> in order to provide devices with a means of determining if a given UDP flow is
> a VXLAN flow or if it is a standard flow for offload purposes.
> 
> The main disadvantage with this approach is that loading a driver that calls
> this interface will mean that it will also load the vxlan module and any other
> modules it depends on.  To that end I really hope at some point the value
> becomes static as we could then avoid this extra overhead.  The alternative to
> this though is significantly more complicated as it would likely require
> registering notifiers to capture when the vxlan module is loaded and then
> updating the value.
> 
> This patch adds a new header, if_vxlan.h which I suspect will slowly be
> populated with more details such as the header layout as more devices end up
> needing to figure out how to parse VXLAN for various offloads.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> ---
> 
>  drivers/net/vxlan.c      |    7 +++++++
>  include/linux/if_vxlan.h |   28 ++++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+), 0 deletions(-)
>  create mode 100644 include/linux/if_vxlan.h
> 
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index 62a4438..5b3bb9a 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -898,6 +898,13 @@ static u16 vxlan_src_port(const struct vxlan_dev *vxlan, struct sk_buff *skb)
>  	return (((u64) hash * range) >> 32) + vxlan->port_min;
>  }
>  
> +/* simple accessor function for getting VXLAN destination port number */
> +int vxlan_dst_port(void)
> +{
> +	return vxlan_port;
> +}
> +EXPORT_SYMBOL_GPL(vxlan_dst_port);
> +
>  static int handle_offloads(struct sk_buff *skb)
>  {
>  	if (skb_is_gso(skb)) {
> diff --git a/include/linux/if_vxlan.h b/include/linux/if_vxlan.h
> new file mode 100644
> index 0000000..b997eac
> --- /dev/null
> +++ b/include/linux/if_vxlan.h
> @@ -0,0 +1,28 @@
> +/*
> + * VXLAN: Virtual eXtensible Local Area Network
> + *
> + * Copyright (c) 2013, Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program; if not, write to the Free Software Foundation, Inc., 59 Temple
> + * Place - Suite 330, Boston, MA 02111-1307 USA.
> + *
> + */
> +
> +#if IS_ENABLED(CONFIG_VXLAN)
> +extern int vxlan_dst_port(void);
> +#else
> +static inline int vxlan_dst_port(void)
> +{
> +	return 0;
> +}
> +#endif

Won't this all break if we what Dave wants and allow listening on multiple ports.
The port should be a property of the vxlan device not global.

Re: [PATCH net-next] vxlan: Provide means for obtaining port information

[Alexander Duyck]

On 03/27/2013 02:38 PM, Stephen Hemminger wrote:
> On Wed, 27 Mar 2013 14:33:11 -0700
> Alexander Duyck <alexander.h.duyck@intel.com> wrote:
>
>> This patch provides a very simple means for device drivers to obtain access to
>> the VXLAN destination port number configuration.  This information is needed
>> in order to provide devices with a means of determining if a given UDP flow is
>> a VXLAN flow or if it is a standard flow for offload purposes.
>>
>> The main disadvantage with this approach is that loading a driver that calls
>> this interface will mean that it will also load the vxlan module and any other
>> modules it depends on.  To that end I really hope at some point the value
>> becomes static as we could then avoid this extra overhead.  The alternative to
>> this though is significantly more complicated as it would likely require
>> registering notifiers to capture when the vxlan module is loaded and then
>> updating the value.
>>
>> This patch adds a new header, if_vxlan.h which I suspect will slowly be
>> populated with more details such as the header layout as more devices end up
>> needing to figure out how to parse VXLAN for various offloads.
>>
>> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
>> ---
>>
>>  drivers/net/vxlan.c      |    7 +++++++
>>  include/linux/if_vxlan.h |   28 ++++++++++++++++++++++++++++
>>  2 files changed, 35 insertions(+), 0 deletions(-)
>>  create mode 100644 include/linux/if_vxlan.h
>>
>> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
>> index 62a4438..5b3bb9a 100644
>> --- a/drivers/net/vxlan.c
>> +++ b/drivers/net/vxlan.c
>> @@ -898,6 +898,13 @@ static u16 vxlan_src_port(const struct vxlan_dev *vxlan, struct sk_buff *skb)
>>  	return (((u64) hash * range) >> 32) + vxlan->port_min;
>>  }
>>  
>> +/* simple accessor function for getting VXLAN destination port number */
>> +int vxlan_dst_port(void)
>> +{
>> +	return vxlan_port;
>> +}
>> +EXPORT_SYMBOL_GPL(vxlan_dst_port);
>> +
>>  static int handle_offloads(struct sk_buff *skb)
>>  {
>>  	if (skb_is_gso(skb)) {
>> diff --git a/include/linux/if_vxlan.h b/include/linux/if_vxlan.h
>> new file mode 100644
>> index 0000000..b997eac
>> --- /dev/null
>> +++ b/include/linux/if_vxlan.h
>> @@ -0,0 +1,28 @@
>> +/*
>> + * VXLAN: Virtual eXtensible Local Area Network
>> + *
>> + * Copyright (c) 2013, Intel Corporation.
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>> + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * this program; if not, write to the Free Software Foundation, Inc., 59 Temple
>> + * Place - Suite 330, Boston, MA 02111-1307 USA.
>> + *
>> + */
>> +
>> +#if IS_ENABLED(CONFIG_VXLAN)
>> +extern int vxlan_dst_port(void);
>> +#else
>> +static inline int vxlan_dst_port(void)
>> +{
>> +	return 0;
>> +}
>> +#endif
> Won't this all break if we what Dave wants and allow listening on multiple ports.
> The port should be a property of the vxlan device not global.

Yes this will break if we ended up supporting a different port per
VXLAN.  The problem is a port per VXLAN is going to be much more
difficult to support for any sort of hardware offload as well.  What we
end up having to do is add a significant number of filters just to
identify all of the possible ports that may be used.  In addition it
will mean having to add some sort of notifier as I mentioned in the
patch description since we will have VXLANs going into and out of
existence, each one with their own port number..

This was essentially a "good enough for now" fix to address the fact
that I needed the port number for offload testing, but I will sit back
and wait for the port per VXLAN stuff to be pushed in before I take any
steps to try to sort that out.

Thanks,

Alex

Re: [PATCH net-next] vxlan: Provide means for obtaining port information

[David Stevens]

> From: Alexander Duyck <alexander.h.duyck@intel.com>
 
> Yes this will break if we ended up supporting a different port per
> VXLAN.  The problem is a port per VXLAN is going to be much more
> difficult to support for any sort of hardware offload as well.  What we
> end up having to do is add a significant number of filters just to
> identify all of the possible ports that may be used. 

        I assume this would only be an issue if you actually have lots
of ports. If you're using only one, or a couple, which I'd expect to
be the common case, your filtering needs should be the same, right?

> In addition it
> will mean having to add some sort of notifier as I mentioned in the
> patch description since we will have VXLANs going into and out of
> existence, each one with their own port number..

        I don't know the context in which you're planning to call this,
but regardless of the port or ports in use by VXLAN, the devices will
come and go and the module may be unloaded, so I don't see how what
you've said here applies to one case and not the other.


> This was essentially a "good enough for now" fix to address the fact
> that I needed the port number for offload testing, but I will sit back
> and wait for the port per VXLAN stuff to be pushed in before I take any
> steps to try to sort that out.

        What I was suggesting "for now" is that you create an interface
that need not change to support multiple ports. For example:

int vxlan_get_ports(struct net_device *dev, int *ports, int portcount)
{
        if (portcount <= 0 || vxlan_port == 0)
                return 0;
        if (ports)
                ports[0] = vxlan_port;
        return 1;
}

The return value would be the count of ports in use, and we'd fill up to a 
maximum
of portcount in the "ports" array. If you don't have a "dev" you're 
interested in,
pass it as NULL and in the future this code would match dev only if it is 
non-NULL;
otherwise it would go through the vxlan device list adding ports not in 
the list to it.

And for the right functionality now, change the VXLAN driver to leave 
vxlan_port
zero until a device is instantiated, so you can detect no ports at the 
moment,
and set it to zero again when the last one is deleted.

You need a mechanism to delete your filters if there are no VXLAN devices,
whether or not multiple ports are supported, because "0" is a legal count
now too. If I haven't created any vxlan devices, I can use port 8472 for
something else and your loading of the vxlan driver as a side-effect of
checking the port shouldn't prevent that. That's especially true in a
distro release where, while I might be able to change the port, I'd have
to burn one regardless if your driver were loaded, if it can't go without
a binding when there are no vxlan devices on the system.

                                                                +-DLS

Re: [PATCH net-next] vxlan: Provide means for obtaining port information

[Alexander Duyck]

On 03/28/2013 06:23 AM, David Stevens wrote:
>> From: Alexander Duyck <alexander.h.duyck@intel.com>
>  
>> Yes this will break if we ended up supporting a different port per
>> VXLAN.  The problem is a port per VXLAN is going to be much more
>> difficult to support for any sort of hardware offload as well.  What we
>> end up having to do is add a significant number of filters just to
>> identify all of the possible ports that may be used. 
>         I assume this would only be an issue if you actually have lots
> of ports. If you're using only one, or a couple, which I'd expect to
> be the common case, your filtering needs should be the same, right?

The stuff I was looking at only supported one port for VXLAN
identification.  However I might be able to influence the design so I
plan to see if maybe I can push back a bit to come up with a more
flexible solution. 

>> In addition it
>> will mean having to add some sort of notifier as I mentioned in the
>> patch description since we will have VXLANs going into and out of
>> existence, each one with their own port number..
>         I don't know the context in which you're planning to call this,
> but regardless of the port or ports in use by VXLAN, the devices will
> come and go and the module may be unloaded, so I don't see how what
> you've said here applies to one case and not the other.

In the current setup or in a setup with a fixed IANA port number we
wouldn't need the notifier since the module can only be unloaded if
there are no dependencies and any driver that used the provided function
would be dependent on the module preventing it from being unloaded.

As is, the notifier idea has already been dropped as well.  What we will
probably look at doing once a port-per-vxlan type solution is
implemented is maybe look at adding a pair of add/remove VXLAN port ndo
ops.  It would be something similar to what is already out there for
VLAN since that way the VXLAN itself would then be expected to request
the offloads, instead of us trying to anticipate them.

>> This was essentially a "good enough for now" fix to address the fact
>> that I needed the port number for offload testing, but I will sit back
>> and wait for the port per VXLAN stuff to be pushed in before I take any
>> steps to try to sort that out.
>         What I was suggesting "for now" is that you create an interface
> that need not change to support multiple ports. For example:
>
> int vxlan_get_ports(struct net_device *dev, int *ports, int portcount)
> {
>         if (portcount <= 0 || vxlan_port == 0)
>                 return 0;
>         if (ports)
>                 ports[0] = vxlan_port;
>         return 1;
> }
>
> The return value would be the count of ports in use, and we'd fill up to a 
> maximum
> of portcount in the "ports" array. If you don't have a "dev" you're 
> interested in,
> pass it as NULL and in the future this code would match dev only if it is 
> non-NULL;
> otherwise it would go through the vxlan device list adding ports not in 
> the list to it.
>
> And for the right functionality now, change the VXLAN driver to leave 
> vxlan_port
> zero until a device is instantiated, so you can detect no ports at the 
> moment,
> and set it to zero again when the last one is deleted.
>
> You need a mechanism to delete your filters if there are no VXLAN devices,
> whether or not multiple ports are supported, because "0" is a legal count
> now too. If I haven't created any vxlan devices, I can use port 8472 for
> something else and your loading of the vxlan driver as a side-effect of
> checking the port shouldn't prevent that. That's especially true in a
> distro release where, while I might be able to change the port, I'd have
> to burn one regardless if your driver were loaded, if it can't go without
> a binding when there are no vxlan devices on the system.
>
>                                                                 +-DLS
>

The patch I have now is good enough to get the job done for hardware
testing.  The bit of work I did was more of a "free time" thing and that
has pretty much been used up.

I'll just hold off on submitting any further patches for this until
after the whole port numbering thing is sorted out.

Thanks,

Alex