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

Ben Pfaff blp at ovn.org
Thu Jan 4 00:57:20 UTC 2018


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.

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.

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

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

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.

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

Thanks,

Ben.


More information about the dev mailing list