Thanks for the feedback. >> actions= >> clone(truncate(100), push_vlan, resubmit, ...) >> where we don't need to worry about missing the truncate_unset() because >> truncated packet is not visible outside the clone(). > > I see how "clone" helps with this conceptually, but I'm not sure why the > "sample" is necessary. I think that the proposed value here is that > "sample" allows the truncate to be canceled if no output occurs after > "truncate" and before the end of the "sample" action. But it's also > possible for the translation code to see whether there's an output > action within the clone and, if there is none, then to refrain from > emitting the datapath "truncate" action entirely. That seems like a > more efficient way to implement it. Will that work? >
Yes that will work, but we have to add a new datapath clone action. The reason I use "sample" is try not to add another new datapath action to kernel. Current "sample" action happens to provide a list of actions to execute, so I repurpose it to clone. >> We definitely should put some limit on the action types available inside >> clone(). For this patch, there is no restriction. > > Why should we limit the actions available inside clone? > Currently there is a limit of max 10 actions to execute inside sample. "#define DEFERRED_ACTION_FIFO_SIZE 10". So I assume we need to limit the number or bump up this value. Other than this, I'm not sure whether any action can put inside clone, for example some nested case: clone(clone(sample(...))). >> Signed-off-by: William Tu <[email protected]> > > This incremental is needed to avoid putting anything emitted by > xlate_commit_actions() into the middle of the sample action, otherwise > the OVN test cases segfault (after my patches are applied): > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 9bcefcd..c30f93b 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -4296,8 +4296,6 @@ compose_clone_action(struct xlate_ctx *ctx, > { > struct flow old_flow, old_base_flow; > size_t actions_offset; > - size_t sample_offset = nl_msg_start_nested(ctx->odp_actions, > - OVS_ACTION_ATTR_SAMPLE); > > /* Ensure that any prior actions are applied. */ > xlate_commit_actions(ctx); > @@ -4307,6 +4305,8 @@ compose_clone_action(struct xlate_ctx *ctx, > old_base_flow = ctx->base_flow; > > /* Sample with 100% Probability */ > + size_t sample_offset = nl_msg_start_nested(ctx->odp_actions, > + OVS_ACTION_ATTR_SAMPLE); > nl_msg_put_u32(ctx->odp_actions, OVS_SAMPLE_ATTR_PROBABILITY, > UINT32_MAX); > actions_offset = nl_msg_start_nested(ctx->odp_actions, > OVS_SAMPLE_ATTR_ACTIONS); > Thanks for the fix. William _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
