On Fri, May 23, 2014 at 01:15:58AM +0200, Thomas Graf wrote:
> Signed-off-by: Thomas Graf <[email protected]>
Thanks for the patches.
Definitions in netlink-protocol.h have to be written from the
perspective that we might be on a non-Linux host and compiling with a
non-GCC compiler (in particular MSVC). The definition of
__ALIGN_KERNEL is problematic from the latter perspective, since MSVC
doesn't have typeof. I am also not sure whether definition of __u32
is universal in practice; I would tend to use uint32_t.
The comment on nl_sock_create_mmap() says that it falls back to an
unmapped socket if it cannot create a mapped one, but the
implementation appears to fail hard in that case. The return value
convention also seems to be mixed up versus the comment.
We do not usually write code of this form:
if ((retval = nl_sock_create(protocol, sockp)) < 0) {
instead:
retval = nl_sock_create(protocol, sockp);
if (retval < 0) {
I guess that the double cast in mmap_frame() is due to the issue that
we usually document by using the ALIGNED_CAST macro.
One of the if statements in nl_sock_send_mmap() lacks {} around its
conditional statement.
I don't think that it is a good idea to use nl_sock_wait() and
poll_block() inside nl_sock_send_mmap(), because this could interact
with any poll set being built up by the caller. The caller ordinarily
wouldn't be doing that (it's not the model we use) but it still
doesn't seem quite wise. I would be more inclined to use the poll()
function directly here to wait for a slot to become available.
Using nl_sock_send_linear() can cause messages to be reordered. Do we
need to wait for the tx ring to empty before calling it?
Do we need a memory barrier before setting nm_status? (Can the kernel
running on a different CPU consume the TX buffer before we call
sendto()?)
I need to continue reading the patch at nl_sock_recv_mmap().
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
