On Oct 11, 2013, at 9:02 AM, Ben Pfaff <b...@nicira.com> wrote:
> On Wed, Oct 09, 2013 at 01:47:50PM -0700, Jarno Rajahalme wrote:
>> Use the offset of the last member in struct flow instead of the
>> struct size to help catch changes in the declaration.
>>
>> Add flow_random_hash_fields() used for testing in places where
>> struct flow was used without zero initialization before.
>>
>> With these changes we do not need to keep updating explicit padding
>> in struct flow as fields are added or deleted.
>>
>> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
>
> ...
>
>> @@ -603,7 +604,11 @@ flow_wildcards_init_catchall(struct flow_wildcards *wc)
>> }
>>
>> /* Initializes 'wc' as an exact-match set of wildcards; that is, 'wc' does
>> not
>> - * wildcard any bits or fields. */
>> + * wildcard any bits or fields.
>> + * Note that also bits that are always zeroes (padding, extra bits in fields
>> + * like IPv6 flow label), and fields that are mutually exclusive (e.g., IPv4
>> + * and IPv6 addresses) are included.
>> + */
>> void
>> flow_wildcards_init_exact(struct flow_wildcards *wc)
>> {
>
> I don't like this semantic of initializing pads to 0xff very much. But
> I see that flow_wildcards_init_exact() has only one caller worth
> considering, which is this code in rule_dpif_lookup_in_table():
>
> } else if (frag && ofproto->up.frag_handling == OFPC_FRAG_DROP) {
> cls_rule = &ofproto->drop_frags_rule->up.cr;
> if (wc) {
> flow_wildcards_init_exact(wc);
> }
> } else {
>
> I don't understand why we need to use flow_wildcards_init_exact() when
> dropping fragments. If we didn't use it, then we could probably drop
> the function entirely. Do you have any idea why we use it here?
> (Justin?)
Maybe that's just to be on the safe side? If the policy of the ofproto is to
drop all frags, then maybe one mega-flow for that effect would be enough? It
seems that instead of forcing an exact match, looking at the in_port and
nw_frag would be sufficient?
>
>> @@ -65,7 +63,11 @@ main(int argc, char *argv[])
>> struct flow_wildcards wc;
>> struct flow flow;
>>
>> - random_bytes(&flow, sizeof flow);
>> + /* The generated flows will have random bits where zeroes would
>> + * normally be required (padding, extra bits in fields like IPv6
>> + * flow label, but this test will never use those bits, so it
>> does
>> + * not matter. */
>> + flow_random_hash_fields(&flow);
>
> Is this comment correct? It doesn't look to me like
> flow_random_hash_fields() puts random bits in the whole flow, only in
> selected fields.
No, it is simply a leftover from the stage when I did not have the
flow_random_hash_fields() yet. Sorry for leaving the comment in.
Jarno
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
