On Mon, Sep 15, 2014 at 02:25:11PM +1200, Joe Stringer wrote:
> Currently, when a revalidator thread first dumps a flow, it creates a
> 'udpif_key' object and caches a copy of a kernel flow key. This allows
> us to perform lookups in the classifier to attribute stats and validate
> the correctness of the datapath flow.
>
> This patch sets up this cache from the handler threads, during flow
> setup. This allows future patches to reduce the cost of flow dumping.
>
> Signed-off-by: Joe Stringer <joestrin...@nicira.com>
I wonder whether it would be better to always use poll_block() in
udpif_upcall_handler(). It would be more consistent with most code in
OVS, and it would mean that the code would not need special case calls
to ovsrcu_quiesce() and coverage_clear(). Here's one way:
while (!latch_is_set(&handler->udpif->exit_latch)) {
if (recv_upcalls(handler)) {
poll_immediate_wake();
} else {
dpif_recv_wait(udpif->dpif, handler->handler_id);
latch_wait(&udpif->exit_latch);
}
poll_block();
}
upcall_receive() zeros an entire struct upcall. On i386, it looks like
struct upcall is 620 bytes. I guess that a large part of this is stub
buffers that do not need to be zeroed. I think that it would be better
to initialize only what needs it; did you consider that?
Same note for ukey_new() calling xzalloc() for a struct udpif_key, which
is 616 bytes here with 512 of those bytes as a stub buffer.
ukey_new() has two cases:
recirc = ofproto_dpif_get_enable_recirc(upcall->ofproto);
ofpbuf_use_stack(&key, &ukey->key_buf, sizeof ukey->key_buf);
if (upcall->key_len) {
ofpbuf_put(&key, upcall->key, upcall->key_len);
} else {
odp_flow_key_from_flow(&key, upcall->flow, &upcall->xout.wc.masks,
upcall->flow->in_port.odp_port, recirc);
}
I think that the second case will only be taken for dpif-netdev. If so,
then I do not think it is necessary to check for recirculation, because
dpif-netdev is compiled in and always supports recirculation. (In any
case, it might be worth a comment explaining when the two cases will
trigger, because it took me a while to figure it out.)
It looks to me like one of the cases where ukey_install_start() would
fail is when a number of packets in a single microflow show up all at
once (e.g. a large TCP packet sent to userspace gets broken up by TSO).
It seems like this might artificially increase handler_install_conflict.
I don't know whether that matters.
I tend to write functions like ukey_install() as just:
return ukey_install_start(udpif, ukey) && ukey_install_finish(ukey, 0);
because it makes simple logic look simpler, in my opinion.
In ukey_acquire(), please don't break a format specifier across lines:
VLOG_INFO("Dumped flow from datapath with no corresponding ukey: 0x%"
PRIx32, hash);
Instead, wrap the whole thing and the word that it's part of:
VLOG_INFO("Dumped flow from datapath with no corresponding ukey: "
"0x%"PRIx32, hash);
revalidate() doesn't count or log flows that were unexpectedly found in
the datapath, that is:
if (ukey_acquire(udpif, f, &ukey, &error)) {
if (error == EBUSY) {
/* Another thread is processing this flow, so don't bother
* processing it.*/
COVERAGE_INC(upcall_ukey_contention);
} else {
This case here:
delete_op_init(&ops[n_ops++], f->key, f->key_len, ukey);
}
continue;
}
Should it?
Acked-by: Ben Pfaff <b...@nicira.com>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
