Thanks a lot for the review comments.
On Thu, Aug 22, 2013 at 12:10 PM, Jesse Gross <je...@nicira.com> wrote:
> On Fri, Aug 16, 2013 at 4:01 PM, Andy Zhou <az...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
