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 <[email protected]>
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
(horrors!).
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.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
