814579 - B2G Multi-SIM: multiplexing data streams of multiple RIL daemons

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Vicamo Yang [:vicamo][:vyang]]

With multiple ril daemons provided in bug 814577, we're going to create a platform independent layer in rilproxy & SystemWorkerManager. In rilproxy, it multiplexes all data from and to each ril daemon while SystemWorkerManager does similar thing but to multiple ril_worker instances.

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://github.com/multi-sim/releases-mozilla-central/pull/4

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://github.com/multi-sim/rilproxy/pull/1

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://github.com/multi-sim/releases-mozilla-central/pull/7
https://github.com/allstarschh/gonk-misc/tree/bugzilla/814579/master

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Plan B - Part 1: Github pull request for rilproxy

Comment 5

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Plan B - Part 2: Github pull request for gonk-misc

Comment 6

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Plan B - Part 3: Github Gecko stable branch

Comment 7

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Plan B - Part 3-1: use nsRefPtr instead

RefPtr was created for compatibility issues in bug 661973. Use nsRefPtr because we'll have to compare the client id of each managed RilClient pointer, and equality operators and other utility classes and functions are only available for nsRefPtr.

Comment 8

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Plan B - Part 3-2: allow multiple RilClient instances

Comment 9

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Plan B - Part 3-3: connect client socket by id

These three patches are the stepping stone for MultiSIM support. Full implementation will come in bug 814581.

Comment 10

[Vicamo Yang [:vicamo][:vyang]]

All the patches/pulls here keeps compatibility of current b2g arch. Part 1 & 2 have to be merged at the same time, and part 3 can be merged independent to other parts.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 697826 [details] [diff] [review]
Plan B - Part 3-1: use nsRefPtr instead

>diff --git a/dom/system/gonk/SystemWorkerManager.cpp b/dom/system/gonk/SystemWorkerManager.cpp
>     // Now that we're set up, connect ourselves to the RIL thread.
>-    mozilla::RefPtr<RILReceiver> receiver = new RILReceiver(wctd);
>+    nsRefPtr<RILReceiver> receiver = new RILReceiver(wctd);

