[ovs-dev] [ovs-discuss] [ACLv2 02/19] ofproto: Add ofproto_get_flow_stats functions.

Jesse Gross jesse at nicira.com
Fri Sep 4 21:21:26 UTC 2009



Ben Pfaff wrote:
> Jesse Gross <jesse at nicira.com> writes:
>
>   
>> The function allows aggregate packet and byte counts to be retrieved
>> for flows that match the given flow and wildcards.  The set of flows
>> to be matched against can either be the normal OpenFlow flows or the
>> currently active exact match flows in the kernel.
>>     
>
> That seems like a reasonable thing to do.  But that isn't what
> this code does: it adds a way to get hidden or not-hidden rules.
> That is a different proposition, because some hidden rules are
> "currently active exact match flows in the kernel" ("caemfinks"
> for short) but others are not.  In fact, the same is true about
> not-hidden rules.  So there are in fact four categories:
>
>         * Hidden rules that are caemfinks: subrules.
>
>         * Hidden rules that are not caemfinks: wildcarded rules
>           created by in-band control.
>
>         * Non-hidden rules that are caemfinks: OpenFlow
>           exact-match flows that happen to be installed in the
>           kernel.
>
>         * Non-hidden rules that are not caemfinks: OpenFlow
>           wildcarded rules and OpenFlow exact-match flows that
>           happen not to be installed in the kernel.
>
> At this point in the review process I don't know which of these
> you really want to list, but it definitely needs to be carefully
> thought out.
>
>   

This is a good point.  I was essentially equating hidden rules with 
caemfinks.  Case #2 wasn't an issue because ACL's are only active when 
there is no controller and in-band control is only active when there is 
a controller, so there is no overlap.  However, I forgot about OpenFlow 
exact-match flows that could be installed.  I changed it to look at 
installed rules instead of hidden rules.

>> @@ -2555,6 +2562,30 @@ handle_aggregate_stats_request(struct ofproto *p, struct ofconn *ofconn,
>>      return 0;
>>  }
>>  
>> +void
>> +ofproto_get_flow_stats(struct ofproto *ofproto, const flow_t *flow,
>> +                       uint32_t wildcards, bool hidden_rules,
>> +                       uint64_t *packet_count, uint64_t *byte_count)
>>     
>
> This could really use a comment.  I would have guessed that
> "hidden_rules" specifies whether hidden rules should be included,
> with non-hidden rules always included, but my guess would be wrong.
>
>   
>> +{
>> +    struct cls_rule target;
>> +    struct aggregate_stats_cbdata cbdata;
>> +
>>     
> [...]
>   

I now have a parameter called "installed_flows_only", which hopefully 
should be a little more clear.




More information about the dev mailing list