diff mbox

[ovs-dev] ofproto: Add support of OFPR_PACKET_OUT as packet-in reason

Message ID 1492190410-28595-1-git-send-email-yihung.wei@gmail.com
State Superseded
Headers show

Commit Message

Yi-Hung Wei April 14, 2017, 5:20 p.m. UTC 
This patch adds support of OFPR_PACKET_OUT as the packet-in reason.
This packet-in reason is a required feature for OF1.4+, and it indicates
that the associated packet-in message to the controller is triggered when
the switch is processing a packet-out message. This reason code is enabled
by default when OF1.4+ is used.

Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
---
 Documentation/topics/design.rst   |  1 +
 Documentation/topics/openflow.rst |  3 ++-
 include/openvswitch/ofp-util.h    |  2 ++
 lib/ofp-util.c                    | 16 ++++++++--------
 ofproto/connmgr.c                 |  5 +++--
 ofproto/ofproto-dpif-xlate.c      |  7 ++++++-
 ofproto/ofproto-dpif-xlate.h      |  3 +++
 ofproto/ofproto-dpif.c            |  1 +
 tests/ofproto-dpif.at             | 12 ++++++++++--
 tests/ofproto.at                  | 12 ++++++------
 10 files changed, 42 insertions(+), 20 deletions(-)

Comments

Ben Pfaff April 14, 2017, 9:17 p.m. UTC | #1 
On Fri, Apr 14, 2017 at 10:20:10AM -0700, Yi-Hung Wei wrote:
> This patch adds support of OFPR_PACKET_OUT as the packet-in reason.
> This packet-in reason is a required feature for OF1.4+, and it indicates
> that the associated packet-in message to the controller is triggered when
> the switch is processing a packet-out message. This reason code is enabled
> by default when OF1.4+ is used.
> 
> Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>

Thank you for working on this!  I would very much like to say that OVS
supports everything that OpenFlow 1.4 requires (so that we can enable it
by default), so it's nice to see some steps in that direction.

In openflow.rst, you mention that "All required features are now
supported" for packet-in reasons.  Are there optional packet-in reasons
that are still not supported?  If all packet-in reasons are now
supported, then I would prefer to delete the whole item, because these
items are supposed to list areas where there is still potential work to
do.

I don't think that the change to connmgr_send_async_msg() makes sense.
The 'reason' code passed to ofconn_receives_async_msg() is supposed to
be one of the bit indexes into the members of struct ofputil_async_cfg.
Those members are not well documented, but if you look around, for
example at ofputil_async_cfg_default(), you can see that they are
supposed to have OpenFlow version independent meanings.  Can you explain
this change?  By doing a version-dependent mapping before calling
ofconn_receives_async_msg(), we will lose some nuances; for example, we
will lose the difference between explicit and implicit misses.

In xlate_actions(), I believe that you can write:
        .in_packet_out = xin->in_packet_out ? true : false,
as
        .in_packet_out = xin->in_packet_out,
since both are booleans.

Thanks,

Ben.
Yi-Hung Wei April 17, 2017, 9:13 p.m. UTC | #2 
On Fri, Apr 14, 2017 at 2:17 PM, Ben Pfaff <blp@ovn.org> wrote:
> On Fri, Apr 14, 2017 at 10:20:10AM -0700, Yi-Hung Wei wrote:
>> This patch adds support of OFPR_PACKET_OUT as the packet-in reason.
>> This packet-in reason is a required feature for OF1.4+, and it indicates
>> that the associated packet-in message to the controller is triggered when
>> the switch is processing a packet-out message. This reason code is enabled
>> by default when OF1.4+ is used.
>>
>> Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
>
> Thank you for working on this!  I would very much like to say that OVS
> supports everything that OpenFlow 1.4 requires (so that we can enable it
> by default), so it's nice to see some steps in that direction.
>
> In openflow.rst, you mention that "All required features are now
> supported" for packet-in reasons.  Are there optional packet-in reasons
> that are still not supported?  If all packet-in reasons are now
> supported, then I would prefer to delete the whole item, because these
> items are supposed to list areas where there is still potential work to
> do.
Thanks for review. Yes, all packet-in reasons are now supported. I
will delete the
whole item in v2.

