On Tue, Mar 24, 2009 at 6:20 AM, David Miller <davem@xxxxxxxxxxxxx> wrote:> From: Stephen Hemminger <shemminger@xxxxxxxxxxxxxxxxxxxx>> Date: Mon, 23 Mar 2009 08:51:22 -0700>>> That ensures big enough header for locally generated packets, but>> any drivers that need bigger headroom still must handle bridged packets>> that come in with smaller space. When bridging packets, the skb comes>> from the allocation by the receiving driver. Almost all drivers will>> use dev_alloc_skb() which will allocate NET_SKB_PAD (16) bytes of>> additional headroom. This is used to hold copy of ethernet header for>> the bridge/netfilter code.>>>> So your patch is fine as an optimization but a driver can not safely>> depend on any additional headroom. The driver must check if there>> is space, and if no space is available, reallocate and copy.>> We had some plans to deal with this kind of issue for wireless> too. Let me see if I can find the RFC patch from that discussion...>> Here it is, similar code would be added to the ipv4/ipv6 forwarding> paths:>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h> index 7c1d446..6c06fba 100644> --- a/include/linux/netdevice.h> +++ b/include/linux/netdevice.h> @@ -600,6 +600,7 @@ struct net_device> * Cache line mostly used on receive path (including eth_type_trans())> */> unsigned long last_rx; /* Time of last Rx */> + unsigned int rx_alloc_extra;> /* Interface address info used in eth_type_trans() */> unsigned char dev_addr[MAX_ADDR_LEN]; /* hw address, (before bcast> because most packets are unicast) */> diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c> index bdd7c35..531e483 100644> --- a/net/bridge/br_forward.c> +++ b/net/bridge/br_forward.c> @@ -42,6 +42,22 @@ int br_dev_queue_push_xmit(struct sk_buff *skb)> if (nf_bridge_maybe_copy_header(skb))> kfree_skb(skb);> else {> + unsigned int headroom = skb_headroom(skb);> + unsigned int hh_len = LL_RESERVED_SPACE(skb->dev);> +> + if (headroom < hh_len) {> + struct net_device *in_dev;> + unsigned int extra;> +> + in_dev = __dev_get_by_index(dev_net(skb->dev),> + skb->iif);> + BUG_ON(!in_dev);> +> + extra = hh_len - headroom;> + if (extra >= in_dev->rx_alloc_extra)> + in_dev->rx_alloc_extra = extra;> + }> +> skb_push(skb, ETH_HLEN);>> dev_queue_xmit(skb);
Dynamically adjusting is a good idea, but the rx_alloc_extra can onlygo up not the other way down in your code. Another thought is that ifyou re-allocate skb here the driver would be saved from checking theheadroom in the fastpath, am I right?
- Leo_______________________________________________Bridge mailing listBridge@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx://lists.linux-foundation.org/mailman/listinfo/bridge
