[ovs-dev] [PATCH] revalidator: Fix ukey stats cache updating.

Alex Wang alexw at nicira.com
Fri Apr 25 15:10:23 UTC 2014


found this in the morning, will keep investigating,


./learn.at:362: (ovs-ofctl dump-ports br0 2; ovs-ofctl dump-ports br0 3) |
sed 's/ (xid=0x[0-9a-fA-F]*)//'

--- -   2014-04-25 01:25:43.022048477 -0700

+++ /root/openvswitch/tests/testsuite.dir/at-groups/328/stdout  2014-04-25
01:25:43.017711899 -0700

@@ -1,7 +1,7 @@

 OFPST_PORT reply: 1 ports

   port  2: rx pkts=0, bytes=0, drop=0, errs=0, frame=0, over=0, crc=0

-           tx pkts=2, bytes=120, drop=0, errs=0, coll=0

+           tx pkts=6, bytes=360, drop=0, errs=0, coll=0

 OFPST_PORT reply: 1 ports

   port  3: rx pkts=0, bytes=0, drop=0, errs=0, frame=0, over=0, crc=0

-           tx pkts=18, bytes=1080, drop=0, errs=0, coll=0

+           tx pkts=14, bytes=840, drop=0, errs=0, coll=0


On Thu, Apr 24, 2014 at 10:34 PM, Alex Wang <alexw at nicira.com> wrote:

> i'll kick an overnight run on "learning action - self-modifying flow with
> idle_timeout".~ (seq 10000).  if it survives, tmr morning,
> i'll ack it~~
>
>
> On Thu, Apr 24, 2014 at 10:26 PM, Joe Stringer <joestringer at nicira.com>wrote:
>
>> revalidate_ukey() had a bug where it would update the ukey->stats even
>> if it decided not to push stats (as an optimisation). ukey->stats should
>> only be updated when those stats are pushed.
>>
>> This bug would arise in the following situation:
>> * A flow has been dumped before.
>> * The flow needs to be revalidated.
>> * The flow is low-throughput.
>> * The flow has new statistics to push.
>>
>> Such cases rely on flow deletion to update the stats. However, that code
>> pushes the delta between the ukey->stats and the final flow dump. If the
>> ukey stats cache is updated without the stats being pushed, those stats
>> would be lost.
>>
>> This caused intermittent testsuite failures on "learning action -
>> self-modifying flow with idle_timeout". Introduced by 698ffe3623f1b630ae
>> "revalidator: Only revalidate high-throughput flows."
>>
>> Bug #1238927.
>>
>> Signed-off-by: Joe Stringer <joestringer at nicira.com>
>> ---
>>  ofproto/ofproto-dpif-upcall.c |    3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>> index 84afc56..5871772 100644
>> --- a/ofproto/ofproto-dpif-upcall.c
>> +++ b/ofproto/ofproto-dpif-upcall.c
>> @@ -1245,7 +1245,6 @@ revalidate_ukey(struct udpif *udpif, struct
>> udpif_key *ukey,
>>      push.n_bytes = stats->n_bytes > ukey->stats.n_bytes
>>          ? stats->n_bytes - ukey->stats.n_bytes
>>          : 0;
>> -    ukey->stats = *stats;
>>
>>      if (!ukey->flow_exists) {
>>          /* Don't bother revalidating if the flow was already deleted. */
>> @@ -1258,6 +1257,8 @@ revalidate_ukey(struct udpif *udpif, struct
>> udpif_key *ukey,
>>          goto exit;
>>      }
>>
>> +    /* We will push the stats, so update the ukey stats cache. */
>> +    ukey->stats = *stats;
>>      if (!push.n_packets && !udpif->need_revalidate) {
>>          ok = true;
>>          goto exit;
>> --
>> 1.7.10.4
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140425/90519f84/attachment-0005.html>


More information about the dev mailing list