Acked-by: Ethan Jackson <et...@nicira.com>
On Mon, Feb 11, 2013 at 2:34 PM, Ben Pfaff <b...@nicira.com> wrote:
> I think you're right.
>
> I changed
> in_port = get_ofp_port(ctx->ofproto, ctx->flow.in_port);
> if (!in_port || stp_forward_in_state(in_port->stp_state)) {
> xlate_table_action(ctx, ctx->flow.in_port, 0, true);
> } else {
> /* Forwarding is disabled by STP. Let OFPP_NORMAL and the
> learning
> * action look at the packet, then drop it. */
> struct flow old_base_flow = ctx->base_flow;
> size_t old_size = ctx->odp_actions->size;
> xlate_table_action(ctx, ctx->flow.in_port, 0, true);
> ctx->base_flow = old_base_flow;
> ctx->odp_actions->size = old_size;
> }
> to
> in_port = get_ofp_port(ctx->ofproto, ctx->flow.in_port);
> if (!in_port || may_receive(in_port, ctx)) {
> if (!in_port || stp_forward_in_state(in_port->stp_state)) {
> xlate_table_action(ctx, ctx->flow.in_port, 0, true);
> } else {
> /* Forwarding is disabled by STP. Let OFPP_NORMAL and the
> * learning action look at the packet, then drop it. */
> struct flow old_base_flow = ctx->base_flow;
> size_t old_size = ctx->odp_actions->size;
> xlate_table_action(ctx, ctx->flow.in_port, 0, true);
> ctx->base_flow = old_base_flow;
> ctx->odp_actions->size = old_size;
> }
> }
> in compose_output_action__(). I think this takes care of it. I'm
> appending a full revised patch. How does it look?
>
> On Fri, Feb 08, 2013 at 01:29:27PM -0800, Ethan Jackson wrote:
>> The patch pretty much looks good to me. I still don't think we have
>> the patch port code exactly correct though. Suppose we're neither in
>> the forwarding state, nor the learning state. It looks to me like
>> we'll still run through the learning code when sending through the
>> patch port, though we don't do that for a normal port. I think what
>> we really want is something more akin to what we do in xlate_actions
>> where we call may_receive() directly.
>>
>>
>> Ethan
>>
>>
>>
>> On Thu, Feb 7, 2013 at 11:04 AM, Ben Pfaff <b...@nicira.com> wrote:
>> > Until now the flow translation code has done one get_ofp_port() call
>> > initially to check for special processing, then one for each level of
>> > action processing. Only one call is actually necessary, though, because
>> > the in_port of a flow doesn't change in ordinary circumstances, and so this
>> > commit eliminates the unnecessary calls.
>> >
>> > The one case where the in_port can change is when a packet passes through
>> > a patch port. The implementation here was buggy anyway: when the patch
>> > port's peer had forwarding disabled by STP, then the code would drop all
>> > ODP actions, even those that were executed before the packet crossed the
>> > patch port. This commit fixes that case.
>> >
>> > With a complicated flow table involving multiple levels of resubmit, this
>> > increases flow setup performance by 2-3%.
>> >
>> > Signed-off-by: Ben Pfaff <b...@nicira.com>
>
> --8<--------------------------cut here-------------------------->8--
>
> From: Ben Pfaff <b...@nicira.com>
> Date: Mon, 11 Feb 2013 14:32:52 -0800
> Subject: [PATCH] ofproto-dpif: Reduce number of get_ofp_port() calls during
> flow xlate.
>
> Until now the flow translation code has done one get_ofp_port() call
> initially to check for special processing, then one for each level of
> action processing. Only one call is actually necessary, though, because
> the in_port of a flow doesn't change in ordinary circumstances, and so this
> commit eliminates the unnecessary calls.
>
> The one case where the in_port can change is when a packet passes through
> a patch port. The implementation here was buggy anyway: when the patch
> port's peer had forwarding disabled by STP, then the code would drop all
> ODP actions, even those that were executed before the packet crossed the
> patch port. This commit fixes that case.
>
> With a complicated flow table involving multiple levels of resubmit, this
> increases flow setup performance by 2-3%.
>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> ofproto/ofproto-dpif.c | 59 +++++++++++++++++++++++++++++------------------
> 1 files changed, 36 insertions(+), 23 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 109e57c..f1d88f1 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -3311,15 +3311,11 @@ send_packet_in_miss(struct ofproto_dpif *ofproto,
> const struct ofpbuf *packet,
>
> static enum slow_path_reason
> process_special(struct ofproto_dpif *ofproto, const struct flow *flow,
> - const struct ofpbuf *packet)
> + const struct ofport_dpif *ofport, const struct ofpbuf
> *packet)
> {
> - struct ofport_dpif *ofport = get_ofp_port(ofproto, flow->in_port);
> -
> if (!ofport) {
> return 0;
> - }
> -
> - if (ofport->cfm && cfm_should_process_flow(ofport->cfm, flow)) {
> + } else if (ofport->cfm && cfm_should_process_flow(ofport->cfm, flow)) {
> if (packet) {
> cfm_process_heartbeat(ofport->cfm, packet);
> }
> @@ -3335,8 +3331,9 @@ process_special(struct ofproto_dpif *ofproto, const
> struct flow *flow,
> stp_process_packet(ofport, packet);
> }
> return SLOW_STP;
> + } else {
> + return 0;
> }
> - return 0;
> }
>
> static struct flow_miss *
> @@ -5546,6 +5543,7 @@ send_packet(const struct ofport_dpif *ofport, struct
> ofpbuf *packet)
>
> /* OpenFlow to datapath action translation. */
>
> +static bool may_receive(const struct ofport_dpif *, struct action_xlate_ctx
> *);
> static void do_xlate_actions(const struct ofpact *, size_t ofpacts_len,
> struct action_xlate_ctx *);
> static void xlate_normal(struct action_xlate_ctx *);
> @@ -5726,6 +5724,7 @@ compose_output_action__(struct action_xlate_ctx *ctx,
> uint16_t ofp_port,
> struct ofport_dpif *peer = ofport_get_peer(ofport);
> struct flow old_flow = ctx->flow;
> const struct ofproto_dpif *peer_ofproto;
> + struct ofport_dpif *in_port;
>
> if (!peer) {
> xlate_report(ctx, "Nonexistent patch port peer");
> @@ -5743,7 +5742,22 @@ compose_output_action__(struct action_xlate_ctx *ctx,
> uint16_t ofp_port,
> ctx->flow.metadata = htonll(0);
> memset(&ctx->flow.tunnel, 0, sizeof ctx->flow.tunnel);
> memset(ctx->flow.regs, 0, sizeof ctx->flow.regs);
> - xlate_table_action(ctx, ctx->flow.in_port, 0, true);
> +
> + in_port = get_ofp_port(ctx->ofproto, ctx->flow.in_port);
> + if (!in_port || may_receive(in_port, ctx)) {
> + if (!in_port || stp_forward_in_state(in_port->stp_state)) {
> + xlate_table_action(ctx, ctx->flow.in_port, 0, true);
> + } else {
> + /* Forwarding is disabled by STP. Let OFPP_NORMAL and the
> + * learning action look at the packet, then drop it. */
> + struct flow old_base_flow = ctx->base_flow;
> + size_t old_size = ctx->odp_actions->size;
> + xlate_table_action(ctx, ctx->flow.in_port, 0, true);
> + ctx->base_flow = old_base_flow;
> + ctx->odp_actions->size = old_size;
> + }
> + }
> +
> ctx->flow = old_flow;
> ctx->ofproto = ofproto_dpif_cast(ofport->up.ofproto);
>
> @@ -6273,16 +6287,9 @@ static void
> do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
> struct action_xlate_ctx *ctx)
> {
> - const struct ofport_dpif *port;
> bool was_evictable = true;
> const struct ofpact *a;
>
> - port = get_ofp_port(ctx->ofproto, ctx->flow.in_port);
> - if (port && !may_receive(port, ctx)) {
> - /* Drop this flow. */
> - return;
> - }
> -
> if (ctx->rule) {
> /* Don't let the rule we're working on get evicted underneath us. */
> was_evictable = ctx->rule->up.evictable;
> @@ -6466,12 +6473,6 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
> ofpacts_len,
> }
>
> out:
> - /* We've let OFPP_NORMAL and the learning action look at the packet,
> - * so drop it now if forwarding is disabled. */
> - if (port && !stp_forward_in_state(port->stp_state)) {
> - ofpbuf_clear(ctx->odp_actions);
> - add_sflow_action(ctx);
> - }
> if (ctx->rule) {
> ctx->rule->up.evictable = was_evictable;
> }
> @@ -6534,6 +6535,7 @@ xlate_actions(struct action_xlate_ctx *ctx,
> static bool hit_resubmit_limit;
>
> enum slow_path_reason special;
> + struct ofport_dpif *in_port;
>
> COVERAGE_INC(ofproto_dpif_xlate);
>
> @@ -6587,7 +6589,8 @@ xlate_actions(struct action_xlate_ctx *ctx,
> }
> }
>
> - special = process_special(ctx->ofproto, &ctx->flow, ctx->packet);
> + in_port = get_ofp_port(ctx->ofproto, ctx->flow.in_port);
> + special = process_special(ctx->ofproto, &ctx->flow, in_port,
> ctx->packet);
> if (special) {
> ctx->slow |= special;
> } else {
> @@ -6596,7 +6599,17 @@ xlate_actions(struct action_xlate_ctx *ctx,
> uint32_t local_odp_port;
>
> add_sflow_action(ctx);
> - do_xlate_actions(ofpacts, ofpacts_len, ctx);
> +
> + if (!in_port || may_receive(in_port, ctx)) {
> + do_xlate_actions(ofpacts, ofpacts_len, ctx);
> +
> + /* We've let OFPP_NORMAL and the learning action look at the
> + * packet, so drop it now if forwarding is disabled. */
> + if (in_port && !stp_forward_in_state(in_port->stp_state)) {
> + ofpbuf_clear(ctx->odp_actions);
> + add_sflow_action(ctx);
> + }
> + }
>
> if (ctx->max_resubmit_trigger && !ctx->resubmit_hook) {
> if (!hit_resubmit_limit) {
> --
> 1.7.2.5
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
