On Tue, Aug 23, 2011 at 10:48 AM, Pravin Shelar <pshe...@nicira.com> wrote:
> diff --git a/datapath/actions.c b/datapath/actions.c
> index 8aec438..0150be2 100644
> --- a/datapath/actions.c
> +++ b/datapath/actions.c
> @@ -61,6 +53,9 @@ static int strip_vlan(struct sk_buff *skb)
> skb->csum = csum_sub(skb->csum, csum_partial(skb->data
> + ETH_HLEN, VLAN_HLEN, 0));
>
> + veth = (struct vlan_ethhdr *) skb->data;
> + *current_tci = veth->h_vlan_TCI;
> +
> memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
>
> eh = (struct ethhdr *)skb_pull(skb, VLAN_HLEN);
Might as well use __skb_pull() here since everything else in this
function is assuming that the skb is long enough.
> +static int push_vlan(struct sk_buff *skb, __be16 new_tci)
> +{
> + if (unlikely(vlan_tx_tag_present(skb))) {
> + u16 current_tag;
>
> + current_tag = vlan_tx_tag_get(skb);
> +
> + if (!__vlan_put_tag(skb, current_tag))
> + return -ENOMEM;
You need to update skb->csum in the CHECKSUM_COMPLETE case, similar to
what we do when popping a tag that is actually present in the skb.
> diff --git a/datapath/linux/compat/include/linux/if_ether.h
> b/datapath/linux/compat/include/linux/if_ether.h
> index b8390e2..75ab82c 100644
> --- a/datapath/linux/compat/include/linux/if_ether.h
> +++ b/datapath/linux/compat/include/linux/if_ether.h
> @@ -7,6 +7,7 @@
> #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,28)
>
> #define ETH_P_TEB 0x6558 /* Trans Ether Bridging */
> +#define ETH_P_FCOE 0x8906 /* Fibre Channel over Ethernet */
I think this wasn't actually added until 2.6.30.
> diff --git a/datapath/linux/compat/netdevice.c
> b/datapath/linux/compat/netdevice.c
> index 1a6f327..a06da0c 100644
> --- a/datapath/linux/compat/netdevice.c
> +++ b/datapath/linux/compat/netdevice.c
> @@ -1,5 +1,7 @@
> #include <linux/if_link.h>
> #include <linux/netdevice.h>
> +#include <linux/if_vlan.h>
> +
Extra blank line?
> +u32 rpl_netif_skb_features(struct sk_buff *skb)
> +{
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,26)
> + __be16 protocol = skb->protocol;
> + u32 features = skb->dev->features;
> +
> + if (protocol == htons(ETH_P_8021Q)) {
> + struct vlan_ethhdr *veh = (struct vlan_ethhdr *)skb->data;
> + protocol = veh->h_vlan_encapsulated_proto;
> + } else if (!vlan_tx_tag_present(skb)) {
> + return harmonize_features(skb, protocol, features);
> + }
> +
> + features &= (skb->dev->vlan_features | NETIF_F_HW_VLAN_TX);
> +
> + if (protocol != htons(ETH_P_8021Q)) {
> + return harmonize_features(skb, protocol, features);
> + } else {
> + features &= NETIF_F_SG | NETIF_F_HIGHDMA | NETIF_F_FRAGLIST |
> + NETIF_F_GEN_CSUM | NETIF_F_HW_VLAN_TX;
> + return harmonize_features(skb, protocol, features);
> + }
> +#else
> + return 0;
The reason why this was protected by the version guard before is it
used vlan_features, which didn't exist before 2.6.26. This function
is a little more general, so it can be a little less restrictive in
the non-vlan case, even if it isn't used otherwise.
> +struct sk_buff *rpl_skb_gso_segment(struct sk_buff *skb, u32 features)
> +{
> + int vlan_depth = ETH_HLEN;
> + __be16 type = skb->protocol;
> +
> + while (type == htons(ETH_P_8021Q)) {
> + struct vlan_hdr *vh;
> +
> + if (unlikely(!pskb_may_pull(skb, vlan_depth + VLAN_HLEN)))
> + return ERR_PTR(-EINVAL);
> +
> + vh = (struct vlan_hdr *)(skb->data + vlan_depth);
> + type = vh->h_vlan_encapsulated_proto;
> + vlan_depth += VLAN_HLEN;
> + }
> +
> + /* this hack needed to get regular skb_gso_segment() */
> +#undef skb_gso_segment
> + skb->protocol = type;
> +
> + return skb_gso_segment(skb, features);
I think you should reset the original skb->protocol before returning.
> +}
> +
> #endif /* kernel version < 2.6.36 */
Why is this in a block for kernels before 2.6.36? I think it should be 2.6.38.
Ethan, you mentioned before that you saw a bug somewhere in here. Can
you review the userspace portions?
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
