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

[Joe Stringer]

I agree, it seems like a separate issue. With the new failure, the stats
are all accounted for, but are attributed to different ports than expected
(possibly because the packets are actually going across the different
port). The previous one caused stats to be omitted.


On 26 April 2014 10:12, Alex Wang <alexw at nicira.com> wrote:

> It is even harder to reproduce the new failure, rarer than the flow-dumps
> one.  Just ran into this once in my life...
>
> More importantly, the flow-dumps pkt counts are sync'ed in the failed case.
>
> From my understanding, this new issue, relates to reading stats from
> netdev-dummy, which is different
> area from this fix.
>
> So, this change makes sense to me.  I'll apply it first,
>
>
> On Fri, Apr 25, 2014 at 8:10 AM, Alex Wang <alexw at nicira.com> wrote:
>
>> 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/20140428/38bdb89d/attachment-0005.html>