[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