[ovs-dev] [PATCH v2] dpif-netdev: Polling threads directly call ofproto upcall functions.

Daniele Di Proietto ddiproietto at vmware.com
Wed Jun 18 16:05:53 UTC 2014


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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140618/739fe98b/attachment-0005.html>


More information about the dev mailing list