[ovs-dev] [PATCH 08/10] dpif: Make dpif_flow_dump_next() thread-safe.

Joe Stringer joestringer at nicira.com
Tue Jan 14 17:57:43 UTC 2014


On 13 January 2014 15:18, Ben Pfaff <blp at nicira.com> 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 <joestringer at nicira.com>
>
> 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 <blp at nicira.com> 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 <joestringer at nicira.com>
>
> 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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140114/14beb36e/attachment-0003.html>


More information about the dev mailing list