Re: [RFC][net-next-2.6 PATCH 2/4] net: 8021Q consolidate header_ops routines

[Jesse Gross <jesse@nicira.com>]

On Thu, Oct 21, 2010 at 3:10 PM, John Fastabend
<john.r.fastabend@intel.com> wrote:
>  static inline struct sk_buff *vlan_check_reorder_header(struct sk_buff *skb)
>  {
>        if (vlan_dev_info(skb->dev)->flags & VLAN_FLAG_REORDER_HDR) {
> @@ -269,33 +236,26 @@ static int vlan_dev_hard_header(struct sk_buff *skb, struct net_device *dev,
>                                const void *daddr, const void *saddr,
>                                unsigned int len)
>  {
> -       struct vlan_hdr *vhdr;
> -       unsigned int vhdrlen = 0;
> -       u16 vlan_tci = 0;
>        int rc;
>
>        if (WARN_ON(skb_headroom(skb) < dev->hard_header_len))
>                return -ENOSPC;
>
> +       /* When this flag is not set we make the vlan_tci visible
> +        * by setting the skb field.
> +        *
> +        * We do not immediately insert the tag here the intent
> +        * of setting VLAN_FLAG_REORDER_HDR is to make the vlan
> +        * info avaiable to tap devices and the QOS layer. Adding
> +        * the tag present bit shoould be enough for other layers
> +        * to learn the vlan tag.
> +        */

There's a typo in the comment: "shoould".

>        if (!(vlan_dev_info(dev)->flags & VLAN_FLAG_REORDER_HDR)) {
> -               vhdr = (struct vlan_hdr *) skb_push(skb, VLAN_HLEN);
> +               u16 vlan_tci = 0;
>
>                vlan_tci = vlan_dev_info(dev)->vlan_id;
>                vlan_tci |= vlan_dev_get_egress_qos_mask(dev, skb);
> -               vhdr->h_vlan_TCI = htons(vlan_tci);
> -
> -               /*
> -                *  Set the protocol type. For a packet of type ETH_P_802_3/2 we
> -                *  put the length in here instead.
> -                */
> -               if (type != ETH_P_802_3 && type != ETH_P_802_2)
> -                       vhdr->h_vlan_encapsulated_proto = htons(type);
> -               else
> -                       vhdr->h_vlan_encapsulated_proto = htons(len);
> -
> -               skb->protocol = htons(ETH_P_8021Q);
> -               type = ETH_P_8021Q;
> -               vhdrlen = VLAN_HLEN;
> +               skb = __vlan_hwaccel_put_tag(skb, vlan_tci);
>        }

The only possible downside that I can see to this is that in the
non-accelerated case it requires some extra work because we'll need to
move the MAC addresses around as well.  However, given that
VLAN_FLAG_REORDER_HDR is generally set, I think this is a good code
consolidation.

>
>        /* Before delegating work to the lower layer, enter our MAC-address */
> @@ -304,9 +264,7 @@ static int vlan_dev_hard_header(struct sk_buff *skb, struct net_device *dev,
>
>        /* Now make the underlying real hard header */
>        dev = vlan_dev_info(dev)->real_dev;
> -       rc = dev_hard_header(skb, dev, type, daddr, saddr, len + vhdrlen);
> -       if (rc > 0)
> -               rc += vhdrlen;
> +       rc = dev_hard_header(skb, dev, type, daddr, saddr, len);
>        return rc;

Might as well just drop the rc variable.  It's not adding anything anymore.