[ovs-dev] [PATCH 1/3] ofproto-dpif-upcall: Avoid unnecessarily installing datapath flows.

[Andy Zhou]

All three patches look good to me. Guolin, please feel free to do
additional review or testing.

Acked-by: Andy Zhou <azhou at nicira.com>


On Fri, Jan 10, 2014 at 3:18 PM, Ben Pfaff <blp at nicira.com> wrote:

> handle_upcalls() always installed a flow for every packet, as long as
> the datapath didn't already have too many flows, but there are cases where
> we don't want to do this:
>
>     - If we get multiple packets in a single microflow all in one batch
>       (perhaps due to GSO breaking up a large TCP packet for sending to
>       userspace, or for another reason), then we only need to install the
>       datapath flow once.
>
>     - For a slow-pathed flow received via a slow-path action in the kernel,
>       we know that the kernel flow is already there (because otherwise it
>       would have been received as "no match" instead of an action), so
>       there is no benefit to reinstalling it.
>
> Noticed because a CFM slow-pathed flow was getting reinstalled every time
> a CFM packet was received.
>
> Reported-by: Guolin Yang <gyang at nicira.com>
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
>  ofproto/ofproto-dpif-upcall.c |   20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index dd24f5c..c3a1a03 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -1,4 +1,4 @@
> -/* Copyright (c) 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
> +/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
>   *
>   * Licensed under the Apache License, Version 2.0 (the "License");
>   * you may not use this file except in compliance with the License.
> @@ -208,6 +208,8 @@ struct flow_miss {
>      struct odputil_keybuf mask_buf;
>
>      struct xlate_out xout;
> +
> +    bool put;
>  };
>
>  static void upcall_destroy(struct upcall *);
> @@ -971,6 +973,7 @@ handle_upcalls(struct handler *handler, struct list
> *upcalls)
>                  miss->stats.used = time_msec();
>                  miss->stats.tcp_flags = 0;
>                  miss->odp_in_port = odp_in_port;
> +                miss->put = false;
>
>                  n_misses++;
>              } else {
> @@ -1107,10 +1110,23 @@ handle_upcalls(struct handler *handler, struct
> list *upcalls)
>              miss->flow.vlan_tci = 0;
>          }
>
> -        if (may_put) {
> +        /* Do not install a flow into the datapath if:
> +         *
> +         *    - The datapath already has too many flows.
> +         *
> +         *    - An earlier iteration of this loop already put the same
> flow.
> +         *
> +         *    - Slow-pathed flows only: we received this packet via some
> flow
> +         *      installed in the kernel already. */
> +        if (may_put
> +            && !miss->put
> +            && (!miss->xout.slow
> +                || upcall->dpif_upcall.type == DPIF_UC_MISS)) {
>              struct ofpbuf mask;
>              bool megaflow;
>
> +            miss->put = true;
> +
>              atomic_read(&enable_megaflows, &megaflow);
>              ofpbuf_use_stack(&mask, &miss->mask_buf, sizeof
> miss->mask_buf);
>              if (megaflow) {
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140110/ec3c5e73/attachment-0003.html>