Re: [B.A.T.M.A.N.] [PATCHv2 3/3] batman-adv: Fragment and send skbs larger than mtu

Antonio Quartulli
2013-05-15T07:27:35Z

[-- Attachment #1: Type: text/plain, Size: 2792 bytes --]

Hi³, :)

On Mon, May 13, 2013 at 10:23:02PM +0200, Martin Hundebøll wrote:

[..]

> +
> +/**
> + * batadv_frag_create - create a fragment from skb.
> + * @skb: skb to create fragment from.
> + * @frag_head: header to use in new fragment.
> + * @mtu: Size of new fragment.
> + *
> + * Split the passed skb into two fragments: A new one with size matching the
> + * passed mtu and the old one with the rest. The new skb contains data from the
> + * tail of the old skb.
> + *
> + * Returns the new fragment, NULL on error.
> + */
> +static struct sk_buff *batadv_frag_create(struct sk_buff *skb,
> + struct batadv_frag_packet *frag_head,
> + unsigned int mtu)
> +{
> + struct sk_buff *skb_fragment;
> + unsigned header_size = sizeof(*frag_head);
> + unsigned fragment_size = mtu - header_size - ETH_HLEN;
> +
> + skb_fragment = dev_alloc_skb(mtu);

If I am not wrong, the external Ethernet header is not counted in the MTU of an
interface. Therefore if the MTU of eth0 is X, it should mean that eth0 will send
a packet of 1500 of payload + the external Ethernet encapsulation. isn't it?

> + if (!skb_fragment)
> + goto err;
> +
> + /* Eat the last mtu-bytes of the skb */
> + skb_reserve(skb_fragment, header_size);

why not reserving header_size + ETH_HLEN (ETH_HLEN would be the space for the
external Ethernet header).

> + skb_split(skb, skb_fragment, skb->len - fragment_size);
> +
> + /* Add the header */
> + skb_push(skb_fragment, header_size);
> + memcpy(skb_fragment->data, frag_head, header_size);
> +
> +err:
> + return skb_fragment;
> +}
> +

[...]

> @@ -110,7 +109,19 @@ int batadv_send_skb_to_orig(struct sk_buff *skb,
> /* batadv_find_router() increases neigh_nodes refcount if found. */
> neigh_node = batadv_find_router(bat_priv, orig_node, recv_if);
> if (!neigh_node)
> - return ret;
> + goto out;
> +
> + /* Check if the skb is too large to send in one piece and fragment
> + * it if needed.
> + */
> + if (atomic_read(&bat_priv->fragmentation) &&
> + skb->len + ETH_HLEN > neigh_node->if_incoming->net_dev->mtu) {

same as before: the external Ethernet header should not be taken into
consideration while checking the MTU (no?).

My suggestion is to do:

"skb->len > neigh_node->if_incoming->net_dev->mtu) {"

> + /* Fragment and send packet. */
> + if (batadv_frag_send_packet(skb, orig_node, neigh_node))
> + ret = NET_XMIT_SUCCESS;
> +
> + goto out;
> + }
>
> /* try to network code the packet, if it is received on an interface
> * (i.e. being forwarded). If the packet originates from this node or if

Thank you for your work Martin! :)

Cheers,

--
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]