>
> I don't think that the change to connmgr_send_async_msg() makes sense.
> The 'reason' code passed to ofconn_receives_async_msg() is supposed to
> be one of the bit indexes into the members of struct ofputil_async_cfg.
> Those members are not well documented, but if you look around, for
> example at ofputil_async_cfg_default(), you can see that they are
> supposed to have OpenFlow version independent meanings.  Can you explain
> this change?  By doing a version-dependent mapping before calling
> ofconn_receives_async_msg(), we will lose some nuances; for example, we
> will lose the difference between explicit and implicit misses.
Yes, thanks for pointing out this problem. Actually, it is due to
another issue that should be addressed
by a separate patch. I just sent it out as in v2.

>
> In xlate_actions(), I believe that you can write:
>         .in_packet_out = xin->in_packet_out ? true : false,
> as
>         .in_packet_out = xin->in_packet_out,
> since both are booleans.
Sure.

>
> Thanks,
>
> Ben.

Thanks,

-Yi-Hung
diff mbox

Patch

diff --git a/Documentation/topics/design.rst b/Documentation/topics/design.rst
index adc407ac9227..eb290ab68692 100644
--- a/Documentation/topics/design.rst
+++ b/Documentation/topics/design.rst
@@ -79,6 +79,7 @@  that the message is suppressed.
   ``OFPR_INVALID_TTL``                          ---    ---
   ``OFPR_ACTION_SET`` (OF1.4+)                  yes    ---
   ``OFPR_GROUP`` (OF1.4+)                       yes    ---
+  ``OFPR_PACKET_OUT`` (OF1.4+)                  yes    ---
   =========================================== ======= =====
 
 .. table:: ``OFPT_FLOW_REMOVED`` / ``NXT_FLOW_REMOVED``
diff --git a/Documentation/topics/openflow.rst b/Documentation/topics/openflow.rst
index 771246706958..7adf930989a4 100644
--- a/Documentation/topics/openflow.rst
+++ b/Documentation/topics/openflow.rst
@@ -279,7 +279,8 @@  features are listed in the previous section.
 
   Distinguish ``OFPR_APPLY_ACTION``, ``OFPR_ACTION_SET``, ``OFPR_GROUP``,
   ``OFPR_PACKET_OUT``.  ``NO_MATCH`` was renamed to ``OFPR_TABLE_MISS``.
-  (OFPR_ACTION_SET and OFPR_GROUP are now supported)
+
+  All required features are now supported.
 
   (EXT-136)
 
diff --git a/include/openvswitch/ofp-util.h b/include/openvswitch/ofp-util.h
index f664055c3939..53a7a9d254ed 100644
--- a/include/openvswitch/ofp-util.h
+++ b/include/openvswitch/ofp-util.h
@@ -473,6 +473,8 @@  const char *ofputil_packet_in_reason_to_string(enum ofp_packet_in_reason,
                                                size_t bufsize);
 bool ofputil_packet_in_reason_from_string(const char *,
                                           enum ofp_packet_in_reason *);
+int ofputil_encode_packet_in_reason(enum ofp_packet_in_reason,
+                                    enum ofp_version);
 
 /* A packet-in message, including continuation data.  The format of
  * continuation data is subject to change and thus it is supposed to be opaque
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 1f038c61ea97..28e06fcf770f 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -3613,9 +3613,9 @@  ofputil_decode_packet_in(const struct ofp_header *oh, bool loose,
     return 0;
 }
 
-static int
-encode_packet_in_reason(enum ofp_packet_in_reason reason,
-                        enum ofp_version version)
+int
+ofputil_encode_packet_in_reason(enum ofp_packet_in_reason reason,
+                                enum ofp_version version)
 {
     switch (reason) {
     case OFPR_NO_MATCH:
@@ -3661,7 +3661,7 @@  ofputil_put_packet_in(const struct ofputil_packet_in *pin,
 
     /* Add other properties. */
     ofpprop_put_u8(msg, NXPINT_REASON,
-                   encode_packet_in_reason(pin->reason, version));
+                   ofputil_encode_packet_in_reason(pin->reason, version));
 
     size_t start = ofpprop_start(msg, NXPINT_METADATA);
     oxm_put_raw(msg, &pin->flow_metadata, version);
@@ -3793,7 +3793,7 @@  ofputil_encode_ofp10_packet_in(const struct ofputil_packet_in *pin)
     opi = ofpbuf_put_zeros(msg, offsetof(struct ofp10_packet_in, data));
     opi->total_len = htons(pin->packet_len);
     opi->in_port = htons(ofp_to_u16(pin->flow_metadata.flow.in_port.ofp_port));
