Thanks for the review! I fixed all these things in my next version (v3). Ryan
From: Daniele Di Proietto <ddiproie...@vmware.com<mailto:ddiproie...@vmware.com>> Date: Wednesday, June 18, 2014 9:05 AM To: Ryan Wilson <wr...@nicira.com<mailto:wr...@nicira.com>> Cc: "dev@openvswitch.org<mailto:dev@openvswitch.org>" <dev@openvswitch.org<mailto:dev@openvswitch.org>> Subject: Re: [ovs-dev] [PATCH v2] dpif-netdev: Polling threads directly call ofproto upcall functions. Hi Ryan, some quick thoughts about the code: diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 6c281fe..626b3e6 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -15,8 +15,6 @@ */ #include <config.h> -#include "dpif.h" - #include <ctype.h> #include <errno.h> #include <fcntl.h> @@ -35,6 +33,7 @@ #include "cmap.h" #include "csum.h" #include "dpif.h" +#include "dpif-netdev.h" #include "dpif-provider.h" #include "dummy.h" #include "dynamic-string.h" Since we're changing includes, can we be consistent with CodingStyle? " #include directives should appear in the following order: 1. #include <config.h> 2. The module's own headers, if any. Including this before any other header (besides <config.h>) ensures that the module's header file is self-contained (see HEADER FILES) below. " I am not sure if there are conflicts/requirements that prevent us from enforcing those rules here. Feel free to ignore this comment if you already tried and it's not feasible. @@ -76,10 +75,7 @@ DEFINE_STATIC_PER_THREAD_DATA(uint32_t, recirc_depth, 0) /* Configuration parameters. */ enum { MAX_FLOWS = 65536 }; /* Maximum number of flows in flow table. */ -/* Queues. */ -enum { MAX_QUEUE_LEN = 128 }; /* Maximum number of packets per queue. */ -enum { QUEUE_MASK = MAX_QUEUE_LEN - 1 }; -BUILD_ASSERT_DECL(IS_POW2(MAX_QUEUE_LEN)); +static exec_upcall_func *exec_upcall_cb = NULL; Can we avoid having a static callback and put this into the dpif (or, better, dp_netdev) structure? My idea is that (for testing purpose) we could have dpifs that are not linked to ofproto, but run on their own. @@ -481,6 +447,10 @@ create_dp_netdev(const char *name, const struct dpif_class *class, dp->port_seq = seq_create(); latch_init(&dp->exit_latch); + /* Disable upcalls by default. */ + fat_rwlock_init(&dp->upcall_rwlock); + fat_rwlock_wrlock(&dp->upcall_rwlock); + Here, the clang thread safety analyzer complains about 'dp->upcall_rwlock' not being unlocked at the end of the function. Maybe you can use dp_netdev_disable_upcall() (changing the interface a little) and add OVS_NO_THREAD_SAFETY_ANALYSIS to it. diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index b38f226..4f63b0c 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -22,6 +22,7 @@ #include "connmgr.h" #include "coverage.h" #include "dpif.h" +#include "dpif-netdev.h" In my opinion it would be better to avoid including dpif-netdev specific definitions in ofproto. Also, see below. @@ -291,6 +297,8 @@ udpif_stop_threads(struct udpif *udpif) xpthread_join(udpif->revalidators[i].thread, NULL); } + dp_netdev_disable_upcall(udpif->dpif); + This will fail for dpifs with 'dpif-linux' class, because get_dp_netdev() will assert. A better solution might be to add these calls to the dpif interface. for (i = 0; i < udpif->n_revalidators; i++) { struct revalidator *revalidator = &udpif->revalidators[i]; @@ -341,6 +349,8 @@ udpif_start_threads(struct udpif *udpif, size_t n_handlers, "handler", udpif_upcall_handler, handler); } + dp_netdev_enable_upcall(udpif->dpif); + Same thing here. I am not very familiar with the ofproto part of the upcall code, so I didn't check everything. Thanks, Daniele
_______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
