On Wed, Jun 30, 2010 at 09:00:36PM +0200, Andreas Langer wrote:
diff --git a/batman-adv/fragmentation.c b/batman-adv/fragmentation.c new file mode 100644 index 0000000..f36ad0e --- /dev/null +++ b/batman-adv/fragmentation.c @@ -0,0 +1,150 @@ +/*
- Copyright (C) 2007-2010 B.A.T.M.A.N. contributors:\
Can you please explain why the copyright already starts in 2007?
- /* skb is alsways the first packet,tmp_skb always the second */
alsways -> always?
- /* move free entry to end */
- tfp->skb = NULL;
- tfp->seqno = 0;
- list_move_tail(&tfp->list, head);
- skb_pull(tmp_skb, sizeof(struct unicast_frag_packet));
- pskb_expand_head(skb, 0, tmp_skb->len, GFP_ATOMIC);
pskb_expand_head can fail (return != 0). You cannot just write data in not allocated memory.
- memcpy(skb_put(skb, tmp_skb->len), tmp_skb->data, tmp_skb->len);
- kfree_skb(tmp_skb);
- return skb;
+}
diff --git a/batman-adv/fragmentation.h b/batman-adv/fragmentation.h new file mode 100644 index 0000000..d0acf73 --- /dev/null +++ b/batman-adv/fragmentation.h @@ -0,0 +1,34 @@ +/*
- Copyright (C) 2007-2010 B.A.T.M.A.N. contributors:
Can you please explain why the copyright already starts in 2007?
--- a/batman-adv/hard-interface.c +++ b/batman-adv/hard-interface.c @@ -166,16 +166,18 @@ int hardif_min_mtu(void) /* allow big frames if all devices are capable to do so * (have MTU > 1500 + BAT_HEADER_LEN) */ int min_mtu = ETH_DATA_LEN;
/* FIXME: each batman_if will be attached to a softif */
struct bat_priv *bat_priv = netdev_priv(soft_device);
rcu_read_lock(); list_for_each_entry_rcu(batman_if, &if_list, list) { if ((batman_if->if_status == IF_ACTIVE) || (batman_if->if_status == IF_TO_BE_ACTIVATED))
min_mtu = MIN(batman_if->net_dev->mtu - BAT_HEADER_LEN,min_mtu);
min_mtu = atomic_read(&bat_priv->frag_enabled) ?ETH_DATA_LEN : MIN(batman_if->net_dev->mtu -BAT_HEADER_LEN, min_mtu);
Why do you go through all devices if you already decided that the min_mtu is ETH_DATA_LEN when bat_priv->frag_enabled != 0. The atomic_read ? x : y stuff doesn't make the last statement more readable.
- return min_mtu;
}
Is there a good reason to remove the newline?
@@ -265,8 +267,30 @@ int hardif_enable_interface(struct batman_if *batman_if) orig_hash_add_if(batman_if, bat_priv->num_ifaces);
atomic_set(&batman_if->seqno, 1);
atomic_set(&batman_if->frag_seqno, 1); bat_info(soft_device, "Adding interface: %s\n", batman_if->dev);
if (atomic_read(&bat_priv->frag_enabled) && batman_if->net_dev->mtu <
ETH_DATA_LEN + BAT_HEADER_LEN)bat_info(soft_device,"The MTU of interface %s is too small (%i) to handle ""the transport of batman-adv packets. Packets going ""over this interface will be fragmented on layer2 ""which could impact the performance. Setting the MTU ""to %i would solve the problem.\n",batman_if->dev, ETH_DATA_LEN + BAT_HEADER_LEN,ETH_DATA_LEN + BAT_HEADER_LEN);if (!atomic_read(&bat_priv->frag_enabled) && batman_if->net_dev->mtu <
ETH_DATA_LEN + BAT_HEADER_LEN)bat_info(soft_device,"The MTU of interface %s is too small (%i) to handle ""the transport of batman-adv packets. If you experience"" problems getting traffic through try increasing the ""MTU to %i.\n",batman_if->dev, ETH_DATA_LEN + BAT_HEADER_LEN,ETH_DATA_LEN + BAT_HEADER_LEN);
BAT_HEADER_LEN has implicit the type size_t. So please adjust the 4 %i to %zi.
diff --git a/batman-adv/soft-interface.c b/batman-adv/soft-interface.c index 0d89597..ccc97c6 100644 --- a/batman-adv/soft-interface.c +++ b/batman-adv/soft-interface.c @@ -216,20 +220,72 @@ int interface_tx(struct sk_buff *skb, struct net_device *dev) if (batman_if->if_status != IF_ACTIVE) goto dropped;
if (my_skb_push(skb, sizeof(struct unicast_packet)) < 0)goto dropped;
if (atomic_read(&bat_priv->frag_enabled) &&data_len + sizeof(unicast_packet) >batman_if->net_dev->mtu) {hdr_len = sizeof(struct unicast_frag_packet);frag_skb = dev_alloc_skb(data_len / 2 + hdr_len + 1);
Where does the "+ 1" come from? If it used to compensate uneven data_len, then maybe the calculation should be changed to something like
data_len - (data_len / 2) + hdr_len
skb_split(skb, frag_skb, data_len/2);if (!(my_skb_push(frag_skb, hdr_len) >= 0 &&my_skb_push(skb, hdr_len) >= 0)) {
Didn't you wanted to check if one of the my_skb_push failed and not if both failed?
kfree_skb(frag_skb);goto dropped;}ucast_frag1 = (struct unicast_frag_packet *)skb->data;ucast_frag2 =(struct unicast_frag_packet *)frag_skb->data;ucast_frag1->version = COMPAT_VERSION;ucast_frag1->packet_type = BAT_UNICAST_FRAG;ucast_frag1->ttl = TTL;memcpy(ucast_frag1->orig,batman_if->net_dev->dev_addr, ETH_ALEN);memcpy(ucast_frag1->dest, orig_node->orig, ETH_ALEN);memcpy(ucast_frag2, ucast_frag1,sizeof(struct unicast_frag_packet));ucast_frag1->flags |= UNI_FRAG_HEAD;ucast_frag2->flags &= ~UNI_FRAG_HEAD;/* no zero at seqno */if (atomic_read(&batman_if->frag_seqno) == FRAG_MAX_SEQ)atomic_set(&batman_if->frag_seqno, 0);
It is not a good idea to split two atomic_ operations which depends on each other.
Best regards, Sven