Date:	Thu, 1 May 2014 17:54:42 +0900
From:	Simon Horman <horms@...ge.net.au>
To:	Jesse Gross <jesse@...ira.com>
Cc:	YAMAMOTO Takashi <yamamoto@...inux.co.jp>,
	"dev@...nvswitch.org" <dev@...nvswitch.org>,
	netdev <netdev@...r.kernel.org>,
	Pravin Shelar <pshelar@...ira.com>,
	ravi kerur <rkerur@...il.com>
Subject: Re: [ovs-dev] [PATCH v2.56] datapath: Add basic MPLS support to
 kernel

On Wed, Apr 30, 2014 at 03:56:44PM -0700, Jesse Gross wrote:
> On Tue, Apr 29, 2014 at 10:58 PM, Simon Horman <horms@...ge.net.au> wrote:
> > On Tue, Apr 29, 2014 at 11:41:57AM -0700, Jesse Gross wrote:
> >> On Mon, Apr 28, 2014 at 5:13 PM, Simon Horman <horms@...ge.net.au> wrote:
> >> > On Mon, Apr 28, 2014 at 02:37:47PM -0700, Jesse Gross wrote:
> >> >> On Mon, Apr 28, 2014 at 12:00 AM, Simon Horman <horms@...ge.net.au> wrote:
> >> >> > On Fri, Apr 25, 2014 at 12:57:20PM -0700, Jesse Gross wrote:
> >> >> >> On Fri, Apr 25, 2014 at 1:06 AM, YAMAMOTO Takashi
> >> >> >> <yamamoto@...inux.co.jp> wrote:
> >> >> >> >> On Thu, Apr 24, 2014 at 05:57:29PM +0900, YAMAMOTO Takashi wrote:
> >> >> >> >>> hi,
> >> >> >> >>>
> >> >> >> >>> > + * 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.
> >
> >> > 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@...ge.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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists