[ovs-dev] [PATCH 2/2] netflow: Implement NetFlow active timeouts.

[Jesse Gross]

Ben Pfaff wrote:
>> diff --git a/secchan/ofproto.c b/secchan/ofproto.c
>> index 4266cbf..11f48b2 100644
>> --- a/secchan/ofproto.c
>> +++ b/secchan/ofproto.c
>> @@ -94,9 +94,8 @@ struct rule {
>>      uint64_t packet_count;      /* Number of packets received. */
>>      uint64_t byte_count;        /* Number of bytes received. */
>>      uint64_t accounted_bytes;   /* Number of bytes passed to account_cb. */
>> -    uint8_t tcp_flags;          /* Bitwise-OR of all TCP flags seen. */
>> -    uint8_t ip_tos;             /* Last-seen IP type-of-service. */
>>      tag_type tags;              /* Tags (set only by hooks). */
>> +    struct netflow_flow nf_flow; /* Per-flow NetFlow tracking data. */
>>      uint16_t nf_output_iface;   /* Output interface index for NetFlow. */
>>     
>
> Would it fit the abstraction to move nf_output_iface into "struct
> netflow_flow"?  Because of padding rules that struct has at least
> 16 bits free at its tail anyhow.  (I guess that
> netflow_flow_clear() would need to be more careful, too.)
>
>   

Good idea, I was actually thinking about doing this but forgot.

>> @@ -544,7 +544,8 @@ ofproto_set_snoops(struct ofproto *ofproto, const struct svec *snoops)
>>  
>>  int
>>  ofproto_set_netflow(struct ofproto *ofproto, const struct svec *collectors,
>> -        uint8_t engine_type, uint8_t engine_id, bool add_id_to_iface)
>> +                    uint8_t engine_type, uint8_t engine_id,
>> +                    int active_timeout, bool add_id_to_iface)
>>     
>
> We're dangerously close to needing a "struct netflow_options" or
> some such.
>   

I created a netflow_options struct and it is much cleaner now.

>   
>> +static bool
>> +is_controller_rule(struct rule *rule)
>> +{
>> +    /* If the only action is send to the controller then don't report
>> +     * NetFlow expiration messages since it is just part of the control
>> +     * logic for the network and not real traffic. */
>> +
>> +    return rule->n_odp_actions == 1 &&
>> +           rule->odp_actions[0].type == ODPAT_CONTROLLER;
>>     
>
> Is this test always correct?  At least in the past, we've had
> some flows that can't be handled in the kernel datapath so that
> we've had to send them down to userspace, which also uses
> ODPAT_CONTROLLER.  Should we test the OpenFlow actions (of the
> super-rule if necessary) instead?
>
>   

I'm pretty sure that this test is always correct - odp_actions is 
generated in
ofproto and isn't filled from the datapath so it shouldn't include flows 
that the kernel couldn't handle.  However, using the OpenFlow actions is 
more obviously correct and perhaps less likely to break in the future, 
so I just changed it to use that instead.

>> @@ -550,6 +551,13 @@ bridge_reconfigure(void)
>>          if (cfg_has("netflow.%s.engine-id", br->name)) {
>>              engine_id = cfg_get_int(0, "netflow.%s.engine-id", br->name);
>>          }
>> +        if (cfg_has("netflow.%s.engine-id", br->name)) {
>> +            engine_id = cfg_get_int(0, "netflow.%s.engine-id", br->name);
>> +        }
>>     
>
> I think that you made a second copy of the code for getting
> engine-id.
>   

Not sure where that came from.

I made these changes along with a couple bug fixes and pushed.