[ovs-dev] [PATCHv2 2/2] datapath: add ipv6 'set' action

Jesse Gross jesse at nicira.com
Fri Nov 9 01:30:05 UTC 2012


On Thu, Nov 8, 2012 at 5:14 AM, Ansis Atteka <aatteka at 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).

+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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20121108/59920af9/attachment-0003.html>


More information about the dev mailing list