On Wed, May 14, 2014 at 5:39 PM, Simon Horman <ho...@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 <ho...@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 <ho...@verge.net.au>
Applied, thanks. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
