diff mbox series

[ovs-dev] ofproto-dpif: Fix NXT_RESUME flow stats

Message ID 1537548411-25464-1-git-send-email-yihung.wei@gmail.com
State Accepted
Headers show
Series [ovs-dev] ofproto-dpif: Fix NXT_RESUME flow stats | expand

Commit Message

Yi-Hung Wei Sept. 21, 2018, 4:46 p.m. UTC
Currently, OVS does not update the flow stats after a packet is
restarted by NXT_RESUME message.  This patch fixes the aforementioned
issue and adds an unit test to prevent regression.

Fixes: 77ab5fd2a95b ("Implement serializing the state of packet traversal
                      in "continuations".")
VMware-BZ: #2198435
Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
---
 ofproto/ofproto-dpif-xlate.c | 10 ++++++----
 ofproto/ofproto-dpif-xlate.h |  4 ++--
 ofproto/ofproto-dpif.c       | 13 ++++++++++++-
 tests/ofproto-dpif.at        | 33 +++++++++++++++++++++++++++++++++
 4 files changed, 53 insertions(+), 7 deletions(-)

Comments

Ben Pfaff Sept. 21, 2018, 8:22 p.m. UTC | #1
On Fri, Sep 21, 2018 at 09:46:51AM -0700, Yi-Hung Wei wrote:
> Currently, OVS does not update the flow stats after a packet is
> restarted by NXT_RESUME message.  This patch fixes the aforementioned
> issue and adds an unit test to prevent regression.
> 
> Fixes: 77ab5fd2a95b ("Implement serializing the state of packet traversal
>                       in "continuations".")
> VMware-BZ: #2198435
> Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>

Thanks.  I applied this to master.  Does it need backports?
Yi-Hung Wei Sept. 21, 2018, 8:39 p.m. UTC | #2
On Fri, Sep 21, 2018 at 1:22 PM Ben Pfaff <blp@ovn.org> wrote:
>
> On Fri, Sep 21, 2018 at 09:46:51AM -0700, Yi-Hung Wei wrote:
> > Currently, OVS does not update the flow stats after a packet is
> > restarted by NXT_RESUME message.  This patch fixes the aforementioned
> > issue and adds an unit test to prevent regression.
> >
> > Fixes: 77ab5fd2a95b ("Implement serializing the state of packet traversal
> >                       in "continuations".")
> > VMware-BZ: #2198435
> > Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
>
> Thanks.  I applied this to master.  Does it need backports?

Thanks for the quick review.

This issue was introduced in commit 77ab5fd2a95b ("Implement
serializing the state of packet traversal  in "continuations".") at
2.6.  Not sure if this patch can be cleanly applied.

Can we try to backport it back to 2.6 ?

Thanks,

-Yi-Hung
Ben Pfaff Sept. 21, 2018, 8:53 p.m. UTC | #3
On Fri, Sep 21, 2018 at 01:39:00PM -0700, Yi-Hung Wei wrote:
> On Fri, Sep 21, 2018 at 1:22 PM Ben Pfaff <blp@ovn.org> wrote:
> >
> > On Fri, Sep 21, 2018 at 09:46:51AM -0700, Yi-Hung Wei wrote:
> > > Currently, OVS does not update the flow stats after a packet is
> > > restarted by NXT_RESUME message.  This patch fixes the aforementioned
> > > issue and adds an unit test to prevent regression.
> > >
> > > Fixes: 77ab5fd2a95b ("Implement serializing the state of packet traversal
> > >                       in "continuations".")
> > > VMware-BZ: #2198435
> > > Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
> >
> > Thanks.  I applied this to master.  Does it need backports?
> 
> Thanks for the quick review.
> 
> This issue was introduced in commit 77ab5fd2a95b ("Implement
> serializing the state of packet traversal  in "continuations".") at
> 2.6.  Not sure if this patch can be cleanly applied.
> 
> Can we try to backport it back to 2.6 ?

It cleanly applied and built back to 2.8, so I did that.
diff mbox series

Patch

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 507e14dd0d00..d0e7c6b9f55d 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -7464,19 +7464,21 @@  enum ofperr
 xlate_resume(struct ofproto_dpif *ofproto,
              const struct ofputil_packet_in_private *pin,
              struct ofpbuf *odp_actions,
-             enum slow_path_reason *slow)
+             enum slow_path_reason *slow,
+             struct flow *flow,
+             struct xlate_cache *xcache)
 {
     struct dp_packet packet;
     dp_packet_use_const(&packet, pin->base.packet,
                         pin->base.packet_len);
 
-    struct flow flow;
-    flow_extract(&packet, &flow);
+    flow_extract(&packet, flow);
 
     struct xlate_in xin;
     xlate_in_init(&xin, ofproto, ofproto_dpif_get_tables_version(ofproto),
-                  &flow, 0, NULL, ntohs(flow.tcp_flags),
+                  flow, 0, NULL, ntohs(flow->tcp_flags),
                   &packet, NULL, odp_actions);
+    xin.xcache = xcache;
 
     struct ofpact_note noop;
     ofpact_init_NOTE(&noop);
diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
index 2cbb3c909a91..0a5a52887b80 100644
--- a/ofproto/ofproto-dpif-xlate.h
+++ b/ofproto/ofproto-dpif-xlate.h
@@ -230,8 +230,8 @@  void xlate_out_uninit(struct xlate_out *);
 
 enum ofperr xlate_resume(struct ofproto_dpif *,
                          const struct ofputil_packet_in_private *,
-                         struct ofpbuf *odp_actions, enum slow_path_reason *);
-
+                         struct ofpbuf *odp_actions, enum slow_path_reason *,
+                         struct flow *, struct xlate_cache *);
 int xlate_send_packet(const struct ofport_dpif *, bool oam, struct dp_packet *);
 
 void xlate_mac_learning_update(const struct ofproto_dpif *ofproto,
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 0a0c69a7aea0..5ccf23955e6a 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5051,18 +5051,29 @@  nxt_resume(struct ofproto *ofproto_,
            const struct ofputil_packet_in_private *pin)
 {
     struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
+    struct dpif_flow_stats stats;
+    struct xlate_cache xcache;
+    struct flow flow;
+    xlate_cache_init(&xcache);
 
     /* Translate pin into datapath actions. */
     uint64_t odp_actions_stub[1024 / 8];
     struct ofpbuf odp_actions = OFPBUF_STUB_INITIALIZER(odp_actions_stub);
     enum slow_path_reason slow;
-    enum ofperr error = xlate_resume(ofproto, pin, &odp_actions, &slow);
+    enum ofperr error = xlate_resume(ofproto, pin, &odp_actions, &slow,
+                                     &flow, &xcache);
 
     /* Steal 'pin->packet' and put it into a dp_packet. */
     struct dp_packet packet;
     dp_packet_init(&packet, pin->base.packet_len);
     dp_packet_put(&packet, pin->base.packet, pin->base.packet_len);
 
+    /* Run the side effects from the xcache. */
+    dpif_flow_stats_extract(&flow, &packet, time_msec(), &stats);
+    ovs_mutex_lock(&ofproto_mutex);
+    ofproto_dpif_xcache_execute(ofproto, &xcache, &stats);
+    ovs_mutex_unlock(&ofproto_mutex);
+
     pkt_metadata_from_flow(&packet.md, &pin->base.flow_metadata.flow);
 
     /* Fix up in_port. */
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 6b1499f99d78..693a64cab685 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -5393,6 +5393,39 @@  CHECK_CONTINUATION([patch ports], [4], [1],
        -- set interface patch11 type=patch options:peer=patch10 \
                                 ofport_request=11])
 
+AT_SETUP([ofproto-dpif - continuation flow stats])
+AT_KEYWORDS([continuations pause resume])
+OVS_VSWITCHD_START
+
+add_of_ports --pcap br0 `seq 1 2`
+
+flow="in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)"
+
+AT_DATA([flows.txt], [dnl
+table=0 in_port=1 actions=set_field:1->reg1 controller(pause) resubmit(,2)
+table=2 reg1=0x1 actions=2
+])
+
+AT_CHECK([ovs-ofctl -O OpenFlow13 add-flows br0 flows.txt])
+
+AT_CAPTURE_FILE([ofctl_monitor0.log])
+ovs-ofctl monitor br0 resume --detach --no-chdir \
+--pidfile=ovs-ofctl0.pid 2> ofctl_monitor0.log
+
+# Run a packet through the switch.
+AT_CHECK([ovs-appctl netdev-dummy/receive p1 "$flow"], [0], [stdout])
+
+# Check flow stats
+AT_CHECK([ovs-ofctl dump-flows br0], [0], [stdout])
+AT_CHECK([strip_xids < stdout | sed -n 's/duration=[[0-9]]*\.[[0-9]]*s/duration=0.0s/p' | sed -n 's/idle_age=[[0-9]]*/idle_age=0/p' | grep 'table=2'], [0], [dnl
+ cookie=0x0, duration=0.0s, table=2, n_packets=1, n_bytes=106, idle_age=0, reg1=0x1 actions=output:2
+])
+
+# The packet should be recieved by port 2
+AT_CHECK([test 1 = `$PYTHON "$top_srcdir/utilities/ovs-pcap.in" p2-tx.pcap | wc -l`])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
 
 # Check that pause works after the packet is cloned.
 AT_SETUP([ofproto-dpif - continuation after clone])