On Thu, Sep 05, 2013 at 04:12:39PM -0700, Jarno Rajahalme wrote:
>
> 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?
Thanks. I somehow missed that this is not necessary if a group action
occurs in apply actions - I had assumed buckets always used the set behaviour,
regardless of if processing was taking place in apply actions or set
actions.
I think that your suggestion of a flag in ctx should work, I will see
about making it so.
> > + 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?
To be honest I am entirely unclear on what behaviour is desired by the spec.
As the implementation stands the behaviour is the same for both
apply actions and write actions.
>
> > + 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.
Yes, my reading of the spec is that this should be ok.
But it is not clear to me how to detect when there will be no further
output. Perhaps if it is the last bucket of the group and there
are no more actions or instructions in the rule?
>
> > + }
> > +}
> > +
> > +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.
I am not sure either.
> Also, since (at least the
> directly) dependent flows should be removed when the group is deleted, this
> should not happen?
I believe that is a "should" rather than a "must" portion of the spec. And
I believe that currently Open vSwitch's implementation is more on the
"should" side than the "must" side. But none the less I think it would be
reasonable to just stop processing, as occurs on output to an invalid port,
rather than send a packet_in.
Shall I remove the use of xlate_miss() here?
>
> > + }
> > + } 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
