With two questions for clarification below, Acked-by: Jarno Rajahalme <ja...@ovn.org>
> On Jan 18, 2016, at 11:27 PM, Ben Pfaff <b...@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) _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev