[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