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

[Andy Zhou]

Thanks a lot for the review comments.

On Thu, Aug 22, 2013 at 12:10 PM, Jesse Gross <jesse at nicira.com> wrote:

> 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.
>
> There was a memset(0) in current code. I did not verify it was safe to
remove. Will remove them as suggested if it is.


> >  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.

Thanks, will fix

> >  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?
>
Good point, I will move those check out of the fast path.

>
> > 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 agree, alignof is better.


> 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.
>
Good idea.  Will do both.

>
> > -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.
>
 O.K. I will move it into flow.c

>
> 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.
>
I will fix up the patch, then take the performance number and modify the
commit message.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130822/a7e41d15/attachment-0003.html>