[ovs-dev] [no-slow 5/6] ofproto-dpif: Don't slow-path controller actions.

Justin Pettit jpettit at ovn.org
Wed Jan 10 22:28:08 UTC 2018



> On Jan 3, 2018, at 4:57 PM, Ben Pfaff <blp at ovn.org> wrote:
> 
> On Thu, Dec 21, 2017 at 02:25:14PM -0800, Justin Pettit wrote:
>> Controller actions have become more commonly used for purposes other
>> than just making forwarding decisions (e.g., packet logging).  A packet
>> that needs to be copied to the controller and forwarded would always be
>> sent to ovs-vswitchd to be handled, which could negatively affect
>> performance and cause heavier CPU utilization in ovs-vswitchd.
>> 
>> This commit changes the behavior so that OpenFlow controller actions
>> become userspace datapath actions while continuing to let packet
>> forwarding and manipulation continue to be handled by the datapath
>> directly.
>> 
>> This patch still slow-paths controller actions with the "pause" flag
>> set.  A future patch will stop slow-pathing these pause actions as
>> well.
>> 
>> Signed-off-by: Justin Pettit <jpettit at ovn.org>
> 
> I took a second look at this patch, by request.
> 
> The NEWS item doesn't explain what this feature means to users.

Done.

> It wasn't clear to me whether UACF_DONT_SEND re-implements existing
> behavior or introduces a behavioral change.  If it introduces a change,
> maybe one of the tutorials (like the Faucet tutorial?) relies on it, so
> we might need to change them.

This should only be visible when looking at the datapath flows and tracing traffic.  I did go through the Faucet tutorial, though, and made some changes to the ofproto/trace output.

> I don't know how many flags you expect.  If it's only one or two, maybe
> they should just be "bool"s.  (There's no ABI to freeze here, so they
> could be converted to a flag word if they got bigger.)

Okay, I went ahead and did that.

> In xlate_controller_action(), a comment refers to the slowpath meter,
> but I think that it should refer to the controller meter.

Fixed.

> I wonder whether some of the data in the user_action_cookie for
> controller actions should actually be put into the frozen_state.  I
> think that all of it other than the recirc_id could actually go in the
> frozen_state, but the userdata in particular can have an arbitrary size,
> which in extremis might make the datapath action too large and which we
> don't really need to copy between the kernel and userspace repeatedly.

Good point.  I reworked the series to use a fixed length userspace cookie and moved the userdata into the frozen state.

> The comment on struct user_action_cookie here seems inappropriate, since
> I believe that recirc_id will always be nonzero:
>        uint32_t recirc_id;     /* Non-zero if recirc id allocated. */

Agreed.  I dropped it.

I'll send out v2 in just a moment.

Thanks for the reviews!

--Justin




More information about the dev mailing list