Re: [PATCH net v2 2/3] geneve: Relax MTU constraints
On Thu, Feb 18, 2016 at 8:54 AM, David Wragg wrote: > Tom Herbert writes: >> Please implement like in ip_tunnel_change_mtu (or better yet call it), >> that is the precedent for tunnels. > > I've made geneve_change_mtu follow ip_tunnel_change_mtu in v2. > > If it were to call it instead, are you suggesting just passing in > t_hlen? Or restructuring geneve.c to re-use the whole ip_tunnel > infrastructure? > I'll leave that to you to decide if that is feasible or makes sense, but ip_tunnel does do some other interesting things. Support for geneve could easily be implemented using ip_tunnel_encap facility. The default MTU on the device is set based on the MTU of the outgoing interface and tunnel overhead-- this should mitigate the possibility of a lot of fragmentation happening within the tunnel. Also, the output infrastructure caches the route for the tunnel which is a nice performance win. > Also, I'm not sure where the 0xFFF8 comes from in > __ip_tunnel_change_mtu. Any ideas why 0xFFF8 rather than 0x? It > goes all the way back to the inital import of the kernel into git. > Yes, that's pretty ugly. Feel free to replace that with a #define or at least put a comment about it for the benefit of future generations. Thanks, Tom > David
Re: [PATCH net v2 2/3] geneve: Relax MTU constraints
From: David Wragg Date: Thu, 18 Feb 2016 16:54:14 + > Tom Herbert writes: >> Please implement like in ip_tunnel_change_mtu (or better yet call it), >> that is the precedent for tunnels. > > I've made geneve_change_mtu follow ip_tunnel_change_mtu in v2. > > If it were to call it instead, are you suggesting just passing in > t_hlen? Or restructuring geneve.c to re-use the whole ip_tunnel > infrastructure? > > Also, I'm not sure where the 0xFFF8 comes from in > __ip_tunnel_change_mtu. Any ideas why 0xFFF8 rather than 0x? It > goes all the way back to the inital import of the kernel into git. Some 8 byte multiple requirement, perhaps to do with fragmentation.
Re: [PATCH net v2 2/3] geneve: Relax MTU constraints
Tom Herbert writes: > Please implement like in ip_tunnel_change_mtu (or better yet call it), > that is the precedent for tunnels. I've made geneve_change_mtu follow ip_tunnel_change_mtu in v2. If it were to call it instead, are you suggesting just passing in t_hlen? Or restructuring geneve.c to re-use the whole ip_tunnel infrastructure? Also, I'm not sure where the 0xFFF8 comes from in __ip_tunnel_change_mtu. Any ideas why 0xFFF8 rather than 0x? It goes all the way back to the inital import of the kernel into git. David
Re: [PATCH net v2 2/3] geneve: Relax MTU constraints
On Tue, Feb 16, 2016 at 4:33 AM, David Wragg wrote: > Jesse Gross writes: >> On Wed, Feb 10, 2016 at 3:21 PM, Tom Herbert wrote: >>> On Wed, Feb 10, 2016 at 12:59 PM, Jesse Gross wrote: On Wed, Feb 10, 2016 at 12:41 PM, David Wragg wrote: > Tom Herbert writes: >> The correct thing to do is determine the maximum amount of >> encapsulation overhead that can ever be set in a packet and use for >> setting the MTU. For instance, when RCO is enable in GUE, the size of >> the option is included in tunnel->encap_hlen even though it will not >> be used in all packets (via ip_tunnel_change_mtu). If there is no way >> to determine a maximum overhead a priori from configuration, then >> maximum overhead could be assumed to be maximum possible encapsulation >> header size which for Geneve is 132 bytes IIRC. > > Ok, I'll come up with a patch to address this. I don't think that this really applies in this situation. The concerns here relate to what the MTU is actually set to but this patch affects the range of MTUs allowed to be set by the user. I don't see a reason to disallow the user from setting a precise value if they know what it should be. >>> Right, but if the user sets a bad value and packets are silently >>> dropped on the floor then that seems like a bad result that could have >>> easily been prevented. >> >> Sure, we might as well prevent the extreme edge cases that can never >> be valid. In the case of Geneve though, this would be the minimum >> header size, not the maximum, since it's possible that the user >> actually knows how big the options are that they want to use. >> >> But as I said, the practical impact is low because IP_MAX_MTU is so >> much larger than the MTU of real devices. There will always be many >> values that the MTU can be set to that result in dropped packets. This >> is true of all tunnel types. > > I agree with Jesse, and if there was a debate about lifting the MTU > limit on all tunnel types to IP_MAX_MTU, I'd be for that. > > But for the other tunnel types, I followed the precedent of the code > that was already there, and geneve might as well be consistent. > Please implement like in ip_tunnel_change_mtu (or better yet call it), that is the precedent for tunnels. Tom > Patch sent to the lists. > > David
Re: [PATCH net v2 2/3] geneve: Relax MTU constraints
Jesse Gross writes: > On Wed, Feb 10, 2016 at 3:21 PM, Tom Herbert wrote: >> On Wed, Feb 10, 2016 at 12:59 PM, Jesse Gross wrote: >>> On Wed, Feb 10, 2016 at 12:41 PM, David Wragg wrote: Tom Herbert writes: > The correct thing to do is determine the maximum amount of > encapsulation overhead that can ever be set in a packet and use for > setting the MTU. For instance, when RCO is enable in GUE, the size of > the option is included in tunnel->encap_hlen even though it will not > be used in all packets (via ip_tunnel_change_mtu). If there is no way > to determine a maximum overhead a priori from configuration, then > maximum overhead could be assumed to be maximum possible encapsulation > header size which for Geneve is 132 bytes IIRC. Ok, I'll come up with a patch to address this. >>> >>> I don't think that this really applies in this situation. The concerns >>> here relate to what the MTU is actually set to but this patch affects >>> the range of MTUs allowed to be set by the user. I don't see a reason >>> to disallow the user from setting a precise value if they know what it >>> should be. >>> >> Right, but if the user sets a bad value and packets are silently >> dropped on the floor then that seems like a bad result that could have >> easily been prevented. > > Sure, we might as well prevent the extreme edge cases that can never > be valid. In the case of Geneve though, this would be the minimum > header size, not the maximum, since it's possible that the user > actually knows how big the options are that they want to use. > > But as I said, the practical impact is low because IP_MAX_MTU is so > much larger than the MTU of real devices. There will always be many > values that the MTU can be set to that result in dropped packets. This > is true of all tunnel types. I agree with Jesse, and if there was a debate about lifting the MTU limit on all tunnel types to IP_MAX_MTU, I'd be for that. But for the other tunnel types, I followed the precedent of the code that was already there, and geneve might as well be consistent. Patch sent to the lists. David
Re: [PATCH net v2 2/3] geneve: Relax MTU constraints
On Wed, Feb 10, 2016 at 3:21 PM, Tom Herbert wrote: > On Wed, Feb 10, 2016 at 12:59 PM, Jesse Gross wrote: >> On Wed, Feb 10, 2016 at 12:41 PM, David Wragg wrote: >>> Tom Herbert writes: The correct thing to do is determine the maximum amount of encapsulation overhead that can ever be set in a packet and use for setting the MTU. For instance, when RCO is enable in GUE, the size of the option is included in tunnel->encap_hlen even though it will not be used in all packets (via ip_tunnel_change_mtu). If there is no way to determine a maximum overhead a priori from configuration, then maximum overhead could be assumed to be maximum possible encapsulation header size which for Geneve is 132 bytes IIRC. >>> >>> Ok, I'll come up with a patch to address this. >> >> I don't think that this really applies in this situation. The concerns >> here relate to what the MTU is actually set to but this patch affects >> the range of MTUs allowed to be set by the user. I don't see a reason >> to disallow the user from setting a precise value if they know what it >> should be. >> > Right, but if the user sets a bad value and packets are silently > dropped on the floor then that seems like a bad result that could have > easily been prevented. Sure, we might as well prevent the extreme edge cases that can never be valid. In the case of Geneve though, this would be the minimum header size, not the maximum, since it's possible that the user actually knows how big the options are that they want to use. But as I said, the practical impact is low because IP_MAX_MTU is so much larger than the MTU of real devices. There will always be many values that the MTU can be set to that result in dropped packets. This is true of all tunnel types.
Re: [PATCH net v2 2/3] geneve: Relax MTU constraints
On Wed, Feb 10, 2016 at 12:59 PM, Jesse Gross wrote: > On Wed, Feb 10, 2016 at 12:41 PM, David Wragg wrote: >> Tom Herbert writes: >>> The correct thing to do is determine the maximum amount of >>> encapsulation overhead that can ever be set in a packet and use for >>> setting the MTU. For instance, when RCO is enable in GUE, the size of >>> the option is included in tunnel->encap_hlen even though it will not >>> be used in all packets (via ip_tunnel_change_mtu). If there is no way >>> to determine a maximum overhead a priori from configuration, then >>> maximum overhead could be assumed to be maximum possible encapsulation >>> header size which for Geneve is 132 bytes IIRC. >> >> Ok, I'll come up with a patch to address this. > > I don't think that this really applies in this situation. The concerns > here relate to what the MTU is actually set to but this patch affects > the range of MTUs allowed to be set by the user. I don't see a reason > to disallow the user from setting a precise value if they know what it > should be. > Right, but if the user sets a bad value and packets are silently dropped on the floor then that seems like a bad result that could have easily been prevented. > In any case, I don't think it is likely to have much impact. By > default with tunnels the output device is not fixed and therefore the > base MTU that is used is IP_MAX_MTU. Subtracting some tunnel overhead > amount from this is still likely quite a bit higher than any physical > MTU. > > If you really want, I would subtract the base Geneve header size from > IP_MAX_MTU to get the true max but it's probably not a big deal in any > case.
Re: [PATCH net v2 2/3] geneve: Relax MTU constraints
On Wed, Feb 10, 2016 at 12:41 PM, David Wragg wrote: > Tom Herbert writes: >> The correct thing to do is determine the maximum amount of >> encapsulation overhead that can ever be set in a packet and use for >> setting the MTU. For instance, when RCO is enable in GUE, the size of >> the option is included in tunnel->encap_hlen even though it will not >> be used in all packets (via ip_tunnel_change_mtu). If there is no way >> to determine a maximum overhead a priori from configuration, then >> maximum overhead could be assumed to be maximum possible encapsulation >> header size which for Geneve is 132 bytes IIRC. > > Ok, I'll come up with a patch to address this. I don't think that this really applies in this situation. The concerns here relate to what the MTU is actually set to but this patch affects the range of MTUs allowed to be set by the user. I don't see a reason to disallow the user from setting a precise value if they know what it should be. In any case, I don't think it is likely to have much impact. By default with tunnels the output device is not fixed and therefore the base MTU that is used is IP_MAX_MTU. Subtracting some tunnel overhead amount from this is still likely quite a bit higher than any physical MTU. If you really want, I would subtract the base Geneve header size from IP_MAX_MTU to get the true max but it's probably not a big deal in any case.
Re: [PATCH net v2 2/3] geneve: Relax MTU constraints
Tom Herbert writes: > The correct thing to do is determine the maximum amount of > encapsulation overhead that can ever be set in a packet and use for > setting the MTU. For instance, when RCO is enable in GUE, the size of > the option is included in tunnel->encap_hlen even though it will not > be used in all packets (via ip_tunnel_change_mtu). If there is no way > to determine a maximum overhead a priori from configuration, then > maximum overhead could be assumed to be maximum possible encapsulation > header size which for Geneve is 132 bytes IIRC. Ok, I'll come up with a patch to address this. David
Re: [PATCH net v2 2/3] geneve: Relax MTU constraints
On Tue, Feb 9, 2016 at 5:47 PM, David Wragg wrote:
> Allow the MTU of geneve devices to be set to large values, in order to
> exploit underlying networks with larger frame sizes.
>
> GENEVE does not have a fixed encapsulation overhead (an openvswitch
> rule can add variable length options), so there is no relevant maximum
> MTU to enforce. A maximum of IP_MAX_MTU is used instead.
> Encapsulated packets that are too big for the underlying network will
> get dropped on the floor.
>
This defeats the purpose of having an MTU. The advertised MTU
indicates how large a packet that the interface if willing to handle.
Upper layers use this information, and if they need to send a packet
larger than MTU they take appropriate action such as fragmenting
packet or sending an ICMP PTB error. If a packets are sent on the
interface that are smaller than MTU and are being "dropped on the
floor" for being too big, a sender would get no indication at all why
its packets are being dropped.
The correct thing to do is determine the maximum amount of
encapsulation overhead that can ever be set in a packet and use for
setting the MTU. For instance, when RCO is enable in GUE, the size of
the option is included in tunnel->encap_hlen even though it will not
be used in all packets (via ip_tunnel_change_mtu). If there is no way
to determine a maximum overhead a priori from configuration, then
maximum overhead could be assumed to be maximum possible encapsulation
header size which for Geneve is 132 bytes IIRC.
Tom
> Signed-off-by: David Wragg
> ---
> drivers/net/geneve.c | 12 +++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
> index 0b14ac3..05cef11 100644
> --- a/drivers/net/geneve.c
> +++ b/drivers/net/geneve.c
> @@ -1039,6 +1039,16 @@ static netdev_tx_t geneve_xmit(struct sk_buff *skb,
> struct net_device *dev)
> return geneve_xmit_skb(skb, dev, info);
> }
>
> +static int geneve_change_mtu(struct net_device *dev, int new_mtu)
> +{
> + /* GENEVE overhead is not fixed, so we can't enforce a more
> + precise max MTU. */
> + if (new_mtu < 68 || new_mtu > IP_MAX_MTU)
> + return -EINVAL;
> + dev->mtu = new_mtu;
> + return 0;
> +}
> +
> static int geneve_fill_metadata_dst(struct net_device *dev, struct sk_buff
> *skb)
> {
> struct ip_tunnel_info *info = skb_tunnel_info(skb);
> @@ -1083,7 +1093,7 @@ static const struct net_device_ops geneve_netdev_ops = {
> .ndo_stop = geneve_stop,
> .ndo_start_xmit = geneve_xmit,
> .ndo_get_stats64= ip_tunnel_get_stats64,
> - .ndo_change_mtu = eth_change_mtu,
> + .ndo_change_mtu = geneve_change_mtu,
> .ndo_validate_addr = eth_validate_addr,
> .ndo_set_mac_address= eth_mac_addr,
> .ndo_fill_metadata_dst = geneve_fill_metadata_dst,
> --
> 2.5.0
>
Re: [PATCH net v2 2/3] geneve: Relax MTU constraints
Sergei Shtylyov writes: >The networking code formats comments: > > /* Like > * this. > */ Thanks. And I noticed another silly mistake. Will respin. David
Re: [PATCH net v2 2/3] geneve: Relax MTU constraints
On 02/09/2016 07:47 PM, David Wragg wrote:
Allow the MTU of geneve devices to be set to large values, in order to
exploit underlying networks with larger frame sizes.
GENEVE does not have a fixed encapsulation overhead (an openvswitch
rule can add variable length options), so there is no relevant maximum
MTU to enforce. A maximum of IP_MAX_MTU is used instead.
Encapsulated packets that are too big for the underlying network will
get dropped on the floor.
Signed-off-by: David Wragg
---
drivers/net/geneve.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 0b14ac3..05cef11 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -1039,6 +1039,16 @@ static netdev_tx_t geneve_xmit(struct sk_buff *skb,
struct net_device *dev)
return geneve_xmit_skb(skb, dev, info);
}
+static int geneve_change_mtu(struct net_device *dev, int new_mtu)
+{
+ /* GENEVE overhead is not fixed, so we can't enforce a more
+ precise max MTU. */
The networking code formats comments:
/* Like
* this.
*/
+ if (new_mtu < 68 || new_mtu > IP_MAX_MTU)
+ return -EINVAL;
+ dev->mtu = new_mtu;
+ return 0;
+}
+
static int geneve_fill_metadata_dst(struct net_device *dev, struct sk_buff
*skb)
{
struct ip_tunnel_info *info = skb_tunnel_info(skb);
[...]
MBR, Sergei
[PATCH net v2 2/3] geneve: Relax MTU constraints
Allow the MTU of geneve devices to be set to large values, in order to
exploit underlying networks with larger frame sizes.
GENEVE does not have a fixed encapsulation overhead (an openvswitch
rule can add variable length options), so there is no relevant maximum
MTU to enforce. A maximum of IP_MAX_MTU is used instead.
Encapsulated packets that are too big for the underlying network will
get dropped on the floor.
Signed-off-by: David Wragg
---
drivers/net/geneve.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 0b14ac3..05cef11 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -1039,6 +1039,16 @@ static netdev_tx_t geneve_xmit(struct sk_buff *skb,
struct net_device *dev)
return geneve_xmit_skb(skb, dev, info);
}
+static int geneve_change_mtu(struct net_device *dev, int new_mtu)
+{
+ /* GENEVE overhead is not fixed, so we can't enforce a more
+ precise max MTU. */
+ if (new_mtu < 68 || new_mtu > IP_MAX_MTU)
+ return -EINVAL;
+ dev->mtu = new_mtu;
+ return 0;
+}
+
static int geneve_fill_metadata_dst(struct net_device *dev, struct sk_buff
*skb)
{
struct ip_tunnel_info *info = skb_tunnel_info(skb);
@@ -1083,7 +1093,7 @@ static const struct net_device_ops geneve_netdev_ops = {
.ndo_stop = geneve_stop,
.ndo_start_xmit = geneve_xmit,
.ndo_get_stats64= ip_tunnel_get_stats64,
- .ndo_change_mtu = eth_change_mtu,
+ .ndo_change_mtu = geneve_change_mtu,
.ndo_validate_addr = eth_validate_addr,
.ndo_set_mac_address= eth_mac_addr,
.ndo_fill_metadata_dst = geneve_fill_metadata_dst,
--
2.5.0
