[Bridge] [PATCH] net/bridge: use the maximum hard_header_len of ports for bridging device

[Li Yang]

On Tue, Mar 24, 2009 at 6:20 AM, David Miller <davem at davemloft.net> wrote:
> From: Stephen Hemminger <shemminger at 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