[ovs-dev] [PATCH 04/13] Upstream GRE: Add segmentation offload for GRE TAP device.

[Jesse Gross]

On Thu, Nov 22, 2012 at 7:56 AM, Pravin B Shelar <pshelar at nicira.com> wrote:
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index f2af494..585aca7 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> +static inline int unclone_skb(struct sk_buff *skb, gfp_t pri)

I would call this skb_unclone() instead so that it is more clear what
section of code it belongs to.

> diff --git a/net/ipv4/gre.c b/net/ipv4/gre.c
> index b837551..7568853 100644
> --- a/net/ipv4/gre.c
> +++ b/net/ipv4/gre.c
> +static struct sk_buff *gre_gso_segment(struct sk_buff *skb,
> +                                      netdev_features_t features)
> +{
> +       struct sk_buff *segs = ERR_PTR(-EINVAL);
> +       struct gre_base_hdr *greh;
> +       unsigned char *mac = skb_mac_header(skb);
> +       int mac_len = skb->mac_len;
> +       int net_hlen = skb_network_header_len(skb);
> +       int doffset;
> +       int ghl;
> +
> +       if (unlikely(skb_shinfo(skb)->gso_type &
> +                               ~(SKB_GSO_TCPV4 |
> +                                       SKB_GSO_UDP |
> +                                       SKB_GSO_DODGY |
> +                                       SKB_GSO_TCP_ECN |
> +                                       SKB_GSO_GRE |
> +                                       0)))
> +               goto out;
> +
> +       if (unlikely(!pskb_may_pull(skb, sizeof(*greh))))
> +               goto out;
> +
> +       greh = (struct gre_base_hdr *)skb_transport_header(skb);
> +       ghl = gre_header_len(greh);
> +
> +       if (unlikely(!pskb_may_pull(skb, ghl)))
> +               goto out;
> +
> +       __skb_pull(skb, ghl);
> +       skb_reset_mac_header(skb);
> +       skb_set_network_header(skb, skb->mac_len);

I don't think it's safe to assume that the inner and outer MAC lengths
are the same.  In an ideal world this would actually work for both
Ethernet and IP encapsulated GRE frames but even in the former case,
we could be running Ethernet over GRE over IP over Infiniband.

> +       doffset = skb_mac_header(skb) - mac;
> +       /* segment inner packet. */
> +       segs = skb_gso_segment(skb, 0);
> +       if (!segs || IS_ERR(segs))
> +               goto out;
> +
> +       skb = segs;
> +       do {
> +               unsigned char *smac;
> +
> +               skb_push(skb, doffset);
> +
> +               skb_reset_mac_header(skb);
> +               skb_set_network_header(skb, mac_len);
> +               skb_set_transport_header(skb,
> +                                        skb_network_offset(skb) + net_hlen);
> +               smac = skb_mac_header(skb);
> +               skb->mac_len = mac_len;
> +               /* Copy entire outer header from original skb. */
> +               memmove(smac, mac, doffset);

All of this makes me wonder whether we should factor a MAC layer
handler out of skb_gso_segment().  That way we wouldn't need to have
to fool around with all the layer reseting and copying at this point
since it would just get handled by skb_segment().

> +static int gre_gso_send_check(struct sk_buff *skb)
> +{
> +       return 0;
> +}

Doesn't this have to call into the lower layers?

> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index 829fe3d..6b4e57f 100644
> --- a/net/ipv4/ip_gre.c
> +++ b/net/ipv4/ip_gre.c
>  static int ipgre_tap_init(struct net_device *dev)
>  {
> +       struct ip_tunnel *tunnel = netdev_priv(dev);
>         __gre_tunnel_init(dev);
> +
> +       if (!(tunnel->parms.o_flags & TUNNEL_SEQ)) {
> +               dev->features           |= NETIF_F_TSO;
> +               dev->hw_features        |= NETIF_F_TSO;
> +       }

Can't we implement support for sequence numbers in GSO?  Also, what
about TSO6 and other TCP GSO features?