[ovs-dev] [PATCH 8/9] dpif-netdev: Add DPDK netdev.

[Ben Pfaff]

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: gerald.rogers at intel.com
> Signed-off-by: Pravin B Shelar <pshelar at nicira.com>

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".