Acked-by: Alex Wang <al...@nicira.com>
And applied,
On Fri, Apr 25, 2014 at 3:12 PM, Alex Wang <al...@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 <al...@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 <al...@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
>>> <joestrin...@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 <joestrin...@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
>>>>
>>>>
>>>
>>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
