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

Ryan Wilson 76511 wryan at vmware.com
Wed Jun 18 18:08:32 UTC 2014


Thanks for the review! I fixed all these things in my next version (v3).

Ryan

From: Daniele Di Proietto <ddiproietto at vmware.com<mailto:ddiproietto at vmware.com>>
Date: Wednesday, June 18, 2014 9:05 AM
To: Ryan Wilson <wryan at nicira.com<mailto:wryan at nicira.com>>
Cc: "dev at openvswitch.org<mailto:dev at openvswitch.org>" <dev at openvswitch.org<mailto:dev at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140618/b6f28e5b/attachment-0005.html>


More information about the dev mailing list