On Mon, Aug 15, 2011 at 8:34 PM, pravin shelar <pshe...@nicira.com> wrote: > This patch makes sampling support more generic/simple by adding > Datapath SAMPLING action. When sampling is turned on, a SAMPLING > action is added to all flows for given bridge. > This patch cleanup sampling upcall by striping it down to minimum > essential that userspace can not figure out for itself. Now sample > packet is sent along with actions checksum. So that sflow module can > make sure it has consistent actions.
I'm not sure that this does much to simplify or genericize sampling. For the sample action itself, I was expecting something that could be used to execute any set of actions with a given probability but this is still tied towards sending packets to userspace with a fixed probability across the datapath. This would truly be generic and the word "sFlow" would not appear in the kernel at all but could be implemented entirely in userspace based on these primitives. Does this make sense to you? The use of the checksum for actions surprised me a little bit, as it is semantically equivalent to what we have today but perhaps not as accurate. Ben made a couple of good suggestions in the previous thread about how to do this cleanly and generically. Did you run into problems with those? The one piece that I'm the least sure of is how to handle stats. In my ideal world, we would drop the sample pool altogether and have userspace figure it out for itself. The information is clearly present in userspace to calculate any packet count that may be required but it might not be instantaneously accurate. I'm unsure of the consequences of this. I think it's reasonable to report some kind of stats (current flow stats seems to fit) but if you're trying to get the current port counters then that isn't really useful. Directly reporting port count seems a little out of place to me. Ben, (or anyone else) do you have any thoughts? _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
