On 13 January 2014 15:18, Ben Pfaff <[email protected]> wrote: > On Fri, Jan 10, 2014 at 11:43:13AM -0800, Joe Stringer wrote: > > This patch makes it the caller's responsibility to allocate and pass a > > buffer down to the dpif_flow_dump_next() implementation, to act as > > storage for the next flow. The implementation can expect to be called > > from multiple threads with the same 'state' and different 'buffer's. > > > > When flow_dump_next() returns non-zero, the implementation must ensure > > that subsequent calls with the same arguments also return non-zero. > > Subsequent calls with the same 'state' and different 'buffer' may return > > zero, but should make progress towards returning non-zero. > > > > Furthermore, the 'stats' argument becomes a pointer, not a double > > pointer. If this argument is non-null, then the 'dpif' will populate it > > with the stats of the flow. This should make reallocation unnecessary. > > > > Signed-off-by: Joe Stringer <[email protected]> > > dpif_flow_dump_next() deserves a comment update to explain what's > going on. > > The "Thread-safety" comment at the top of dpif.h also needs an update. >
OK. > I'm a little concerned that this could make life difficult for > out-of-tree dpif implementations. The in-tree ones are OK with > maintaining all per-thread state in a single ofpbuf. But I can > imagine dpifs that don't. If you think so, too, then it might make > sense to force each thread to supply a more general form of state than > just a single ofpbuf. I had two thoughts on this; One is that it's a buffer, and the implementation could use the buffer in any way it sees fit. They could reserve N bytes at the start of the buffer and each time they reset, reset to base + N---So long as dpif_flow_dump_next() provides the correct behaviour. If my descriptions over the ownership and use of the buffer are articulated well enough, this should be possible. The other thought is it's more generic if we have an opaque thread state pointer. This would make it clear that the caller is not supposed to modify the buffer, and that it should only be used by the flow_dump functions. This just means that we need new functions to allocate/deallocate this new type of state. Would this affect how you plan to use this interface, Ethan? On 13 January 2014 15:18, Ben Pfaff <[email protected]> wrote: > On Fri, Jan 10, 2014 at 11:43:13AM -0800, Joe Stringer wrote: > > This patch makes it the caller's responsibility to allocate and pass a > > buffer down to the dpif_flow_dump_next() implementation, to act as > > storage for the next flow. The implementation can expect to be called > > from multiple threads with the same 'state' and different 'buffer's. > > > > When flow_dump_next() returns non-zero, the implementation must ensure > > that subsequent calls with the same arguments also return non-zero. > > Subsequent calls with the same 'state' and different 'buffer' may return > > zero, but should make progress towards returning non-zero. > > > > Furthermore, the 'stats' argument becomes a pointer, not a double > > pointer. If this argument is non-null, then the 'dpif' will populate it > > with the stats of the flow. This should make reallocation unnecessary. > > > > Signed-off-by: Joe Stringer <[email protected]> > > dpif_flow_dump_next() deserves a comment update to explain what's > going on. > > The "Thread-safety" comment at the top of dpif.h also needs an update. > > I'm a little concerned that this could make life difficult for > out-of-tree dpif implementations. The in-tree ones are OK with > maintaining all per-thread state in a single ofpbuf. But I can > imagine dpifs that don't. If you think so, too, then it might make > sense to force each thread to supply a more general form of state than > just a single ofpbuf. > > Thanks, > > Ben. >
_______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
