Thanks for the review,
On 18 September 2014 10:33, Ben Pfaff <b...@nicira.com> wrote:
> 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();
> }
>
I'm happy with this, anyone else in particular you'd like to bounce it off?
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.
>
Yes, this was a somewhat careless inclusion in the patchset. I'll fix them
up.
> 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.)
>
I'll look out for other cases like this that could do with a comment to
explain how they're used. I'm also looking at changing the ukey to include
a 'struct flow' instead, which would merge the two cases here. The note on
recirculation is useful, thanks.
> 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.
>
Patch #12 improves this further, to distinguish cases like this vs. two
different flows hashing to the same value. When respinning, I'll look at
whether it's better to merge that patch into this one.
>
> 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);
>
>
I'll fix these up.
> 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?
>
Perhaps better to log it than stay silent. The most common case for this is
if an ovs-dpctl user pushes a flow into the datapath. VLOG_INFO?
>
> Acked-by: Ben Pfaff <b...@nicira.com>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
