[ovs-dev] [PATCH 31/41] openflow: Better abstract handling of packet-in messages.
Jarno Rajahalme
jarno at ovn.org
Wed Jan 20 01:09:30 UTC 2016
With two questions for clarification below,
Acked-by: Jarno Rajahalme <jarno at ovn.org>
> On Jan 18, 2016, at 11:27 PM, Ben Pfaff <blp at ovn.org> wrote:
>
(snip)
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> index 347cb61..48e2e8e 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -41,6 +41,7 @@
> #include "ofpbuf.h"
> #include "openflow/netronome-ext.h"
> #include "packets.h"
> +#include "pktbuf.h"
> #include "random.h"
> #include "tun-metadata.h"
> #include "unaligned.h"
> @@ -3311,9 +3312,18 @@ ofputil_encode_flow_removed(const struct ofputil_flow_removed *fr,
> return msg;
> }
>
> +/* Decodes the packet-in message starting at 'oh' into '*pin'. Populates
> + * 'pin->packet' and 'pin->len' with the part of the packet actually included
> + * in the message, and '*total_len' with the original length of the packet
> + * (which is larger than 'packet->len' if only part of the packet was
> + * included). Stores the packet's buffer ID in '*buffer_id' (UINT32_MAX if it
> + * was not buffered).
> + *
> + * Returns 0 if successful, otherwise an OpenFlow error code. */
> enum ofperr
> -ofputil_decode_packet_in(struct ofputil_packet_in *pin,
> - const struct ofp_header *oh)
> +ofputil_decode_packet_in(const struct ofp_header *oh,
> + struct ofputil_packet_in *pin,
> + size_t *total_len, uint32_t *buffer_id)
> {
> enum ofpraw raw;
> struct ofpbuf b;
> @@ -3339,28 +3349,26 @@ ofputil_decode_packet_in(struct ofputil_packet_in *pin,
>
> pin->reason = opi->reason;
> pin->table_id = opi->table_id;
> - pin->buffer_id = ntohl(opi->buffer_id);
> - pin->total_len = ntohs(opi->total_len);
> -
> - if (cookie) {
> - pin->cookie = *cookie;
> - }
> + *buffer_id = ntohl(opi->buffer_id);
> + *total_len = ntohs(opi->total_len);
> + pin->cookie = cookie ? *cookie : OVS_BE64_MAX;
Is this a bug fix in case there was no cookie?
>
> pin->packet = b.data;
> - pin->packet_len = b.size;
> + pin->len = b.size;
> } else if (raw == OFPRAW_OFPT10_PACKET_IN) {
> const struct ofp10_packet_in *opi;
>
> opi = ofpbuf_pull(&b, offsetof(struct ofp10_packet_in, data));
>
> pin->packet = opi->data;
> - pin->packet_len = b.size;
> + pin->len = b.size;
>
> match_init_catchall(&pin->flow_metadata);
> - match_set_in_port(&pin->flow_metadata, u16_to_ofp(ntohs(opi->in_port)));
> + match_set_in_port(&pin->flow_metadata,
> + u16_to_ofp(ntohs(opi->in_port)));
> pin->reason = opi->reason;
> - pin->buffer_id = ntohl(opi->buffer_id);
> - pin->total_len = ntohs(opi->total_len);
> + *buffer_id = ntohl(opi->buffer_id);
> + *total_len = ntohs(opi->total_len);
> } else if (raw == OFPRAW_OFPT11_PACKET_IN) {
> const struct ofp11_packet_in *opi;
> ofp_port_t in_port;
> @@ -3369,16 +3377,16 @@ ofputil_decode_packet_in(struct ofputil_packet_in *pin,
> opi = ofpbuf_pull(&b, sizeof *opi);
>
> pin->packet = b.data;
> - pin->packet_len = b.size;
> + pin->len = b.size;
>
> - pin->buffer_id = ntohl(opi->buffer_id);
> + *buffer_id = ntohl(opi->buffer_id);
> error = ofputil_port_from_ofp11(opi->in_port, &in_port);
> if (error) {
> return error;
> }
> match_init_catchall(&pin->flow_metadata);
> match_set_in_port(&pin->flow_metadata, in_port);
> - pin->total_len = ntohs(opi->total_len);
> + *total_len = ntohs(opi->total_len);
> pin->reason = opi->reason;
> pin->table_id = opi->table_id;
> } else if (raw == OFPRAW_NXT_PACKET_IN) {
> @@ -3400,11 +3408,11 @@ ofputil_decode_packet_in(struct ofputil_packet_in *pin,
> pin->table_id = npi->table_id;
> pin->cookie = npi->cookie;
>
> - pin->buffer_id = ntohl(npi->buffer_id);
> - pin->total_len = ntohs(npi->total_len);
> + *buffer_id = ntohl(npi->buffer_id);
> + *total_len = ntohs(npi->total_len);
>
> pin->packet = b.data;
> - pin->packet_len = b.size;
> + pin->len = b.size;
> } else {
> OVS_NOT_REACHED();
> }
> @@ -3412,77 +3420,103 @@ ofputil_decode_packet_in(struct ofputil_packet_in *pin,
> return 0;
> }
>
(snip)
>
> diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
> index c3e486c..fb8f251 100644
> --- a/ofproto/connmgr.c
> +++ b/ofproto/connmgr.c
> @@ -1483,33 +1483,6 @@ ofconn_receives_async_msg(const struct ofconn *ofconn,
> /* The default "table-miss" behaviour for OpenFlow1.3+ is to drop the
> * packet rather than to send the packet to the controller.
> *
> - * This function returns false to indicate the packet should be dropped if
> - * the controller action was the result of the default table-miss behaviour
> - * and the controller is using OpenFlow1.3+.
> - *
> - * Otherwise true is returned to indicate the packet should be forwarded to
> - * the controller */
> -static bool
> -ofconn_wants_packet_in_on_miss(struct ofconn *ofconn,
> - const struct ofproto_packet_in *pin)
> -{
> - if (pin->miss_type == OFPROTO_PACKET_IN_MISS_WITHOUT_FLOW) {
> - enum ofputil_protocol protocol = ofconn_get_protocol(ofconn);
> -
> - if (protocol != OFPUTIL_P_NONE
> - && ofputil_protocol_to_ofp_version(protocol) >= OFP13_VERSION
> - && (ofproto_table_get_miss_config(ofconn->connmgr->ofproto,
> - pin->up.table_id)
> - == OFPUTIL_TABLE_MISS_DEFAULT)) {
> - return false;
> - }
> - }
> - return true;
> -}
> -
> -/* The default "table-miss" behaviour for OpenFlow1.3+ is to drop the
> - * packet rather than to send the packet to the controller.
> - *
> * This function returns true to indicate that a packet_in message
> * for a "table-miss" should be sent to at least one controller.
> * That is there is at least one controller with controller_id 0
> @@ -1582,9 +1555,6 @@ ofconn_send(const struct ofconn *ofconn, struct ofpbuf *msg,
>
> /* Sending asynchronous messages. */
>
> -static void schedule_packet_in(struct ofconn *, struct ofproto_packet_in,
> - enum ofp_packet_in_reason wire_reason);
> -
> /* Sends an OFPT_PORT_STATUS message with 'opp' and 'reason' to appropriate
> * controllers managed by 'mgr'. For messages caused by a controller
> * OFPT_PORT_MOD, specify 'source' as the controller connection that sent the
> @@ -1678,41 +1648,6 @@ connmgr_send_flow_removed(struct connmgr *mgr,
> }
> }
>
> -/* Normally a send-to-controller action uses reason OFPR_ACTION. However, in
> - * OpenFlow 1.3 and later, packet_ins generated by a send-to-controller action
> - * in a "table-miss" flow (one with priority 0 and completely wildcarded) are
> - * sent as OFPR_NO_MATCH. This function returns the reason that should
> - * actually be sent on 'ofconn' for 'pin'. */
> -static enum ofp_packet_in_reason
> -wire_reason(struct ofconn *ofconn, const struct ofproto_packet_in *pin)
> -{
> - enum ofputil_protocol protocol = ofconn_get_protocol(ofconn);
> -
> - if (pin->miss_type == OFPROTO_PACKET_IN_MISS_FLOW
> - && pin->up.reason == OFPR_ACTION
> - && protocol != OFPUTIL_P_NONE
> - && ofputil_protocol_to_ofp_version(protocol) >= OFP13_VERSION) {
> - return OFPR_NO_MATCH;
> - }
> -
> - switch (pin->up.reason) {
> - case OFPR_ACTION_SET:
> - case OFPR_GROUP:
> - case OFPR_PACKET_OUT:
> - if (!(protocol & OFPUTIL_P_OF14_UP)) {
> - /* Only supported in OF1.4+ */
> - return OFPR_ACTION;
> - }
> - /* Fall through. */
> - case OFPR_NO_MATCH:
> - case OFPR_ACTION:
> - case OFPR_INVALID_TTL:
> - case OFPR_N_REASONS:
> - default:
> - return pin->up.reason;
> - }
> -}
> -
> /* Given 'pin', sends an OFPT_PACKET_IN message to each OpenFlow controller as
> * necessary according to their individual configurations.
> *
> @@ -1724,13 +1659,27 @@ connmgr_send_packet_in(struct connmgr *mgr,
> struct ofconn *ofconn;
>
> LIST_FOR_EACH (ofconn, node, &mgr->all_conns) {
> - enum ofp_packet_in_reason reason = wire_reason(ofconn, pin);
> -
> - if (ofconn_wants_packet_in_on_miss(ofconn, pin)
> - && ofconn_receives_async_msg(ofconn, OAM_PACKET_IN, reason)
> - && ofconn->controller_id == pin->controller_id) {
> - schedule_packet_in(ofconn, *pin, reason);
> + enum ofputil_protocol protocol = ofconn_get_protocol(ofconn);
> + if (protocol == OFPUTIL_P_NONE || !rconn_is_connected(ofconn->rconn)
> + || ofconn->controller_id != pin->controller_id
> + || !ofconn_receives_async_msg(ofconn, OAM_PACKET_IN,
> + pin->up.reason)) {
> + continue;
> }
> +
> + struct ofpbuf *msg = ofputil_encode_packet_in(
> + &pin->up, protocol, ofconn->packet_in_format,
> + pin->max_len >= 0 ? pin->max_len : ofconn->miss_send_len,
> + ofconn->pktbuf);
> +
> + struct ovs_list txq;
> + bool is_miss = (pin->up.reason == OFPR_NO_MATCH ||
> + pin->up.reason == OFPR_EXPLICIT_MISS ||
> + pin->up.reason == OFPR_IMPLICIT_MISS);
> + pinsched_send(ofconn->schedulers[is_miss],
> + pin->up.flow_metadata.flow.in_port.ofp_port /* XXX */,
I see you added the ‘XXX’. What’s wrong or risky about this?
> + msg, &txq);
> + do_send_packet_ins(ofconn, &txq);
> }
> }
(snip)
More information about the dev
mailing list