diff mbox

[net-next,1/5] vxlan: Make VXLAN default UDP port number available for others

Message ID 1394470142-13992-2-git-send-email-shahed.shaikh@qlogic.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Shahed Shaikh March 10, 2014, 4:48 p.m. UTC
From: Shahed Shaikh <shahed.shaikh@qlogic.com>

Although vxlan module has capability to notify udp ports to
other interested net devices using .ndo_add_rx_vxlan_port and
.ndo_del_rx_vxlan_port, there could be some devices which support
vxlan offload but not interested in updating udp port numbers.
This may be because some hardware do not support programming multiple
udp ports and their drivers may decide to program only default udp port
into adapter. So that adapter, at least, can do offloading for
default udp port number.

Signed-off-by: Shahed Shaikh <shahed.shaikh@qlogic.com>
---
 drivers/net/vxlan.c | 6 +-----
 include/net/vxlan.h | 6 ++++++
 2 files changed, 7 insertions(+), 5 deletions(-)

Comments

Stephen Hemminger March 10, 2014, 7:43 p.m. UTC | #1
On Mon, 10 Mar 2014 12:48:58 -0400
Shahed Shaikh <shahed.shaikh@qlogic.com> wrote:

> From: Shahed Shaikh <shahed.shaikh@qlogic.com>
> 
> Although vxlan module has capability to notify udp ports to
> other interested net devices using .ndo_add_rx_vxlan_port and
> .ndo_del_rx_vxlan_port, there could be some devices which support
> vxlan offload but not interested in updating udp port numbers.
> This may be because some hardware do not support programming multiple
> udp ports and their drivers may decide to program only default udp port
> into adapter. So that adapter, at least, can do offloading for
> default udp port number.
> 
> Signed-off-by: Shahed Shaikh <shahed.shaikh@qlogic.com>

Rather than compiling in the value, it makes more sense to make
the default port variable in vxlan driver exported. That way if user
overrides it with a module parameter the offload will still work.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Or Gerlitz March 10, 2014, 7:57 p.m. UTC | #2
On Mon, Mar 10, 2014 at 6:48 PM, Shahed Shaikh <shahed.shaikh@qlogic.com> wrote:
> From: Shahed Shaikh <shahed.shaikh@qlogic.com>
>
> Although vxlan module has capability to notify udp ports to
> other interested net devices using .ndo_add_rx_vxlan_port and
> .ndo_del_rx_vxlan_port, there could be some devices which support
> vxlan offload but not interested in updating udp port numbers.
> This may be because some hardware do not support programming multiple
> udp ports and their drivers may decide to program only default udp port
> into adapter. So that adapter, at least, can do offloading for
> default udp port number.

Indeed, but the default port number can be unused while another port
is used. The ndo will be invoked only behalf
of an actual instancing of udp port for listener socket (==
destination port you want the hw to indentify), what's wrong
with support this ndo also for devices that supported limited (say
one) such port?


>
> Signed-off-by: Shahed Shaikh <shahed.shaikh@qlogic.com>
> ---
>  drivers/net/vxlan.c | 6 +-----
>  include/net/vxlan.h | 6 ++++++
>  2 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index eb59b14..ace758f 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -71,11 +71,7 @@ struct vxlanhdr {
>         __be32 vx_vni;
>  };
>
> -/* UDP port for VXLAN traffic.
> - * The IANA assigned port is 4789, but the Linux default is 8472
> - * for compatibility with early adopters.
> - */
> -static unsigned short vxlan_port __read_mostly = 8472;
> +static unsigned short vxlan_port __read_mostly = VXLAN_DEFAULT_PORT;
>  module_param_named(udp_port, vxlan_port, ushort, 0444);
>  MODULE_PARM_DESC(udp_port, "Destination UDP port");
>
> diff --git a/include/net/vxlan.h b/include/net/vxlan.h
> index 5deef1a..4c16629 100644
> --- a/include/net/vxlan.h
> +++ b/include/net/vxlan.h
> @@ -8,6 +8,12 @@
>  #define VNI_HASH_BITS  10
>  #define VNI_HASH_SIZE  (1<<VNI_HASH_BITS)
>
> +/* UDP port for VXLAN traffic.
> + * The IANA assigned port is 4789, but the Linux default is 8472
> + * for compatibility with early adopters.
> + */
> +#define VXLAN_DEFAULT_PORT 8472
> +
>  struct vxlan_sock;
>  typedef void (vxlan_rcv_t)(struct vxlan_sock *vh, struct sk_buff *skb, __be32 key);
>
> --
> 1.8.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Shahed Shaikh March 11, 2014, 5:37 a.m. UTC | #3
> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Tuesday, March 11, 2014 1:13 AM
> To: Shahed Shaikh
> Cc: David Miller; netdev; Dept-HSG Linux NIC Dev
> Subject: Re: [PATCH net-next 1/5] vxlan: Make VXLAN default UDP port
> number available for others
> 
> On Mon, 10 Mar 2014 12:48:58 -0400
> Shahed Shaikh <shahed.shaikh@qlogic.com> wrote:
> 
> > From: Shahed Shaikh <shahed.shaikh@qlogic.com>
> >
> > Although vxlan module has capability to notify udp ports to other
> > interested net devices using .ndo_add_rx_vxlan_port and
> > .ndo_del_rx_vxlan_port, there could be some devices which support
> > vxlan offload but not interested in updating udp port numbers.
> > This may be because some hardware do not support programming multiple
> > udp ports and their drivers may decide to program only default udp
> > port into adapter. So that adapter, at least, can do offloading for
> > default udp port number.
> >
> > Signed-off-by: Shahed Shaikh <shahed.shaikh@qlogic.com>
> 
> Rather than compiling in the value, it makes more sense to make the default
> port variable in vxlan driver exported. That way if user overrides it with a
> module parameter the offload will still work.

