On Fri, May 23, 2014 at 01:16:01AM +0200, Thomas Graf wrote: > If the message was received via a shared memory ring the frame > is usable as ofpbuf base directly without copying the buffer > unnecessarily. A new destructor() callback is introduced which > allows marking the frame unused after the ofpbuf has been > uninitialized. > > Signed-off-by: Thomas Graf <tg...@suug.ch>
I've resisted a general-purpose destructor here for a long time but I guess it's inevitable. This introduces a new constraint on use of ofpbufs: an ofpbuf received from a mmaped netlink socket must be completely processed and destroyed before the netlink socket is closed (otherwise later destruction of the ofpbuf will segfault because the memory it accesses is unmapped). I guess that this is generally what we do, but it has not been a concern before. I wonder whether the port deletion path is exactly correct. There are already a number of "classes" of ofpbuf: OFPBUF_MALLOC, OFPBUF_SOURCE, and so on. I would have expected this commit to add a generic destructor-based class. Instead, it adds the destructor in parallel to the class. This seems odd to me; it means, for example, that if the buffer has to be expanded, the mmaped buffer will not be released until the ofpbuf is freed, even though the mmaped buffer will not be in use after the expansion. And it seems to me to in general complicate thinking about ofpbufs, because now there are two questions about where memory comes from: not just "what kind of memory?" but both "what kind of memory?" plus "is there a destructor?". So I would lean toward adding a new OFPBUF_* constant that means to use the destructor. (The destructor_data member may not be necessary in this case, since the code could, I imagine, rely on ofpbuf_base() to be where it put it, and I guess that it could even check that to some extent by asserting on the offset of the base within a page.) I'm not sure that it is necessary to check that the ofpbuf is empty before deciding to use the mmaped memory in-place. nl_sock_recv() is documented to replace 'buf', not to append to it, and it already monkeys with the allocation to expand it as necessary. So I'd suggest unconditionally using the mmapped memory. Thanks, Ben. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
