On Thu, Apr 16, 2015 at 3:15 PM, Jesse Gross <je...@nicira.com> wrote:
> On Tue, Apr 14, 2015 at 8:46 PM, Pravin B Shelar <pshe...@nicira.com> wrote:
>> diff --git a/datapath/linux/compat/stt.c b/datapath/linux/compat/stt.c
>> new file mode 100644
>> index 0000000..f294ecd
>> --- /dev/null
>> +++ b/datapath/linux/compat/stt.c
>> +static struct sk_buff *handle_offloads(struct sk_buff *skb, int
>> min_headroom)
>> +{
> [...]
>> + if (skb_is_gso(skb)) {
>> + struct sk_buff *nskb;
>> + char cb[sizeof(skb->cb)];
>> +
>> + memcpy(cb, skb->cb, sizeof(cb));
>> +
>> + nskb = __skb_gso_segment(skb, 0, false);
>> + if (IS_ERR(nskb)) {
>> + err = PTR_ERR(nskb);
>> + goto error;
>> + }
>
> We don't really have a backported version of __skb_gso_segment, is it
> capable of handling MPLS headers that might have been added on earlier
> kernels?
>
I will separate patch to fix this issue, since it is bug without STT patches.
>> +static u8 skb_get_l4_proto(struct sk_buff *skb, __be16 l3_proto)
>> +{
>> + if (l3_proto == htons(ETH_P_IP)) {
>> + unsigned int nh_ofs = skb_network_offset(skb);
>> +
>> + if (unlikely(!pskb_may_pull(skb, nh_ofs + sizeof(struct
>> iphdr))))
>> + return 0;
>
> Is this sufficient to cover the checks in stt_can_offload() as well?
>
ok, I will add tcp-header len to the pull request.
>> +static bool set_offloads(struct sk_buff *skb)
> [...]
>> + switch (proto_type) {
>> + case (STT_PROTO_IPV4 | STT_PROTO_TCP):
>> + /* TCP/IPv4 */
>> + csum_offset = offsetof(struct tcphdr, check);
>> + gso_type = SKB_GSO_TCPV4;
>> + l3_header_size = sizeof(struct iphdr);
>> + l4_header_size = sizeof(struct tcphdr);
>
> It looks like we only set skb->protocol in the IPv6 cases, presumably
> because we only support STT over IPv4. However, this seems easy to
> forget to update if we introduce IPv6 support one day, so I would set
> it unconditionally.
>
ok.
>> +static unsigned int nf_ip_hook(FIRST_PARAM
>> + struct sk_buff *skb,
>> + const struct net_device *in,
>> + const struct net_device *out,
>> + int (*okfn)(struct sk_buff *))
> [...]
>> + stt_sock = stt_find_sock(dev_net(skb->dev), tcp_hdr(skb)->dest);
>> + if (unlikely(!stt_sock))
>> + return NF_ACCEPT;
>
> Can you remove the unlikely() here since we can still get non-STT TCP
> traffic here?
>
ok.
>> + __skb_pull(skb, ip_hdr_len + sizeof(struct tcphdr));
>
> What about TCP header length?
since STT does not generate options we can just assume there are no
options in the packet.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