Makes sense. I will re-spin this patch to export default port variable. Thanks for the feedback.

Thanks,
Shahed
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Shahed Shaikh March 11, 2014, 5:41 a.m. UTC | #4
> -----Original Message-----
> From: Or Gerlitz [mailto:or.gerlitz@gmail.com]
> Sent: Tuesday, March 11, 2014 1:28 AM
> To: Shahed Shaikh
> Cc: David Miller; netdev; Dept-HSG Linux NIC Dev
> Subject: Re: [PATCH net-next 1/5] vxlan: Make VXLAN default UDP port
> number available for others
> 
> On Mon, Mar 10, 2014 at 6:48 PM, Shahed Shaikh
> <shahed.shaikh@qlogic.com> wrote:
> > From: Shahed Shaikh <shahed.shaikh@qlogic.com>
> >
> > Although vxlan module has capability to notify udp ports to other
> > interested net devices using .ndo_add_rx_vxlan_port and
> > .ndo_del_rx_vxlan_port, there could be some devices which support
> > vxlan offload but not interested in updating udp port numbers.
> > This may be because some hardware do not support programming multiple
> > udp ports and their drivers may decide to program only default udp
> > port into adapter. So that adapter, at least, can do offloading for
> > default udp port number.
> 
> Indeed, but the default port number can be unused while another port is
> used. The ndo will be invoked only behalf of an actual instancing of udp port
> for listener socket (== destination port you want the hw to indentify), what's
> wrong with support this ndo also for devices that supported limited (say
> one) such port?


 If driver implements .ndo for udp port and user creates multiple vxlan device with different 
udp ports, it may end up programming the udp port which may not go through the adapter 
and no offload will happen. OTOH, if drive does not implement .ndo and if user is aware that driver
 is capable of offloading for default port, he can at least crate vxlan device on top of qlcnic interface
 with default udp port. So, there is no chance for other udp port numbers to replace default udp port and disturb offloading.

Like Stephen suggested, exporting udp port variable of vxlan driver will be more suitable approach.

Thanks,
Shahed  
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Or Gerlitz March 11, 2014, 6:42 a.m. UTC | #5
On Tue, Mar 11, 2014 at 7:41 AM, Shahed Shaikh <shahed.shaikh@qlogic.com> wrote:
>> -----Original Message-----
>> From: Or Gerlitz [mailto:or.gerlitz@gmail.com]
>> Sent: Tuesday, March 11, 2014 1:28 AM
>> To: Shahed Shaikh
>> Cc: David Miller; netdev; Dept-HSG Linux NIC Dev
>> Subject: Re: [PATCH net-next 1/5] vxlan: Make VXLAN default UDP port
>> number available for others
>>
>> On Mon, Mar 10, 2014 at 6:48 PM, Shahed Shaikh
>> <shahed.shaikh@qlogic.com> wrote:
>> > From: Shahed Shaikh <shahed.shaikh@qlogic.com>
>> >
>> > Although vxlan module has capability to notify udp ports to other
>> > interested net devices using .ndo_add_rx_vxlan_port and
>> > .ndo_del_rx_vxlan_port, there could be some devices which support
>> > vxlan offload but not interested in updating udp port numbers.
>> > This may be because some hardware do not support programming multiple
>> > udp ports and their drivers may decide to program only default udp
>> > port into adapter. So that adapter, at least, can do offloading for
>> > default udp port number.
>>
>> Indeed, but the default port number can be unused while another port is
>> used. The ndo will be invoked only behalf of an actual instancing of udp port
>> for listener socket (== destination port you want the hw to indentify), what's
>> wrong with support this ndo also for devices that supported limited (say
>> one) such port?
>
>
>  If driver implements .ndo for udp port and user creates multiple vxlan device with different
> udp ports, it may end up programming the udp port which may not go through the adapter
> and no offload will happen. OTOH, if drive does not implement .ndo and if user is aware that driver
>  is capable of offloading for default port, he can at least crate vxlan device on top of qlcnic interface
>  with default udp port. So, there is no chance for other udp port numbers to replace default udp port and disturb offloading.

