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

Joe Stringer joe at ovn.org
Wed Jun 14 18:43:44 UTC 2017


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


More information about the dev mailing list