On 05/23/14 at 09:57am, Ben Pfaff wrote: > 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.
Good point. I'll verify if that is the case and see if I can come up with asserts to catch mishebaviour in this regard. > 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.) OK. I thought about adding a new class but realized that stack memory shares the same constraints except for the individual destructor. I'm perfectly fine with adding a new class for the mmaped case. We'll definitely need this as we extend usability of the shared memory region and eventually reuse it when injecting packets back into the kernel. > 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. OK, good to know. This will simplify the code. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
