On Fri, Nov 22, 2013 at 11:56:19AM -0800, Jarno Rajahalme wrote:
> On Nov 21, 2013, at 5:10 PM, Ben Pfaff <b...@nicira.com> wrote:
> > I am not done studying this code, but I have some preliminary
> > comments.
> >
> > Some of the fields that allow prefix length are in network byte order,
> > and others (the registers) are in host byte order, but the code that
> > looks at the fields doesn't seem to distinguish. "Prefixes" therefore
> > turn into suffixes for the registers, but only on little-endian
> > machines.
> >
>
> This is a mistake, as I did not notice that the registers are in
> host byte order. For now I?m inclined to simply remove the support
> for registers, as there is no known use case for prefix lookup for
> them yet.
Yes, I think that is the best solution for now.
> > In minimask_get_prefix_len(), I think that:
> > if (mask) {
> > mask_tz = raw_ctz(mask);
> > if (~mask & (~0u << mask_tz)) {
> > return 0; /* Mask not contiguous. */
> > }
> > nbits += 32 - mask_tz;
> > } else {
> > mask_tz = 32; /* End of mask seen. */
> > }
> > could be simplified to just:
> > if (~mask & (~mask + 1)) {
> > return 0; /* Mask not contiguous. */
> > }
> > mask_tz = ctz32(mask);
> > nbits += 32 - mask_tz;
> > using the alternative test for a non-CIDR mask found in ip_is_cidr().
> > (I did not test this change.)
>
> This works (substituting ctz for ctz32 ? should we rename ctz as ctz32?)
I think ctz32 would be a better name.
> > In trie_insert(), is the call to minimask_get_prefix_len() necessary?
> > I may be missing something but I'm not sure how the rule's prefix len
> > could be different from the rule's subtable's prefix length.
>
> We could pass in the prefix len as an argument, and also make sure
> the call is ever made with non-zero prefix lengths. Previously, the
> prefix mask offset from minimask_get_prefix_len() was used to get
> the prefix pointer, but now that I have separated them out, the call
> to minimask_get_prefix_len() can be removed. Thanks for pointing
> this out!
Makes sense, thanks.
Is it OK for me to wait for a v4 to continue review?
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
