On Wed, Nov 20, 2019 at 11:53 AM Ben Pfaff <[email protected]> wrote: > > This should be less confusing now. > > Reported-by: Han Zhou <[email protected]> > Signed-off-by: Ben Pfaff <[email protected]> > --- > lib/ovs-actions.xml | 31 ++++++++++++++++++++++++------- > ofproto/ofproto-dpif-xlate.c | 8 ++++---- > 2 files changed, 28 insertions(+), 11 deletions(-) > > diff --git a/lib/ovs-actions.xml b/lib/ovs-actions.xml > index e52cd849e37b..ab8e08b84d8b 100644 > --- a/lib/ovs-actions.xml > +++ b/lib/ovs-actions.xml > @@ -933,15 +933,32 @@ for <var>i</var> in [1,<var>n_slaves</var>]: > based on which bucket was selected. An obvious design would be for the > bucket to communicate the value via <code>set_field</code> on a > register. This does not work because registers are part of the > - metadata that <code>group</code> saves and restores. A design that > - would work would be for the bucket to recursively invoke the rest of > - the pipeline with <code>resubmit</code> rather than to attempt to > - return it. Another possibility is for the bucket to use > - <code>push</code> to put the value on the stack for the caller to > - <code>pop</code> off, since <code>group</code> preserves only packet > - data and metadata, not the stack. > + metadata that <code>group</code> saves and restores. The following > + alternative bucket designs do work: > </p> > > + <ul> > + <li> > + Recursively invoke the rest of the pipeline with > + <code>resubmit</code>. > + </li> > + > + <li> > + <p> > + Use <code>resubmit</code> into a table that uses <code>push</code> > + to put the value on the stack for the caller to <code>pop</code> > + off. This works because <code>group</code> preserves only packet > + data and metadata, not the stack. > + </p> > + > + <p> > + (This design requires indirection through <code>resubmit</code> > + because actions sets may not contain <code>push</code> or > + <code>pop</code> actions.) > + </p> > + </li> > + </ul> > + > <p> > An <code>exit</code> action within a group bucket terminates only > execution of that bucket, not other buckets or the overall pipeline. > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index f92cb62c80ce..cebae7a5b2b3 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -4468,11 +4468,11 @@ xlate_group_bucket(struct xlate_ctx *ctx, struct ofputil_bucket *bucket, > * > * Note that group buckets are action sets, hence they cannot modify the > * main action set. Also any stack actions are ignored when executing an > - * action set, so group buckets cannot change the stack either. > + * action set, so group buckets cannot directly change the stack either. > * However, we do allow resubmit actions in group buckets, which could > - * break the above assumptions. It is up to the controller to not mess up > - * with the action_set and stack in the tables resubmitted to from > - * group buckets. */ > + * recursively execute actions that do modify the action set or change the > + * stack. The controller must be careful about what it does to the > + * action_set and stack in the tables resubmitted to from group buckets. */ > ctx->xin->flow = old_flow; > > /* The group bucket popping MPLS should have no effect after bucket > -- > 2.21.0 >
Thanks Ben. Acked-by: Han Zhou <[email protected]> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
