Message-ID: <CAEP_g=-qKYW+VXQ7DDuPbFamKa-wpHUcRdVOTNyutNTdaH-vLw@mail.gmail.com>
Date:	Wed, 30 Apr 2014 15:56:44 -0700
From:	Jesse Gross <jesse@...ira.com>
To:	Simon Horman <horms@...ge.net.au>
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 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.

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.

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.
--
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