[ovs-dev] [PATCH 2/4] connmgr: Remove unnecessary reason fixup logic

Simon Horman horms at verge.net.au
Wed Mar 12 09:01:33 UTC 2014


On Wed, Mar 12, 2014 at 05:00:09PM +0900, YAMAMOTO Takashi wrote:
> > On Wed, Mar 12, 2014 at 12:00:12PM +0900, YAMAMOTO Takashi wrote:
> >> > A packet_in message may be sent for one of two reasons.
> >> > 
> >> > 1. As the result of an controller action supplied in a rule.
> >> >    This is executed if a packet matches the match for the rule.
> >> >    The packet_in reason is explicitly part of the action and
> >> >    is thus correct.
> >> 
> >> for OFPACT_CONTROLLER (NX thing) it's correct.
> >> for OFPACT_OUTPUT (OF standard way), it isn't.
> >> 
> >> for the latter, reason=OFPR_ACTION is hardcoded in xlate_output_action.
> >> OF1.3 (1.3.3 5.4.) states reason=table-miss for table-miss flow entry
> >> and thus needs the fixup logic in question.
> > 
> > Thanks, I think understand now.
> > 
> > But I wonder if there are some problems that cause the fixup never to be
> > invoked.
> 
> i'm sure it worked correctly, at least when the patch was merged.
> 
> > 
> > i. For the reasons described in patch 3 of this series
> >    rule_dpif_is_table_miss() always returns false and thus
> >    pin->generated_by_table_miss is always false.
> > 
> >    This means that the if condition in wire_reason() is never true.
> 
> table-miss flows (in the sense of OF1.3) are not OVS internal flows.
> they are ordinary flows with priority=0 and empty match.
> a controller can freely install and remove them.
> 
> > 
> > ii. It seems to me that the fixup should be invoked if
> >     pin->generated_by_table_miss is false as what you describe
> >     immediately above is that the fixup logic is needed
> >     explicit output actions rather than for table misses.
> 
> i guess you are confused by ofproto->miss_rule and OF1.3 table-miss flows.
> they are different things.  generated_by_table_miss is true if the pin
> is caused by OF1.3 table-miss flow.  it isn't for ofproto->miss_rule.

Yes, I was not aware of that.