Why change this?  (It shouldn't matter, just curious.)

>diff --git a/ipc/ril/Ril.cpp b/ipc/ril/Ril.cpp

>+static nsRefPtr<RilClient> sClient;
>+static nsRefPtr<RilConsumer> sConsumer;

These should be |static StaticRefPtr<...>|.

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Plan B - Part 3-2: allow multiple RilClient instances

Sorry, forgot git-push before creating hg patches.

Comment 13

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #11)
> Comment on attachment 697826 [details] [diff] [review]
> Plan B - Part 3-1: use nsRefPtr instead
> 
> >diff --git a/dom/system/gonk/SystemWorkerManager.cpp b/dom/system/gonk/SystemWorkerManager.cpp
> >     // Now that we're set up, connect ourselves to the RIL thread.
> >-    mozilla::RefPtr<RILReceiver> receiver = new RILReceiver(wctd);
> >+    nsRefPtr<RILReceiver> receiver = new RILReceiver(wctd);
> 
> Why change this?  (It shouldn't matter, just curious.)

Just make every ref counted, ril related pointer use nsRefPtr.

> >diff --git a/ipc/ril/Ril.cpp b/ipc/ril/Ril.cpp
> 
> >+static nsRefPtr<RilClient> sClient;
> >+static nsRefPtr<RilConsumer> sConsumer;
> 
> These should be |static StaticRefPtr<...>|.

Thanks, but the two lines are removed in following patches though.

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Plan B - Part 3-1: use nsRefPtr instead : v2

Address review comment #11, use StaticRefPtr instead.

Comment 15

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Plan B - Part 3-2: allow multiple RilClient instances

Rebase after the patch uploaded in comment #14.

Comment 16

[Mozilla RelEng Bot]

Try run for 725983e1650f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=725983e1650f
Results (out of 19 total builds):
    exception: 4
    failure: 15
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-725983e1650f

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Mozilla RelEng Bot from comment #16)
> Try run for 725983e1650f is complete.
> Detailed breakdown of the results available here:
>     https://tbpl.mozilla.org/?tree=Try&rev=725983e1650f
> Results (out of 19 total builds):
>     exception: 4
>     failure: 15

I cancelled it. Another new run is in https://tbpl.mozilla.org/?tree=Try&rev=802e645c0eea

Comment 18

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

What's our timeline for this needing to land? It honestly might be easier to move RIL over to the new UnixSocket class at this point, which is made for handling as many socket connections as we need. If this bug is not tracking v1.1, it might be worth it to spend the time to do that. I can take a look at it, or advise someone else on what needs to be done.

Comment 19

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 697425 [details] [diff] [review]
Plan B - Part 1: Github pull request for rilproxy

reviewed in person and merged.

Comment 20

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 697427 [details] [review]
Plan B - Part 2: Github pull request for gonk-misc

Reviewed & merged.

Comment 21

[Mozilla RelEng Bot]

Try run for 725983e1650f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=725983e1650f
Results (out of 20 total builds):
    exception: 4
    warnings: 1
    failure: 15
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-725983e1650f

Comment 22

[Mozilla RelEng Bot]

Try run for 802e645c0eea is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=802e645c0eea
Results (out of 306 total builds):
    exception: 2
    success: 282
    warnings: 20
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-802e645c0eea

Comment 23

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #18)
> What's our timeline for this needing to land? It honestly
> might be easier to move RIL over to the new UnixSocket class
> at this point, which is made for handling as many socket
> connections as we need. If this bug is not tracking
> v1.1, it might be worth it to spend the time to do that. I can
> take a look at it, or advise someone else on what needs to be
> done.

Two things in concern. One, actually we are now working with partners to refactor all RIL related DOM APIs. I mean, the due date for this, multiple rild support, has already passed. We're just stablizing, polishing the internal interface/arch changes, which have been shared with partners for a while, and moving them in m-c. Hopefully we can merge all internal changes at the end of Jan and start to review/merge public interfaces in Feb.

Two, I always want to remove rilproxy, which now exists for only one reason - permission, and that's one of the reasons that we have this Plan B. By removing rilproxy, Gecko will connect to all ril daemons directly. However, rilds create their sockets by invoking an Android specific call - socket_local_server() and I have no idea whether or not UnixSocket can cover all the differences now.

I've filed bug 826931 for this.

Comment 24

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #23)
> Two things in concern. One, actually we are now working with partners to
> refactor all RIL related DOM APIs. I mean, the due date for this, multiple
> rild support, has already passed. We're just stablizing, polishing the
> internal interface/arch changes, which have been shared with partners for a
> while, and moving them in m-c. Hopefully we can merge all internal changes
> at the end of Jan and start to review/merge public interfaces in Feb.

Oh. Ok. I'll go ahead and review this then, and we can punt the move to unixsocket down the line a bit

> Two, I always want to remove rilproxy, which now exists for only one reason
> - permission, and that's one of the reasons that we have this Plan B. By
> removing rilproxy, Gecko will connect to all ril daemons directly. However,
> rilds create their sockets by invoking an Android specific call -
> socket_local_server() and I have no idea whether or not UnixSocket can cover
> all the differences now.

The only thing socket_local_server does is set where the socket is created on the file system, I think? Shouldn't matter.

Comment 25

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 697836 [details] [diff] [review]
Plan B - Part 3-2: allow multiple RilClient instances

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

There's a few issues with XPCOM, but more importantly I just don't understand how we can do things like send only to the first rild we connect to? More comments might help.

::: ipc/ril/Ril.cpp
@@ +80,3 @@
>  };
>  
> +static nsTArray< nsRefPtr<RilClient> > sConnectedClients;

We can't have a static nsTArray, since nsTArray is an XPCOM object. Making it static will trigger allocators at the wrong time. Make this a StaticAutoPtr that is allocated on first usage in StartRIL. Then checkout out the ClearOnShutdown stuff at

http://dxr.allizom.org/mozilla-central/xpcom/base/ClearOnShutdown.h?from=clearonshutdown

To see how to have this autoclear on shutdown without having to setup observers.

@@ +363,5 @@
>  }
>  
>  
>  static void
>  DisconnectFromRil(Monitor* aMonitor)

You don't clear the client out of sConnectedClients here, so if we call disconnect, we won't actually clear the mClient pointer out of the static client array. You'll drop the reference in Ril on disconnect, but it'll still live on here.

@@ +408,5 @@
>  
>      RilRawData *msg = *aMessage;
>      *aMessage = nullptr;
>  
> +    nsRefPtr<RilClient> client = sConnectedClients[0];

So... How does this work?

@@ +419,5 @@
>      return true;
>  }
>  
>  void
>  StopRil()

Shouldn't this take the consumer to stop? IF we're planning on shutting down /all/ clients whenever this is called, you should make a comment about that in the header.

::: ipc/ril/Ril.h
@@ +7,5 @@
>  #ifndef mozilla_ipc_Ril_h
>  #define mozilla_ipc_Ril_h 1
>  
>  #include "mozilla/RefPtr.h"
> +#include "nsAutoPtr.h"

Nit: We may be able to remove the refptr include above this now?

Comment 26

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #25)
> Comment on attachment 697836 [details] [diff] [review]
> Plan B - Part 3-2: allow multiple RilClient instances
-----------------------------------------------------------------
> More comments might help.

Sure. Any change that results in questions during review process represents that it's either not clear enough or completely wrong. Either way, comments or rewrites are required to get it merged.

