* [PATCH][v3] dev : fix mtu check when TSO is enabled @ 2011-03-14 20:39 Daniel Lezcano 2011-03-14 23:59 ` David Miller 0 siblings, 1 reply; 12+ messages in thread From: Daniel Lezcano @ 2011-03-14 20:39 UTC (permalink / raw) To: davem; +Cc: eric.dumazet, kaber, nightnord, netdev In case the device where is coming from the packet has TSO enabled, we should not check the mtu size value as this one could be bigger than the expected value. This is the case for the macvlan driver when the lower device has TSO enabled. The macvlan inherit this feature and forward the packets without fragmenting them. Then the packets go through dev_forward_skb and are dropped. This patch fix this by checking TSO is not enabled when we want to check the mtu size. Signed-off-by: Daniel Lezcano <daniel.lezcano@free.fr> Cc: Eric Dumazet <eric.dumazet@gmail.com> Cc: Patrick McHardy <kaber@trash.net> Cc: Andrian Nord <nightnord@gmail.com> --- net/core/dev.c | 24 ++++++++++++++++++++++-- 1 files changed, 22 insertions(+), 2 deletions(-) diff --git a/net/core/dev.c b/net/core/dev.c index 6561021..29329d3 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -1503,6 +1503,27 @@ static inline void net_timestamp_check(struct sk_buff *skb) __net_timestamp(skb); } +static inline bool is_skb_forwardable(struct net_device *dev, + struct sk_buff *skb) +{ + unsigned int len; + + if (!(dev->flags & IFF_UP)) + return false; + + len = dev->mtu + dev->hard_header_len + VLAN_HLEN; + if (skb->len < len) + return true; + + /* if TSO is enabled, we don't care about the length as the packet + * could be forwarded without being segmented before + */ + if (skb->dev && skb->dev->features & NETIF_F_TSO) + return true; + + return false; +} + /** * dev_forward_skb - loopback an skb to another netif * @@ -1526,8 +1547,7 @@ int dev_forward_skb(struct net_device *dev, struct sk_buff *skb) skb_orphan(skb); nf_reset(skb); - if (unlikely(!(dev->flags & IFF_UP) || - (skb->len > (dev->mtu + dev->hard_header_len + VLAN_HLEN)))) { + if (unlikely(!is_skb_forwardable(dev, skb))) { atomic_long_inc(&dev->rx_dropped); kfree_skb(skb); return NET_RX_DROP; -- 1.7.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH][v3] dev : fix mtu check when TSO is enabled 2011-03-14 20:39 [PATCH][v3] dev : fix mtu check when TSO is enabled Daniel Lezcano @ 2011-03-14 23:59 ` David Miller 2011-03-15 13:57 ` Daniel Lezcano 2011-03-22 0:05 ` Michał Mirosław 0 siblings, 2 replies; 12+ messages in thread From: David Miller @ 2011-03-14 23:59 UTC (permalink / raw) To: daniel.lezcano; +Cc: eric.dumazet, kaber, nightnord, netdev From: Daniel Lezcano <daniel.lezcano@free.fr> Date: Mon, 14 Mar 2011 21:39:50 +0100 > + len = dev->mtu + dev->hard_header_len + VLAN_HLEN; > + if (skb->len < len) > + return true; This is not a correct translation of the original test: > - (skb->len > (dev->mtu + dev->hard_header_len + VLAN_HLEN)))) { You need to use "<=" in your version, which currently rejects all full sized frames. :-) > + > + /* if TSO is enabled, we don't care about the length as the packet > + * could be forwarded without being segmented before > + */ > + if (skb->dev && skb->dev->features & NETIF_F_TSO) > + return true; I am trying to understand why you aren't simply checking also if this is a segmented frame? Perhaps skb_is_gso() && device has NETIF_F_TSO set? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH][v3] dev : fix mtu check when TSO is enabled 2011-03-14 23:59 ` David Miller @ 2011-03-15 13:57 ` Daniel Lezcano 2011-03-15 18:17 ` Stephen Hemminger 2011-03-22 0:05 ` Michał Mirosław 1 sibling, 1 reply; 12+ messages in thread From: Daniel Lezcano @ 2011-03-15 13:57 UTC (permalink / raw) To: David Miller; +Cc: eric.dumazet, kaber, nightnord, netdev On 03/15/2011 12:59 AM, David Miller wrote: > From: Daniel Lezcano<daniel.lezcano@free.fr> > Date: Mon, 14 Mar 2011 21:39:50 +0100 > >> + len = dev->mtu + dev->hard_header_len + VLAN_HLEN; >> + if (skb->len< len) >> + return true; > This is not a correct translation of the original test: > >> - (skb->len> (dev->mtu + dev->hard_header_len + VLAN_HLEN)))) { > You need to use "<=" in your version, which currently rejects all > full sized frames. :-) Right, thanks. >> + >> + /* if TSO is enabled, we don't care about the length as the packet >> + * could be forwarded without being segmented before >> + */ >> + if (skb->dev&& skb->dev->features& NETIF_F_TSO) >> + return true; > I am trying to understand why you aren't simply checking also if this > is a segmented frame? Perhaps skb_is_gso()&& device has NETIF_F_TSO > set? Maybe I am misunderstanding but the packet was forwarded by another device. In our case from macvlan: macvlan_start_xmit macvlan_queue_xmit dest->forward dev_skb_forward When we reached dev_skb_forward, that means we passed through dev_hard_start_xmit where the packet was already segmented so we should exit at the first test (skb->len < len). I don't see the point of adding the skb_is_gso. But maybe I am missing something, can you explain ? Thanks -- Daniel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH][v3] dev : fix mtu check when TSO is enabled 2011-03-15 13:57 ` Daniel Lezcano @ 2011-03-15 18:17 ` Stephen Hemminger 2011-03-16 13:56 ` Daniel Lezcano 0 siblings, 1 reply; 12+ messages in thread From: Stephen Hemminger @ 2011-03-15 18:17 UTC (permalink / raw) To: Daniel Lezcano; +Cc: David Miller, eric.dumazet, kaber, nightnord, netdev On Tue, 15 Mar 2011 14:57:40 +0100 Daniel Lezcano <daniel.lezcano@free.fr> wrote: > On 03/15/2011 12:59 AM, David Miller wrote: > > From: Daniel Lezcano<daniel.lezcano@free.fr> > > Date: Mon, 14 Mar 2011 21:39:50 +0100 > > > >> + len = dev->mtu + dev->hard_header_len + VLAN_HLEN; > >> + if (skb->len< len) > >> + return true; > > This is not a correct translation of the original test: > > > >> - (skb->len> (dev->mtu + dev->hard_header_len + VLAN_HLEN)))) { > > You need to use "<=" in your version, which currently rejects all > > full sized frames. :-) > > Right, thanks. > > >> + > >> + /* if TSO is enabled, we don't care about the length as the packet > >> + * could be forwarded without being segmented before > >> + */ > >> + if (skb->dev&& skb->dev->features& NETIF_F_TSO) > >> + return true; > > I am trying to understand why you aren't simply checking also if this > > is a segmented frame? Perhaps skb_is_gso()&& device has NETIF_F_TSO > > set? > > Maybe I am misunderstanding but the packet was forwarded by another device. > In our case from macvlan: > > macvlan_start_xmit > macvlan_queue_xmit > dest->forward > dev_skb_forward > > When we reached dev_skb_forward, that means we passed through > dev_hard_start_xmit where the packet was already segmented so we should > exit at the first test (skb->len < len). I don't see the point of adding > the skb_is_gso. > But maybe I am missing something, can you explain ? The macvlan device only has one downstream device (slave). If kernel is working properly, macvlan device should have a subset of the features of the underlying device and macvlan device should have same MTU as underlying device. If the feature/MTU flags were correct, then the path calling macvlan should be respecting the MTU. -- ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH][v3] dev : fix mtu check when TSO is enabled 2011-03-15 18:17 ` Stephen Hemminger @ 2011-03-16 13:56 ` Daniel Lezcano 2011-03-16 15:35 ` Stephen Hemminger 0 siblings, 1 reply; 12+ messages in thread From: Daniel Lezcano @ 2011-03-16 13:56 UTC (permalink / raw) To: Stephen Hemminger; +Cc: David Miller, eric.dumazet, kaber, nightnord, netdev On 03/15/2011 07:17 PM, Stephen Hemminger wrote: > On Tue, 15 Mar 2011 14:57:40 +0100 > Daniel Lezcano<daniel.lezcano@free.fr> wrote: > >> On 03/15/2011 12:59 AM, David Miller wrote: >>> From: Daniel Lezcano<daniel.lezcano@free.fr> >>> Date: Mon, 14 Mar 2011 21:39:50 +0100 >>> >>>> + len = dev->mtu + dev->hard_header_len + VLAN_HLEN; >>>> + if (skb->len< len) >>>> + return true; >>> This is not a correct translation of the original test: >>> >>>> - (skb->len> (dev->mtu + dev->hard_header_len + VLAN_HLEN)))) { >>> You need to use "<=" in your version, which currently rejects all >>> full sized frames. :-) >> Right, thanks. >> >>>> + >>>> + /* if TSO is enabled, we don't care about the length as the packet >>>> + * could be forwarded without being segmented before >>>> + */ >>>> + if (skb->dev&& skb->dev->features& NETIF_F_TSO) >>>> + return true; >>> I am trying to understand why you aren't simply checking also if this >>> is a segmented frame? Perhaps skb_is_gso()&& device has NETIF_F_TSO >>> set? >> Maybe I am misunderstanding but the packet was forwarded by another device. >> In our case from macvlan: >> >> macvlan_start_xmit >> macvlan_queue_xmit >> dest->forward >> dev_skb_forward >> >> When we reached dev_skb_forward, that means we passed through >> dev_hard_start_xmit where the packet was already segmented so we should >> exit at the first test (skb->len< len). I don't see the point of adding >> the skb_is_gso. >> But maybe I am missing something, can you explain ? > The macvlan device only has one downstream device (slave). > If kernel is working properly, macvlan device should have a subset > of the features of the underlying device Right, dev->features = lowerdev->features & MACVLAN_FEATURES > and macvlan device should > have same MTU as underlying device. Right, ... if (!tb[IFLA_MTU]) dev->mtu = lowerdev->mtu; ... > If the feature/MTU flags > were correct, then the path calling macvlan should be respecting > the MTU. But if the TSO is enabled on the macvlan (inherited from eg e1000), the packet won't be fragmented to the mtu size no ? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH][v3] dev : fix mtu check when TSO is enabled 2011-03-16 13:56 ` Daniel Lezcano @ 2011-03-16 15:35 ` Stephen Hemminger 2011-03-16 16:19 ` Daniel Lezcano 0 siblings, 1 reply; 12+ messages in thread From: Stephen Hemminger @ 2011-03-16 15:35 UTC (permalink / raw) To: Daniel Lezcano; +Cc: David Miller, eric.dumazet, kaber, nightnord, netdev On Wed, 16 Mar 2011 14:56:09 +0100 Daniel Lezcano <daniel.lezcano@free.fr> wrote: > On 03/15/2011 07:17 PM, Stephen Hemminger wrote: > > On Tue, 15 Mar 2011 14:57:40 +0100 > > Daniel Lezcano<daniel.lezcano@free.fr> wrote: > > > >> On 03/15/2011 12:59 AM, David Miller wrote: > >>> From: Daniel Lezcano<daniel.lezcano@free.fr> > >>> Date: Mon, 14 Mar 2011 21:39:50 +0100 > >>> > >>>> + len = dev->mtu + dev->hard_header_len + VLAN_HLEN; > >>>> + if (skb->len< len) > >>>> + return true; > >>> This is not a correct translation of the original test: > >>> > >>>> - (skb->len> (dev->mtu + dev->hard_header_len + VLAN_HLEN)))) { > >>> You need to use "<=" in your version, which currently rejects all > >>> full sized frames. :-) > >> Right, thanks. > >> > >>>> + > >>>> + /* if TSO is enabled, we don't care about the length as the packet > >>>> + * could be forwarded without being segmented before > >>>> + */ > >>>> + if (skb->dev&& skb->dev->features& NETIF_F_TSO) > >>>> + return true; > >>> I am trying to understand why you aren't simply checking also if this > >>> is a segmented frame? Perhaps skb_is_gso()&& device has NETIF_F_TSO > >>> set? > >> Maybe I am misunderstanding but the packet was forwarded by another device. > >> In our case from macvlan: > >> > >> macvlan_start_xmit > >> macvlan_queue_xmit > >> dest->forward > >> dev_skb_forward > >> > >> When we reached dev_skb_forward, that means we passed through > >> dev_hard_start_xmit where the packet was already segmented so we should > >> exit at the first test (skb->len< len). I don't see the point of adding > >> the skb_is_gso. > >> But maybe I am missing something, can you explain ? > > The macvlan device only has one downstream device (slave). > > If kernel is working properly, macvlan device should have a subset > > of the features of the underlying device > > Right, dev->features = lowerdev->features & MACVLAN_FEATURES > > > and macvlan device should > > have same MTU as underlying device. > > Right, > > ... > > if (!tb[IFLA_MTU]) > dev->mtu = lowerdev->mtu; > > ... > > If the feature/MTU flags > > were correct, then the path calling macvlan should be respecting > > the MTU. > > But if the TSO is enabled on the macvlan (inherited from eg e1000), the > packet won't be fragmented to the mtu size no ? That is the responsiblity of the hardware that receives the packet. Macvlan should be passing it through to the lowerdev and since the hardware supports TSO, it will fragment it. -- ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH][v3] dev : fix mtu check when TSO is enabled 2011-03-16 15:35 ` Stephen Hemminger @ 2011-03-16 16:19 ` Daniel Lezcano 2011-03-16 16:45 ` Stephen Hemminger 0 siblings, 1 reply; 12+ messages in thread From: Daniel Lezcano @ 2011-03-16 16:19 UTC (permalink / raw) To: Stephen Hemminger; +Cc: David Miller, eric.dumazet, kaber, nightnord, netdev On 03/16/2011 04:35 PM, Stephen Hemminger wrote: > On Wed, 16 Mar 2011 14:56:09 +0100 > Daniel Lezcano<daniel.lezcano@free.fr> wrote: > >> On 03/15/2011 07:17 PM, Stephen Hemminger wrote: >>> On Tue, 15 Mar 2011 14:57:40 +0100 >>> Daniel Lezcano<daniel.lezcano@free.fr> wrote: >>> >>>> On 03/15/2011 12:59 AM, David Miller wrote: >>>>> From: Daniel Lezcano<daniel.lezcano@free.fr> >>>>> Date: Mon, 14 Mar 2011 21:39:50 +0100 >>>>> >>>>>> + len = dev->mtu + dev->hard_header_len + VLAN_HLEN; >>>>>> + if (skb->len< len) >>>>>> + return true; >>>>> This is not a correct translation of the original test: >>>>> >>>>>> - (skb->len> (dev->mtu + dev->hard_header_len + VLAN_HLEN)))) { >>>>> You need to use "<=" in your version, which currently rejects all >>>>> full sized frames. :-) >>>> Right, thanks. >>>> >>>>>> + >>>>>> + /* if TSO is enabled, we don't care about the length as the packet >>>>>> + * could be forwarded without being segmented before >>>>>> + */ >>>>>> + if (skb->dev&& skb->dev->features& NETIF_F_TSO) >>>>>> + return true; >>>>> I am trying to understand why you aren't simply checking also if this >>>>> is a segmented frame? Perhaps skb_is_gso()&& device has NETIF_F_TSO >>>>> set? >>>> Maybe I am misunderstanding but the packet was forwarded by another device. >>>> In our case from macvlan: >>>> >>>> macvlan_start_xmit >>>> macvlan_queue_xmit >>>> dest->forward >>>> dev_skb_forward >>>> >>>> When we reached dev_skb_forward, that means we passed through >>>> dev_hard_start_xmit where the packet was already segmented so we should >>>> exit at the first test (skb->len< len). I don't see the point of adding >>>> the skb_is_gso. >>>> But maybe I am missing something, can you explain ? >>> The macvlan device only has one downstream device (slave). >>> If kernel is working properly, macvlan device should have a subset >>> of the features of the underlying device >> Right, dev->features = lowerdev->features& MACVLAN_FEATURES >> >>> and macvlan device should >>> have same MTU as underlying device. >> Right, >> >> ... >> >> if (!tb[IFLA_MTU]) >> dev->mtu = lowerdev->mtu; >> >> ... >>> If the feature/MTU flags >>> were correct, then the path calling macvlan should be respecting >>> the MTU. >> But if the TSO is enabled on the macvlan (inherited from eg e1000), the >> packet won't be fragmented to the mtu size no ? > That is the responsiblity of the hardware that receives the packet. > Macvlan should be passing it through to the lowerdev and since the hardware > supports TSO, it will fragment it. Ok, but in the case the macvlan is in bridge mode, the dev_skb_forward function will forward the packet (which is not fragmented) to to another macvlan port without going through the hardware driver. In this function, the packet length is checked against the mtu size and of course the packet is dropped in case the lower device support the TSO (if the packet is larger than the mtu size). Dave suggested to check skb_is_gso and against the TSO feature of the macvlan but I don't understand why we should check skb_is_gso too. if (skb_is_gso(skb)&& (skb->dev&& skb->dev->features& NETIF_F_TSO)) return true; ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH][v3] dev : fix mtu check when TSO is enabled 2011-03-16 16:19 ` Daniel Lezcano @ 2011-03-16 16:45 ` Stephen Hemminger 2011-03-21 22:58 ` Eric W. Biederman 0 siblings, 1 reply; 12+ messages in thread From: Stephen Hemminger @ 2011-03-16 16:45 UTC (permalink / raw) To: Daniel Lezcano; +Cc: David Miller, eric.dumazet, kaber, nightnord, netdev On Wed, 16 Mar 2011 17:19:14 +0100 Daniel Lezcano <daniel.lezcano@free.fr> wrote: > On 03/16/2011 04:35 PM, Stephen Hemminger wrote: > > On Wed, 16 Mar 2011 14:56:09 +0100 > > Daniel Lezcano<daniel.lezcano@free.fr> wrote: > > > >> On 03/15/2011 07:17 PM, Stephen Hemminger wrote: > >>> On Tue, 15 Mar 2011 14:57:40 +0100 > >>> Daniel Lezcano<daniel.lezcano@free.fr> wrote: > >>> > >>>> On 03/15/2011 12:59 AM, David Miller wrote: > >>>>> From: Daniel Lezcano<daniel.lezcano@free.fr> > >>>>> Date: Mon, 14 Mar 2011 21:39:50 +0100 > >>>>> > >>>>>> + len = dev->mtu + dev->hard_header_len + VLAN_HLEN; > >>>>>> + if (skb->len< len) > >>>>>> + return true; > >>>>> This is not a correct translation of the original test: > >>>>> > >>>>>> - (skb->len> (dev->mtu + dev->hard_header_len + VLAN_HLEN)))) { > >>>>> You need to use "<=" in your version, which currently rejects all > >>>>> full sized frames. :-) > >>>> Right, thanks. > >>>> > >>>>>> + > >>>>>> + /* if TSO is enabled, we don't care about the length as the packet > >>>>>> + * could be forwarded without being segmented before > >>>>>> + */ > >>>>>> + if (skb->dev&& skb->dev->features& NETIF_F_TSO) > >>>>>> + return true; > >>>>> I am trying to understand why you aren't simply checking also if this > >>>>> is a segmented frame? Perhaps skb_is_gso()&& device has NETIF_F_TSO > >>>>> set? > >>>> Maybe I am misunderstanding but the packet was forwarded by another device. > >>>> In our case from macvlan: > >>>> > >>>> macvlan_start_xmit > >>>> macvlan_queue_xmit > >>>> dest->forward > >>>> dev_skb_forward > >>>> > >>>> When we reached dev_skb_forward, that means we passed through > >>>> dev_hard_start_xmit where the packet was already segmented so we should > >>>> exit at the first test (skb->len< len). I don't see the point of adding > >>>> the skb_is_gso. > >>>> But maybe I am missing something, can you explain ? > >>> The macvlan device only has one downstream device (slave). > >>> If kernel is working properly, macvlan device should have a subset > >>> of the features of the underlying device > >> Right, dev->features = lowerdev->features& MACVLAN_FEATURES > >> > >>> and macvlan device should > >>> have same MTU as underlying device. > >> Right, > >> > >> ... > >> > >> if (!tb[IFLA_MTU]) > >> dev->mtu = lowerdev->mtu; > >> > >> ... > >>> If the feature/MTU flags > >>> were correct, then the path calling macvlan should be respecting > >>> the MTU. > >> But if the TSO is enabled on the macvlan (inherited from eg e1000), the > >> packet won't be fragmented to the mtu size no ? > > That is the responsiblity of the hardware that receives the packet. > > Macvlan should be passing it through to the lowerdev and since the hardware > > supports TSO, it will fragment it. > > Ok, but in the case the macvlan is in bridge mode, the dev_skb_forward > function will forward the packet (which is not fragmented) to to another > macvlan port without going through the hardware driver. In this > function, the packet length is checked against the mtu size and of > course the packet is dropped in case the lower device support the TSO > (if the packet is larger than the mtu size). Dave suggested to check > skb_is_gso and against the TSO feature of the macvlan but I don't > understand why we should check skb_is_gso too. > > if (skb_is_gso(skb)&& (skb->dev&& skb->dev->features& NETIF_F_TSO)) > return true; > > Then it is up to macvlan to do the same thing as bridge code. static inline unsigned packet_length(const struct sk_buff *skb) { return skb->len - (skb->protocol == htons(ETH_P_8021Q) ? VLAN_HLEN : 0); } int br_dev_queue_push_xmit(struct sk_buff *skb) { /* ip_fragment doesn't copy the MAC header */ if (nf_bridge_maybe_copy_header(skb) || (packet_length(skb) > skb->dev->mtu && !skb_is_gso(skb))) { kfree_skb(skb); } else { skb_push(skb, ETH_HLEN); dev_queue_xmit(skb); } return 0; } Ps: not sure adding macvlan bridge mode was a good idea, we already have a working bridge code. And there are too many missing pieces in the macvlan implementation of bridging -- ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH][v3] dev : fix mtu check when TSO is enabled 2011-03-16 16:45 ` Stephen Hemminger @ 2011-03-21 22:58 ` Eric W. Biederman 2011-03-22 2:31 ` Jesse Gross 0 siblings, 1 reply; 12+ messages in thread From: Eric W. Biederman @ 2011-03-21 22:58 UTC (permalink / raw) To: Stephen Hemminger Cc: Daniel Lezcano, David Miller, eric.dumazet, kaber, nightnord, netdev Stephen Hemminger <shemminger@vyatta.com> writes: > On Wed, 16 Mar 2011 17:19:14 +0100 > Daniel Lezcano <daniel.lezcano@free.fr> wrote: > >> On 03/16/2011 04:35 PM, Stephen Hemminger wrote: >> > On Wed, 16 Mar 2011 14:56:09 +0100 >> > Daniel Lezcano<daniel.lezcano@free.fr> wrote: >> > >> >> On 03/15/2011 07:17 PM, Stephen Hemminger wrote: >> >>> On Tue, 15 Mar 2011 14:57:40 +0100 >> >>> Daniel Lezcano<daniel.lezcano@free.fr> wrote: >> >>> >> >>>> On 03/15/2011 12:59 AM, David Miller wrote: >> >>>>> From: Daniel Lezcano<daniel.lezcano@free.fr> >> >>>>> Date: Mon, 14 Mar 2011 21:39:50 +0100 >> >>>>> >> >>>>>> + len = dev->mtu + dev->hard_header_len + VLAN_HLEN; >> >>>>>> + if (skb->len< len) >> >>>>>> + return true; >> >>>>> This is not a correct translation of the original test: >> >>>>> >> >>>>>> - (skb->len> (dev->mtu + dev->hard_header_len + VLAN_HLEN)))) { >> >>>>> You need to use "<=" in your version, which currently rejects all >> >>>>> full sized frames. :-) >> >>>> Right, thanks. >> >>>> >> >>>>>> + >> >>>>>> + /* if TSO is enabled, we don't care about the length as the packet >> >>>>>> + * could be forwarded without being segmented before >> >>>>>> + */ >> >>>>>> + if (skb->dev&& skb->dev->features& NETIF_F_TSO) >> >>>>>> + return true; >> >>>>> I am trying to understand why you aren't simply checking also if this >> >>>>> is a segmented frame? Perhaps skb_is_gso()&& device has NETIF_F_TSO >> >>>>> set? >> >>>> Maybe I am misunderstanding but the packet was forwarded by another device. >> >>>> In our case from macvlan: >> >>>> >> >>>> macvlan_start_xmit >> >>>> macvlan_queue_xmit >> >>>> dest->forward >> >>>> dev_skb_forward >> >>>> >> >>>> When we reached dev_skb_forward, that means we passed through >> >>>> dev_hard_start_xmit where the packet was already segmented so we should >> >>>> exit at the first test (skb->len< len). I don't see the point of adding >> >>>> the skb_is_gso. >> >>>> But maybe I am missing something, can you explain ? >> >>> The macvlan device only has one downstream device (slave). >> >>> If kernel is working properly, macvlan device should have a subset >> >>> of the features of the underlying device >> >> Right, dev->features = lowerdev->features& MACVLAN_FEATURES >> >> >> >>> and macvlan device should >> >>> have same MTU as underlying device. >> >> Right, >> >> >> >> ... >> >> >> >> if (!tb[IFLA_MTU]) >> >> dev->mtu = lowerdev->mtu; >> >> >> >> ... >> >>> If the feature/MTU flags >> >>> were correct, then the path calling macvlan should be respecting >> >>> the MTU. >> >> But if the TSO is enabled on the macvlan (inherited from eg e1000), the >> >> packet won't be fragmented to the mtu size no ? >> > That is the responsiblity of the hardware that receives the packet. >> > Macvlan should be passing it through to the lowerdev and since the hardware >> > supports TSO, it will fragment it. >> >> Ok, but in the case the macvlan is in bridge mode, the dev_skb_forward >> function will forward the packet (which is not fragmented) to to another >> macvlan port without going through the hardware driver. In this >> function, the packet length is checked against the mtu size and of >> course the packet is dropped in case the lower device support the TSO >> (if the packet is larger than the mtu size). Dave suggested to check >> skb_is_gso and against the TSO feature of the macvlan but I don't >> understand why we should check skb_is_gso too. >> >> if (skb_is_gso(skb)&& (skb->dev&& skb->dev->features& NETIF_F_TSO)) >> return true; >> >> > > Then it is up to macvlan to do the same thing as bridge code. > > static inline unsigned packet_length(const struct sk_buff *skb) > { > return skb->len - (skb->protocol == htons(ETH_P_8021Q) ? VLAN_HLEN : 0); > } Which is incorrect in this context. skb->len at this point in the code includes the ethernet header, as we are down below dev_queue_xmit. The test really does need to be dev->mtu + dev->hard_header_len. As for conditionally include the VLAN_HLEN that is likely doable but I think if we are being pedantic that case needs to deal with accelerated vlan headers. > int br_dev_queue_push_xmit(struct sk_buff *skb) > { > /* ip_fragment doesn't copy the MAC header */ > if (nf_bridge_maybe_copy_header(skb) || > (packet_length(skb) > skb->dev->mtu && !skb_is_gso(skb))) { > kfree_skb(skb); > } else { > skb_push(skb, ETH_HLEN); > dev_queue_xmit(skb); > } e> > return 0; > } > Ps: not sure adding macvlan bridge mode was a good idea, we already have > a working bridge code. And there are too many missing pieces in the macvlan > implementation of bridging The macvlan is a trivial bridge that doesn't need to do learning as it knows every mac address it can receive, so it doesn't need to implement learning or stp. Which makes it simple stupid and and fast. Strictly speaking the problems we are running into are the problems of hardware acceleration and software devices, not properly providing the same interfaces as the hardware. To not take a significant hit on performance we do want hardware acceleration on the path to real hardware from the transmitter. To use the network bridge to cross network namespaces we would have to add veth pairs and unless something has drastically changed we would still need to emulate all of the hardware acceleration in so the dev_forward_skb so that we don't provide a device to the bridge that has lesser capabilities than the real hardware and downgrade the capabilities of the entire bridge. So I don't think there is much that would be gained by using bridges instead of the macvlan driver. Eric ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH][v3] dev : fix mtu check when TSO is enabled 2011-03-21 22:58 ` Eric W. Biederman @ 2011-03-22 2:31 ` Jesse Gross 2011-03-22 3:02 ` Eric W. Biederman 0 siblings, 1 reply; 12+ messages in thread From: Jesse Gross @ 2011-03-22 2:31 UTC (permalink / raw) To: Eric W. Biederman Cc: Stephen Hemminger, Daniel Lezcano, David Miller, eric.dumazet, kaber, nightnord, netdev On Mon, Mar 21, 2011 at 3:58 PM, Eric W. Biederman <ebiederm@xmission.com> wrote: > Stephen Hemminger <shemminger@vyatta.com> writes: > >> On Wed, 16 Mar 2011 17:19:14 +0100 >> Daniel Lezcano <daniel.lezcano@free.fr> wrote: >> >>> On 03/16/2011 04:35 PM, Stephen Hemminger wrote: >>> > On Wed, 16 Mar 2011 14:56:09 +0100 >>> > Daniel Lezcano<daniel.lezcano@free.fr> wrote: >>> > >>> >> On 03/15/2011 07:17 PM, Stephen Hemminger wrote: >>> >>> On Tue, 15 Mar 2011 14:57:40 +0100 >>> >>> Daniel Lezcano<daniel.lezcano@free.fr> wrote: >>> >>> >>> >>>> On 03/15/2011 12:59 AM, David Miller wrote: >>> >>>>> From: Daniel Lezcano<daniel.lezcano@free.fr> >>> >>>>> Date: Mon, 14 Mar 2011 21:39:50 +0100 >>> >>>>> >>> >>>>>> + len = dev->mtu + dev->hard_header_len + VLAN_HLEN; >>> >>>>>> + if (skb->len< len) >>> >>>>>> + return true; >>> >>>>> This is not a correct translation of the original test: >>> >>>>> >>> >>>>>> - (skb->len> (dev->mtu + dev->hard_header_len + VLAN_HLEN)))) { >>> >>>>> You need to use "<=" in your version, which currently rejects all >>> >>>>> full sized frames. :-) >>> >>>> Right, thanks. >>> >>>> >>> >>>>>> + >>> >>>>>> + /* if TSO is enabled, we don't care about the length as the packet >>> >>>>>> + * could be forwarded without being segmented before >>> >>>>>> + */ >>> >>>>>> + if (skb->dev&& skb->dev->features& NETIF_F_TSO) >>> >>>>>> + return true; >>> >>>>> I am trying to understand why you aren't simply checking also if this >>> >>>>> is a segmented frame? Perhaps skb_is_gso()&& device has NETIF_F_TSO >>> >>>>> set? >>> >>>> Maybe I am misunderstanding but the packet was forwarded by another device. >>> >>>> In our case from macvlan: >>> >>>> >>> >>>> macvlan_start_xmit >>> >>>> macvlan_queue_xmit >>> >>>> dest->forward >>> >>>> dev_skb_forward >>> >>>> >>> >>>> When we reached dev_skb_forward, that means we passed through >>> >>>> dev_hard_start_xmit where the packet was already segmented so we should >>> >>>> exit at the first test (skb->len< len). I don't see the point of adding >>> >>>> the skb_is_gso. >>> >>>> But maybe I am missing something, can you explain ? >>> >>> The macvlan device only has one downstream device (slave). >>> >>> If kernel is working properly, macvlan device should have a subset >>> >>> of the features of the underlying device >>> >> Right, dev->features = lowerdev->features& MACVLAN_FEATURES >>> >> >>> >>> and macvlan device should >>> >>> have same MTU as underlying device. >>> >> Right, >>> >> >>> >> ... >>> >> >>> >> if (!tb[IFLA_MTU]) >>> >> dev->mtu = lowerdev->mtu; >>> >> >>> >> ... >>> >>> If the feature/MTU flags >>> >>> were correct, then the path calling macvlan should be respecting >>> >>> the MTU. >>> >> But if the TSO is enabled on the macvlan (inherited from eg e1000), the >>> >> packet won't be fragmented to the mtu size no ? >>> > That is the responsiblity of the hardware that receives the packet. >>> > Macvlan should be passing it through to the lowerdev and since the hardware >>> > supports TSO, it will fragment it. >>> >>> Ok, but in the case the macvlan is in bridge mode, the dev_skb_forward >>> function will forward the packet (which is not fragmented) to to another >>> macvlan port without going through the hardware driver. In this >>> function, the packet length is checked against the mtu size and of >>> course the packet is dropped in case the lower device support the TSO >>> (if the packet is larger than the mtu size). Dave suggested to check >>> skb_is_gso and against the TSO feature of the macvlan but I don't >>> understand why we should check skb_is_gso too. >>> >>> if (skb_is_gso(skb)&& (skb->dev&& skb->dev->features& NETIF_F_TSO)) >>> return true; >>> >>> >> >> Then it is up to macvlan to do the same thing as bridge code. >> >> static inline unsigned packet_length(const struct sk_buff *skb) >> { >> return skb->len - (skb->protocol == htons(ETH_P_8021Q) ? VLAN_HLEN : 0); >> } > > Which is incorrect in this context. skb->len at this point in the code > includes the ethernet header, as we are down below dev_queue_xmit. > > The test really does need to be dev->mtu + dev->hard_header_len. > > As for conditionally include the VLAN_HLEN that is likely doable but I > think if we are being pedantic that case needs to deal with accelerated > vlan headers. If vlan acceleration is in use then the protocol is not ETH_P_8021Q and the tag is not included in skb->len. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH][v3] dev : fix mtu check when TSO is enabled 2011-03-22 2:31 ` Jesse Gross @ 2011-03-22 3:02 ` Eric W. Biederman 0 siblings, 0 replies; 12+ messages in thread From: Eric W. Biederman @ 2011-03-22 3:02 UTC (permalink / raw) To: Jesse Gross Cc: Stephen Hemminger, Daniel Lezcano, David Miller, eric.dumazet, kaber, nightnord, netdev Jesse Gross <jesse@nicira.com> writes: > On Mon, Mar 21, 2011 at 3:58 PM, Eric W. Biederman > <ebiederm@xmission.com> wrote: >> Stephen Hemminger <shemminger@vyatta.com> writes: >> >>> On Wed, 16 Mar 2011 17:19:14 +0100 >>> Daniel Lezcano <daniel.lezcano@free.fr> wrote: >>> >>>> On 03/16/2011 04:35 PM, Stephen Hemminger wrote: >>>> > On Wed, 16 Mar 2011 14:56:09 +0100 >>>> > Daniel Lezcano<daniel.lezcano@free.fr> wrote: >>>> > >>>> >> On 03/15/2011 07:17 PM, Stephen Hemminger wrote: >>>> >>> On Tue, 15 Mar 2011 14:57:40 +0100 >>>> >>> Daniel Lezcano<daniel.lezcano@free.fr> wrote: >>>> >>> >>>> >>>> On 03/15/2011 12:59 AM, David Miller wrote: >>>> >>>>> From: Daniel Lezcano<daniel.lezcano@free.fr> >>>> >>>>> Date: Mon, 14 Mar 2011 21:39:50 +0100 >>>> >>>>> >>>> >>>>>> + len = dev->mtu + dev->hard_header_len + VLAN_HLEN; >>>> >>>>>> + if (skb->len< len) >>>> >>>>>> + return true; >>>> >>>>> This is not a correct translation of the original test: >>>> >>>>> >>>> >>>>>> - (skb->len> (dev->mtu + dev->hard_header_len + VLAN_HLEN)))) { >>>> >>>>> You need to use "<=" in your version, which currently rejects all >>>> >>>>> full sized frames. :-) >>>> >>>> Right, thanks. >>>> >>>> >>>> >>>>>> + >>>> >>>>>> + /* if TSO is enabled, we don't care about the length as the packet >>>> >>>>>> + * could be forwarded without being segmented before >>>> >>>>>> + */ >>>> >>>>>> + if (skb->dev&& skb->dev->features& NETIF_F_TSO) >>>> >>>>>> + return true; >>>> >>>>> I am trying to understand why you aren't simply checking also if this >>>> >>>>> is a segmented frame? Perhaps skb_is_gso()&& device has NETIF_F_TSO >>>> >>>>> set? >>>> >>>> Maybe I am misunderstanding but the packet was forwarded by another device. >>>> >>>> In our case from macvlan: >>>> >>>> >>>> >>>> macvlan_start_xmit >>>> >>>> macvlan_queue_xmit >>>> >>>> dest->forward >>>> >>>> dev_skb_forward >>>> >>>> >>>> >>>> When we reached dev_skb_forward, that means we passed through >>>> >>>> dev_hard_start_xmit where the packet was already segmented so we should >>>> >>>> exit at the first test (skb->len< len). I don't see the point of adding >>>> >>>> the skb_is_gso. >>>> >>>> But maybe I am missing something, can you explain ? >>>> >>> The macvlan device only has one downstream device (slave). >>>> >>> If kernel is working properly, macvlan device should have a subset >>>> >>> of the features of the underlying device >>>> >> Right, dev->features = lowerdev->features& MACVLAN_FEATURES >>>> >> >>>> >>> and macvlan device should >>>> >>> have same MTU as underlying device. >>>> >> Right, >>>> >> >>>> >> ... >>>> >> >>>> >> if (!tb[IFLA_MTU]) >>>> >> dev->mtu = lowerdev->mtu; >>>> >> >>>> >> ... >>>> >>> If the feature/MTU flags >>>> >>> were correct, then the path calling macvlan should be respecting >>>> >>> the MTU. >>>> >> But if the TSO is enabled on the macvlan (inherited from eg e1000), the >>>> >> packet won't be fragmented to the mtu size no ? >>>> > That is the responsiblity of the hardware that receives the packet. >>>> > Macvlan should be passing it through to the lowerdev and since the hardware >>>> > supports TSO, it will fragment it. >>>> >>>> Ok, but in the case the macvlan is in bridge mode, the dev_skb_forward >>>> function will forward the packet (which is not fragmented) to to another >>>> macvlan port without going through the hardware driver. In this >>>> function, the packet length is checked against the mtu size and of >>>> course the packet is dropped in case the lower device support the TSO >>>> (if the packet is larger than the mtu size). Dave suggested to check >>>> skb_is_gso and against the TSO feature of the macvlan but I don't >>>> understand why we should check skb_is_gso too. >>>> >>>> if (skb_is_gso(skb)&& (skb->dev&& skb->dev->features& NETIF_F_TSO)) >>>> return true; >>>> >>>> >>> >>> Then it is up to macvlan to do the same thing as bridge code. >>> >>> static inline unsigned packet_length(const struct sk_buff *skb) >>> { >>> return skb->len - (skb->protocol == htons(ETH_P_8021Q) ? VLAN_HLEN : 0); >>> } >> >> Which is incorrect in this context. skb->len at this point in the code >> includes the ethernet header, as we are down below dev_queue_xmit. >> >> The test really does need to be dev->mtu + dev->hard_header_len. >> >> As for conditionally include the VLAN_HLEN that is likely doable but I >> think if we are being pedantic that case needs to deal with accelerated >> vlan headers. > > If vlan acceleration is in use then the protocol is not ETH_P_8021Q > and the tag is not included in skb->len. Except for the case of double tagged packets. Eric ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH][v3] dev : fix mtu check when TSO is enabled 2011-03-14 23:59 ` David Miller 2011-03-15 13:57 ` Daniel Lezcano @ 2011-03-22 0:05 ` Michał Mirosław 1 sibling, 0 replies; 12+ messages in thread From: Michał Mirosław @ 2011-03-22 0:05 UTC (permalink / raw) To: David Miller; +Cc: daniel.lezcano, eric.dumazet, kaber, nightnord, netdev 2011/3/15 David Miller <davem@davemloft.net>: > From: Daniel Lezcano <daniel.lezcano@free.fr> > Date: Mon, 14 Mar 2011 21:39:50 +0100 >> + >> + /* if TSO is enabled, we don't care about the length as the packet >> + * could be forwarded without being segmented before >> + */ >> + if (skb->dev && skb->dev->features & NETIF_F_TSO) >> + return true; > I am trying to understand why you aren't simply checking also if this > is a segmented frame? Perhaps skb_is_gso() && device has NETIF_F_TSO > set? Is the check of netdev features even necessary? Devices without TSO enabled the skbs are segmented before ndo_start_xmit() anyway. With TSO there's a question if the receiving end of macvlan can handle GRO/LRO packets? BTW, checking only NETIF_F_TSO will break at least for IPv6 TSO (NETIF_F_TSO6) or whatever else can be segmented in hardware. Best Regards, Michał Mirosław ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2011-03-22 3:02 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-03-14 20:39 [PATCH][v3] dev : fix mtu check when TSO is enabled Daniel Lezcano 2011-03-14 23:59 ` David Miller 2011-03-15 13:57 ` Daniel Lezcano 2011-03-15 18:17 ` Stephen Hemminger 2011-03-16 13:56 ` Daniel Lezcano 2011-03-16 15:35 ` Stephen Hemminger 2011-03-16 16:19 ` Daniel Lezcano 2011-03-16 16:45 ` Stephen Hemminger 2011-03-21 22:58 ` Eric W. Biederman 2011-03-22 2:31 ` Jesse Gross 2011-03-22 3:02 ` Eric W. Biederman 2011-03-22 0:05 ` Michał Mirosław
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).