[ovs-dev] [PATCHv2] bridge: Rate limit port creations and deletions.

Ethan Jackson ethan at nicira.com
Thu Apr 12 23:35:28 UTC 2012


This is cleaner than using the need_reconfigure flag to guarantee at
least one port modification so I've folded it in.

I'll merge this when I've had a chance to test it.

Ethan

On Thu, Apr 12, 2012 at 16:31, Ben Pfaff <blp at nicira.com> wrote:
> On Wed, Apr 11, 2012 at 01:04:37PM -0700, Ethan Jackson wrote:
>> In some datapaths, adding or deleting OpenFlow ports can take quite
>> a bit of time.  If there are lots of OpenFlow ports which needed to
>> be added in a run loop, this can cause Open vSwitch to lock up and
>> stop setting up flows while trying to catch up.  This patch lessons
>> the severity of the problem by only doing a few OpenFlow port adds
>> or deletions per run loop allowing other work to be done in
>> between.
>>
>> Bug #10672.
>> Signed-off-by: Ethan Jackson <ethan at nicira.com>
>> ---
>>
>> I've tested this version on my virtual machine, but still need to try it on a
>> real server before merging it.  I don't expect it to need to change so I'm
>> sending it out for review now.
>
> It looks OK to me.
>
> I couldn't resist the urge to tinker with the details, so here's an
> incremental.  Feel free to take it or leave it as you like.
>
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 314fc71..57a1b89 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -167,9 +167,9 @@ static size_t bridge_get_controllers(const struct bridge *br,
>  static void bridge_add_del_ports(struct bridge *,
>                                  const unsigned long int *splinter_vlans);
>  static void bridge_add_ofproto_ports(struct bridge *,
> -                                     long long int port_action_timer);
> +                                     long long int *port_action_timer);
>  static void bridge_del_ofproto_ports(struct bridge *,
> -                                     long long int port_action_timer);
> +                                     long long int *port_action_timer);
>  static void bridge_refresh_ofp_port(struct bridge *);
>  static void bridge_configure_datapath_id(struct bridge *);
>  static void bridge_configure_flow_eviction_threshold(struct bridge *);
> @@ -416,8 +416,7 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg)
>
>     COVERAGE_INC(bridge_reconfigure);
>
> -    time_refresh();
> -    port_action_timer = time_msec() + OFP_PORT_ACTION_WINDOW;
> +    port_action_timer = LLONG_MAX;
>     need_reconfigure = false;
>
>     /* Create and destroy "struct bridge"s, "struct port"s, and "struct
> @@ -442,7 +441,7 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg)
>     bridge_del_ofprotos();
>     HMAP_FOR_EACH (br, node, &all_bridges) {
>         if (br->ofproto) {
> -            bridge_del_ofproto_ports(br, port_action_timer);
> +            bridge_del_ofproto_ports(br, &port_action_timer);
>         }
>     }
>
> @@ -455,7 +454,7 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg)
>     HMAP_FOR_EACH_SAFE (br, next, node, &all_bridges) {
>         if (!br->ofproto) {
>             if (bridge_add_ofprotos(br)) {
> -                bridge_del_ofproto_ports(br, port_action_timer);
> +                bridge_del_ofproto_ports(br, &port_action_timer);
>             } else {
>                 bridge_destroy(br);
>             }
> @@ -463,7 +462,7 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg)
>     }
>     HMAP_FOR_EACH (br, node, &all_bridges) {
>         bridge_refresh_ofp_port(br);
> -        bridge_add_ofproto_ports(br, port_action_timer);
> +        bridge_add_ofproto_ports(br, &port_action_timer);
>     }
>
>     /* Complete the configuration. */
> @@ -1053,23 +1052,20 @@ add_del_bridges(const struct ovsrec_open_vswitch *cfg)
>  }
>
>  static bool
> -may_port_action(long long int port_action_timer)
> +may_port_action(long long int *port_action_timer)
>  {
> -    if (time_msec() < port_action_timer) {
> -        time_refresh();
> -    }
> -
> -    if (time_msec() < port_action_timer) {
> -        return true;
> +    if (need_reconfigure) {
> +        return false;
>     }
>
> -    /* Guarantee at least one port is acted on per run loop. */
> -    if (!need_reconfigure) {
> +    time_refresh();
> +    if (*port_action_timer == LLONG_MAX) {
> +        *port_action_timer = time_msec() + OFP_PORT_ACTION_WINDOW;
> +    } else if (time_msec() > *port_action_timer) {
>         need_reconfigure = true;
> -        return true;
> +        return false;
>     }
> -
> -    return false;
> +    return true;
>  }
>
>  /* Delete each ofproto port on 'br' that doesn't have a corresponding "struct
> @@ -1080,7 +1076,7 @@ may_port_action(long long int port_action_timer)
>  * deletions before any port additions. */
>  static void
>  bridge_del_ofproto_ports(struct bridge *br,
> -                         long long int port_action_timer)
> +                         long long int *port_action_timer)
>  {
>     struct ofproto_port_dump dump;
>     struct ofproto_port ofproto_port;
> @@ -1179,7 +1175,7 @@ bridge_refresh_ofp_port(struct bridge *br)
>  * Delete any "struct port" that thereby ends up with no ifaces. */
>  static void
>  bridge_add_ofproto_ports(struct bridge *br,
> -                         long long int port_action_timer)
> +                         long long int *port_action_timer)
>  {
>     struct port *port, *next_port;
>
>



More information about the dev mailing list