On Tue, Mar 18, 2014 at 01:54:04PM -0700, Pravin wrote:
> Following patch adds DPDK netdev-class to userspace datapath. Now
> OVS can use DPDK port for IO by just configuring DPDK port and then
> adding dpdk type port to userspace datapath.
>
> Refer to INSTALL.DPDK doc for further info.
>
> This is based a patch from Gerald Rogers.
>
> Signed-off-by: [email protected]
> Signed-off-by: Pravin B Shelar <[email protected]>
Minor Stuff
===========
netdev-dpdk.c should #include <config.h> as its first non-comment
line, and should not #define _GNU_SOURCE directly (config.h will do
that).
dpdk_watchdog() will never exit, so it should probably make itself a
detached thread with pthread_detach(pthread_self());
Most of the items reported in netdev_dpdk_get_config() aren't
configuration, that is, it wouldn't make sense to set them using
netdev_set_config(). The ones that just report status should be moved
to netdev_dpdk_get_status(). ifindex should be removed (there's a
function specifically for ifindex).
m4 quoting in acinclude.m4 should be improved, from
AM_CONDITIONAL(DPDK_NETDEV, test -n "$RTE_SDK")
to
AM_CONDITIONAL([DPDK_NETDEV], [test -n "$RTE_SDK"])
acinclude.m4 defines a variable SLICE_SIZE_MAX that it doesn't use
anywhere (and won't get dumped into the Makefile or elsewhere).
In struct netdev_dpdk, I think that OVS_ACQ_AFTER(mutex) should be
OVS_ACQ_AFTER(dpdk_mutex), and that accept OVS_GUARDED_BY(mutex)
should be changed similarly. (I have Clang but I didn't install DPDK,
so I can't be sure.)
In lib/netdev-dpdk.h, let's avoid using a __ prefix since we're not
part of the C implementation (underscore prefixes are supposed to be
reserved).
Style
=====
INSTALL.DPDK has a few typos: s/dkdp/dpdk/, s/Fist/First/,
s/hugefs/hugetblfs/, s/continues/continuous/
I see a few places where function names should be moved to the
beginning of a line in netdev-dpdk.c.
No cast should be necessary here in free_dpdk_buf():
+ pkt = (struct rte_mbuf *) b->private_p;
In dpdk_watchdog(), "for (;;)" is more common in OVS than "while (1)".
This comment in dpdk_eth_dev_init() seems very urgent, so it might be
a good idea to explain further:
+ /* DO NOT CHANGE NUMBER OF RX DESCRIPTORS */
This comment in the same function also seems odd:
return 0; /* return the number of args to delete */
Many call to VLOG functions in the new file include an explicit
new-line. This is not necessary, and customarily we don't do that.
We don't usually put "inline" on functions defined in C files, since
it doesn't usually help code generation and does inhibit "function
defined but not referenced" warnings.
In netdev_dpdk_send(), "2big" is cute but I'd prefer "too big" or
"oversize".
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
