[ovs-dev] [PATCH] dpif: execute ct together with recirc when slow path

[Wei Li]

Great idea, this can process actions like 
"ct(...nat),recirc(...),output(...)" correctly, output packet will be a 
NATed packet

Before implementing your idea, how deal with my patch?

1: drop it

2:accept it for fixing limited scenario (i will do checkpatch)

What's your suggestion?

在 2017/6/15 2:43, Joe Stringer 写道:
> On 14 June 2017 at 05:04, wei <liw at dtdream.com> wrote:
>> ct(table=X,zone=X) will xlate 2 actions (ct and recirc), so they should be executed together.
> Hi, thanks for the contribution.
>
> There's a few things in this patch that do not match standard OVS
> contribution guidelines. Using utilities/checkpatch.py, you can pick
> up on most of these and address them before submission. Would you mind
> fixing these up? The following website goes into more detail about
> contributing to OVS:
>
> http://docs.openvswitch.org/en/latest/internals/contributing/submitting-patches/
>
> At minimum, all patches need to provide signoff before they can be
> applied to OVS. The use of whitespace in this patch also does not
> match the usual OVS style.
>
> Regarding the patch details, it seems to follow a similar workaround
> used for meters in dpif_execute_helper_cb(). I think that this will
> likely work, as the only copy of the packet that requires the results
> of ct() is the copy that is recirculated. That is to say, if the
> datapath actions looked like "...,ct(...),recirc(...),output(...)", it
> would be correct to assemble two execute calls to the datapath, one
> for the ct+recirc, and one for the output.
>
> That said, at some point we should consider addressing the core
> problem which is that dpif_execute_helper_cb() is written to try to
> execute one action at a time. I don't expect you to implement this,
> but I want to jot these thoughts down while they're fresh in my mind.
> Rather than executing one action at a time and building up specific
> solutions to group ct+recirc or meter+subsequent actions, we should
> probably attempt to group together as many actions as can be executed
> in the datapath and execute in one transaction. What I could imagine
> is something like the current meters loop logic existing in a generic
> way in odp_execute_actions(), taking into account which actions *must*
> be executed in userspace and which actions *must* be executed in the
> datapath.
>
> Let's say for example that actions of type A must be executed in the
> datapath, actions type B could be executed anywhere, and actions type
> C must be executed in userspace. A could represent meter, ct, recirc,
> tunnel_{pop,push}, userspace, or output. B includes the majority of
> actions. C could include ARP modification and perhaps a short list of
> other actions. I'll use "U" to indicate a userspace action that we
> insert to make things work correctly (more description below).
>
> Actions list "BBBABBC" could be executed "BBB" in userspace, then
> assemble a datapath execute for "A", then run "BBC" in userspace again
> (so long as 'A' does not impact running of BBC). However, consider if
> "A" is actually a meter action. Then it could potentially drop the
> packet, which means that "BBC" should not be run. Therefore, perhaps
> to execute this list, "BBB" should be executed in userspace, then
> assemble a datapath execute for "ABBU", where "U" action should
> contain enough context that the executed packet can come back up and
> finally execute the "C" action later on.
>
> With an actions list like "CBBBABBABA", I would expect that "CBBB"
> could be executed in userspace, then "ABBABA" could be executed as one
> list in kernel. Equally, "C" could be executed in userspace and
> "BBBABBABA" could be executed in kernel.
>
> Now, maybe in practice these situations are not hit that often so
> there is little impact on correctness or performance. In that case
> maybe it's not that important. However there is some potential that if
> some of these cases are hit regularly, a more general solution could
> improve correctness and performance and even tidy up the code while
> we're at it.
>
> Cheers,
> Joe