[ovs-dev] [ovs-discuss] [ACLv2 06/19] ofproto: Add flow expiration callback.

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



Ben Pfaff wrote:
> Jesse Gross <jesse at nicira.com> writes:
>
>   
>> Allow notification when a kernel exact match flow expires.  The
>> expiration triggers both when a flow expires or if a packet is
>> processed but not installed.  Provides the flow as well as packet
>> and byte counts.
>>     
>
> [...]
>
>   
>> @@ -2011,8 +2016,17 @@ xlate_output_action(struct action_xlate_ctx *ctx,
>>          if (!ctx->ofproto->ofhooks->normal_cb(ctx->flow, ctx->packet,
>>                                                ctx->out, ctx->tags,
>>                                                ctx->ofproto->aux)) {
>> +            struct odp_flow_stats stats;
>>              COVERAGE_INC(ofproto_uninstallable);
>>              ctx->may_setup_flow = false;
>> +
>> +            if (ctx->packet) {
>> +                flow_extract_stats(ctx->flow, ctx->packet, &stats);
>> +                ctx->ofproto->ofhooks->flow_expire_cb(ctx->flow,
>> +                                                      stats.n_packets,
>> +                                                      stats.n_packets,
>> +                                                      ctx->ofproto->aux);
>> +            }
>>     
>
> It's clever to catch the packet here in xlate_output_action(),
> but does this actually do what you want all the time?  If you
> want all the packets that are "processed but not installed" then
> this will miss packets that are qualified to be set up as flows
> but not actually set up as flows.  It will miss, for example,
> (most) packets sent by ofproto_send_packet() or
> handle_packet_out(), I think.  So really I think the caller of
> xlate_actions() needs to make this ->flow_expire_cb() call to
> avoid missing these cases.
>
>   

This is a very good catch as there are several tricky corner cases 
here.  In addition to the cases you mention, there were also similar 
conditions involving revalidation.  The ACL code is not very forgiving 
of incorrect flow expiration messages: both missing and overly 
aggressive expirations will lead to incorrect stats.  So if I trigger 
flow_expire_cb() in ofproto_send_packet() after it sent a packet that 
happens to match a still-active flow, then that flow will have the wrong 
stats.  What I do now is pass an additional flag saying that the flow 
will not ever be installed.  This causes the stats to be credited 
immediately for that packet and then there is no need to send a flow 
expiration message later.

> Otherwise: I think that the second stats.n_packets above should
> be stats.n_bytes.
>
>   

Yes, thank you.

>> diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
>> index ce4a42e..dae42dd 100644
>> --- a/ofproto/ofproto.h
>> +++ b/ofproto/ofproto.h
>> @@ -113,6 +113,8 @@ struct ofhooks {
>>                              size_t n_actions, unsigned long long int n_bytes,
>>                              void *aux);
>>      void (*account_checkpoint_cb)(void *aux);
>> +    void (*flow_expire_cb)(const flow_t *flow, uint64_t packets,
>> +                           uint64_t bytes, void *aux);
>>     
>
> Really I think that we need to start putting in detailed comments
> on the function pointers in this struct, as done in
> e.g. dpif-provider.h or netdev-provider.h.  Otherwise it will
> become different for folks trying to read the code for the first
> time to figure out the detailed semantics.
>   

I documented all of the existing and new callbacks to at least some 
level of detail.




More information about the dev mailing list