On 24 April 2014 04:56, Alex Wang <al...@nicira.com> wrote:
>
> On Tue, Apr 22, 2014 at 10:24 PM, Alex Wang <al...@nicira.com> wrote:
>
>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>>> index 416cfdc..906bf17 100644
>>> --- a/ofproto/ofproto-dpif-upcall.c
>>> +++ b/ofproto/ofproto-dpif-upcall.c
>>> @@ -45,6 +45,8 @@
>>>
>>> VLOG_DEFINE_THIS_MODULE(ofproto_dpif_upcall);
>>>
>>> +COVERAGE_DEFINE(upcall_duplicate_flow);
>>> +
>>> /* A thread that reads upcalls from dpif, forwards each upcall's packet,
>>> * and possibly sets up a kernel flow as a cache. */
>>> struct handler {
>>> @@ -161,6 +163,7 @@ struct udpif_key {
>>> long long int created; /* Estimation of creation time. */
>>>
>>> bool mark; /* Used by mark and sweep GC
>>> algorithm. */
>>> + bool flow_exists; /* Ensures flows are only deleted
>>> once. */
>>>
>>>
>>
>> Do we still need mark? I think the function of 'mark' and 'flow_exists'
>> is overlapped.
>>
>
Strictly speaking, no. But I think there are a couple of benefits:
(1) If they are separate, then we can detect that the flow was already
handled and chosen to be kept:
* Revalidator finishes a round of revalidation. mark is set to false.
flow_exists is true.
* Consider when we dump a flow, revalidate it and choose to keep it. We set
the mark to true. Flow_exists is true.
* If we dump a duplicate of the flow, then "mark" will be true. This means
that the flow was handled already. If it was just handled, there is
unlikely to be any changes. We can skip execution.
(2) If they are separate, then we know when the datapath has skipped a flow
during flow_dump (if we've seen the flow before at all):
* If a flow is dumped and we decide to delete it, "mark" and "flow_exists"
will both be set to false.
* If a flow is dumped and we decide to keep it, "mark" and "flow_exists"
will both be set to true.
* If the datapath doesn't dump a flow that does exist (due to some race
condition), then "mark" will be false from previous revalidator_sweep() and
"flow_exists" will be true.
* Revalidator_sweep(): If a flow has "mark" set false and "flow_exists" set
true, then we haven't seen the flow. Currently we delete such flows from
the datapath, so that we can garbage collect the ukey and update the stats
for that flow.
Thanks for the review Alex.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
