[ovs-dev] [PATCHv5 05/12] upcall: Create ukeys in handler threads.

[Ben Pfaff]

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 <joestringer at 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 <blp at nicira.com>