Hi,
I am somewhat confused about this patch.
I became confused while trying to make use of pin->generated_by_table_miss
to drop controller actions in connmgr_send_packet_in()
for OpenFlow1.3 if its default table-miss policy is in effect.
I have successfully implemented a prototype pf that logic.
But my confusion about this patch remains.
On Tue, Oct 22, 2013 at 10:05:19PM -0700, Ben Pfaff wrote:
> From: YAMAMOTO Takashi <yamam...@valinux.co.jp>
>
> As per spec, make packet-in reason for OpenFlow1.3 table-miss flow
> entries no_match rather than action.
>
> Signed-off-by: YAMAMOTO Takashi <yamam...@valinux.co.jp>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> OPENFLOW-1.1+ | 3 ---
> ofproto/connmgr.c | 32 ++++++++++++++++++++++++++++----
> ofproto/connmgr.h | 5 +++++
> ofproto/fail-open.c | 1 +
> ofproto/ofproto-dpif-upcall.c | 1 +
> ofproto/ofproto-dpif-xlate.c | 2 ++
> ofproto/ofproto-dpif.c | 5 +++++
> ofproto/ofproto-dpif.h | 1 +
> ofproto/ofproto-provider.h | 12 ++++++++++++
> 9 files changed, 55 insertions(+), 7 deletions(-)
>
> diff --git a/OPENFLOW-1.1+ b/OPENFLOW-1.1+
> index 22be72c..bd6ced6 100644
> --- a/OPENFLOW-1.1+
> +++ b/OPENFLOW-1.1+
> @@ -130,9 +130,6 @@ didn't compare the specs carefully yet.)
> - Change the default table-miss action (in the absense of table-miss
> entry) from packet_in to drop for OF1.3+. Decide what to do if
> a switch is configured to support multiple OF versions.
> - - Distinguish table-miss flow entry and make its packet_in reason
> - OFPR_NO_MATCH. (OFPR_TABLE_MISS for OF1.4)
> - Also, make it use the appropriate pin scheduler.
Looking over the spec, it seems to me that OpenFlow1.0, 1,1 or 1.2
should also use OFPR_NO_MATCH.
Furthermore it seems to me, looking at the current master branch, which is
admittedly about a month more recent than this patch, that Open vSwtich
uses OFPR_NO_MATCH regardless of the Open Flow version used.
I base this on the assumption that miss-rules are added by
add_internal_flow(). That function adds rules with controller actions with
the reason set to OFPR_NO_MATCH.
> [required for OF1.3+]
>
> * IPv6 extension header handling support. Fully implementing this
> diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
> index f5bef59..d42ab4f 100644
> --- a/ofproto/connmgr.c
> +++ b/ofproto/connmgr.c
> @@ -1435,7 +1435,8 @@ ofconn_send(const struct ofconn *ofconn, struct ofpbuf
> *msg,
>
> /* Sending asynchronous messages. */
>
> -static void schedule_packet_in(struct ofconn *, struct ofproto_packet_in);
> +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'. */
> @@ -1482,6 +1483,25 @@ 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)
> +{
> + if (pin->generated_by_table_miss && pin->up.reason == OFPR_ACTION) {
> + enum ofputil_protocol protocol = ofconn_get_protocol(ofconn);
> + enum ofp_version version = ofputil_protocol_to_ofp_version(protocol);
> +
> + if (version >= OFP13_VERSION) {
> + return OFPR_NO_MATCH;
> + }
> + }
> + return pin->up.reason;
> +}
> +
> /* Given 'pin', sends an OFPT_PACKET_IN message to each OpenFlow controller
> as
> * necessary according to their individual configurations.
> *
> @@ -1493,9 +1513,11 @@ connmgr_send_packet_in(struct connmgr *mgr,
> struct ofconn *ofconn;
>
> LIST_FOR_EACH (ofconn, node, &mgr->all_conns) {
> - if (ofconn_receives_async_msg(ofconn, OAM_PACKET_IN, pin->up.reason)
> + enum ofp_packet_in_reason reason = wire_reason(ofconn, pin);
> +
> + if (ofconn_receives_async_msg(ofconn, OAM_PACKET_IN, reason)
> && ofconn->controller_id == pin->controller_id) {
> - schedule_packet_in(ofconn, *pin);
> + schedule_packet_in(ofconn, *pin, reason);
> }
> }
> }
> @@ -1513,13 +1535,15 @@ do_send_packet_in(struct ofpbuf *ofp_packet_in, void
> *ofconn_)
> /* Takes 'pin', composes an OpenFlow packet-in message from it, and passes it
> * to 'ofconn''s packet scheduler for sending. */
> static void
> -schedule_packet_in(struct ofconn *ofconn, struct ofproto_packet_in pin)
> +schedule_packet_in(struct ofconn *ofconn, struct ofproto_packet_in pin,
> + enum ofp_packet_in_reason wire_reason)
> {
> struct connmgr *mgr = ofconn->connmgr;
> uint16_t controller_max_len;
>
> pin.up.total_len = pin.up.packet_len;
>
> + pin.up.reason = wire_reason;
> if (pin.up.reason == OFPR_ACTION) {
> controller_max_len = pin.send_len; /* max_len */
> } else {
> diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h
> index e9ffbbc..10caaf4 100644
> --- a/ofproto/connmgr.h
> +++ b/ofproto/connmgr.h
> @@ -68,6 +68,11 @@ struct ofproto_packet_in {
> struct list list_node; /* For queuing. */
> uint16_t controller_id; /* Controller ID to send to. */
> int send_len; /* Length that the action requested sending.
> */
> +
> + /* True if the packet_in was generated directly by a table-miss flow,
> that
> + * is, a flow with priority 0 that wildcards all fields. (Our
> + * interpretation of "directly" is "not via groups".) */
> + bool generated_by_table_miss;
> };
>
> /* Basics. */
> diff --git a/ofproto/fail-open.c b/ofproto/fail-open.c
> index b0caf8d..bae9dca 100644
> --- a/ofproto/fail-open.c
> +++ b/ofproto/fail-open.c
> @@ -130,6 +130,7 @@ send_bogus_packet_ins(struct fail_open *fo)
> pin.up.reason = OFPR_NO_MATCH;
> pin.up.fmd.in_port = OFPP_LOCAL;
> pin.send_len = b.size;
> + pin.generated_by_table_miss = false;
> connmgr_send_packet_in(fo->connmgr, &pin);
>
> ofpbuf_uninit(&b);
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 491f11e..24a5729 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -849,6 +849,7 @@ handle_upcalls(struct udpif *udpif, struct list *upcalls)
> pin->up.cookie = OVS_BE64_MAX;
> flow_get_metadata(&miss->flow, &pin->up.fmd);
> pin->send_len = 0; /* Not used for flow table misses. */
> + pin->generated_by_table_miss = false;
> ofproto_dpif_send_packet_in(miss->ofproto, pin);
> }
> }
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index ed9ca7c..1eb0953 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -1856,6 +1856,8 @@ execute_controller_action(struct xlate_ctx *ctx, int
> len,
>
> pin->controller_id = controller_id;
> pin->send_len = len;
> + pin->generated_by_table_miss = (ctx->rule
> + && rule_dpif_is_table_miss(ctx->rule));
> ofproto_dpif_send_packet_in(ctx->xbridge->ofproto, pin);
> ofpbuf_delete(packet);
> }
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 67c21f5..be7f807 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -4308,6 +4308,11 @@ rule_dpif_is_fail_open(const struct rule_dpif *rule)
> {
> return is_fail_open_rule(&rule->up);
> }
> +
> +bool
> +rule_dpif_is_table_miss(const struct rule_dpif *rule)
> +{
> + return rule_is_table_miss(&rule->up);
> }
>
> ovs_be64
> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> index ab5dfbb..c93d37c 100644
> --- a/ofproto/ofproto-dpif.h
> +++ b/ofproto/ofproto-dpif.h
> @@ -75,6 +75,7 @@ void rule_dpif_credit_stats(struct rule_dpif *rule ,
> const struct dpif_flow_stats *);
>
> bool rule_dpif_is_fail_open(const struct rule_dpif *);
> +bool rule_dpif_is_table_miss(const struct rule_dpif *);
>
> struct rule_actions *rule_dpif_get_actions(const struct rule_dpif *);
>
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index de566e3..07bb266 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -398,6 +398,18 @@ struct rule_actions *rule_get_actions(const struct rule
> *rule)
> struct rule_actions *rule_get_actions__(const struct rule *rule)
> OVS_REQUIRES(rule->mutex);
>
> +/* Returns true if 'rule' is an OpenFlow 1.3 "table-miss" rule, false
> + * otherwise.
> + *
> + * ("Table-miss" rules are special because a packet_in generated through one
> + * uses OFPR_NO_MATCH as its reason, whereas packet_ins generated by any
> other
> + * rule use OFPR_ACTION.) */
> +static inline bool
> +rule_is_table_miss(const struct rule *rule)
> +{
> + return rule->cr.priority == 0 && cls_rule_is_catchall(&rule->cr);
> +}
> +
Again assuming that miss-rules are added by add_internal_flow().
add_internal_flows() adds rules with the mask of register 0 set to
all 1s due to the following line in add_internal_flow().
match_set_reg(&fm.match, 0, id);
So I believe that rule_is_table_miss() will not return true for
miss-rules. I believe that the following will, though perhaps it
is too broad.
return rule->table_id == TBL_INTERNAL;
> /* A set of actions within a "struct rule".
> *
> *
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
