[ovs-dev] [PATCH 2/2] datapath: optimize flow compare and mask functions

Jesse Gross jesse at nicira.com
Thu Aug 22 19:10:20 UTC 2013


On Fri, Aug 16, 2013 at 4:01 PM, Andy Zhou <azhou at nicira.com> wrote:
> diff --git a/datapath/flow.c b/datapath/flow.c
> index ab803ef..25eb8c1 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
>  void ovs_flow_key_mask(struct sw_flow_key *dst, const struct sw_flow_key *src,
>                        const struct sw_flow_mask *mask)
[...]
> +       for (i = 0; i < mask->range.start;  i += sizeof(long))
> +               *d++ = 0;
> +
> +       for (i = 0; i < range_n_bytes(&mask->range); i += sizeof(long))
> +               *d++ = *s++ & *m++;
> +
> +       for (i = mask->range.end; i < sizeof(*dst); i += sizeof(long))
> +               *d++ = 0;

Do we actually need to zero out the areas before and after the key?
Now that we've made the masking, comparison, and hashing functions all
use the same lengths I don't think anyone will look at those regions.

>  static u32 ovs_flow_hash(const struct sw_flow_key *key, int key_start,
>                          int key_end)
>  {
> -       return jhash2((u32 *)((u8 *)key + key_start),
> -                     DIV_ROUND_UP(key_end - key_start, sizeof(u32)), 0);
> +       u8 *hash_bytes = (u8 *)key + key_start;
> +       int hash_len = key_end - key_start;
> +
> +       return jhash2((u32 *)hash_bytes, hash_len, 0);

jhash2() takes the length as a number of 4 byte words but it looks to
me like this is passing a byte length.

>  static bool __cmp_key(const struct sw_flow_key *key1,
>                 const struct sw_flow_key *key2,  int key_start, int key_end)
>  {
> -       return !memcmp((u8 *)key1 + key_start,
> -                       (u8 *)key2 + key_start, (key_end - key_start));
> +       const long *cp1 = (long *)(u8 *)key1 + key_start;
> +       const long *cp2 = (long *)(u8 *)key2 + key_start;
> +       long diffs = 0;
> +       int i;
> +
> +       BUG_ON(key_start != rounddown(key_start, sizeof(long)));
> +       BUG_ON(key_end != roundup(key_end, sizeof(long)));

This seems like a pretty expensive place to put a sanity check. Is it
really necessary or can we at least move it somewhere cheaper?

> diff --git a/datapath/flow.h b/datapath/flow.h
> index c177f55..22bd630 100644
> --- a/datapath/flow.h
> +++ b/datapath/flow.h
> @@ -127,7 +127,7 @@ struct sw_flow_key {
>                         } nd;
>                 } ipv6;
>         };
> -};
> +} __aligned(sizeof(long));

Should this use alignof instead of sizeof?

I might also add some compile time assertions here as well, such as
one that the size is a multiple of sizeof long and if you take my
suggestion about not zeroing when masking then perhaps one that sizeof
long is a multiple of sizeof u32.

> -static inline u16 ovs_sw_flow_key_range_actual_size(const struct sw_flow_key_range *range)
> +static inline u16 range_n_bytes(const struct sw_flow_key_range *range)

Can you add an ovs_ prefix here for consistency? Or actually it might
be better to just move it into flow.c since I think that's the only
place that it gets used.

Finally, do you have any performance numbers from this patch
specifically. It might be good to put that in the commit message
instead of just quoting the previous patch.



More information about the dev mailing list