-    opi->reason = encode_packet_in_reason(pin->reason, OFP10_VERSION);
+    opi->reason = ofputil_encode_packet_in_reason(pin->reason, OFP10_VERSION);
     opi->buffer_id = htonl(UINT32_MAX);
 
     return msg;
@@ -3817,7 +3817,7 @@  ofputil_encode_nx_packet_in(const struct ofputil_packet_in *pin,
     npi = msg->msg;
     npi->buffer_id = htonl(UINT32_MAX);
     npi->total_len = htons(pin->packet_len);
-    npi->reason = encode_packet_in_reason(pin->reason, version);
+    npi->reason = ofputil_encode_packet_in_reason(pin->reason, version);
     npi->table_id = pin->table_id;
     npi->cookie = pin->cookie;
     npi->match_len = htons(match_len);
@@ -3863,7 +3863,7 @@  ofputil_encode_ofp11_packet_in(const struct ofputil_packet_in *pin)
         pin->flow_metadata.flow.in_port.ofp_port);
     opi->in_phy_port = opi->in_port;
     opi->total_len = htons(pin->packet_len);
-    opi->reason = encode_packet_in_reason(pin->reason, OFP11_VERSION);
+    opi->reason = ofputil_encode_packet_in_reason(pin->reason, OFP11_VERSION);
     opi->table_id = pin->table_id;
 
     return msg;
