> On Jan 3, 2018, at 4:57 PM, Ben Pfaff <[email protected]> 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 <[email protected]> > > 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
