On Tue, Aug 09, 2011 at 10:46:38AM -0700, pravin shelar wrote: > This patch is first step toward generic/simple sampling > action which will be completely independent of sflow. > Following patch cleanup sampling upcall by striping it down to > minimum essential data that userspace can not figure out for > itself. > > Signed-off by: Pravin Shelar <pshe...@nicira.com>
(That's "-by", not " by".) I like the general approach here--the sflow support is a big wart--but I think that the userspace interface changes represent a regression. Before, by passing the actions down to userspace, there was always a consistent view of those actions. With this change (as comments in the code make clear) the actions might have changed or the facet might even have been deleted. A really good fix would probably require being able to somehow keep old copies of flows that have changed around for a while in userspace, link them to the sflow messages, and free them when they are known not to be needed anymore. I think that's probably feasible, but in fact I think that we can avoid regression with something simpler. Suppose we keep the "sample pool" netlink option that this patch removes, and add a way for userspace to set a small amount of user data on a flow, and pass that along as the "user data" netlink option. 64 bits of user data should be plenty of space to store what the userspace sflow implementation actually needs (16 bits of final VLAN TCI plus 32 bits for sFlow's idea of output port). Plus, those same 64 bits would be more than enough for the sequence number that the "really good fix" mentioned in the previous paragraph would probably need. With the current patch, the obvious place to put this user data would be in the flow itself. Another approach (that you have probably already considered) is to make sFlow sampling a flow action, in which case the user data could be that action's argument. Thanks, Ben. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
