Closed Bug 983576 Opened 7 years ago Closed 7 years ago

[Bluetooth] Cleanup start/stop code in BlueZ backend

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
1.4 S4 (28mar)

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(5 files, 2 obsolete files)

The BlueZ backend still blocks a lot in its start/stop code and is accesses global variables on different threads. We should change the code to

  - minimize the blocking calls in the BT thread,
  - push as much init code as possible to the I/O thread, and
  - only access global variables from a single thread.
Comment on attachment 8391187 [details] [diff] [review]
[01] Bug 983576: Move DispatchToBtThread to mozilla::bluetooth namespace

Review of attachment 8391187 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nit addressed.

::: dom/bluetooth/bluez/BluetoothDBusService.cpp
@@ +318,5 @@
>  
> +static nsresult
> +DispatchToBtThread(nsIRunnable* aRunnable)
> +{
> +  /* Due to the fact that the startup and shutdown of the Bluetooth 

nit: trailing whitespace
Attachment #8391187 - Flags: review?(echou) → review+
(In reply to Eric Chou [:ericchou] [:echou] from comment #5)
> Comment on attachment 8391187 [details] [diff] [review]
> [01] Bug 983576: Move DispatchToBtThread to mozilla::bluetooth namespace
> 
> Review of attachment 8391187 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with nit addressed.
> 
> ::: dom/bluetooth/bluez/BluetoothDBusService.cpp
> @@ +318,5 @@
> >  
> > +static nsresult
> > +DispatchToBtThread(nsIRunnable* aRunnable)
> > +{
> > +  /* Due to the fact that the startup and shutdown of the Bluetooth 
> 
> nit: trailing whitespace

Ah, it's fixed in the second patch. :)
Usually, my editor cleans up trailing white spaces for me, but I recently had to disable this feature for some files. I'll cleanup the code for the next version. :)
Comment on attachment 8391188 [details] [diff] [review]
[02] Bug 983576: Refactor BlueZ start/stop code

Review of attachment 8391188 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits addressed and the question answered.

::: dom/bluetooth/bluez/BluetoothDBusService.cpp
@@ +1881,5 @@
>      MOZ_ASSERT(mConnection);
>    }
>  
>    void Run()
>    {

suggestion: please add a thread assertion at the beginning of Run().

@@ +1941,4 @@
>    bool mQueryDefaultAdapter;
>  };
>  
>  class StartBluetoothRunnable MOZ_FINAL : public nsRunnable

Question: After checked the code of StopBluetoothRunnable, I realized that most of the code originally in StopBluetoothRunnable::Run() is going to be moved to DeleteDBusConnectionTask which would be run on IO thread. My question is why we don't do the same thing to StartBluetoothRunnable? I mean can we move line 1963 ~ 1990 to StartDBusConnectionTask?

@@ +2016,5 @@
> +      // Forward this runnable to BT thread
> +      return DispatchToBtThread(this);
> +    }
> +
> +    MOZ_ASSERT(!NS_IsMainThread()); // BT thread

Hm, this line seems a bit redundant because the if-statement above is clear enough to me.
Attachment #8391188 - Flags: review?(echou) → review+
Hi Eric

suggestion: please add a thread assertion at the beginning of Run().
> 
> @@ +1941,4 @@
> >    bool mQueryDefaultAdapter;
> >  };
> >  
> >  class StartBluetoothRunnable MOZ_FINAL : public nsRunnable
> 
> Question: After checked the code of StopBluetoothRunnable, I realized that
> most of the code originally in StopBluetoothRunnable::Run() is going to be
> moved to DeleteDBusConnectionTask which would be run on IO thread. My
> question is why we don't do the same thing to StartBluetoothRunnable? I mean
> can we move line 1963 ~ 1990 to StartDBusConnectionTask?

I'm a bit confused by the line numbers here. Lines 1963 to 1978 load libbluedroid, which can block. Lines 1979 to 1990 run on the BT thread because of the call to dbus_bus_add_match at lines 1991 to 2007. The docs for dbus_bus_add_match [1] say that this function blocks if we supply an error structures as its third parameter. I'd love to move this call (and lines 1979~1990) to the I/O thread, but that would mean to leave out the error parameter. I don't know how we'd detect these errors then. libdbus sucks. :(

In the stop code there is a call to dbus_bus_remove_match, which has similar semantics. I removed the error parameter there and moved the call to the I/O thread. This simplifies the stop code considerably and I don't think that detecting this error in the stop code is strictly necessary.

Best regards
Thomas

[1] http://dbus.freedesktop.org/doc/api/html/group__DBusBus.html#ga4eb6401ba014da3dbe3dc4e2a8e5b3ef
Comment on attachment 8391189 [details] [diff] [review]
[03] Bug 983576: Fail if sDBusConnection is not set

Review of attachment 8391189 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

We sometimes use the name "I/O thread" and sometimes use "DBus thread" to represent the same thread in comments. Can we unify them?
Attachment #8391189 - Flags: review?(echou) → review+
(In reply to Eric Chou [:ericchou] [:echou] from comment #10)
> Comment on attachment 8391189 [details] [diff] [review]
> [03] Bug 983576: Fail if sDBusConnection is not set
> 
> Review of attachment 8391189 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me.
> 
> We sometimes use the name "I/O thread" and sometimes use "DBus thread" to
> represent the same thread in comments. Can we unify them?

Sure I'll add another patch to the next round of reviews. I think 'DBus thread' is the historical name, used when there was still a separate thread for DBus.
Comment on attachment 8391190 [details] [diff] [review]
[04] Bug 983576: Annotate global BlueZ variables

Review of attachment 8391190 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for making our code more readable!
Attachment #8391190 - Flags: review?(echou) → review+
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #11)
> (In reply to Eric Chou [:ericchou] [:echou] from comment #10)
> > Comment on attachment 8391189 [details] [diff] [review]
> > [03] Bug 983576: Fail if sDBusConnection is not set
> > 
> > Review of attachment 8391189 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks good to me.
> > 
> > We sometimes use the name "I/O thread" and sometimes use "DBus thread" to
> > represent the same thread in comments. Can we unify them?
> 
> Sure I'll add another patch to the next round of reviews. I think 'DBus
> thread' is the historical name, used when there was still a separate thread
> for DBus.

Thanks. :)
Hi Thomas,

> I'm a bit confused by the line numbers here. Lines 1963 to 1978 load
> libbluedroid, which can block. Lines 1979 to 1990 run on the BT thread
> because of the call to dbus_bus_add_match at lines 1991 to 2007.

I should avoid using line number to indicate code blocks. It's too easy to get confused. :|

What I meant "line 1963 ~ 1990":

====================

RawDBusConnection* connection = new RawDBusConnection();
nsresult rv = connection->EstablishDBusConnection();

...

for (uint32_t i = 0; i < ArrayLength(sBluetoothDBusSignals); ++i) {
  dbus_bus_add_match(connection->GetConnection(), sBluetoothDBusSignals[i], &err);
  if (dbus_error_is_set(&err)) {
    LOG_AND_FREE_DBUS_ERROR(&err);
  }
}

====================

My basic idea was moving this block to StartDBusConnectionTask::Run().

> The docs for dbus_bus_add_match [1] say that this function blocks if we
> supply an error structures as its third parameter. I'd love to move this
> call (and lines 1979~1990) to the I/O thread, but that would mean to leave
> out the error parameter. I don't know how we'd detect these errors then.
> libdbus sucks. :(
> 

Got it. So my question becomes: it sounds like you tried to avoid getting dbus_bus_add_match run on I/O thread. Do we have to do this?
Hi Eric,

I had another look at the implementation of |dbus_bus_add_match|. It just sets up a message and waits for the reply. I think this could be reimplemented by us on the I/O thread.

But I also checked what exactly happens in |EstablishDBusConnection| and that's really scary. The method opens a connection using |dbus_bus_get_private|, which does tons of address lookups, TCP connect, and similar operations. Reimplementing this in a non-blocking way means replacing most (all?) of the DBus connection setup.
(In reply to Eric Chou [:ericchou] [:echou] from comment #12)
> Comment on attachment 8391190 [details] [diff] [review]
> [04] Bug 983576: Annotate global BlueZ variables
> 
> Review of attachment 8391190 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for making our code more readable!

Sure, you're welcome. Some of these variables have at least questionable thread-safety and already indicate potential follow-up patches. :)
Depends on: 985949
Changes since v1:

  - cleaned up white space error
Attachment #8391187 - Attachment is obsolete: true
Attachment #8394154 - Flags: review+
Changes since v1:

  - cleaned up white space error
  - fix thread assertions
Attachment #8391188 - Attachment is obsolete: true
Attachment #8394155 - Flags: review+
Comment on attachment 8394157 [details] [diff] [review]
[05] Bug 983576: Annotate all assertions for non-main threads in BlueZ backend

Review of attachment 8394157 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks. :)
Attachment #8394157 - Flags: review?(echou) → review+
You need to log in before you can comment on or make changes to this bug.