[ovs-dev] [DPDK Upcalls v2 3/3] dpif-netdev: Streamline miss handling.

Ben Pfaff blp at nicira.com
Tue Aug 12 18:24:23 UTC 2014

On Wed, Aug 06, 2014 at 07:40:25PM -0700, Ethan Jackson wrote:
> This patch avoids the relatively inefficient miss handling processes
> dictated by the dpif process, by calling into ofproto-dpif directly
> through a callback.
> Signed-off-by: Ethan Jackson <ethan at nicira.com>

In dp_netdev_input(), the variable name 'may_miss' didn't make sense
to me.  I had to figure out what it meant.  I think that 'any_miss'
would be a better name.

In ofproto-dpif-upcall.c, there are two blank lines above upcall_cb

upcall_cb() can use struct assignment instead of memcpy():

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 4c7835b..95d4181 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -956,7 +956,7 @@ upcall_cb(const struct ofpbuf *packet, const struct flow *flow,
         atomic_read(&enable_megaflows, &megaflow);
         if (megaflow) {
             /* XXX: This could be avoided with sufficient API changes. */
-            memcpy(wc, &upcall.xout.wc, sizeof *wc);
+            *wc = upcall.xout.wc;
         } else {
             memset(wc, 0xff, sizeof *wc);

Did you test with megaflows disabled, by the way?  I am not sure
whether all-1-bits is an appropriate wildcard mask.

I think that dpif-netdev will actually jump to NULL if an upcall_cb is
not registered.  I don't think this is clear in the interface.

The upcall_callback interface seems unclear to me.  Here is my
suggestion (I reordered some parameters):

/* A callback to process an upcall, currently implemented only by dpif-netdev.
 * The caller provides the 'packet' and 'flow' to process, the 'type' of the
 * upcall, and if 'type' is DPIF_UC_ACTION then the 'userdata' attached to the
 * action.
 * The callback must fill in 'actions' with the datapath actions to apply to
 * 'packet'.  'wc' and 'put_actions' will either be both null or both nonnull.
 * If they are nonnull, then the caller will install a flow entry to process
 * all future packets that match 'flow' and 'wc'; the callback must store a
 * wildcard mask suitable for that purpose into 'wc'.  If the actions to store
 * into the flow entry are the same as 'actions', then the callback may leave
 * 'put_actions' empty; otherwise it must store the desired actions into
 * 'put_actions'.
 * Returns 0 if successful, otherwise a positive errno value. */
typedef int upcall_callback(const struct ofpbuf *packet,
                            const struct flow *flow,
                            enum dpif_upcall_type type,
                            const struct nlattr *userdata,
                            struct ofpbuf *actions,
                            struct flow_wildcards *wc,
                            struct ofpbuf *put_actions,
                            void *aux);

upcall_cb() in ofproto-dpif-upcall.c drops packets if the number of
flows exceeds the flow limit.  I think that it should instead forward
the packet without installing a flow (the current upcall interface
doesn't allow that; perhaps it should).

dp_netdev_init() fully initializes match.wc even though upcall_cb() is
going to have to overwrite all of it anyway.  I'd skip that.

dp_netdev_flow_add() goes to some trouble to log matches as if they
were provided as datapath flows, even though they aren't.  I think
that match_format() would be simpler and probably less confusing.

More information about the dev mailing list