> ::: ipc/ril/Ril.cpp
> @@ +408,5 @@
> >  
> >      RilRawData *msg = *aMessage;
> >      *aMessage = nullptr;
> >  
> > +    nsRefPtr<RilClient> client = sConnectedClients[0];
> 
> So... How does this work?

It's replaced in part 3-3, which passes an additional aClientId to replace the hard-coded '0' here.

Comment 27

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 697830 [details] [diff] [review]
Plan B - Part 3-3: connect client socket by id

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

I'm not sure I have enough information to properly review this, and it feels more like this is looking for feedback than a full review anyways. How does the RIL Worker know how many clients it will need to start? Right now it's just one since it's hardcoded to zero. I suppose since we wrote it, we can trust it to not collide ids, but since I don't have that information here... 

Honestly, you could probably put parts 3-2 and 3-3 in the same patch. Might make it easier to review.

Comment 28

[Vicamo Yang [:vicamo][:vyang]]

I think I'd better get bug 826931 fixed first to skip ref counting hell in this bug.

Comment 29

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 3: connect client socket by id

Rewrite based on bug 826931.

Comment 30

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 704683 [details] [diff] [review]
Part 3: connect client socket by id

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

::: ipc/ril/Ril.cpp
@@ +168,3 @@
>      , mShutdown(false)
>  {
> +    if (!aClientId) {

This seems confusing. If aClientId is 0, just don't use it? So are ril sockets now called "rild", "rild1", "rild2", etc?

Comment 31

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #30)
> Comment on attachment 704683 [details] [diff] [review]
> Part 3: connect client socket by id
-----------------------------------------------------------------
> This seems confusing. If aClientId is 0, just don't use it? So
> are ril sockets now called "rild", "rild1", "rild2", etc?

Yes, that's for compatibility of both single SIM and multi SIMs. The related parts can also be found in https://github.com/mozilla-b2g/platform_external_qemu/blob/master/vl-android.c#L3741 and https://github.com/mozilla-b2g/platform_hardware_ril/blob/master/rild/rild.c#L200

Comment 32

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 3: connect client socket by id : v2

1) Re-gen patch from hg
2) Connect to different port in desktop debug.

Comment 33

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 3: connect client socket by id : v3

Rebase.

Comment 34

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a7369b9bc9d

Comment 35

[Daniel Holbert [:dholbert]]

Backed out, for Mn & X orange on B2G Arm Opt.

The backout cset was:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/9a7369b9bc9d

Mn Failure logs:
 https://tbpl.mozilla.org/php/getParsedLog.php?id=19378468&tree=Mozilla-Inbound
 https://tbpl.mozilla.org/php/getParsedLog.php?id=19379237&tree=Mozilla-Inbound

X failure logs:
 https://tbpl.mozilla.org/php/getParsedLog.php?id=19378470&tree=Mozilla-Inbound
 https://tbpl.mozilla.org/php/getParsedLog.php?id=19379255&tree=Mozilla-Inbound

Not 100% sure (from a glance) that the Mn orange is related (though it probably is) -- but I'm pretty sure the xpcshell orange is are, because it's in gonk/tests/test_ril_worker_icc.js and the error is "ReferenceError: CLIENT_ID is not defined"  (and this bug is about RIL, and its commit message mentions "client...id")

Comment 36

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #35)
> Not 100% sure (from a glance) that the Mn orange is related

For the record, Mn went green on this push, which was before the backout:
  https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=e649c4c4cb6f

So, it's still unclear whether this bug's patch was responsible for the Mn failure.  Definitely need to sort out the xpcshell issue before this re-lands, though.

Comment 37

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 3: connect client socket by id : v4

Add CLIENT_ID in newWorker()

Comment 38

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #37)
> Created attachment 709629 [details] [diff] [review]

try https://tbpl.mozilla.org/?tree=Try&rev=d095db219a40

Comment 39

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a86feb9c86b

Comment 40

[Dietrich Ayala (:dietrich)]

Blocking-, no longer targeting Leo release. Product will retarget for future release.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6a86feb9c86b

Comment 42

[José Antonio Olivera Ortega [:jaoo]]

Plan B - Part * patches were uplifted to B2g v1-train and the corresponding gecko patch (Part 3) has not been uplifted to gecko-18.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #42)
> Plan B - Part * patches were uplifted to B2g v1-train and the corresponding
> gecko patch (Part 3) has not been uplifted to gecko-18.

I don't see any blocking/tracking/approval status for this bug, so I don't know why you would have thought it would have been...

Comment 44

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #43)
> (In reply to José Antonio Olivera Ortega [:jaoo] from comment #42)
> > Plan B - Part * patches were uplifted to B2g v1-train and the corresponding
> > gecko patch (Part 3) has not been uplifted to gecko-18.
> 
> I don't see any blocking/tracking/approval status for this bug, so I don't
> know why you would have thought it would have been...

Uplifting the gecko part here is not needed as I've figured out. Forget these tow last comments from my side. I'm sorry for the noise.