> 
> YAMAMOTO Takashi
> 
> > 
> > And I believe there are further problems which we reach (like a bonus level
> > of an arcade game) once the above issues are addressed:
> > 
> > Bonus i. The reason is forced to no_match even for
> >          OFPACT_CONTROLLER with reason set to action.
> > 
> > 	 I wonder if this could be resolved by adding a from_output_action
> > 	 field to struct ofproto_packet_in and setting it using a parameter
> > 	 passed to execute_controller_action().
> > 
> > Bonus ii. parse_controller() emits an output action rather than a
> >           controller action if the reason is action and the controller_id
> > 	  is 0. This seems to assume that an output action always
> > 	  has action as its reason. But as we know this is not
> > 	  the case for OpenFlow 1.3+
> > 
> > 	  I propose simplifying parse_controller() to always emit
> > 	  a controller action.
> > 
> > My proposal is as follows:
> > 
> > * Drop this patch
> > * Address the issues described above
> > * Add a test to exercise the fixup (I have one locally)
> > * Rebase patch 4 of this series
> > 
> > 
> > Does that sound reasonable to you or am I still confused about
> > how this is fixup?
> > 
> >> 
> >> YAMAMOTO Takashi
> >> 
> >> > 
> >> > 2. As the result of the failure to match a packet against any rule.
> >> >    In this case a rule present in TBL_INTERNAL is used. This rule
> >> >    has the packet_in reason set to OFPR_NO_MATCH.
> >> > 
> >> >    This is the behaviour described in OpenFlow 1.1 and 1.2 when
> >> >    a miss occurs. And to date it is the behaviour provided
> >> >    by Open vSwtich for all OpenFlow versions.
> >> > 
> >> >    When this behaviour is in effect OFPR_NO_MATCH is the desired
> >> >    packet_in reason and by virtue of the TBL_INTERNAL rule described
> >> >    above is always that value.
> >> > 
> >> > So for both cases the packet_in reason is always correct.
> >> > And following this reasoning there seems to be no need to fix it up.
> >> > 
> >> > What there is a need for, which this patch does not address, is for packets
> >> > to be dropped rather than forwarded to OpenFlow1.3 controllers when a miss
> >> > occurs and an alternate behaviour has not been selected using Table Mod. I
> >> > plan to address this in subsequent patches.
> >> > 
> >> > As well as not been necessary, I believe that the logic that this patch
> >> > removes has not been executed because pin->generated_by_table_miss is never
> >> > true without a separate patch that I have proposed, "ofproto-dpip: Set
> >> > generated_by_table_miss for miss rule".  So I do not expect there to be any
> >> > run-time affect of this change.
> >> > 
> >> > This patch does not remove pin->generated_by_table_miss, although it
> >> > is now set but never used, as I plan to use when implementing the
> >> > OpenFlow1.3 drop-on-miss behaviour described above.
> >> > 
> >> > Cc: YAMAMOTO Takashi <yamamoto at valinux.co.jp>
> >> > Signed-off-by: Simon Horman <horms at verge.net.au>
> >> > ---
> >> >  ofproto/connmgr.c | 32 ++++----------------------------
> >> >  1 file changed, 4 insertions(+), 28 deletions(-)
> >> > 
> >> > diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
> >> > index 033ab7d..954a0f9 100644
> >> > --- a/ofproto/connmgr.c
> >> > +++ b/ofproto/connmgr.c
> >> > @@ -1452,8 +1452,7 @@ ofconn_send(const struct ofconn *ofconn, struct ofpbuf *msg,
> >> >  ^L
> >> >  /* Sending asynchronous messages. */
> >> >  
> >> > -static void schedule_packet_in(struct ofconn *, struct ofproto_packet_in,
> >> > -                               enum ofp_packet_in_reason wire_reason);
> >> > +static void schedule_packet_in(struct ofconn *, struct ofproto_packet_in);
> >> >  
> >> >  /* Sends an OFPT_PORT_STATUS message with 'opp' and 'reason' to appropriate
> >> >   * controllers managed by 'mgr'.  For messages caused by a controller
> >> > @@ -1526,25 +1525,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)
> >> > -{
> >> > -    if (pin->generated_by_table_miss && pin->up.reason == OFPR_ACTION) {
> >> > -        enum ofputil_protocol protocol = ofconn_get_protocol(ofconn);
> >> > -
> >> > -        if (protocol != OFPUTIL_P_NONE
> >> > -            && ofputil_protocol_to_ofp_version(protocol) >= 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.
> >> >   *
> >> > @@ -1556,11 +1536,9 @@ 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_receives_async_msg(ofconn, OAM_PACKET_IN, reason)
> >> > +        if (ofconn_receives_async_msg(ofconn, OAM_PACKET_IN, pin->up.reason)
> >> >              && ofconn->controller_id == pin->controller_id) {
> >> > -            schedule_packet_in(ofconn, *pin, reason);
> >> > +            schedule_packet_in(ofconn, *pin);
> >> >          }
> >> >      }
> >> >  }
> >> > @@ -1586,8 +1564,7 @@ do_send_packet_ins(struct ofconn *ofconn, struct list *txq)
> >> >  /* 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,
> >> > -                   enum ofp_packet_in_reason wire_reason)
> >> > +schedule_packet_in(struct ofconn *ofconn, struct ofproto_packet_in pin)
> >> >  {
> >> >      struct connmgr *mgr = ofconn->connmgr;
> >> >      uint16_t controller_max_len;
> >> > @@ -1595,7 +1572,6 @@ schedule_packet_in(struct ofconn *ofconn, struct ofproto_packet_in pin,
> >> >  
> >> >      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 {
> >> > -- 
> >> > 1.8.5.2
> >> 
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
> 



More information about the dev mailing list