[ovs-dev] [PATCH v2] datapath: make skb->csum consistent with rest of networking stack.

Pravin Shelar pshelar at nicira.com
Tue Jun 11 22:55:13 UTC 2013


On Tue, Jun 11, 2013 at 3:30 PM, Jesse Gross <jesse at nicira.com> wrote:
> On Tue, Jun 11, 2013 at 1:20 PM, Pravin B Shelar <pshelar at nicira.com> wrote:
>> diff --git a/datapath/actions.c b/datapath/actions.c
>> index 0dac658..6d60cd0 100644
>> --- a/datapath/actions.c
>> +++ b/datapath/actions.c
>> @@ -132,9 +132,17 @@ static int set_eth_addr(struct sk_buff *skb,
>>         if (unlikely(err))
>>                 return err;
>>
>> +       if (get_ip_summed(skb) == OVS_CSUM_COMPLETE)
>> +               skb->csum = csum_sub(skb->csum, csum_partial(eth_hdr(skb),
>> +                                                            ETH_HLEN, 0));
>> +
>>         memcpy(eth_hdr(skb)->h_source, eth_key->eth_src, ETH_ALEN);
>>         memcpy(eth_hdr(skb)->h_dest, eth_key->eth_dst, ETH_ALEN);
>>
>> +       if (get_ip_summed(skb) == OVS_CSUM_COMPLETE)
>> +               skb->csum = csum_add(skb->csum, csum_partial(eth_hdr(skb),
>> +                                                            ETH_HLEN, 0));
>> +
>
> I guess we could make this marginally more efficient by using 2 *
> ETH_ALEN instead of ETH_HLEN since that's the part that is actually
> changing.
>
>> diff --git a/datapath/linux/compat/include/linux/skbuff.h b/datapath/linux/compat/include/linux/skbuff.h
>> index d485b39..9bf7493 100644
>> --- a/datapath/linux/compat/include/linux/skbuff.h
>> +++ b/datapath/linux/compat/include/linux/skbuff.h
>> +static inline void skb_postpush_rcsum(struct sk_buff *skb,
>> +                                     const void *start, unsigned int len)
>> +{
>> +       if (skb->ip_summed == CHECKSUM_COMPLETE)
>> +               skb->csum = csum_add(skb->csum, csum_partial(start, len, 0));
>> +}
>
> Do we think that this will be generally useful outside of OVS? I
> suspect that there are very few, if any, other users in the network
> stack that add data to packets on the receive side. If that's the case
> then we might want to just keep in internal.
>
>> diff --git a/datapath/vport-internal_dev.c b/datapath/vport-internal_dev.c
>> index 9a159cd..4fdbc59 100644
>> --- a/datapath/vport-internal_dev.c
>> +++ b/datapath/vport-internal_dev.c
>> @@ -267,6 +267,7 @@ static int internal_dev_recv(struct vport *vport, struct sk_buff *skb)
>>         struct net_device *netdev = netdev_vport_priv(vport)->dev;
>>         int len;
>>
>> +       forward_ip_summed(skb, false);
>>  #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,37)
>>         if (unlikely(vlan_deaccel_tag(skb)))
>>                 return 0;
>> @@ -281,7 +282,7 @@ static int internal_dev_recv(struct vport *vport, struct sk_buff *skb)
>>         skb->dev = netdev;
>>         skb->pkt_type = PACKET_HOST;
>>         skb->protocol = eth_type_trans(skb, netdev);
>> -       forward_ip_summed(skb, false);
>> +       skb_postpull_rcsum(skb, eth_hdr(skb), ETH_HLEN);
>>
>>         netif_rx(skb);
>
> Since we're doing checksum manipulations, it's probably better to put
> forward_ip_summed() at the end since it basically represents the end
> of OVS's internal checksum normalization.

sent out update patch according to comments.



More information about the dev mailing list