Ben,
Thank you very much for the review so far! See responses below:
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.
> vswitch.xml should document the new Flow_Table column.
>
Thanks for pointing this out, I did not know where the documentation for the
database columns was.
> Thanks for correcting the comment on miniflow_init__().
>
> I'm not sure that I would list the fields that can be used for prefix
> lookup so explicitly in the classifier.h comment, because lists of
> this kind tend to get out of date.
>
Agree, will remove.
> trie_destroy() doesn't follow the usual OVS convention that a destroy
> function should accept NULL, but making it accept NULL would allow
> some callers to be slightly simplified.
>
OK.
> I don't see anything in classifier_set_prefix_fields() or its callers
> to detect and disallow listing the same field twice. Does it matter?
>
Doing the same field twice does not break anything, but should be at least
logged as a warning, since it is likely an error rather than intentional.
> mf_from_id() assert-fails on a bad id, but
> classifier_set_prefix_fields() still checks for a null return value.
>
OK, must have seen this sometime, but did not remember.
> minimask_get_prefix_len() adds restriction to the fields that can be
> used for prefix matching, that is not mentioned elsewhere: it must be
> a multiple of 4 bytes long. trie_insert() adds a subtle variation
> that it must be a multiple of 32 bits long. The latter seems more
> appropriate, since we could have, say, 24-bit field that is aligned on
> a 4-byte boundary, and none of the code seems to deal with anything
> that is not a multiple of 32 bits.
Good catch, I must have considered 4-byte and 32-bit requirements as the same
as I was writing that. Will make it 32-bit, and enforce it centrally at
classifier_set_prefix_fields().
>
> minimask_get_prefix_len() doesn't seem to be on a fast path, so it
> might be better to write really clear code by using minimask_get() in
> place of directly dealing with 'map' and values’.
Agree.
> 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 also separated getting the prefix data offset from minimask_get_prefix_len(),
which helped to clear it up even more :-)
>
> The !mf test at the top of trie_insert() seems odd. Is it really
> possible for mf to be null here?
No it is not. This was a leftover from before there was a proper configuration
interface. I cleaned up these asserts.
>
> 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!
>
> trie_prefix_equal_bits() looks to me like it doesn't handle the case
> where ofs...plen crosses a 32-bit boundary, since it only looks at a
> single word in the prefix[] array. But the code for that function is
> really opaque to me so maybe I just misunderstand.
I think what happens is that prefix bits for each 32-bit word get to different
tree nodes, and this should still work. I’ll double check and add commentary.
>
> trie_equal_bits() and trie_prefix_equal_bits() are very similar, I
> suspect that they could be written as trivial wrappers around a common
> function.
>
I’ll try that.
Jarno
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
