[ovs-dev] [PATCH] bridge: Fix bug where IDL wakeup causes 100% CPU.

Alex Wang alexw at nicira.com
Mon Sep 29 16:45:19 UTC 2014


Thx a lot for the fix, that was a bad move~

Acked-by: Alex Wang <alexw at nicira.com>

On Mon, Sep 29, 2014 at 3:09 AM, Joe Stringer <joestringer at nicira.com>
wrote:

> Commit 9c537baf613a16e (bridge: Refactor the stats and status update.)
> inadvertently changed the way that the status transaction is destroyed,
> which could cause the main thread to constantly wake up.
>
> The bug occurs when a transaction returns TXN_INCOMPLETE, then there are
> no subsequent changes to connectivity or 'status_txn_try_again'.
> ovsdb_idl_run() processes the transaction's reply and updates the status
> to TXN_SUCCESS. The logic in status_update_wait() wakes up the main
> thread instantly so that the transaction can be cleaned up, however the
> logic to clean it up in run_status_update() is inaccessible until the
> next connectivity change. This happens every time through the main loop,
> causing the main thread to spin at 100%.
>
> This patch fixes the behaviour  by ensuring that ovsdb_idl_txn_commit()
> gets a chance to run whenever there is an ongoing status transaction.
>
> Steps to reproduce: Unload/Reload kernel module && restart ovs-vswitchd.
>
> Signed-off-by: Joe Stringer <joestringer at nicira.com>
> ---
> I considered implementing this fix using ovsdb_idl_get_seqno() for the
> entire logic block, but concluded that it doesn't make sense to update
> the status whenever there is any kind of status change to any ongoing
> transaction. Rather, it makes sense that if there is a status
> transaction outstanding, to finish that transaction and clean it up when
> it is done.
> ---
>  vswitchd/bridge.c |   18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 5ba8d64..61896d3 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -2656,16 +2656,13 @@ run_stats_update(void)
>  static void
>  run_status_update(void)
>  {
> -    uint64_t seq;
> -
> -    /* Check the need to update status. */
> -    seq = seq_read(connectivity_seq_get());
> -    if (seq != connectivity_seqno || status_txn_try_again) {
> -        enum ovsdb_idl_txn_status status;
> +    if (!status_txn) {
> +        uint64_t seq;
>
>          /* Rate limit the update.  Do not start a new update if the
>           * previous one is not done. */
> -        if (!status_txn) {
> +        seq = seq_read(connectivity_seq_get());
> +        if (seq != connectivity_seqno || status_txn_try_again) {
>              struct bridge *br;
>
>              connectivity_seqno = seq;
> @@ -2687,6 +2684,13 @@ run_status_update(void)
>                  }
>              }
>          }
> +    }
> +
> +    /* Make progress on the transaction. If it finishes, then destroy the
> +     * transaction. Otherwise, keep it so that we can check progress
> during
> +     * the next run through the main loop. */
> +    if (status_txn) {
> +        enum ovsdb_idl_txn_status status;
>
>          status = ovsdb_idl_txn_commit(status_txn);
>          if (status != TXN_INCOMPLETE) {
> --
> 1.7.10.4
>
>



More information about the dev mailing list