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
