| Message ID | 1460207825-3622-2-git-send-email-manish.chopra@qlogic.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Sat, Apr 9, 2016 at 10:17 AM, Manish Chopra <manish.chopra@qlogic.com> wrote: > Rationale behind this change is that with some OVS configuration > UDP ports doesn't get notified to the driver using > .ndo_[add|del]_vxlan_port. So for the driver to work with > these specific ports in that environment we need to have them configured > on adapter by default for the required hardware offload support. I think you are referring to old out of tree code - no version of upstream OVS does this. In addition, any old code won't work against the new kernels that would include this driver update anyways so there won't be a benefit in any case. Please just use the normal registration mechanism that is already exposed. I also noticed that in the Geneve case you aren't currently registering for port notifications and just using the assigned port number in all cases, which isn't right.
> -----Original Message----- > From: Jesse Gross [mailto:jesse@kernel.org] > Sent: Saturday, April 09, 2016 9:36 PM > To: Manish Chopra <manish.chopra@qlogic.com> > Cc: David Miller <davem@davemloft.net>; netdev <netdev@vger.kernel.org>; > Ariel Elior <Ariel.Elior@qlogic.com>; Yuval Mintz <Yuval.Mintz@qlogic.com> > Subject: Re: [PATCH net-next 1/6] net: Make vxlan/geneve default udp ports > public > > On Sat, Apr 9, 2016 at 10:17 AM, Manish Chopra <manish.chopra@qlogic.com> > wrote: > > Rationale behind this change is that with some OVS configuration > > UDP ports doesn't get notified to the driver using > > .ndo_[add|del]_vxlan_port. So for the driver to work with > > these specific ports in that environment we need to have them configured > > on adapter by default for the required hardware offload support. > > I think you are referring to old out of tree code - no version of > upstream OVS does this. In addition, any old code won't work against > the new kernels that would include this driver update anyways so there > won't be a benefit in any case. > > Please just use the normal registration mechanism that is already > exposed. I also noticed that in the Geneve case you aren't currently > registering for port notifications and just using the assigned port > number in all cases, which isn't right. In the past I have tried openvswitch release package [V2.4.0] from openvswitch.org http://openvswitch.org/releases/ I have tried configuring ovs there on some redhat 7.x kernel with above mentioned package utility "ovs-vsctl" where ports were not notified to drivers. Till earlier versions of openvswitch package [V2.3.2] - It was fine and UDP ports were used to be notified to the driver I thought that the same [No port config notification to the drivers] problem would be with latest upstream OVS, isn't it ? which is why configuration of at least default ports is done by default in drivers. For upstream OVS, is there any other openvswitch package used ? Can't the above release package be used over net-next/upstream kernel ? If latest OVS notifies to configure UDP ports to the driver - I would simply skip default ports configuration by default and instead use .ndo_[add|del]_xxxx for both vxlan and geneve. Thanks, Manish
On Sat, Apr 9, 2016 at 5:46 PM, Manish Chopra <manish.chopra@qlogic.com> wrote: >> -----Original Message----- >> From: Jesse Gross [mailto:jesse@kernel.org] >> Sent: Saturday, April 09, 2016 9:36 PM >> To: Manish Chopra <manish.chopra@qlogic.com> >> Cc: David Miller <davem@davemloft.net>; netdev <netdev@vger.kernel.org>; >> Ariel Elior <Ariel.Elior@qlogic.com>; Yuval Mintz <Yuval.Mintz@qlogic.com> >> Subject: Re: [PATCH net-next 1/6] net: Make vxlan/geneve default udp ports >> public >> >> On Sat, Apr 9, 2016 at 10:17 AM, Manish Chopra <manish.chopra@qlogic.com> >> wrote: >> > Rationale behind this change is that with some OVS configuration >> > UDP ports doesn't get notified to the driver using >> > .ndo_[add|del]_vxlan_port. So for the driver to work with >> > these specific ports in that environment we need to have them configured >> > on adapter by default for the required hardware offload support. >> >> I think you are referring to old out of tree code - no version of >> upstream OVS does this. In addition, any old code won't work against >> the new kernels that would include this driver update anyways so there >> won't be a benefit in any case. >> >> Please just use the normal registration mechanism that is already >> exposed. I also noticed that in the Geneve case you aren't currently >> registering for port notifications and just using the assigned port >> number in all cases, which isn't right. > > In the past I have tried openvswitch release package [V2.4.0] from openvswitch.org > http://openvswitch.org/releases/ > > I have tried configuring ovs there on some redhat 7.x kernel with above mentioned package utility "ovs-vsctl" where ports were not notified to drivers. > Till earlier versions of openvswitch package [V2.3.2] - It was fine and UDP ports were used to be notified to the driver > > I thought that the same [No port config notification to the drivers] problem would be with latest upstream OVS, isn't it ? which is why configuration of at least default ports is done by default in drivers. > For upstream OVS, is there any other openvswitch package used ? Can't the above release package be used over net-next/upstream kernel ? There was a bug in code that was backported for use with older kernels that prevented notifications from happening to the drivers. This is fixed in the most recent version. In addition, this was never present in the original code that is part of the upstream kernel. All versions of userspace are compatible with all kernel modules, regardless of how they were released. > If latest OVS notifies to configure UDP ports to the driver - I would simply skip default ports configuration by default and instead use .ndo_[add|del]_xxxx for both vxlan and geneve. That would be great, thanks.
diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c index a9fbf17..4f8a1bb 100644 --- a/drivers/net/geneve.c +++ b/drivers/net/geneve.c @@ -23,8 +23,6 @@ #define GENEVE_NETDEV_VER "0.6" -#define GENEVE_UDP_PORT 6081 - #define GENEVE_N_VID (1u << 24) #define GENEVE_VID_MASK (GENEVE_N_VID - 1) @@ -1361,7 +1359,7 @@ static int geneve_configure(struct net *net, struct net_device *dev, static int geneve_newlink(struct net *net, struct net_device *dev, struct nlattr *tb[], struct nlattr *data[]) { - __be16 dst_port = htons(GENEVE_UDP_PORT); + __be16 dst_port = htons(GENEVE_DEF_UDP_PORT); __u8 ttl = 0, tos = 0; bool metadata = false; union geneve_addr remote = geneve_remote_unspec; diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c index 9f36340..1d7af21 100644 --- a/drivers/net/vxlan.c +++ b/drivers/net/vxlan.c @@ -62,7 +62,7 @@ * 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_DEF_UDP_PORT; module_param_named(udp_port, vxlan_port, ushort, 0444); MODULE_PARM_DESC(udp_port, "Destination UDP port"); diff --git a/include/net/geneve.h b/include/net/geneve.h index e6c23dc..3c3ee4a 100644 --- a/include/net/geneve.h +++ b/include/net/geneve.h @@ -5,6 +5,7 @@ #include <net/udp_tunnel.h> #endif +#define GENEVE_DEF_UDP_PORT 6081 /* Geneve Header: * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ diff --git a/include/net/vxlan.h b/include/net/vxlan.h index 2f168f0..5d1b27f 100644 --- a/include/net/vxlan.h +++ b/include/net/vxlan.h @@ -9,6 +9,8 @@ #include <linux/udp.h> #include <net/dst_metadata.h> +#define VXLAN_DEF_UDP_PORT 8472 + /* VXLAN protocol (RFC 7348) header: * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ * |R|R|R|R|I|R|R|R| Reserved |