Re: [ovs-dev] [PATCH 2/4] connmgr: Remove unnecessary reason fixup logic
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 > >> > Signed-off-by: Simon Horman > >> > --- > >> > ofproto/connmgr.c | 32
Re: [ovs-dev] [PATCH 2/4] connmgr: Remove unnecessary reason fixup logic
> 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. 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 >> > Signed-off-by: Simon Horman >> > --- >> > 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 asynchronou
Re: [ovs-dev] [PATCH 2/4] connmgr: Remove unnecessary reason fixup logic
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. 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. 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. 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 > > Signed-off-by: Simon Horman > > --- > > 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
Re: [ovs-dev] [PATCH 2/4] connmgr: Remove unnecessary reason fixup logic
> 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.
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
> Signed-off-by: Simon Horman
> ---
> 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 o
