> On Mar 20, 2015, at 5:30 PM, Simon Horman <ho...@verge.net.au> wrote:
>
> I'm a little confused about the motivation for this change.
>
> IIRC the idea of having recirculation fields in the xlate_ctx structure was
> to avoid having to pass around extra function parameters, like the ones
> this patch adds. I'm not necessarily opposed to that. But I think it is
> worth adding some context to the discussion.
Ben proposed that part of this patch. I feel that there is a balance between
the “context” and function parameters. In this case the context would be very
short lived and only affect a direct and short function call path, and hence I
think function parameters make more sense.
Jarno
>
> On Thu, Mar 19, 2015 at 06:03:25PM -0700, Jarno Rajahalme wrote:
>> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
>> ---
>> ofproto/ofproto-dpif-xlate.c | 66
>> +++++++++++++++++++++++++-----------------
>> ofproto/ofproto-dpif-xlate.h | 6 ----
>> 2 files changed, 39 insertions(+), 33 deletions(-)
>>
>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>> index 0e28c77..ece9670 100644
>> --- a/ofproto/ofproto-dpif-xlate.c
>> +++ b/ofproto/ofproto-dpif-xlate.c
>> @@ -208,14 +208,13 @@ struct xlate_ctx {
>> uint16_t user_cookie_offset;/* Used for user_action_cookie fixup. */
>> bool exit; /* No further actions should be processed. */
>>
>> - bool use_recirc; /* Should generate recirc? */
>> - struct xlate_recirc recirc; /* Information used for generating
>> - * recirculation actions */
>> -
>> /* True if a packet was but is no longer MPLS (due to an MPLS pop
>> action).
>> * This is a trigger for recirculation in cases where translating an
>> action
>> * or looking up a flow requires access to the fields of the packet after
>> - * the MPLS label stack that was originally present. */
>> + * the MPLS label stack that was originally present.
>> + *
>> + * XXX: output to a table and patch port do not currently recirculate
>> even
>> + * if this is true. */
>> bool was_mpls;
>>
>> /* OpenFlow 1.1+ action set.
>> @@ -353,7 +352,16 @@ static bool input_vid_is_valid(uint16_t vid, struct
>> xbundle *, bool warn);
>> static uint16_t input_vid_to_vlan(const struct xbundle *, uint16_t vid);
>> static void output_normal(struct xlate_ctx *, const struct xbundle *,
>> uint16_t vlan);
>> -static void compose_output_action(struct xlate_ctx *, ofp_port_t ofp_port);
>> +
>> +/* Optional bond recirculation parameter to compose_output_action(). */
>> +struct xlate_bond_recirc {
>> + uint32_t recirc_id; /* !0 Use recirculation instead of output. */
>> + uint8_t hash_alg; /* !0 Compute hash for recirc before. */
>> + uint32_t hash_basis; /* Compute hash for recirc before. */
>> +};
>> +
>> +static void compose_output_action(struct xlate_ctx *, ofp_port_t ofp_port,
>> + const struct xlate_bond_recirc *xr);
>>
>> static struct xbridge *xbridge_lookup(struct xlate_cfg *,
>> const struct ofproto_dpif *);
>> @@ -1670,28 +1678,28 @@ output_normal(struct xlate_ctx *ctx, const struct
>> xbundle *out_xbundle,
>> uint16_t vid;
>> ovs_be16 tci, old_tci;
>> struct xport *xport;
>> + struct xlate_bond_recirc xr;
>> + bool use_recirc = false;
>>
>> vid = output_vlan_to_vid(out_xbundle, vlan);
>> if (list_is_empty(&out_xbundle->xports)) {
>> /* Partially configured bundle with no slaves. Drop the packet. */
>> return;
>> } else if (!out_xbundle->bond) {
>> - ctx->use_recirc = false;
>> xport = CONTAINER_OF(list_front(&out_xbundle->xports), struct xport,
>> bundle_node);
>> } else {
>> struct xlate_cfg *xcfg = ovsrcu_get(struct xlate_cfg *, &xcfgp);
>> struct flow_wildcards *wc = &ctx->xout->wc;
>> - struct xlate_recirc *xr = &ctx->recirc;
>> struct ofport_dpif *ofport;
>>
>> if (ctx->xbridge->enable_recirc) {
>> - ctx->use_recirc = bond_may_recirc(
>> - out_xbundle->bond, &xr->recirc_id, &xr->hash_basis);
>> + use_recirc = bond_may_recirc(
>> + out_xbundle->bond, &xr.recirc_id, &xr.hash_basis);
>>
>> - if (ctx->use_recirc) {
>> + if (use_recirc) {
>> /* Only TCP mode uses recirculation. */
>> - xr->hash_alg = OVS_HASH_ALG_L4;
>> + xr.hash_alg = OVS_HASH_ALG_L4;
>> bond_update_post_recirc_rules(out_xbundle->bond, false);
>>
>> /* Recirculation does not require unmasking hash fields. */
>> @@ -1708,9 +1716,9 @@ output_normal(struct xlate_ctx *ctx, const struct
>> xbundle *out_xbundle,
>> return;
>> }
>>
>> - /* If ctx->xout->use_recirc is set, the main thread will handle
>> stats
>> + /* If use_recirc is set, the main thread will handle stats
>> * accounting for this bond. */
>> - if (!ctx->use_recirc) {
>> + if (!use_recirc) {
>> if (ctx->xin->resubmit_stats) {
>> bond_account(out_xbundle->bond, &ctx->xin->flow, vid,
>> ctx->xin->resubmit_stats->n_bytes);
>> @@ -1738,7 +1746,7 @@ output_normal(struct xlate_ctx *ctx, const struct
>> xbundle *out_xbundle,
>> }
>> *flow_tci = tci;
>>
>> - compose_output_action(ctx, xport->ofp_port);
>> + compose_output_action(ctx, xport->ofp_port, use_recirc ? &xr : NULL);
>> *flow_tci = old_tci;
>> }
>>
>> @@ -2673,7 +2681,7 @@ build_tunnel_send(const struct xlate_ctx *ctx, const
>> struct xport *xport,
>>
>> static void
>> compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port,
>> - bool check_stp)
>> + const struct xlate_bond_recirc *xr, bool check_stp)
>> {
>> const struct xport *xport = get_ofp_port(ctx->xbridge, ofp_port);
>> struct flow_wildcards *wc = &ctx->xout->wc;
>> @@ -2731,6 +2739,7 @@ compose_output_action__(struct xlate_ctx *ctx,
>> ofp_port_t ofp_port,
>> if (xport->peer) {
>> const struct xport *peer = xport->peer;
>> struct flow old_flow = ctx->xin->flow;
>> + bool old_was_mpls = ctx->was_mpls;
>> enum slow_path_reason special;
>> uint8_t table_id =
>> rule_dpif_lookup_get_init_table_id(&ctx->xin->flow);
>> struct ofpbuf old_stack = ctx->stack;
>> @@ -2781,6 +2790,10 @@ compose_output_action__(struct xlate_ctx *ctx,
>> ofp_port_t ofp_port,
>> ofpbuf_uninit(&ctx->stack);
>> ctx->stack = old_stack;
>>
>> + /* The peer bridge popping MPLS should have no effect on the
>> original
>> + * bridge. */
>> + ctx->was_mpls = old_was_mpls;
>> +
>> /* The fact that the peer bridge exits (for any reason) does not mean
>> * that the original bridge should exit. Specifically, if the peer
>> * bridge recirculates (which typically modifies the packet), the
>> @@ -2873,9 +2886,8 @@ compose_output_action__(struct xlate_ctx *ctx,
>> ofp_port_t ofp_port,
>> wc,
>>
>> ctx->xbridge->masked_set_action);
>>
>> - if (ctx->use_recirc) {
>> + if (xr) {
>> struct ovs_action_hash *act_hash;
>> - struct xlate_recirc *xr = &ctx->recirc;
>>
>> /* Hash action. */
>> act_hash = nl_msg_put_unspec_uninit(ctx->xout->odp_actions,
>> @@ -2931,9 +2943,10 @@ compose_output_action__(struct xlate_ctx *ctx,
>> ofp_port_t ofp_port,
>> }
>>
>> static void
>> -compose_output_action(struct xlate_ctx *ctx, ofp_port_t ofp_port)
>> +compose_output_action(struct xlate_ctx *ctx, ofp_port_t ofp_port,
>> + const struct xlate_bond_recirc *xr)
>> {
>> - compose_output_action__(ctx, ofp_port, true);
>> + compose_output_action__(ctx, ofp_port, xr, true);
>> }
>>
>> static void
>> @@ -3223,9 +3236,9 @@ flood_packets(struct xlate_ctx *ctx, bool all)
>> }
>>
>> if (all) {
>> - compose_output_action__(ctx, xport->ofp_port, false);
>> + compose_output_action__(ctx, xport->ofp_port, NULL, false);
>> } else if (!(xport->config & OFPUTIL_PC_NO_FLOOD)) {
>> - compose_output_action(ctx, xport->ofp_port);
>> + compose_output_action(ctx, xport->ofp_port, NULL);
>> }
>> }
>>
>> @@ -3492,7 +3505,7 @@ xlate_output_action(struct xlate_ctx *ctx,
>>
>> switch (port) {
>> case OFPP_IN_PORT:
>> - compose_output_action(ctx, ctx->xin->flow.in_port.ofp_port);
>> + compose_output_action(ctx, ctx->xin->flow.in_port.ofp_port, NULL);
>> break;
>> case OFPP_TABLE:
>> xlate_table_action(ctx, ctx->xin->flow.in_port.ofp_port,
>> @@ -3519,7 +3532,7 @@ xlate_output_action(struct xlate_ctx *ctx,
>> case OFPP_LOCAL:
>> default:
>> if (port != ctx->xin->flow.in_port.ofp_port) {
>> - compose_output_action(ctx, port);
>> + compose_output_action(ctx, port, NULL);
>> } else {
>> xlate_report(ctx, "skipping output to input port");
>> }
>> @@ -3578,7 +3591,7 @@ xlate_enqueue_action(struct xlate_ctx *ctx,
>> /* Add datapath actions. */
>> flow_priority = ctx->xin->flow.skb_priority;
>> ctx->xin->flow.skb_priority = priority;
>> - compose_output_action(ctx, ofp_port);
>> + compose_output_action(ctx, ofp_port, NULL);
>> ctx->xin->flow.skb_priority = flow_priority;
>>
>> /* Update NetFlow output port. */
>> @@ -4494,7 +4507,6 @@ xlate_actions(struct xlate_in *xin, struct xlate_out
>> *xout)
>> ctx.table_id = 0;
>> ctx.rule_cookie = OVS_BE64_MAX;
>> ctx.exit = false;
>> - ctx.use_recirc = false;
>> ctx.was_mpls = false;
>>
>> if (!xin->ofpacts && !ctx.rule) {
>> @@ -4596,7 +4608,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out
>> *xout)
>> if (ctx.xbridge->has_in_band
>> && in_band_must_output_to_local_port(flow)
>> && !actions_output_to_local_port(&ctx)) {
>> - compose_output_action(&ctx, OFPP_LOCAL);
>> + compose_output_action(&ctx, OFPP_LOCAL, NULL);
>> }
>>
>> fix_sflow_action(&ctx);
>> diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
>> index a53fa8e..3e596fb 100644
>> --- a/ofproto/ofproto-dpif-xlate.h
>> +++ b/ofproto/ofproto-dpif-xlate.h
>> @@ -36,12 +36,6 @@ struct mac_learning;
>> struct mcast_snooping;
>> struct xlate_cache;
>>
>> -struct xlate_recirc {
>> - uint32_t recirc_id; /* !0 Use recirculation instead of output. */
>> - uint8_t hash_alg; /* !0 Compute hash for recirc before. */
>> - uint32_t hash_basis; /* Compute hash for recirc before. */
>> -};
>> -
>> struct xlate_out {
>> /* Wildcards relevant in translation. Any fields that were used to
>> * calculate the action must be set for caching and kernel
>> --
>> 1.7.10.4
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
