On Wed, Mar 19, 2014 at 12:27 PM, Ben Pfaff <[email protected]> wrote: > 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). > ok.
> dpdk_watchdog() will never exit, so it should probably make itself a > detached thread with pthread_detach(pthread_self()); > ok. > 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). > ok. > 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). > right, This was added for libhugetlb, I will remove it for now. > 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.) > ok. > 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). > ok. > > 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". ok, I will fix it. _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
