| Message ID | 1455648204-58763-5-git-send-email-joe@ovn.org |
|---|---|
| State | Not Applicable |
| Headers | show |
On Tue, Feb 16, 2016 at 10:43 AM, Joe Stringer <joe@ovn.org> wrote: > diff --git a/datapath/linux/compat/lisp.c b/datapath/linux/compat/lisp.c > index e5a6a7fe00a4..0c81dd52ecab 100644 > --- a/datapath/linux/compat/lisp.c > +++ b/datapath/linux/compat/lisp.c > @@ -114,6 +114,7 @@ struct lisphdr { > }; > > #define LISP_HLEN (sizeof(struct udphdr) + sizeof(struct lisphdr)) > +#define LISP_MAX_MTU (LISP_HLEN + sizeof(struct iphdr) + sizeof(struct ethhdr)) This doesn't look right to me - isn't this a very small value? Also, LISP doesn't have an inner Ethernet header, unlike the other tunnels. Based on the upstream discussion, it seems like some additional patches may be coming in this area as well that we might want to roll in to make all tunnels consistent.
On 16 February 2016 at 11:43, Jesse Gross <jesse@kernel.org> wrote: > On Tue, Feb 16, 2016 at 10:43 AM, Joe Stringer <joe@ovn.org> wrote: >> diff --git a/datapath/linux/compat/lisp.c b/datapath/linux/compat/lisp.c >> index e5a6a7fe00a4..0c81dd52ecab 100644 >> --- a/datapath/linux/compat/lisp.c >> +++ b/datapath/linux/compat/lisp.c >> @@ -114,6 +114,7 @@ struct lisphdr { >> }; >> >> #define LISP_HLEN (sizeof(struct udphdr) + sizeof(struct lisphdr)) >> +#define LISP_MAX_MTU (LISP_HLEN + sizeof(struct iphdr) + sizeof(struct ethhdr)) > > This doesn't look right to me - isn't this a very small value? Also, > LISP doesn't have an inner Ethernet header, unlike the other tunnels. You're right, I messed this one up. Looks like it should be IP_MAX_MTU - LISP_HLEN - sizeof(struct ethhdr) > Based on the upstream discussion, it seems like some additional > patches may be coming in this area as well that we might want to roll > in to make all tunnels consistent. OK, I can hold off on this series until that gets resolved.
On 16 February 2016 at 12:35, Joe Stringer <joe@ovn.org> wrote: > On 16 February 2016 at 11:43, Jesse Gross <jesse@kernel.org> wrote: >> On Tue, Feb 16, 2016 at 10:43 AM, Joe Stringer <joe@ovn.org> wrote: >>> diff --git a/datapath/linux/compat/lisp.c b/datapath/linux/compat/lisp.c >>> index e5a6a7fe00a4..0c81dd52ecab 100644 >>> --- a/datapath/linux/compat/lisp.c >>> +++ b/datapath/linux/compat/lisp.c >>> @@ -114,6 +114,7 @@ struct lisphdr { >>> }; >>> >>> #define LISP_HLEN (sizeof(struct udphdr) + sizeof(struct lisphdr)) >>> +#define LISP_MAX_MTU (LISP_HLEN + sizeof(struct iphdr) + sizeof(struct ethhdr)) >> >> This doesn't look right to me - isn't this a very small value? Also, >> LISP doesn't have an inner Ethernet header, unlike the other tunnels. > > You're right, I messed this one up. Looks like it should be > IP_MAX_MTU - LISP_HLEN - sizeof(struct ethhdr) Or rather, this. (IP_MAX_MTU - LISP_HLEN - sizeof(struct iphdr)) Though, the max differs for IPv4/IPv6 - Could drop it to using the ipv6hdr instead.
On Wed, Feb 17, 2016 at 10:26 AM, Joe Stringer <joe@ovn.org> wrote: > On 16 February 2016 at 12:35, Joe Stringer <joe@ovn.org> wrote: >> On 16 February 2016 at 11:43, Jesse Gross <jesse@kernel.org> wrote: >>> On Tue, Feb 16, 2016 at 10:43 AM, Joe Stringer <joe@ovn.org> wrote: >>>> diff --git a/datapath/linux/compat/lisp.c b/datapath/linux/compat/lisp.c >>>> index e5a6a7fe00a4..0c81dd52ecab 100644 >>>> --- a/datapath/linux/compat/lisp.c >>>> +++ b/datapath/linux/compat/lisp.c >>>> @@ -114,6 +114,7 @@ struct lisphdr { >>>> }; >>>> >>>> #define LISP_HLEN (sizeof(struct udphdr) + sizeof(struct lisphdr)) >>>> +#define LISP_MAX_MTU (LISP_HLEN + sizeof(struct iphdr) + sizeof(struct ethhdr)) >>> >>> This doesn't look right to me - isn't this a very small value? Also, >>> LISP doesn't have an inner Ethernet header, unlike the other tunnels. >> >> You're right, I messed this one up. Looks like it should be >> IP_MAX_MTU - LISP_HLEN - sizeof(struct ethhdr) > > Or rather, this. > (IP_MAX_MTU - LISP_HLEN - sizeof(struct iphdr)) > > Though, the max differs for IPv4/IPv6 - Could drop it to using the > ipv6hdr instead. There's currently no support for LISP over IPv6 so I guess we don't need to worry about it for the time being at least.
diff --git a/datapath/linux/compat/lisp.c b/datapath/linux/compat/lisp.c index e5a6a7fe00a4..0c81dd52ecab 100644 --- a/datapath/linux/compat/lisp.c +++ b/datapath/linux/compat/lisp.c @@ -114,6 +114,7 @@ struct lisphdr { }; #define LISP_HLEN (sizeof(struct udphdr) + sizeof(struct lisphdr)) +#define LISP_MAX_MTU (LISP_HLEN + sizeof(struct iphdr) + sizeof(struct ethhdr)) static inline struct lisphdr *lisp_hdr(const struct sk_buff *skb) { @@ -447,6 +448,15 @@ static netdev_tx_t lisp_dev_xmit(struct sk_buff *skb, struct net_device *dev) #endif } +static int lisp_change_mtu(struct net_device *dev, int new_mtu) +{ + if (new_mtu < 68 || new_mtu > LISP_MAX_MTU) + return -EINVAL; + + dev->mtu = new_mtu; + return 0; +} + static const struct net_device_ops lisp_netdev_ops = { #ifdef HAVE_DEV_TSTATS .ndo_init = lisp_init, @@ -456,7 +466,7 @@ static const struct net_device_ops lisp_netdev_ops = { .ndo_open = lisp_open, .ndo_stop = lisp_stop, .ndo_start_xmit = lisp_dev_xmit, - .ndo_change_mtu = eth_change_mtu, + .ndo_change_mtu = lisp_change_mtu, .ndo_validate_addr = eth_validate_addr, .ndo_set_mac_address = eth_mac_addr, }; @@ -553,6 +563,10 @@ static int lisp_configure(struct net *net, struct net_device *dev, if (err) return err; + err = lisp_change_mtu(dev, LISP_MAX_MTU); + if (err) + return err; + list_add(&lisp->next, &ln->lisp_list); return 0; }
Currently, even if the entire path supports jumbo frames, the LISP netdev limits the path MTU to 1500 bytes, and cannot be configured otherwise. Relax the constraints on modifying the device MTU, and set it to the maximum by default. Signed-off-by: Joe Stringer <joe@ovn.org> --- datapath/linux/compat/lisp.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)