@@ -3885,7 +3885,7 @@  ofputil_encode_ofp12_packet_in(const struct ofputil_packet_in *pin,
     struct ofp12_packet_in *opi = ofpbuf_put_zeros(msg, sizeof *opi);
     opi->buffer_id = htonl(UINT32_MAX);
     opi->total_len = htons(pin->packet_len);
-    opi->reason = encode_packet_in_reason(pin->reason, version);
+    opi->reason = ofputil_encode_packet_in_reason(pin->reason, version);
     opi->table_id = pin->table_id;
 
     if (version >= OFP13_VERSION) {
diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index 854868e7be78..27b49944b309 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -1727,10 +1727,11 @@  connmgr_send_async_msg(struct connmgr *mgr,
 
     LIST_FOR_EACH (ofconn, node, &mgr->all_conns) {
         enum ofputil_protocol protocol = ofconn_get_protocol(ofconn);
+        int reason = ofputil_encode_packet_in_reason(am->pin.up.public.reason,
+            ofputil_protocol_to_ofp_version(protocol));
         if (protocol == OFPUTIL_P_NONE || !rconn_is_connected(ofconn->rconn)
             || ofconn->controller_id != am->controller_id
-            || !ofconn_receives_async_msg(ofconn, am->oam,
-                                          am->pin.up.public.reason)) {
+            || !ofconn_receives_async_msg(ofconn, am->oam, reason)) {
             continue;
         }
 
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index a24aef9a43a1..f76fe2769ec5 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -231,6 +231,8 @@  struct xlate_ctx {
     int resubmits;              /* Total number of resubmits. */
     bool in_group;              /* Currently translating ofgroup, if true. */
     bool in_action_set;         /* Currently translating action_set, if true. */
+    bool in_packet_out;         /* Currently translating a packet_out msg, if
+                                 * true. */
 
     uint8_t table_id;           /* OpenFlow table ID where flow was found. */
     ovs_be64 rule_cookie;       /* Cookie of the rule being translated. */
@@ -4545,7 +4547,8 @@  xlate_output_action(struct xlate_ctx *ctx,
         break;
     case OFPP_CONTROLLER:
         execute_controller_action(ctx, max_len,
-                                  (ctx->in_group ? OFPR_GROUP
+                                  (ctx->in_packet_out ? OFPR_PACKET_OUT
+                                   : ctx->in_group ? OFPR_GROUP
                                    : ctx->in_action_set ? OFPR_ACTION_SET
                                    : OFPR_ACTION),
                                   0, NULL, 0);
@@ -5927,6 +5930,7 @@  xlate_in_init(struct xlate_in *xin, struct ofproto_dpif *ofproto,
     xin->resubmits = 0;
     xin->wc = wc;
     xin->odp_actions = odp_actions;
+    xin->in_packet_out = false;
 
     /* Do recirc lookup. */
     xin->frozen_state = NULL;
@@ -6189,6 +6193,7 @@  xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
         .resubmits = xin->resubmits,
         .in_group = false,
         .in_action_set = false,
+        .in_packet_out = xin->in_packet_out ? true : false,
 
         .table_id = 0,
         .rule_cookie = OVS_BE64_MAX,
diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
index 12abfa32bc32..68e114afb9ae 100644
--- a/ofproto/ofproto-dpif-xlate.h
+++ b/ofproto/ofproto-dpif-xlate.h
@@ -155,6 +155,9 @@  struct xlate_in {
 
     /* The frozen state to be resumed, as returned by xlate_lookup(). */
     const struct frozen_state *frozen_state;
+
+    /* If true, the packet to be translated is from a packet_out msg. */
+    bool in_packet_out;
 };
 
 void xlate_ofproto_set(struct ofproto_dpif *, const char *name, struct dpif *,
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 6a5ffb94fa94..a4b86782c19f 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4352,6 +4352,7 @@  packet_xlate(struct ofproto *ofproto_, struct ofproto_packet_out *opo)
     xin.allow_side_effects = false;
     xin.resubmit_stats = NULL;
     xin.xcache = &aux->xcache;
+    xin.in_packet_out = true;
 
     if (xlate_actions(&xin, &xout) != XLATE_OK) {
         error = OFPERR_OFPFMFC_UNKNOWN;   /* Error processing actions. */
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 0c2ea384b422..58b3aacc0e39 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -3025,7 +3025,8 @@  AT_CHECK([ovs-ofctl monitor -P standard --protocols=OpenFlow13 br0 65534 --detac
 for i in 1 2 3 ; do
     ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=10:11:11:11:11:11,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no),tcp(src=8,dst=10),tcp_flags(0x002)'
 done
-OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 6])
+AT_CHECK([ovs-ofctl packet-out br0 'in_port=NONE, packet=505400000007101111111111080045000028000000004006f97cc0a80001c0a800020008000a0000000000000000500200002e7d0000, actions=controller'])
+OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 7])
 OVS_APP_EXIT_AND_WAIT([ovs-ofctl])
 
 AT_CHECK([ovs-appctl time/warp 5000], [0], [ignore])
@@ -3057,6 +3058,9 @@  tcp,vlan_tci=0x0000,dl_src=10:11:11:11:11:11,dl_dst=50:54:00:00:00:07,nw_src=192
 dnl
 OFPT_PACKET_IN (OF1.3) (xid=0x0): cookie=0x0 total_len=54 in_port=1 (via action) data_len=54 (unbuffered)
 tcp,vlan_tci=0x0000,dl_src=10:11:11:11:11:11,dl_dst=50:54:00:00:00:07,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=8,tp_dst=10,tcp_flags=syn tcp_csum:2e7d
+dnl
+OFPT_PACKET_IN (OF1.3) (xid=0x0): total_len=54 in_port=ANY (via action) data_len=54 (unbuffered)
+tcp,vlan_tci=0x0000,dl_src=10:11:11:11:11:11,dl_dst=50:54:00:00:00:07,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=8,tp_dst=10,tcp_flags=syn tcp_csum:2e7d
 ])
 
 AT_CHECK([ovs-appctl time/warp 5000], [0], [ignore])
@@ -3093,7 +3097,8 @@  AT_CHECK([ovs-ofctl monitor -P standard --protocols=OpenFlow14 br0 65534 --detac
 for i in 1 2 3 ; do
     ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=10:11:11:11:11:11,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=64,frag=no),tcp(src=8,dst=10),tcp_flags(0x002)'
 done
-OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 6])
+AT_CHECK([ovs-ofctl packet-out br0 'in_port=NONE, packet=505400000007101111111111080045000028000000004006f97cc0a80001c0a800020008000a0000000000000000500200002e7d0000, actions=controller'])
+OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 7])
 OVS_APP_EXIT_AND_WAIT([ovs-ofctl])
 
 AT_CHECK([ovs-appctl time/warp 5000], [0], [ignore])
@@ -3125,6 +3130,9 @@  tcp,vlan_tci=0x0000,dl_src=10:11:11:11:11:11,dl_dst=50:54:00:00:00:07,nw_src=192
 dnl
 OFPT_PACKET_IN (OF1.4) (xid=0x0): cookie=0x0 total_len=54 in_port=1 (via action_set) data_len=54 (unbuffered)
 tcp,vlan_tci=0x0000,dl_src=10:11:11:11:11:11,dl_dst=50:54:00:00:00:07,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=8,tp_dst=10,tcp_flags=syn tcp_csum:2e7d
+dnl
+OFPT_PACKET_IN (OF1.4) (xid=0x0): total_len=54 in_port=ANY (via packet_out) data_len=54 (unbuffered)
+tcp,vlan_tci=0x0000,dl_src=10:11:11:11:11:11,dl_dst=50:54:00:00:00:07,nw_src=192.168.0.1,nw_dst=192.168.0.2,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=8,tp_dst=10,tcp_flags=syn tcp_csum:2e7d
 ])
 
 AT_CHECK([ovs-appctl time/warp 5000], [0], [ignore])