I see your point, but doesn't this suggests we need to somehow enhance
 the current framework to
let drivers know which vxlan traffic is expected to be received over
them according to the current routing rules?
I understand this is a bit tricky because  vxlan and routing  are l3
constructs while drivers deal with l2, John/Joseph -
what's your thinking here?


> Like Stephen suggested, exporting udp port variable of vxlan driver will be more suitable approach.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Shahed Shaikh March 11, 2014, 7:22 a.m. UTC | #6
> -----Original Message-----
> From: Or Gerlitz [mailto:or.gerlitz@gmail.com]
> Sent: Tuesday, March 11, 2014 12:12 PM
> To: Shahed Shaikh; Joseph Gasparakis; John Fastabend
> Cc: David Miller; netdev; Dept-HSG Linux NIC Dev
> Subject: Re: [PATCH net-next 1/5] vxlan: Make VXLAN default UDP port
> number available for others
> 
...
> >> >
> >> > Although vxlan module has capability to notify udp ports to other
> >> > interested net devices using .ndo_add_rx_vxlan_port and
> >> > .ndo_del_rx_vxlan_port, there could be some devices which support
> >> > vxlan offload but not interested in updating udp port numbers.
> >> > This may be because some hardware do not support programming
> >> > multiple udp ports and their drivers may decide to program only
> >> > default udp port into adapter. So that adapter, at least, can do
> >> > offloading for default udp port number.
> >>
> >> Indeed, but the default port number can be unused while another port
> >> is used. The ndo will be invoked only behalf of an actual instancing
> >> of udp port for listener socket (== destination port you want the hw
> >> to indentify), what's wrong with support this ndo also for devices
> >> that supported limited (say
> >> one) such port?
> >
> >
> >  If driver implements .ndo for udp port and user creates multiple
> > vxlan device with different udp ports, it may end up programming the
> > udp port which may not go through the adapter and no offload will
> > happen. OTOH, if drive does not implement .ndo and if user is aware
> > that driver  is capable of offloading for default port, he can at least crate
> vxlan device on top of qlcnic interface  with default udp port. So, there is no
> chance for other udp port numbers to replace default udp port and disturb
> offloading.
> 
> I see your point, but doesn't this suggests we need to somehow enhance
> the current framework to let drivers know which vxlan traffic is expected to
> be received over them according to the current routing rules?

Agree. Because of this limitation I used default udp port for offloading.

> I understand this is a bit tricky because  vxlan and routing  are l3 constructs
> while drivers deal with l2, John/Joseph - what's your thinking here?

Yes. May be John, Joseph or Stephen can suggest on this.

Thanks,
Shahed

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Hemminger March 11, 2014, 3:28 p.m. UTC | #7
On Tue, 11 Mar 2014 07:22:31 +0000
Shahed Shaikh <shahed.shaikh@qlogic.com> wrote:

> > -----Original Message-----
> > From: Or Gerlitz [mailto:or.gerlitz@gmail.com]
> > Sent: Tuesday, March 11, 2014 12:12 PM
> > To: Shahed Shaikh; Joseph Gasparakis; John Fastabend
> > Cc: David Miller; netdev; Dept-HSG Linux NIC Dev
> > Subject: Re: [PATCH net-next 1/5] vxlan: Make VXLAN default UDP port
> > number available for others
> > 
> ...
> > >> >
> > >> > Although vxlan module has capability to notify udp ports to other
> > >> > interested net devices using .ndo_add_rx_vxlan_port and
> > >> > .ndo_del_rx_vxlan_port, there could be some devices which support
> > >> > vxlan offload but not interested in updating udp port numbers.
> > >> > This may be because some hardware do not support programming
> > >> > multiple udp ports and their drivers may decide to program only
> > >> > default udp port into adapter. So that adapter, at least, can do
> > >> > offloading for default udp port number.
> > >>
> > >> Indeed, but the default port number can be unused while another port
> > >> is used. The ndo will be invoked only behalf of an actual instancing
> > >> of udp port for listener socket (== destination port you want the hw
> > >> to indentify), what's wrong with support this ndo also for devices
> > >> that supported limited (say
> > >> one) such port?
> > >
> > >
> > >  If driver implements .ndo for udp port and user creates multiple
> > > vxlan device with different udp ports, it may end up programming the
> > > udp port which may not go through the adapter and no offload will
> > > happen. OTOH, if drive does not implement .ndo and if user is aware
> > > that driver  is capable of offloading for default port, he can at least crate
> > vxlan device on top of qlcnic interface  with default udp port. So, there is no
> > chance for other udp port numbers to replace default udp port and disturb
> > offloading.
> > 
> > I see your point, but doesn't this suggests we need to somehow enhance
> > the current framework to let drivers know which vxlan traffic is expected to
> > be received over them according to the current routing rules?
> 
> Agree. Because of this limitation I used default udp port for offloading.
> 
> > I understand this is a bit tricky because  vxlan and routing  are l3 constructs
> > while drivers deal with l2, John/Joseph - what's your thinking here?
> 
> Yes. May be John, Joseph or Stephen can suggest on this.
> 
> Thanks,
> Shahed
> 

Is it possible to do "lazy bind" to port, and set up offload when the
first VXLAN is created? For most user's they will use one port and multiplex
by VNI. But there maybe migration scenario's with multiple ports.

The kernel made unfortunate choice of non-standard port, and has to
stick with that. The ip command tries to nudge users to use the correct IANA
port, therefore correct offload should wait until the first tunnel is created.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joseph Gasparakis March 11, 2014, 5:28 p.m. UTC | #8
On Mon, 10 Mar 2014, Or Gerlitz wrote:

> On Tue, Mar 11, 2014 at 7:41 AM, Shahed Shaikh <shahed.shaikh@qlogic.com> wrote:
> >> -----Original Message-----
> >> From: Or Gerlitz [mailto:or.gerlitz@gmail.com]
> >> Sent: Tuesday, March 11, 2014 1:28 AM
> >> To: Shahed Shaikh
> >> Cc: David Miller; netdev; Dept-HSG Linux NIC Dev
> >> Subject: Re: [PATCH net-next 1/5] vxlan: Make VXLAN default UDP port
> >> number available for others
> >>
> >> On Mon, Mar 10, 2014 at 6:48 PM, Shahed Shaikh
> >> <shahed.shaikh@qlogic.com> wrote:
> >> > From: Shahed Shaikh <shahed.shaikh@qlogic.com>
> >> >
> >> > Although vxlan module has capability to notify udp ports to other
> >> > interested net devices using .ndo_add_rx_vxlan_port and
> >> > .ndo_del_rx_vxlan_port, there could be some devices which support
> >> > vxlan offload but not interested in updating udp port numbers.
> >> > This may be because some hardware do not support programming multiple
> >> > udp ports and their drivers may decide to program only default udp
> >> > port into adapter. So that adapter, at least, can do offloading for
> >> > default udp port number.
> >>
> >> Indeed, but the default port number can be unused while another port is
> >> used. The ndo will be invoked only behalf of an actual instancing of udp port
> >> for listener socket (== destination port you want the hw to indentify), what's
> >> wrong with support this ndo also for devices that supported limited (say
> >> one) such port?
> >
> >
> >  If driver implements .ndo for udp port and user creates multiple vxlan device with different
> > udp ports, it may end up programming the udp port which may not go through the adapter
> > and no offload will happen. OTOH, if drive does not implement .ndo and if user is aware that driver
> >  is capable of offloading for default port, he can at least crate vxlan device on top of qlcnic interface
> >  with default udp port. So, there is no chance for other udp port numbers to replace default udp port and disturb offloading.
> 
> I see your point, but doesn't this suggests we need to somehow enhance
>  the current framework to
> let drivers know which vxlan traffic is expected to be received over
> them according to the current routing rules?
> I understand this is a bit tricky because  vxlan and routing  are l3
> constructs while drivers deal with l2, John/Joseph -
> what's your thinking here?
>

For the sake of simplicity: if you have say two VXLANs listening on two 
different UDP ports, and a single port NIC that can only offload traffic 
from one UDP port, how would you know which VXLAN is more useful to 
offload? There might be traffic from the one port (and in this case it 
might be the default one or not) or you might have traffic from both.
The fact is that you have a hw limitation, and you need something 
to help you predict what which VXLAN you are going to have traffic from 
which is not trivial at all.
Therefore even having a way to use the routing tables wouldn't help I 
think.

