Re: [PATCH][v3] dev : fix mtu check when TSO is enabled

[Stephen Hemminger <shemminger@vyatta.com>]

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



--