On Tue, Mar 24, 2009 at 6:20 AM, David Miller <da...@davemloft.net> wrote: > From: Stephen Hemminger <shemmin...@linux-foundation.org> > 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 only go up not the other way down in your code. Another thought is that if you re-allocate skb here the driver would be saved from checking the headroom in the fastpath, am I right? - Leo _______________________________________________ Bridge mailing list Bridge@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/bridge