diff --git a/tests/ofproto.at b/tests/ofproto.at
index 5c0d0762390f..556d469b8d55 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -3481,10 +3481,10 @@  check_async () {
     ovs-appctl -t ovs-ofctl ofctl/set-output-file monitor.log
     : > expout
 
-    # OFPT_PACKET_IN, OFPR_ACTION (controller_id=0)
+    # OFPT_PACKET_IN, OFPR_PACKET_OUT (controller_id=0)
     ovs-ofctl -O OpenFlow14 -v packet-out br0 none controller '0001020304050010203040501234'
-    if test X"$1" = X"OFPR_ACTION"; then shift;
-        echo >>expout "OFPT_PACKET_IN (OF1.4): total_len=14 in_port=ANY (via action) data_len=14 (unbuffered)
+    if test X"$1" = X"OFPR_PACKET_OUT"; then shift;
+        echo >>expout "OFPT_PACKET_IN (OF1.4): total_len=14 in_port=ANY (via packet_out) data_len=14 (unbuffered)
 vlan_tci=0x0000,dl_src=00:10:20:30:40:50,dl_dst=00:01:02:03:04:05,dl_type=0x1234"
     fi
 
@@ -3644,14 +3644,14 @@  check_async 1
 
 # Set miss_send_len to 128, turning on packet-ins for our service connection.
 ovs-appctl -t ovs-ofctl ofctl/send 0509000c0123456700000080
-check_async 2 OFPR_ACTION OFPPR_ADD OFPPR_MODIFY OFPPR_DELETE OFPRR_DELETE OFPRR_GROUP_DELETE
+check_async 2 OFPR_PACKET_OUT OFPPR_ADD OFPPR_MODIFY OFPPR_DELETE OFPRR_DELETE OFPRR_GROUP_DELETE
 
 # Become slave (OF 1.4), which should disable everything except port status.
 ovs-appctl -t ovs-ofctl ofctl/send 051800180000000200000003000000000000000000000001
 check_async 3 OFPPR_ADD OFPPR_MODIFY OFPPR_DELETE
 
 # Use OF 1.4 OFPT_SET_ASYNC to enable a patchwork of asynchronous messages.
-ovs-appctl -t ovs-ofctl ofctl/send 051c0040000000020000000800000005000100080000000200020008000000020003000800000005000400080000001c00050008000000050008000800000018
+ovs-appctl -t ovs-ofctl ofctl/send 051c0040000000020000000800000005000100080000002000020008000000020003000800000005000400080000001c00050008000000050008000800000018
 check_async 4 OFPR_INVALID_TTL OFPPR_DELETE OFPRR_DELETE OFPRR_GROUP_DELETE OFPTR_VACANCY_UP
 
 # Set controller ID 123.
@@ -3663,7 +3663,7 @@  ovs-appctl -t ovs-ofctl ofctl/send 050400180000000300002320000000140000000000000
 
 # Become master (OF 1.4).
 ovs-appctl -t ovs-ofctl ofctl/send 051800180000000400000002000000000000000000000002
-check_async 6 OFPR_ACTION OFPPR_ADD OFPPR_MODIFY OFPRR_DELETE
+check_async 6 OFPR_PACKET_OUT OFPPR_ADD OFPPR_MODIFY OFPRR_DELETE
 
 OVS_APP_EXIT_AND_WAIT([ovs-ofctl])
 OVS_VSWITCHD_STOP