On Fri, Nov 9, 2012 at 3:30 AM, Jesse Gross <je...@nicira.com> wrote:
> On Thu, Nov 8, 2012 at 5:14 AM, Ansis Atteka <aatt...@nicira.com> wrote:
>>
>> diff --git a/datapath/actions.c b/datapath/actions.c
>> index 8ec692d..317aa4a 100644
>> --- a/datapath/actions.c
>> +++ b/datapath/actions.c
>> +static void set_ipv6_addr(struct sk_buff *skb, u8 l4_proto,
>> + __be32 addr[4], const __be32 new_addr[4],
>> + bool recalculate_csum)
>> +{
>> + if (recalculate_csum)
>> + update_ipv6_checksum(skb, l4_proto, addr, new_addr);
>> +
>> + skb_clear_rxhash(skb);
>> + memcpy(addr, new_addr, sizeof(__be32[4]));
>
>
> The sizeof(__be32[4]) seems a little overdone here - you could just use
> sizeof(addr).
My understanding is that in C/C++ arrays decay into pointers, if they
are passed as function arguments. Try:
#include <stdio.h>
void fn(int a[4]) {
printf("%lu vs %lu vs %lu!\n", sizeof(a), sizeof(*a), sizeof(int[4]));
}
int main() {
int in[4];
fn(in);
}
Output is:
8 vs 4 vs 16!
Because sizeof(addr) would be system pointer size, but sizeof(*addr)
would be size of a single array element.
>
>> +static int set_ipv6(struct sk_buff *skb, const struct ovs_key_ipv6
>> *ipv6_key)
>> +{
>> + struct ipv6hdr *nh;
>> + int err;
>> + __be32 *saddr;
>> + __be32 *daddr;
>> + int hdr;
>> + int flags = OVS_IP6T_FH_F_SKIP_RH;
>> + unsigned int offset = 0;
>> +
>> + err = make_writable(skb, skb_network_offset(skb) +
>> + sizeof(struct ipv6hdr));
>> + if (unlikely(err))
>> + return err;
>> +
>> + nh = ipv6_hdr(skb);
>> + saddr = (__be32 *)&nh->saddr;
>> + daddr = (__be32 *)&nh->daddr;
>> +
>> + if (memcmp(ipv6_key->ipv6_src, saddr, sizeof(ipv6_key->ipv6_src)))
>> + set_ipv6_addr(skb, ipv6_key->ipv6_proto, saddr,
>> + ipv6_key->ipv6_src, true);
>> +
>> + hdr = ipv6_find_hdr(skb, &offset, NEXTHDR_ROUTING, NULL, &flags);
>> + if (memcmp(ipv6_key->ipv6_dst, daddr, sizeof(ipv6_key->ipv6_dst)))
>> + set_ipv6_addr(skb, ipv6_key->ipv6_proto, daddr,
>> + ipv6_key->ipv6_dst, hdr != NEXTHDR_ROUTING);
>
>
> We definitely only want to do ipv6_find_hdr() after we have checked that
> we're actually changing the destination address. We could help things even
> more by first doing a quick check of ipv6_ext_hdr(nh->nexthdr) since most of
> the time it will point directly to the L4 header. Once we do that we can
> also group together hdr, offset, and flags under the if statement to keep
> them all in one place.
>
>>
>> +
>> + set_ipv6_tc(nh, ipv6_key->ipv6_tclass);
>> +
>> + set_ipv6_fl(nh, be32_to_cpu(ipv6_key->ipv6_label));
>
>
> It's somewhat more canonical to use ntohl here.
>
>>
>> +
>> + nh->hop_limit = ipv6_key->ipv6_hlimit;
>
>
> This is minor but you could drop the blank lines between these functions.
>
> Also, when we do validation in datapath.c we should check that only the 20
> real bits of the flow label are set.
>
>>
>> diff --git a/datapath/checksum.h b/datapath/checksum.h
>> index 2f2ffee..f1d1c02 100644
>> --- a/datapath/checksum.h
>> +++ b/datapath/checksum.h
>> +#if LINUX_VERSION_CODE < KERNEL_VERSION(3,7,0)
>> +static inline void inet_proto_csum_replace16(__sum16 *sum,
>> + struct sk_buff *skb,
>> + const __be32 *from,
>> + const __be32 *to,
>> + int pseudohdr)
>
>
> Can you put this function next to inet_proto_csum_replace4()? It makes them
> easier to compare.
>
> It should also use the NEED_CSUM_NORMALIZE check and associated rpl_ macro
> for safety, although by 3.7 most of the crazy checksumming stuff should have
> gone away.
>
>>
>> +{
>> + __be32 diff[] = {
>> + ~from[0], ~from[1], ~from[2], ~from[3],
>> + to[0], to[1], to[2], to[3],
>> + };
>> + if (skb->ip_summed != OVS_CSUM_PARTIAL) {
>> + *sum = csum_fold(csum_partial(diff, sizeof(diff),
>> + ~csum_unfold(*sum)));
>> + if (skb->ip_summed == OVS_CSUM_COMPLETE && pseudohdr)
>
>
> These two skb->ip_summed accesses should use get_ip_summed(skb) so that we
> get the normalized values on older kernels.
>
> I need to look at the userspace portions more carefully but I wanted to get
> something back to you quickly.
Ok to all comments except the one with sizeof() usage. I will send out
PATCHv3 once you will reply with user-space datapath comments.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
