On Wed, Apr 30, 2014 at 03:56:44PM -0700, Jesse Gross wrote:

On Tue, Apr 29, 2014 at 10:58 PM, Simon Horman [off-list ref] wrote:

quoted

On Tue, Apr 29, 2014 at 11:41:57AM -0700, Jesse Gross wrote:

quoted

On Mon, Apr 28, 2014 at 5:13 PM, Simon Horman [off-list ref] wrote:

quoted

On Mon, Apr 28, 2014 at 02:37:47PM -0700, Jesse Gross wrote:

quoted

On Mon, Apr 28, 2014 at 12:00 AM, Simon Horman [off-list ref] wrote:

quoted

On Fri, Apr 25, 2014 at 12:57:20PM -0700, Jesse Gross wrote:

quoted

On Fri, Apr 25, 2014 at 1:06 AM, YAMAMOTO Takashi
[off-list ref] wrote:

quoted

quoted

On Thu, Apr 24, 2014 at 05:57:29PM +0900, YAMAMOTO Takashi wrote:

quoted

hi,

quoted

+ * Due to the sample action there may be multiple possible eth types.
+ * In order to correctly validate actions all possible types are tracked
+ * and verified. This is done using struct eth_types.

is there any real-world use cases of these actions inside a sample?
otherwise, how about just rejecting such combinations?
it doesn't seem to worth the code complexity to me.
(sorry if it has been already discussed.  it's the first time for me
to seriously read this long-lived patch.)

Good point, the code is rather complex.

My understanding is that it comes into effect in the case
of sflow or ipfix being configured on the bridge. I tend
to think these are real-world use-cases, though that thinking
is by no means fixed.

My reading of the code is that in the case of sflow and ipfix a single
sample rule appears at the beginning of the flow. And that it may be
possible to replace the code that you are referring to with something
simpler to handle these cases.

it seems that they put only a userland action inside a sample.
it's what i expected from its name "sample".

Yes, that's the only current use case. In theory, this could be used
more generally although nothing has come up yet.

In retrospect, I regret the design of the sample action - not the part
that allows it to handle different actions but the fact that the
results can affect things outside of the sample action list. I think
that if we had made it more like a subroutine then that would have
retained all of the functionality but none of the complexity here.
Perhaps if we can find a clean way to restructure it without breaking
compatibility then that would simplify the validation here.

I have not thought deeply about this but it seems to me that it should be
easy enough to provide compatibility for a new user-space to work with both
new and old datapaths.  But it is not clear to me how to achieve the
reverse: allowing a new datapath to work with both new and old user-spaces.
I assume that we care about such compatibility.

Generally, I would say yes although there is potentially some room for
debate here. No version of OVS userspace has ever put an action other
than userspace in the sample field. I know that other people have
talked about writing different userspaces that run on the OVS kernel
module but I highly doubt that they use this action or would do so
differently. I can't prove that but it might be OK to bite the bullet.

I am also concerned about the sample() action which is exposed via OpenFlow
(as a Nicira extension) and in turn ovs-ofctl.  Thus it seems to me that
there could be users adding flows with sample actions whose behaviour would
either no longer be supported or would be changed.  But I believe that we
should reason about this case the same way that you reason about alternate
user-spaces above.

The sample action exposed through OpenFlow is a little different. It
allows you to specify where in the action list to do sampling but it
doesn't allow arbitrary actions to be embedded. As a result, it still
always embeds a userspace action, which should be safe because it is
idempotent.

Thanks, that nicely removes my concern.

quoted

quoted

Perhaps a way forward would be (for me) to come up with a prototype to
alter the sample action. And we can see how clean it is (or could be) and
what it buys us.

It seems that the motivation for this change is is twofold: To contain 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. And I think it would be good to keep that in mind when
assessing any prototype code.

That sounds reasonable to me.

I have come up with the following.

In terms of gains, using this approach I have reduced the size of MPLS
datapath patch by about 170 lines by replacing the complex logic that
Yamamoto-san questioned with a simple verification of the current ethernet
type (which may be changed by MPLS action).


[PATCH/RFC] Sample action without side effects

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:

* The OVS_SAMPLE_ATTR_FLAGS portion of this patch, which contributes to a
  good portion of the size of the patch is intended to prevent any users
  that were relying on side effects from sample actions from being silently
  broken.

  Any rule that includes a sample action with nested actions that
  may have side effects (e.g. set, push/pop VLAN) will be rejected
  unless OVS_SAMPLE_ATTR_FLAGS is present and its
  OVS_SAMPLE_FLAG_SIDE_EFFECTS bit is set.

  Thus users that were relying on side effects will experience rules
  being rejected by the datapath. Rather than being silently handled
  a different way.

  It seems to me that it is rather unlikely that such users exist.
  And thus it seems reasonable to me to remove the OVS_SAMPLE_ATTR_FLAGS
  portion of this patch.

* It is my understanding that the only user of the user-space datapath is
  ovs-vswitchd.

  As ovs-vswitchd never adds rules to the datapath that have sample actions
  with nested actions with side effects I have not updated the user-space
  datapath other than to call OVS_NOT_REACHED() if a sample action includes
  the new OVS_SAMPLE_ATTR_FLAGS attribute. Primarily to make the
  compiler happy about all values of the ovs_sample_attr enumeration
  being handled by the case statement where that code resides.

  My reasoning is follows:

  + If the user-space datapath executes a sample action with nested
    actions with side effects then it will be done so in the old way.
    That is differently to the kernel datapath. But this will never happen
    because ovs-vswitchd never creates such an action.

  + If the OVS_SAMPLE_ATTR_FLAGS attribute is present when executing a
    sample action in the user-space datapath then ovs-vswtichd will abort. But
    again this will never happen because ovs-vswitchd never creates such
    an action.

Signed-off-by: Simon Horman <horms@verge.net.au>

I'm mostly OK with this, although I think that the side effects
checking is probably not really worth the extra complexity given the
probably that it will ever get hit. Without that the patch becomes
quite short.

Do mean the OVS_SAMPLE_ATTR_FLAGS portion as well as the side effects
checking in the datapath action validation code?

I'm happy to remove all of that if you are happy with that approach.
I agree it would make the patch nice and short.

The other thing that comes to mind is if there is a way to better
avoid cloning the skb. With recirculation, the action generally comes
as the last in the list and so the last_action check is quite
effective. However, with sampling it's always at the beginning and
never actually makes any changes to the packet so it's not really
useful work.

I will give some thought to that.

One, not very well developed, idea I have is to clone the skb
action during execution of the sample action if a nested action may
cause side effects. I'm entirely unsure how cleanly this could
be implemented.

Finally, I wonder if there is a way to merge the logic for keep_skb
and the checks for recirculation/sampling. It's gotten somewhat
complicated because there are all somewhat linked but different.

I agree that would be nice. I think the exact details will
depend on how skb clone avoidance is handled.