On Sep 4, 2013, at 10:19 PM, Simon Horman <ho...@verge.net.au> wrote:
> Allow translation of indirect and all groups.
> Also allow insertion of indirect and all groups by
> changing the maximum permitted number in the groups
> table from 0 to OFPG_MAX.
>
> Implementation note:
>
> After translating the actions for each bucket
> ctx->base_flow is reset to its state prior to translation
> of the buckets actions. This is equivalent to cloning the
> bucket before applying actions. This is my interpretation
> of the OpenFlow 1.3.2 specification section 5.6.1 Group Types,
> which includes the following text. I believe there is room
> for other interpretations.
>
> * On all groups: "The packet is effectively cloned for each bucket; one
> packet is processed for each bucket of the group."
> * On indirect groups: "This group type is effectively identical to an
> all group with one bucket."
>
> Signed-off-by: Simon Horman <ho...@verge.net.au>
>
> ---
> v3
> * Rebase for "ofproto-dpif: Hide struct rule_dpif internally"
> * Update for hidden group_dpif
> * Corrected typo in all groups test
>
> v2
> * First post
> ---
> ofproto/ofproto-dpif-xlate.c | 116 +++++++++++++++++++++++++++++++++++++------
> ofproto/ofproto-dpif.c | 36 ++++++++++++++
> ofproto/ofproto-dpif.h | 11 ++++
> ofproto/ofproto.c | 2 +
> tests/ofproto-dpif.at | 24 +++++++++
> 5 files changed, 175 insertions(+), 14 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 359aa10..9704ecd 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -1693,6 +1693,24 @@ xlate_recursively(struct xlate_ctx *ctx, struct
> rule_dpif *rule)
> }
>
> static void
> +xlate_miss(struct xlate_ctx *ctx)
> +{
> + struct xport *xport;
> + struct rule_dpif *rule;
> +
> + /* XXX
> + * check if table configuration flags
> + * OFPTC_TABLE_MISS_CONTROLLER, default.
> + * OFPTC_TABLE_MISS_CONTINUE,
> + * OFPTC_TABLE_MISS_DROP
> + * When OF1.0, OFPTC_TABLE_MISS_CONTINUE is used. What to do? */
> + xport = get_ofp_port(ctx->xbridge, ctx->xin->flow.in_port.ofp_port);
> + choose_miss_rule(xport ? xport->config : 0, ctx->xbridge->miss_rule,
> + ctx->xbridge->no_packet_in_rule, &rule);
> + xlate_recursively(ctx, rule);
> +}
> +
> +static void
> xlate_table_action(struct xlate_ctx *ctx,
> ofp_port_t in_port, uint8_t table_id, bool may_packet_in)
> {
> @@ -1720,19 +1738,7 @@ xlate_table_action(struct xlate_ctx *ctx,
> if (got_rule) {
> xlate_recursively(ctx, rule);
> } else if (may_packet_in) {
> - struct xport *xport;
> -
> - /* XXX
> - * check if table configuration flags
> - * OFPTC_TABLE_MISS_CONTROLLER, default.
> - * OFPTC_TABLE_MISS_CONTINUE,
> - * OFPTC_TABLE_MISS_DROP
> - * When OF1.0, OFPTC_TABLE_MISS_CONTINUE is used. What to do? */
> - xport = get_ofp_port(ctx->xbridge,
> ctx->xin->flow.in_port.ofp_port);
> - choose_miss_rule(xport ? xport->config : 0,
> - ctx->xbridge->miss_rule,
> - ctx->xbridge->no_packet_in_rule, &rule);
> - xlate_recursively(ctx, rule);
> + xlate_miss(ctx);
> }
>
> ctx->table_id = old_table_id;
> @@ -1745,6 +1751,88 @@ xlate_table_action(struct xlate_ctx *ctx,
> }
>
> static void
> +xlate_group_bucket(struct xlate_ctx *ctx, struct ofputil_bucket *bucket)
> +{
> + struct ofpbuf actions_out, actions_in_buf;
> + struct list actions_in = LIST_INITIALIZER(&actions_in);
> +
> + ofpbuf_use_const(&actions_in_buf, bucket->ofpacts, bucket->ofpacts_len);
> + list_init(&actions_in_buf.list_node);
> + list_insert(&actions_in, &actions_in_buf.list_node);
> +
> + ofpbuf_init(&actions_out, actions_in_buf.size);
> +
> + ofpacts_list_to_action_set(&actions_out, &actions_in);
The code above seems to assume the group/bucket is processed as
part of the action set. However, if the group action is given in ApplyActions
instead, the order of the bucket actions should be retained, and most of the
above is unnecessary.
It seems the ctx needs a flag to signify whether the flow table processing is
still going on, or are we now at the action set processing phase, so that a
proper processing could be applied here?
> + ctx->recurse++;
> + do_xlate_actions(actions_out.data, actions_out.size, ctx);
> + ctx->recurse--;
> +
> + ofpbuf_uninit(&actions_out);
> + ofpbuf_uninit(&actions_in_buf);
> +}
> +
> +static void
> +xlate_all_group(struct xlate_ctx *ctx, struct group_dpif *group)
> + OVS_REQ_RDLOCK(&group->up.rwlock)
> +{
> + struct ofputil_bucket *bucket;
> + struct list *buckets;
> + struct flow old_base_flow = ctx->base_flow;
> +
> + group_dpif_get_buckets(group, &buckets);
> +
> + LIST_FOR_EACH(bucket, list_node, buckets) {
> + xlate_group_bucket(ctx, bucket);
> + /* Roll back base_flow to previous state.
> + * This is equivalent to cloning the packet for each bucket.
> + *
> + * As a side effect any subsequently applied actions will
> + * also effectively be applied to a clone of the packet taken
> + * just before applying the all or indirect group. */
Will this work the same in both ApplyActions and WriteActions case?
> + ctx->base_flow = old_base_flow;
Could this flow copy be avoided, if there is no further output? E.g.,
just a single indirect group action.
> + }
> +}
> +
> +static void
> +xlate_group_action__(struct xlate_ctx *ctx, struct group_dpif *group)
> + OVS_RELEASES(&group->up.rwlock)
> +{
> + switch (group_dpif_get_type(group)) {
> + case OFPGT11_ALL:
> + case OFPGT11_INDIRECT:
> + xlate_all_group(ctx, group);
> + break;
> + case OFPGT11_SELECT:
> + case OFPGT11_FF:
> + default:
> + /* XXX not yet implemented */
> + break;
> + }
> + group_dpif_release(group);
> +}
> +
> +static void
> +xlate_group_action(struct xlate_ctx *ctx, uint32_t group_id)
> +{
> + if (ctx->recurse < MAX_RESUBMIT_RECURSION) {
> + struct group_dpif *group;
> + bool got_group;
> +
> + got_group = group_dpif_lookup(ctx->xbridge->ofproto, group_id,
> &group);
> + if (got_group) {
> + xlate_group_action__(ctx, group);
> + } else {
> + xlate_miss(ctx);
There is not packet in reason code for a non-existing group, so I do not know if
it proper to process group miss as a flow table miss. Also, since (at least the
directly) dependent flows should be removed when the group is deleted, this
should not happen?
> + }
> + } else {
> + static struct vlog_rate_limit recurse_rl = VLOG_RATE_LIMIT_INIT(1,
> 1);
> +
> + VLOG_ERR_RL(&recurse_rl, "resubmit actions recursed over %d times",
> + MAX_RESUBMIT_RECURSION);
> + }
> +}
> +
>
Jarno
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
