[ovs-dev] [PATCH v3] datapath: Add Stateless TCP Tunneling protocol.

Jesse Gross jesse at nicira.com
Fri Apr 10 02:58:12 UTC 2015


On Thu, Apr 9, 2015 at 10:41 AM, Pravin B Shelar <pshelar at nicira.com> wrote:
> diff --git a/datapath/linux/compat/stt.c b/datapath/linux/compat/stt.c
> new file mode 100644
> index 0000000..aaa3650
> --- /dev/null
> +++ b/datapath/linux/compat/stt.c
> +static void copy_skb_metadata(struct sk_buff *to, struct sk_buff *from)
> +{
> +       to->tstamp = from->tstamp;
> +       to->priority = from->priority;
> +       to->mark = from->mark;
> +       to->vlan_tci = from->vlan_tci;
> +       to->vlan_proto = from->vlan_proto;

I think we need a version check here - vlan_proto didn't come until 3.10.

> +static int skb_list_segment(struct sk_buff *head, bool ipv4, int l4_offset)
> +{
> +       struct sk_buff *skb;
> +       struct tcphdr *tcph;
> +       int seg_len;
> +       int hdr_len;
> +       int tcp_len;
> +       u32 seq;
> +
> +       tcph = (struct tcphdr *)(head->data + l4_offset);
> +       tcp_len = tcph->doff * 4;
> +       hdr_len = l4_offset + tcp_len;

I think we need a pskb_may_pull() here.

> +int stt_xmit_skb(struct sk_buff *skb, struct rtable *rt,
> +                __be32 src, __be32 dst, __u8 tos,
> +                __u8 ttl, __be16 df, __be16 src_port, __be16 dst_port,
> +                __be64 tun_id)
[...]
> +       min_headroom = LL_RESERVED_SPACE(rt->dst.dev) + rt->dst.header_len
> +                       + STT_HEADER_LEN + sizeof(struct iphdr)
> +                       + (skb_vlan_tag_present(skb) ? VLAN_HLEN : 0);

I'm not sure that it makes sense to check for the vlan tag here. If we
can do offloading then the tag will be contained in the header. If not
we're going to hit a slow path anyways.

> +       if (skb_headroom(skb) < min_headroom || skb_header_cloned(skb)) {
> +               int head_delta = SKB_DATA_ALIGN(min_headroom -
> +                                               skb_headroom(skb) +
> +                                               16);
> +
> +               ret = pskb_expand_head(skb, max_t(int, head_delta, 0),
> +                                      0, GFP_ATOMIC);
> +               if (unlikely(ret))
> +                       goto err_free_rt;
> +       }
> +
> +       if (!stt_can_offload(skb, inner_h_proto, inner_nw_proto)) {

stt_can_offload() probably needs to check that skb->vlan_proto is the
one that we expect - otherwise we won't get the same thing out on the
other side.

> +               struct sk_buff *nskb;
> +
> +               if (skb_vlan_tag_present(skb)) {
> +                       if (unlikely(!vlan_insert_tag_set_proto(skb,
> +                                                       skb->vlan_proto,
> +                                                       skb_vlan_tag_get(skb)))) {
> +                               ret = -ENOMEM;
> +                               skb = NULL;
> +                               goto err_free_rt;
> +                       }
> +                       vlan_set_tci(skb, 0);
> +               }

Can we replace this with vlan_hwaccel_push_inside()? However, I think
this is only necessary in the case that skb->vlan_proto is not 0x8100.

> +               nskb = handle_offloads(skb);
> +               if (IS_ERR(nskb)) {
> +                       ret = PTR_ERR(nskb);
> +                       goto err_free_rt;
> +               }
> +               skb = nskb;

Can we move the vlan handling into handle_offloads()?

> +static void stt_rcv(struct stt_sock *stt_sock, struct sk_buff *skb)
> +{
[...]
> +       if (skb->next) {
> +               struct stthdr *stth = stt_hdr(skb);
> +               bool csum_partial = !!(stth->flags & STT_CSUM_PARTIAL);
> +               bool ipv4 = !!(stth->flags & STT_PROTO_IPV4);
> +               bool tcp = !!(stth->flags & STT_PROTO_TCP);
> +               int l4_offset = stth->l4_offset;
> +
> +               if (can_segment(skb, ipv4, tcp, csum_partial))
> +                       err = skb_list_segment(skb, ipv4, l4_offset);
> +               else
> +                       err = skb_list_linearize(skb, GFP_ATOMIC);
> +
> +               if (unlikely(err))
> +                       goto drop;
> +       }

Maybe move this into a helper function?

> diff --git a/datapath/vport-stt.c b/datapath/vport-stt.c
> new file mode 100644
> index 0000000..c7282b1
> --- /dev/null
> +++ b/datapath/vport-stt.c
> +/**
> + * struct stt_port - Keeps track of open UDP ports

"TCP" ports - although I'm not really sure that this structure is
keeping track of those either really.

> +static struct vport *stt_tnl_create(const struct vport_parms *parms)
> +{
[...]
> +       stt_sock = stt_sock_add(net, htons(dst_port), stt_rcv, vport);
> +       if (IS_ERR(stt_sock)) {
> +               ovs_vport_free(vport);
> +               return (void *)stt_sock;

ERR_CAST would probably be better here.



More information about the dev mailing list