[ovs-dev] [PATCH v3] datapath: sample action without side effects

Jesse Gross jesse at nicira.com
Thu May 15 22:43:11 UTC 2014


On Wed, May 14, 2014 at 5:39 PM, Simon Horman <horms at verge.net.au> wrote:
> On Wed, May 14, 2014 at 05:22:21PM -0700, Jesse Gross wrote:
>> On Wed, May 14, 2014 at 5:05 PM, Simon Horman <horms at verge.net.au> wrote:
>> > The sample action is rather generic, allowing arbitrary actions to be
>> > executed based on a probability. However its use, within the Open vSwitch
>> > code-base is limited: only a single user-space action is ever nested.
>> >
>> > A consequence of the current implementation of sample actions is that
>> > depending on weather the sample action executed (due to its probability)
>> > any side-effects of nested actions may or may not be present before
>> > executing subsequent actions.  This has the potential to complicate
>> > verification of valid actions by the (kernel) datapath. And indeed adding
>> > support for push and pop MPLS actions inside sample actions is one case
>> > where such case.
>> >
>> > In order to allow all supported actions to be continue to be nested inside
>> > sample actions without the potential need for complex verification code
>> > this patch changes the implementation of the sample action in the kernel
>> > datapath so that sample actions are more like a function call and any side
>> > effects of nested actions are not present when executing subsequent
>> > actions.
>> >
>> > With the above in mind the motivation for this change is twofold:
>> >
>> > * To contain side-effects the sample action in the hope of making it
>> >   easier to deal with in the future and;
>> > * To avoid some rather complex verification code introduced in the MPLS
>> >   datapath patch.
>> >
>> > Some notes about the implementation:
>> >
>> > * This patch silently changes the behaviour of sample actions whose nested
>> >   actions have side-effects. There are no known users of such sample
>> >   actions.
>> >
>> > * sample() does not clone the skb for the only known use-case of the sample
>> >   action: a single nested userspace action. In such a case a clone is not
>> >   needed as the userspace action has no side effects.
>> >
>> >   Given that there are no known users of other nested actions and in order
>> >   to avoid the complexity of predicting if other sequences of actions have
>> >   side-effects in such cases the skb is cloned.
>> >
>> > * As sample() provides a cloned skb in the unlikely case where there are
>> >   nested actions other than a single userspace action it is no longer
>> >   necessary to clone the skb in do_execute_actions() when executing a
>> >   recirculation action just because the keep_skb parameter is set: this
>> >   parameter was only set when processing the nested actions of a sample
>> >   action.  Moreover it is possible to remove the keep_skb parameter of
>> >   do_execute_actions entirely.
>> >
>> > * As sample() provides either a cloned skb or one that has had a
>> >   reference taken (using keep_skb) to do_execute_actions()
>> >   the original skb passed to sample() is never consumed. Thus the
>> >   caller of sample() (also do_execute_actions()) can use its generic
>> >   error handling to free the skb on error.
>>
>> This looks good to me but can you provide a signed-off-by line?
>
> Sure, sorry for missing that.
> Please let me know if you would like me to repost.
>
> Signed-off-by: Simon Horman <horms at verge.net.au>

Applied, thanks.



More information about the dev mailing list