In other words, IMHO the best option for running a NIC that can offload X 
UDP ports, is to use up to X UDP ports. Anything else, requres guessing that 
might not be accurate and hence can make things worse instead of better.

 
> 
> > Like Stephen suggested, exporting udp port variable of vxlan driver will be more suitable approach.
> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Shahed Shaikh March 12, 2014, 9:44 a.m. UTC | #9
> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Tuesday, March 11, 2014 8:58 PM
> To: Shahed Shaikh
> Cc: Or Gerlitz; Joseph Gasparakis; John Fastabend; David Miller; netdev;
> Dept-HSG Linux NIC Dev
> Subject: Re: [PATCH net-next 1/5] vxlan: Make VXLAN default UDP port
> number available for others
> 
...
> 
> Is it possible to do "lazy bind" to port, and set up offload when the first
> VXLAN is created? For most user's they will use one port and multiplex by
> VNI. But there maybe migration scenario's with multiple ports.
> 
Hi Stephen,

If I understand you comment correctly,  does this mean, driver should set up offload only for first N udp ports (here N=1 in case of qlcnic) used by VXLAN driver through vxlan_get_rx_port() -> ndo_add_vxlan_port() and discard subsequent ndo_add_vxlan_port() notification for N+ udp ports?
I see similar implementation in  i40e driver (N = 16).

Thanks,
Shahed

> The kernel made unfortunate choice of non-standard port, and has to stick
> with that. The ip command tries to nudge users to use the correct IANA port,
> therefore correct offload should wait until the first tunnel is created.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Shahed Shaikh March 12, 2014, 10:20 a.m. UTC | #10
> -----Original Message-----
> From: Joseph Gasparakis [mailto:joseph.gasparakis@intel.com]
> Sent: Tuesday, March 11, 2014 10:59 PM
> To: Or Gerlitz
> Cc: Shahed Shaikh; Joseph Gasparakis; John Fastabend; David Miller; netdev;
> Dept-HSG Linux NIC Dev
> Subject: Re: [PATCH net-next 1/5] vxlan: Make VXLAN default UDP port
> number available for others
> 
> 
> 
...
> 
> For the sake of simplicity: if you have say two VXLANs listening on two
> different UDP ports, and a single port NIC that can only offload traffic from
> one UDP port, how would you know which VXLAN is more useful to offload?
> There might be traffic from the one port (and in this case it might be the
> default one or not) or you might have traffic from both.
> The fact is that you have a hw limitation, and you need something to help
> you predict what which VXLAN you are going to have traffic from which is not
> trivial at all.
> Therefore even having a way to use the routing tables wouldn't help I think.
> 
> In other words, IMHO the best option for running a NIC that can offload X
> UDP ports, is to use up to X UDP ports. Anything else, requres guessing that
> might not be accurate and hence can make things worse instead of better.

Makes sense. Thanks for your feedback Joseph.

Thanks,
Shahed
> 
> >
> > > Like Stephen suggested, exporting udp port variable of vxlan driver will
> be more suitable approach.
> >
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index eb59b14..ace758f 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -71,11 +71,7 @@  struct vxlanhdr {
 	__be32 vx_vni;
 };
 
-/* UDP port for VXLAN traffic.
- * The IANA assigned port is 4789, but the Linux default is 8472
- * for compatibility with early adopters.
- */
-static unsigned short vxlan_port __read_mostly = 8472;
+static unsigned short vxlan_port __read_mostly = VXLAN_DEFAULT_PORT;
 module_param_named(udp_port, vxlan_port, ushort, 0444);
 MODULE_PARM_DESC(udp_port, "Destination UDP port");
 
diff --git a/include/net/vxlan.h b/include/net/vxlan.h
index 5deef1a..4c16629 100644
--- a/include/net/vxlan.h
+++ b/include/net/vxlan.h
@@ -8,6 +8,12 @@ 
 #define VNI_HASH_BITS	10
 #define VNI_HASH_SIZE	(1<<VNI_HASH_BITS)
 
+/* UDP port for VXLAN traffic.
+ * The IANA assigned port is 4789, but the Linux default is 8472
+ * for compatibility with early adopters.
+ */
+#define VXLAN_DEFAULT_PORT 8472
+
 struct vxlan_sock;
 typedef void (vxlan_rcv_t)(struct vxlan_sock *vh, struct sk_buff *skb, __be32 key);