Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
dexter has uploaded this change for review. ( https://gerrit.osmocom.org/12625
Change subject: assignment_fsm: fix channel allocator preferences
..
assignment_fsm: fix channel allocator preferences
When the MSC allocates a channel through the ASSIGNMENT REQUEST, it may
ask for a TCH/H and a TCH/F at the same time and tell which of the two
types it prefers.
The process of channel allocation currently selects, based on the BTS,
MSC and MS capabilites exactly one apropriate codec/rate (e.g. TCH/H)
and then trys to allocate it. If that allocation fails, there is no way
to try the second choice and the assignment fails.
For example: The MSC asks for TCH/F and TCH/H, prefering TCH/F, then the
channel allocator will try TCH/F and if it fails (all TCH/F are
currently in use), then TCH/H is never tried.
Since the BSC currently only trys the first best codec/rate that is
supported it also ignores the preference.
Lets fix those problems by including the preference information and both
possible codec/rate settings into the channel allocation decision.
Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51
Related: OS#3503
---
M include/osmocom/bsc/codec_pref.h
M include/osmocom/bsc/gsm_data.h
M src/osmo-bsc/abis_rsl.c
M src/osmo-bsc/assignment_fsm.c
M src/osmo-bsc/codec_pref.c
M src/osmo-bsc/handover_fsm.c
M src/osmo-bsc/osmo_bsc_bssap.c
M tests/codec_pref/codec_pref_test.c
8 files changed, 366 insertions(+), 136 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/25/12625/1
diff --git a/include/osmocom/bsc/codec_pref.h b/include/osmocom/bsc/codec_pref.h
index 51340c1..adefe04 100644
--- a/include/osmocom/bsc/codec_pref.h
+++ b/include/osmocom/bsc/codec_pref.h
@@ -9,14 +9,19 @@
struct bts_codec_conf;
struct bsc_msc_data;
struct gsm_bts;
+struct channel_mode_and_rate;
-int match_codec_pref(enum gsm48_chan_mode *chan_mode,
-bool *full_rate,
-uint16_t *s15_s0,
+enum rate_pref {
+ RATE_PREF_NONE,
+ RATE_PREF_HR,
+ RATE_PREF_FR,
+};
+
+int match_codec_pref(struct channel_mode_and_rate *ch_mode_rate,
const struct gsm0808_channel_type *ct,
const struct gsm0808_speech_codec_list *scl,
const struct bsc_msc_data *msc,
-const struct gsm_bts *bts);
+const struct gsm_bts *bts, enum rate_pref rate_pref);
void gen_bss_supported_codec_list(struct gsm0808_speech_codec_list *scl,
const struct bsc_msc_data *msc,
diff --git a/include/osmocom/bsc/gsm_data.h b/include/osmocom/bsc/gsm_data.h
index 22d80df..a161051 100644
--- a/include/osmocom/bsc/gsm_data.h
+++ b/include/osmocom/bsc/gsm_data.h
@@ -99,6 +99,12 @@
SUBSCR_SCCP_ST_CONNECTED
};
+struct channel_mode_and_rate {
+ enum gsm48_chan_mode chan_mode;
+ bool full_rate;
+ uint16_t s15_s0;
+};
+
struct assignment_request {
bool aoip;
@@ -107,9 +113,12 @@
char msc_rtp_addr[INET_ADDRSTRLEN];
uint16_t msc_rtp_port;
- enum gsm48_chan_mode chan_mode;
- bool full_rate;
- uint16_t s15_s0;
+ /* Prefered rate/codec setting (mandatory) */
+ struct channel_mode_and_rate ch_mode_rate_pref;
+
+ /* Alternate rate/codec setting (optional) */
+ bool ch_mode_rate_alt_present;
+ struct channel_mode_and_rate ch_mode_rate_alt;
};
struct assignment_fsm_data {
@@ -282,6 +291,13 @@
/* pointer to "other" connection, if Call Leg Relocation was
successful */
struct gsm_subscriber_connection *other;
} lcls;
+
+ /* Depending on the preferences that where submitted together with
+* the assignment and the current channel load, the BSC has to select
+* one of the offered codec/rates. The final selection by the BSC is
+* stored here and is used when sending the assignment complete or
+* when performing a handover procedure. */
+ struct channel_mode_and_rate ch_mode_rate;
};
diff --git a/src/osmo-bsc/abis_rsl.c b/src/osmo-bsc/abis_rsl.c
index d67573f..c1e6bf0 100644
--- a/src/osmo-bsc/abis_rsl.c
+++ b/src/osmo-bsc/abis_rsl.c
@@ -1375,22 +1375,12 @@
/* check availability / allocate channel
*
* - First try to allocate SDCCH.
-* - If SDCCH is not available, try whatever MS requested, if not SDCCH.
-* - If there is still no channel available, reject channel request.
+* - If SDCCH is not available, try to a TCH/H (less bandwith).
+* - If there is still no channel available, try a TCH/F.
*
-* Note: If the MS requests not TCH/H, we don't know if the phone
-* supports TCH/H, so we must assign TCH/F or SDCCH.
*/
lchan = lchan_select_by_type(bts, GSM_LCHAN_SDCCH);
- if (!lchan && lctype != GSM_LCHAN_SDCCH) {
- LOGP(DR
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Vadim Yanitskiy has posted comments on this change. ( https://gerrit.osmocom.org/12625 ) Change subject: assignment_fsm: fix channel allocator preferences .. Patch Set 1: (2 comments) https://gerrit.osmocom.org/#/c/12625/1/src/osmo-bsc/abis_rsl.c File src/osmo-bsc/abis_rsl.c: https://gerrit.osmocom.org/#/c/12625/1/src/osmo-bsc/abis_rsl.c@1378 PS1, Line 1378: to Spelling: 'to' is not needed here I think. https://gerrit.osmocom.org/#/c/12625/1/src/osmo-bsc/assignment_fsm.c File src/osmo-bsc/assignment_fsm.c: https://gerrit.osmocom.org/#/c/12625/1/src/osmo-bsc/assignment_fsm.c@258 PS1, Line 258: int Let's use bool. -- To view, visit https://gerrit.osmocom.org/12625 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Gerrit-Change-Number: 12625 Gerrit-PatchSet: 1 Gerrit-Owner: dexter Gerrit-Reviewer: Jenkins Builder (102) Gerrit-CC: Vadim Yanitskiy Gerrit-Comment-Date: Fri, 18 Jan 2019 05:43:20 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Hello Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/12625 to look at the new patch set (#2). Change subject: assignment_fsm: fix channel allocator preferences .. assignment_fsm: fix channel allocator preferences When the MSC allocates a channel through the ASSIGNMENT REQUEST, it may ask for a TCH/H and a TCH/F at the same time and tell which of the two types it prefers. The process of channel allocation currently selects, based on the BTS, MSC and MS capabilites exactly one apropriate codec/rate (e.g. TCH/H) and then trys to allocate it. If that allocation fails, there is no way to try the second choice and the assignment fails. For example: The MSC asks for TCH/F and TCH/H, prefering TCH/F, then the channel allocator will try TCH/F and if it fails (all TCH/F are currently in use), then TCH/H is never tried. Since the BSC currently only trys the first best codec/rate that is supported it also ignores the preference. Lets fix those problems by including the preference information and both possible codec/rate settings into the channel allocation decision. Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Related: OS#3503 --- M include/osmocom/bsc/codec_pref.h M include/osmocom/bsc/gsm_data.h M src/osmo-bsc/abis_rsl.c M src/osmo-bsc/assignment_fsm.c M src/osmo-bsc/codec_pref.c M src/osmo-bsc/handover_fsm.c M src/osmo-bsc/osmo_bsc_bssap.c M tests/codec_pref/codec_pref_test.c 8 files changed, 366 insertions(+), 136 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/25/12625/2 -- To view, visit https://gerrit.osmocom.org/12625 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Gerrit-Change-Number: 12625 Gerrit-PatchSet: 2 Gerrit-Owner: dexter Gerrit-Reviewer: Jenkins Builder (102) Gerrit-CC: Vadim Yanitskiy
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Harald Welte has posted comments on this change. ( https://gerrit.osmocom.org/12625 ) Change subject: assignment_fsm: fix channel allocator preferences .. Patch Set 2: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/12625 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Gerrit-Change-Number: 12625 Gerrit-PatchSet: 2 Gerrit-Owner: dexter Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-CC: Vadim Yanitskiy Gerrit-Comment-Date: Fri, 18 Jan 2019 10:52:53 + Gerrit-HasComments: No Gerrit-HasLabels: Yes
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/12625 ) Change subject: assignment_fsm: fix channel allocator preferences .. Patch Set 2: @laforge, might be a bit quick to vote +2: - there are remarks by Vadim, - the patch is obviously not trivial -- To view, visit https://gerrit.osmocom.org/12625 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Gerrit-Change-Number: 12625 Gerrit-PatchSet: 2 Gerrit-Owner: dexter Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-CC: Neels Hofmeyr Gerrit-CC: Vadim Yanitskiy Gerrit-Comment-Date: Mon, 21 Jan 2019 13:27:49 + Gerrit-HasComments: No Gerrit-HasLabels: No
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Neels Hofmeyr has removed a vote on this change. Change subject: assignment_fsm: fix channel allocator preferences .. Removed Code-Review+2 by Harald Welte -- To view, visit https://gerrit.osmocom.org/12625 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Gerrit-Change-Number: 12625 Gerrit-PatchSet: 2 Gerrit-Owner: dexter Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-CC: Neels Hofmeyr Gerrit-CC: Vadim Yanitskiy
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/12625 ) Change subject: assignment_fsm: fix channel allocator preferences .. Patch Set 2: I removed the +2 vote to prevent merging before I can read the patch -- To view, visit https://gerrit.osmocom.org/12625 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Gerrit-Change-Number: 12625 Gerrit-PatchSet: 2 Gerrit-Owner: dexter Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-CC: Neels Hofmeyr Gerrit-CC: Vadim Yanitskiy Gerrit-Comment-Date: Mon, 21 Jan 2019 13:29:34 + Gerrit-HasComments: No Gerrit-HasLabels: No
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Neels Hofmeyr has posted comments on this change. (
https://gerrit.osmocom.org/12625 )
Change subject: assignment_fsm: fix channel allocator preferences
..
Patch Set 2: Code-Review-1
(2 comments)
(let me first post this and follow up with review on the remaining patch later)
https://gerrit.osmocom.org/#/c/12625/2/include/osmocom/bsc/gsm_data.h
File include/osmocom/bsc/gsm_data.h:
https://gerrit.osmocom.org/#/c/12625/2/include/osmocom/bsc/gsm_data.h@121
PS2, Line 121: struct channel_mode_and_rate ch_mode_rate_alt;
Are there really only two possible mode,rate combinations?
https://gerrit.osmocom.org/#/c/12625/2/src/osmo-bsc/abis_rsl.c
File src/osmo-bsc/abis_rsl.c:
https://gerrit.osmocom.org/#/c/12625/2/src/osmo-bsc/abis_rsl.c@1391
PS2, Line 1391: if (!lchan) {
These changes above make up a separate fix by themselves. I personally
understand this change because I explained the mistake I made here to you, but
the reason definitely should appear in the commit log.
Let's put this in a separate patch and explain like this:
Fix TCH-as-SDCCH allocation on Channel Request
On rsl_rx_chan_rqd(), so far osmo-bsc tried to preferably assign the lchan type
that was asked for in the RACH. Firstly, this contained a bug, and secondly,
it does not make sense to heed that preference, since we do late assignment.
Ignore the preference for the MS' TCH kind.
We do late assignment to avoid codec mismatches. In the "old days", we would
heed the MS' TCH channel kind, even if the MSC or BSC didn't actually allow
or prefer that channel kind. Hence, in the presence of both TCH/F and TCH/H,
the MS could ask for TCH/F (which we would grant on the MO side) and the BSC
or MSC could prefer TCH/H (which we would apply on the MT side), and hence
fabricate a codec mismatch. Instead, since quite some time now, we *always*
assign an SDCCH first, and only later on do another Assignment to hand out
a proper voice lchan that heeds the MS capability bits as well as MSC's and
BSC's preferences.
By completely ignoring the channel kind the MS asked for in the RACH, we
also eliminate this bug in rsl_rx_chan_rqd():
- If the first "lchan_select_by_type(GSM_LCHAN_SDDCH)" fails (all SDDCH in use),
we should try to fall back to any TCH instead, to serve as SDCCH.
- the first "if (!lchan && lctype != GSM_LCHAN_SDCCH)" was an attempt to prefer
a fallback to the lchan type the MS requested.
- the remaining two "if (!lchan && lctype == GSM_LCHAN_SDCCH)" were obviously
only applied if the MS asked for an SDCCH, and skipped if the type was TCH/*.
- hence, missing was: if the MS asked for a TCH, to also try the *other* TCH
kind that the MS did not ask for. (Example: all SDCCH in use, MS asks for
TCH/F, but BSC has only TCH/H lchans; we should assign TCH/H as SDCCH, instead
we said "no resources")
--
To view, visit https://gerrit.osmocom.org/12625
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51
Gerrit-Change-Number: 12625
Gerrit-PatchSet: 2
Gerrit-Owner: dexter
Gerrit-Reviewer: Harald Welte
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Neels Hofmeyr
Gerrit-CC: Vadim Yanitskiy
Gerrit-Comment-Date: Mon, 21 Jan 2019 13:52:44 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/12625 ) Change subject: assignment_fsm: fix channel allocator preferences .. Patch Set 2: (4 comments) https://gerrit.osmocom.org/#/c/12625/2/include/osmocom/bsc/codec_pref.h File include/osmocom/bsc/codec_pref.h: https://gerrit.osmocom.org/#/c/12625/2/include/osmocom/bsc/codec_pref.h@24 PS2, Line 24:const struct gsm_bts *bts, enum rate_pref rate_pref); in ch_mode_rate, we add a 'bool full_rate', and we also add a 'rate_pref'? That seems redundant to me / more complex for error checking, because I could technically pass ch_mode_rate->full_rate = false and rate_pref = RATE_PREF_HR. would it make more sense to add an enum rate_pref to channel_mode_and_rate, instead of the bool? Or instead of enum rate_pref, add a "bool strict_rate_pref", to say whether the ch_mode_rate->full_rate flag is important or can be overruled? https://gerrit.osmocom.org/#/c/12625/2/src/osmo-bsc/assignment_fsm.c File src/osmo-bsc/assignment_fsm.c: https://gerrit.osmocom.org/#/c/12625/2/src/osmo-bsc/assignment_fsm.c@255 PS2, Line 255: struct channel_mode_and_rate *ch_mode_rate) const struct ... https://gerrit.osmocom.org/#/c/12625/2/src/osmo-bsc/assignment_fsm.c@305 PS2, Line 305: static int check_voice_stream_needed(struct gsm_subscriber_connection *conn, struct osmo_fsm_inst *fi) (we already have a flag around called "requires_voice_stream", so prefer if this is called "check_requires_voice_stream()") https://gerrit.osmocom.org/#/c/12625/2/src/osmo-bsc/assignment_fsm.c@324 PS2, Line 324: result_pref = false; don't understand the name 'result_pref' ... from surrounding code I guess 'requires_voice' is better? -- To view, visit https://gerrit.osmocom.org/12625 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Gerrit-Change-Number: 12625 Gerrit-PatchSet: 2 Gerrit-Owner: dexter Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Neels Hofmeyr Gerrit-CC: Vadim Yanitskiy Gerrit-Comment-Date: Mon, 21 Jan 2019 18:17:40 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Hello Neels Hofmeyr, Harald Welte, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/12625 to look at the new patch set (#3). Change subject: assignment_fsm: fix channel allocator preferences .. assignment_fsm: fix channel allocator preferences When the MSC allocates a channel through the ASSIGNMENT REQUEST, it may ask for a TCH/H and a TCH/F at the same time and tell which of the two types it prefers. The process of channel allocation currently selects, based on the BTS, MSC and MS capabilites exactly one apropriate codec/rate (e.g. TCH/H) and then trys to allocate it. If that allocation fails, there is no way to try the second choice and the assignment fails. For example: The MSC asks for TCH/F and TCH/H, prefering TCH/F, then the channel allocator will try TCH/F and if it fails (all TCH/F are currently in use), then TCH/H is never tried. Since the BSC currently only trys the first best codec/rate that is supported it also ignores the preference. Lets fix those problems by including the preference information and both possible codec/rate settings into the channel allocation decision. Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Related: OS#3503 --- M include/osmocom/bsc/codec_pref.h M include/osmocom/bsc/gsm_data.h M src/osmo-bsc/assignment_fsm.c M src/osmo-bsc/codec_pref.c M src/osmo-bsc/handover_fsm.c M src/osmo-bsc/osmo_bsc_bssap.c M tests/codec_pref/codec_pref_test.c 7 files changed, 363 insertions(+), 123 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/25/12625/3 -- To view, visit https://gerrit.osmocom.org/12625 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Gerrit-Change-Number: 12625 Gerrit-PatchSet: 3 Gerrit-Owner: dexter Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Neels Hofmeyr Gerrit-CC: Vadim Yanitskiy
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/12625 ) Change subject: assignment_fsm: fix channel allocator preferences .. Patch Set 4: (2 comments) https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c File src/osmo-bsc/assignment_fsm.c: https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@a351 PS4, Line 351: noticing a bug in this code path: it is leaving the assignment fi allocated. maybe we can... https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@495 PS4, Line 495: if (check_for_existing_lchan(conn)) ...maybe we can deallocate the assignment.fi in this if condition's body -- To view, visit https://gerrit.osmocom.org/12625 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Gerrit-Change-Number: 12625 Gerrit-PatchSet: 4 Gerrit-Owner: dexter Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Neels Hofmeyr Gerrit-CC: Vadim Yanitskiy Gerrit-Comment-Date: Tue, 22 Jan 2019 13:47:31 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/12625 ) Change subject: assignment_fsm: fix channel allocator preferences .. Patch Set 4: Code-Review-1 (1 comment) https://gerrit.osmocom.org/#/c/12625/4/include/osmocom/bsc/gsm_data.h File include/osmocom/bsc/gsm_data.h: https://gerrit.osmocom.org/#/c/12625/4/include/osmocom/bsc/gsm_data.h@121 PS4, Line 121: struct channel_mode_and_rate ch_mode_rate_alt; Are there really only two possible mode,rate combinations? In my idea for this, we would call lchan_select() from that loop that iterates all possible mode,rate combinations. It seems odd to just collect two of the possible combinations and try to pick those only later. Maybe there's something I'm not seeing, might be good to look at it together, in person. -- To view, visit https://gerrit.osmocom.org/12625 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Gerrit-Change-Number: 12625 Gerrit-PatchSet: 4 Gerrit-Owner: dexter Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Neels Hofmeyr Gerrit-CC: Vadim Yanitskiy Gerrit-Comment-Date: Tue, 22 Jan 2019 13:50:35 + Gerrit-HasComments: Yes Gerrit-HasLabels: Yes
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/12625 ) Change subject: assignment_fsm: fix channel allocator preferences .. Patch Set 4: Code-Review+1 (3 comments) https://gerrit.osmocom.org/#/c/12625/2/include/osmocom/bsc/codec_pref.h File include/osmocom/bsc/codec_pref.h: https://gerrit.osmocom.org/#/c/12625/2/include/osmocom/bsc/codec_pref.h@24 PS2, Line 24:const struct gsm_bts *bts, enum rate_pref rate_pref); > in ch_mode_rate, we add a 'bool full_rate', and we also add a 'rate_pref'? […] Also talked about this in person, and obviously ch_mode_rate is an out-argument, while the rate_pref is an input argument. I got confused about that because the function internals looked as if they were using ch_mode_rate->full rate as input, and overlooked that before using ch_mode_rate->full_rate, that value is already set, and it is in fact not an in-arg. https://gerrit.osmocom.org/#/c/12625/4/include/osmocom/bsc/gsm_data.h File include/osmocom/bsc/gsm_data.h: https://gerrit.osmocom.org/#/c/12625/4/include/osmocom/bsc/gsm_data.h@121 PS4, Line 121: struct channel_mode_and_rate ch_mode_rate_alt; > Are there really only two possible mode,rate combinations? […] We talked in person, and there really are only two possible outcomes: one for an FR and one for an HR lchan. So keeping exactly two preferences is perfectly correct. Keeping the lchan_select() out of the codec iteration is also a good idea in terms of complexity of that loop, and in terms of separate testability. https://gerrit.osmocom.org/#/c/12625/4/include/osmocom/bsc/gsm_data.h@300 PS4, Line 300: struct channel_mode_and_rate ch_mode_rate; put this in the user_plane scope? -- To view, visit https://gerrit.osmocom.org/12625 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Gerrit-Change-Number: 12625 Gerrit-PatchSet: 4 Gerrit-Owner: dexter Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Neels Hofmeyr Gerrit-CC: Vadim Yanitskiy Gerrit-Comment-Date: Thu, 24 Jan 2019 15:46:33 + Gerrit-HasComments: Yes Gerrit-HasLabels: Yes
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Neels Hofmeyr has posted comments on this change. (
https://gerrit.osmocom.org/12625 )
Change subject: assignment_fsm: fix channel allocator preferences
..
Patch Set 4: Code-Review-1
(11 comments)
my earlier higher level reservations are resolved, but now I did find low level
problems:
https://gerrit.osmocom.org/#/c/12625/4/include/osmocom/bsc/gsm_data.h
File include/osmocom/bsc/gsm_data.h:
https://gerrit.osmocom.org/#/c/12625/4/include/osmocom/bsc/gsm_data.h@300
PS4, Line 300: struct channel_mode_and_rate ch_mode_rate;
> put this in the user_plane scope?
this ^ should probably go inside the assignment{} scope instead, see other
comments
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c
File src/osmo-bsc/assignment_fsm.c:
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@304
PS4, Line 304: static int check_requires_voice_stream(struct
gsm_subscriber_connection *conn)
-1: code dup: please write one function that takes a chan_mode as input and
returns a requires_voice_stream bool, and call this function twice; once for
the primary chan_mode and once for the alternate chan_mode.
-1: actually, the requires_voice_stream must be identical between first and
alternate preference, right?
-1: it doesn't make sense to fail the assignment if only one of the choices is
not supported.
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@377
PS4, Line 377: if (lchan_type_compat_with_mode(conn->lchan->type,
&req->ch_mode_rate_pref)) {
-1: code dup: please implement this only once in a utility function, and call
it twice: once for the primary choice, and if that failed then for the alt_pref.
Below, I expect it to look something like this:
if (check_for_existing_lchan(conn, &req->ch_mode_rate_pref)
|| check_for_existing_lchan(conn, &req->ch_mode_rate_alt)) {
/* re-using existing lchan succeeded. Discard FSM instance, it is not
needed. */
osmo_fsm_inst_term(conn->assignment.fi);
return;
}
check_new_lchan(conn, &req->ch_mode_rate_pref);
if (!conn->lchan)
check_new_lchan(conn, &req->ch_mode_rate_alt);
if (!conn->lchan) {
assignment_fail(No lchan available...);
return;
}
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@443
PS4, Line 443: if (!conn->assignment.new_lchan &&
req->ch_mode_rate_alt_present) {
-1: code dup (same)
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@479
PS4, Line 479: fi = osmo_fsm_inst_alloc_child(&assignment_fsm, conn->fi,
GSCON_EV_ASSIGNMENT_END);
(FYI, I considered whether it is better to allocate the FSM instance only
*after* we checked whether the current lchan is sufficient, but IMHO it makes
more sense to allocate it and use it as logging context. Also this makes sure
the ASSIGNMENT_END signal gets dispatched to the conn FSM, which we apparently
also forget to do in the re-use case at the moment. Also FYI, the channel
re-use code path is completely broken for many other reasons, see
https://gerrit.osmocom.org/c/osmo-bsc/+/12401 )
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@495
PS4, Line 495: if (check_for_existing_lchan(conn))
> ...maybe we can deallocate the assignment. […]
^ still valid, but don't worry about this one, see
https://gerrit.osmocom.org/c/osmo-bsc/+/12401
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@501
PS4, Line 501: conn->ch_mode_rate = req->ch_mode_rate_pref;
-1: the ch_mode_rate use must not be ambiguous: does it apply to the current
conn->lchan or to the new assignment.new_lchan?
The point is that if the assignment fails for whatever reason, you must be able
to just discard the assignment information and keep on using the previous lchan
without having modified any other state of the conn.
So, instead of modifying the conn->ch_mode_rate, you must stay inside the
conn->assignment.* scope.
Only when the new lchan gets accepted and moved from conn->assignment.new_lchan
to conn->lchan is when the conn->ch_mode_rate is allowed to be modified.
Maybe it makes more sense to place ch_mode_rate into the lchan struct instead?
Or if it is only used during assignment negotiation, put it in the
conn->assignment{} scope.
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@503
PS4, Line 503: /* In case the lchan allocation fails, we try with the alternat
("alternate". use line width.)
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/osmo_bsc_bssap.c
File src/osmo-bsc/osmo_bsc_bssap.c:
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/osmo_bsc_bssap.c@609
PS4, Line 609: static int select_codec(struct assignment_request *req, struct
gsm0808_channel_type *ct,
"select_codecs" ?
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/osmo_bsc_bssap.c@632
PS4, Line 632:RATE_PREF_NONE);
(if fu
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Neels Hofmeyr has posted comments on this change. (
https://gerrit.osmocom.org/12625 )
Change subject: assignment_fsm: fix channel allocator preferences
..
Patch Set 4:
(1 comment)
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c
File src/osmo-bsc/assignment_fsm.c:
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@377
PS4, Line 377: if (lchan_type_compat_with_mode(conn->lchan->type,
&req->ch_mode_rate_pref)) {
> -1: code dup: please implement this only once in a utility function, and call
> it twice: once for the […]
mistake in my comment: should be
if (!conn->assignment.new_lchan)
in both places
--
To view, visit https://gerrit.osmocom.org/12625
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51
Gerrit-Change-Number: 12625
Gerrit-PatchSet: 4
Gerrit-Owner: dexter
Gerrit-Reviewer: Harald Welte
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Max
Gerrit-Reviewer: Neels Hofmeyr
Gerrit-CC: Vadim Yanitskiy
Gerrit-Comment-Date: Thu, 24 Jan 2019 16:58:42 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Max has posted comments on this change. ( https://gerrit.osmocom.org/12625 ) Change subject: assignment_fsm: fix channel allocator preferences .. Patch Set 4: Code-Review-1 (1 comment) https://gerrit.osmocom.org/#/c/12625/4//COMMIT_MSG Commit Message: https://gerrit.osmocom.org/#/c/12625/4//COMMIT_MSG@22 PS4, Line 22: Since the BSC currently only trys the first best codec/rate that is I think it's "tries". -- To view, visit https://gerrit.osmocom.org/12625 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Gerrit-Change-Number: 12625 Gerrit-PatchSet: 4 Gerrit-Owner: dexter Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-CC: Vadim Yanitskiy Gerrit-Comment-Date: Thu, 24 Jan 2019 16:57:04 + Gerrit-HasComments: Yes Gerrit-HasLabels: Yes
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Hello Max, Neels Hofmeyr, Harald Welte, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/12625 to look at the new patch set (#5). Change subject: assignment_fsm: fix channel allocator preferences .. assignment_fsm: fix channel allocator preferences When the MSC allocates a channel through the ASSIGNMENT REQUEST, it may ask for a TCH/H and a TCH/F at the same time and tell which of the two types it prefers. The process of channel allocation currently selects, based on the BTS, MSC and MS capabilites exactly one apropriate codec/rate (e.g. TCH/H) and then tries to allocate it. If that allocation fails, there is no way to try the second choice and the assignment fails. For example: The MSC asks for TCH/F and TCH/H, prefering TCH/F, then the channel allocator will try TCH/F and if it fails (all TCH/F are currently in use), then TCH/H is never tried. Since the BSC currently only trys the first best codec/rate that is supported it also ignores the preference. Lets fix those problems by including the preference information and both possible codec/rate settings into the channel allocation decision. Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Related: OS#3503 --- M include/osmocom/bsc/codec_pref.h M include/osmocom/bsc/gsm_data.h M src/osmo-bsc/assignment_fsm.c M src/osmo-bsc/codec_pref.c M src/osmo-bsc/handover_fsm.c M src/osmo-bsc/osmo_bsc_bssap.c M tests/codec_pref/codec_pref_test.c 7 files changed, 366 insertions(+), 119 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/25/12625/5 -- To view, visit https://gerrit.osmocom.org/12625 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Gerrit-Change-Number: 12625 Gerrit-PatchSet: 5 Gerrit-Owner: dexter Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-CC: Vadim Yanitskiy
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Neels Hofmeyr has posted comments on this change. (
https://gerrit.osmocom.org/12625 )
Change subject: assignment_fsm: fix channel allocator preferences
..
Patch Set 5: Code-Review-1
(3 comments)
https://gerrit.osmocom.org/#/c/12625/5/src/osmo-bsc/assignment_fsm.c
File src/osmo-bsc/assignment_fsm.c:
https://gerrit.osmocom.org/#/c/12625/5/src/osmo-bsc/assignment_fsm.c@340
PS5, Line 340: return -EINVAL;
before, I said "it doesn't make sense to fail the assignment if only one of the
choices is not supported", but now I'm revising that: it is unlikely to get a
GSM48_CMODE that we can't classify, so this is fine.
https://gerrit.osmocom.org/#/c/12625/5/src/osmo-bsc/assignment_fsm.c@497
PS5, Line 497: if (check_for_existing_lchan(conn)) {
instead of spraying CR comments I pushed a suggestion instead. Can you please
take a look whether that is acceptable to you, and actually test it for me?
thanks!
My changes to this patch set are a separate commit on branch neels/12625
http://git.osmocom.org/osmo-bsc/commit/?h=neels/12625
(If they are ok then squash into this one)
The reasons:
- code dup
- missing counter
- missing send_assignment_complete() error handling
- the req struct is coming in during the assignment_start() and should then
remain unchanged, i.e. moved req->chan_mode_rate to conn.assignment
https://gerrit.osmocom.org/#/c/12625/5/src/osmo-bsc/handover_fsm.c
File src/osmo-bsc/handover_fsm.c:
https://gerrit.osmocom.org/#/c/12625/5/src/osmo-bsc/handover_fsm.c@716
PS5, Line 716: sc.cfg =
conn->assignment.req.ch_mode_rate.s15_s0;
Noticing this problem that exists before this patch:
This is reaching into the wrong place. The conn->assignment is only valid
*during* an assignment. If you need the ch_mode_rate after assignment is done,
it has to be copied out of conn->assignment.
The idea is:
- assignment_req data represents the request coming in. the request can be
accepted or denied. If it is accepted, pass req to assignment_start(). From now
on req is cast in stone and is only the input data.
- during assignment, keep all state in and only in conn->assignment.*, leave
conn->lchan and other conn->* unchanged.
- when assignment was successful, copy all relevant data to conn->lchan or
conn->foo, so that now conn->assignment is unused.
- so that if we start another assignment (which might fail) we do not affect
the conn->lchan state.
- if assignment fails, leave conn->* unchanged, hence just continue as if no
assignment happened at all.
I am submitting a patch that puts the s15_s0 bits into struct gsm_lchan after
activation, instead:
https://gerrit.osmocom.org/#/c/osmo-bsc/+/12734
--
To view, visit https://gerrit.osmocom.org/12625
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51
Gerrit-Change-Number: 12625
Gerrit-PatchSet: 5
Gerrit-Owner: dexter
Gerrit-Assignee: Neels Hofmeyr
Gerrit-Reviewer: Harald Welte
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Max
Gerrit-Reviewer: Neels Hofmeyr
Gerrit-CC: Vadim Yanitskiy
Gerrit-Comment-Date: Wed, 30 Jan 2019 16:02:07 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Hello Max, Neels Hofmeyr, Harald Welte, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/12625 to look at the new patch set (#6). Change subject: assignment_fsm: fix channel allocator preferences .. assignment_fsm: fix channel allocator preferences When the MSC allocates a channel through the ASSIGNMENT REQUEST, it may ask for a TCH/H and a TCH/F at the same time and tell which of the two types it prefers. The process of channel allocation currently selects, based on the BTS, MSC and MS capabilites exactly one apropriate codec/rate (e.g. TCH/H) and then tries to allocate it. If that allocation fails, there is no way to try the second choice and the assignment fails. For example: The MSC asks for TCH/F and TCH/H, prefering TCH/F, then the channel allocator will try TCH/F and if it fails (all TCH/F are currently in use), then TCH/H is never tried. Since the BSC currently only trys the first best codec/rate that is supported it also ignores the preference. Lets fix those problems by including the preference information and both possible codec/rate settings into the channel allocation decision. Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Related: OS#3503 --- M include/osmocom/bsc/codec_pref.h M include/osmocom/bsc/gsm_data.h M src/osmo-bsc/assignment_fsm.c M src/osmo-bsc/codec_pref.c M src/osmo-bsc/handover_fsm.c M src/osmo-bsc/osmo_bsc_bssap.c M tests/codec_pref/codec_pref_test.c 7 files changed, 316 insertions(+), 113 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/25/12625/6 -- To view, visit https://gerrit.osmocom.org/12625 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Gerrit-Change-Number: 12625 Gerrit-PatchSet: 6 Gerrit-Owner: dexter Gerrit-Assignee: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-CC: Vadim Yanitskiy
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Hello Max, Neels Hofmeyr, Harald Welte, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/12625 to look at the new patch set (#7). Change subject: assignment_fsm: fix channel allocator preferences .. assignment_fsm: fix channel allocator preferences When the MSC allocates a channel through the ASSIGNMENT REQUEST, it may ask for a TCH/H and a TCH/F at the same time and tell which of the two types it prefers. The process of channel allocation currently selects, based on the BTS, MSC and MS capabilites exactly one apropriate codec/rate (e.g. TCH/H) and then tries to allocate it. If that allocation fails, there is no way to try the second choice and the assignment fails. For example: The MSC asks for TCH/F and TCH/H, prefering TCH/F, then the channel allocator will try TCH/F and if it fails (all TCH/F are currently in use), then TCH/H is never tried. Since the BSC currently only trys the first best codec/rate that is supported it also ignores the preference. Lets fix those problems by including the preference information and both possible codec/rate settings into the channel allocation decision. Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Related: OS#3503 --- M include/osmocom/bsc/codec_pref.h M include/osmocom/bsc/gsm_data.h M src/osmo-bsc/assignment_fsm.c M src/osmo-bsc/bsc_vty.c M src/osmo-bsc/codec_pref.c M src/osmo-bsc/handover_fsm.c M src/osmo-bsc/osmo_bsc_bssap.c M tests/codec_pref/codec_pref_test.c 8 files changed, 321 insertions(+), 117 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/25/12625/7 -- To view, visit https://gerrit.osmocom.org/12625 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Gerrit-Change-Number: 12625 Gerrit-PatchSet: 7 Gerrit-Owner: dexter Gerrit-Assignee: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-CC: Vadim Yanitskiy
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Hello Max, Neels Hofmeyr, Harald Welte, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/12625 to look at the new patch set (#8). Change subject: assignment_fsm: fix channel allocator preferences .. assignment_fsm: fix channel allocator preferences When the MSC allocates a channel through the ASSIGNMENT REQUEST, it may ask for a TCH/H and a TCH/F at the same time and tell which of the two types it prefers. The process of channel allocation currently selects, based on the BTS, MSC and MS capabilites exactly one apropriate codec/rate (e.g. TCH/H) and then tries to allocate it. If that allocation fails, there is no way to try the second choice and the assignment fails. For example: The MSC asks for TCH/F and TCH/H, prefering TCH/F, then the channel allocator will try TCH/F and if it fails (all TCH/F are currently in use), then TCH/H is never tried. Since the BSC currently only trys the first best codec/rate that is supported it also ignores the preference. Lets fix those problems by including the preference information and both possible codec/rate settings into the channel allocation decision. Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Related: OS#3503 --- M include/osmocom/bsc/codec_pref.h M include/osmocom/bsc/gsm_data.h M src/osmo-bsc/assignment_fsm.c M src/osmo-bsc/bsc_vty.c M src/osmo-bsc/codec_pref.c M src/osmo-bsc/handover_fsm.c M src/osmo-bsc/osmo_bsc_bssap.c M tests/codec_pref/codec_pref_test.c 8 files changed, 321 insertions(+), 117 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/25/12625/8 -- To view, visit https://gerrit.osmocom.org/12625 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Gerrit-Change-Number: 12625 Gerrit-PatchSet: 8 Gerrit-Owner: dexter Gerrit-Assignee: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-CC: Vadim Yanitskiy
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Harald Welte has posted comments on this change. ( https://gerrit.osmocom.org/12625 ) Change subject: assignment_fsm: fix channel allocator preferences .. Patch Set 8: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/12625 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Gerrit-Change-Number: 12625 Gerrit-PatchSet: 8 Gerrit-Owner: dexter Gerrit-Assignee: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-CC: Vadim Yanitskiy Gerrit-Comment-Date: Thu, 07 Feb 2019 10:16:31 + Gerrit-HasComments: No Gerrit-HasLabels: Yes
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
tnt has posted comments on this change. ( https://gerrit.osmocom.org/12625 )
Change subject: assignment_fsm: fix channel allocator preferences
..
Patch Set 8:
(2 comments)
https://gerrit.osmocom.org/#/c/12625/8/src/osmo-bsc/osmo_bsc_bssap.c
File src/osmo-bsc/osmo_bsc_bssap.c:
https://gerrit.osmocom.org/#/c/12625/8/src/osmo-bsc/osmo_bsc_bssap.c@635
PS8, Line 635: switch (ct->ch_rate_type & GSM0808_SPEECH_PERM) {
Wait what ? GSM0808_SPEECH_PERM is not a mask, it's an actual possible value of
the field defined in GSM 08.08 3.2.2.11.
"""
Full or Half rate TCH channel. Preference between the permitted speech versions
as indicated in
octet 5, 5a etc., changes between full and half rate allowed also after first
channel allocation as a result of the request
"""
https://gerrit.osmocom.org/#/c/12625/8/src/osmo-bsc/osmo_bsc_bssap.c@637
PS8, Line 637: rc = match_codec_pref(&req->ch_mode_rate_pref, ct,
&conn->codec_list, msc, conn_get_bts(conn),
Given this is used for speech, I'd use the GSM0808_SPEECH_xxx constants. (I
know, they have the same values but still).
--
To view, visit https://gerrit.osmocom.org/12625
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51
Gerrit-Change-Number: 12625
Gerrit-PatchSet: 8
Gerrit-Owner: dexter
Gerrit-Assignee: Neels Hofmeyr
Gerrit-Reviewer: Harald Welte
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Max
Gerrit-Reviewer: Neels Hofmeyr
Gerrit-CC: Vadim Yanitskiy
Gerrit-CC: tnt
Gerrit-Comment-Date: Thu, 07 Feb 2019 10:32:41 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
tnt has posted comments on this change. ( https://gerrit.osmocom.org/12625 ) Change subject: assignment_fsm: fix channel allocator preferences .. Patch Set 8: (1 comment) https://gerrit.osmocom.org/#/c/12625/8/include/osmocom/bsc/gsm_data.h File include/osmocom/bsc/gsm_data.h: https://gerrit.osmocom.org/#/c/12625/8/include/osmocom/bsc/gsm_data.h@104 PS8, Line 104: bool full_rate; Just wondering if it wouldn't be worth directly using GSM_LCHAN_xxx constant in there rather than full_rate flag. Because when eventually getting to 3784 we will need to express SDCCH/TCH_HS/TCH_FS preferences. Also about ch_mode_rate_pref, for the same reason it might be worth making it an array with a int indicating the # of items because there is 3 possible alternative to specify there. Just my 2ct. Ping @hwelte -- To view, visit https://gerrit.osmocom.org/12625 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Gerrit-Change-Number: 12625 Gerrit-PatchSet: 8 Gerrit-Owner: dexter Gerrit-Assignee: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-CC: Vadim Yanitskiy Gerrit-CC: tnt Gerrit-Comment-Date: Thu, 07 Feb 2019 10:49:16 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Neels Hofmeyr has removed a vote on this change. Change subject: assignment_fsm: fix channel allocator preferences .. Removed Code-Review+2 by Harald Welte -- To view, visit https://gerrit.osmocom.org/12625 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Gerrit-Change-Number: 12625 Gerrit-PatchSet: 8 Gerrit-Owner: dexter Gerrit-Assignee: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-CC: Vadim Yanitskiy Gerrit-CC: tnt
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Neels Hofmeyr has posted comments on this change. (
https://gerrit.osmocom.org/12625 )
Change subject: assignment_fsm: fix channel allocator preferences
..
Patch Set 8: Code-Review-1
(2 comments)
developing a habit of removing +2 votes ... to make sure the comments are
looked at first.
https://gerrit.osmocom.org/#/c/12625/8/src/osmo-bsc/osmo_bsc_bssap.c
File src/osmo-bsc/osmo_bsc_bssap.c:
https://gerrit.osmocom.org/#/c/12625/8/src/osmo-bsc/osmo_bsc_bssap.c@635
PS8, Line 635: switch (ct->ch_rate_type & GSM0808_SPEECH_PERM) {
> Wait what ? GSM0808_SPEECH_PERM is not a mask, it's an actual possible value
> of the field defined in […]
do you really need the mask at all?
https://gerrit.osmocom.org/#/c/12625/8/src/osmo-bsc/osmo_bsc_bssap.c@637
PS8, Line 637: rc = match_codec_pref(&req->ch_mode_rate_pref, ct,
&conn->codec_list, msc, conn_get_bts(conn),
> Given this is used for speech, I'd use the GSM0808_SPEECH_xxx constants. […]
agree
--
To view, visit https://gerrit.osmocom.org/12625
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51
Gerrit-Change-Number: 12625
Gerrit-PatchSet: 8
Gerrit-Owner: dexter
Gerrit-Assignee: Neels Hofmeyr
Gerrit-Reviewer: Harald Welte
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Max
Gerrit-Reviewer: Neels Hofmeyr
Gerrit-CC: Vadim Yanitskiy
Gerrit-CC: tnt
Gerrit-Comment-Date: Thu, 07 Feb 2019 15:56:10 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/12625 ) Change subject: assignment_fsm: fix channel allocator preferences .. Patch Set 8: (1 comment) https://gerrit.osmocom.org/#/c/12625/8/include/osmocom/bsc/gsm_data.h File include/osmocom/bsc/gsm_data.h: https://gerrit.osmocom.org/#/c/12625/8/include/osmocom/bsc/gsm_data.h@104 PS8, Line 104: bool full_rate; > Just wondering if it wouldn't be worth directly using GSM_LCHAN_xxx constant > in there rather than fu […] We ignore the MS channel preference on purpose and do "late assignment" instead. In osmo-nitb, we used to accept the TCH/F request from the MS and assign that, while on the MT side the CN's preference would be stronger, which could end up TCH/H (for dyn TS). Hence we would actively allow mismatching TCH kinds. Now, we always look at the MSC preferences during TCH/* Assignment. It is ok if we assign a TCH/F for SDCCH use at first, but it only really makes sense if there are no SDCCH left. We now do that. Recently there have been fixes for that behavior. So this channel_mode_and_rate is really only about TCH/F or TCH/H, no additional channel types should ever become relevant. Even if BTS has only TCH/H pchan configured, an MS will ask for TCH, F preferred. I'm not sure what's the benefit there, maybe that a phone already knows it is going to make a call and attempts to pre-empt the need to re-assign from SDCCH to TCH? It might make sense to revisit that once we properly support Channel Mode Modify commands and late RTP addition. Even then, the choice for SDCCH or TCH will happen before this code path; this here is for voice assignment. -- To view, visit https://gerrit.osmocom.org/12625 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Gerrit-Change-Number: 12625 Gerrit-PatchSet: 8 Gerrit-Owner: dexter Gerrit-Assignee: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-CC: Vadim Yanitskiy Gerrit-CC: tnt Gerrit-Comment-Date: Thu, 07 Feb 2019 16:05:41 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/12625 ) Change subject: assignment_fsm: fix channel allocator preferences .. Patch Set 8: (1 comment) https://gerrit.osmocom.org/#/c/12625/8/include/osmocom/bsc/gsm_data.h File include/osmocom/bsc/gsm_data.h: https://gerrit.osmocom.org/#/c/12625/8/include/osmocom/bsc/gsm_data.h@104 PS8, Line 104: bool full_rate; > We ignore the MS channel preference on purpose and do "late assignment" > instead. […] I hope I understood this right, though. My reply above talks about Channel Request coming in from the MS. I think you were instead referring to an Assignment Command from the MSC with GSM0808_CHAN_SIGN? I think osmo-msc currently never sends Assignment Command with CHAN_SIGN, does it?? In what situations? -- To view, visit https://gerrit.osmocom.org/12625 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Gerrit-Change-Number: 12625 Gerrit-PatchSet: 8 Gerrit-Owner: dexter Gerrit-Assignee: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-CC: Vadim Yanitskiy Gerrit-CC: tnt Gerrit-Comment-Date: Thu, 07 Feb 2019 16:20:32 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Hello Max, Neels Hofmeyr, Harald Welte, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/12625 to look at the new patch set (#9). Change subject: assignment_fsm: fix channel allocator preferences .. assignment_fsm: fix channel allocator preferences When the MSC allocates a channel through the ASSIGNMENT REQUEST, it may ask for a TCH/H and a TCH/F at the same time and tell which of the two types it prefers. The process of channel allocation currently selects, based on the BTS, MSC and MS capabilites exactly one apropriate codec/rate (e.g. TCH/H) and then tries to allocate it. If that allocation fails, there is no way to try the second choice and the assignment fails. For example: The MSC asks for TCH/F and TCH/H, prefering TCH/F, then the channel allocator will try TCH/F and if it fails (all TCH/F are currently in use), then TCH/H is never tried. Since the BSC currently only trys the first best codec/rate that is supported it also ignores the preference. Lets fix those problems by including the preference information and both possible codec/rate settings into the channel allocation decision. Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Related: OS#3503 --- M include/osmocom/bsc/codec_pref.h M include/osmocom/bsc/gsm_data.h M src/osmo-bsc/assignment_fsm.c M src/osmo-bsc/bsc_vty.c M src/osmo-bsc/codec_pref.c M src/osmo-bsc/handover_fsm.c M src/osmo-bsc/osmo_bsc_bssap.c M tests/codec_pref/codec_pref_test.c 8 files changed, 321 insertions(+), 117 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/25/12625/9 -- To view, visit https://gerrit.osmocom.org/12625 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Gerrit-Change-Number: 12625 Gerrit-PatchSet: 9 Gerrit-Owner: dexter Gerrit-Assignee: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-CC: Vadim Yanitskiy Gerrit-CC: tnt
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
tnt has posted comments on this change. ( https://gerrit.osmocom.org/12625 )
Change subject: assignment_fsm: fix channel allocator preferences
..
Patch Set 9:
(1 comment)
https://gerrit.osmocom.org/#/c/12625/9/src/osmo-bsc/osmo_bsc_bssap.c
File src/osmo-bsc/osmo_bsc_bssap.c:
https://gerrit.osmocom.org/#/c/12625/9/src/osmo-bsc/osmo_bsc_bssap.c@635
PS9, Line 635: switch (ct->ch_rate_type & GSM0808_SPEECH_PERM) {
Still that weird masking ...
--
To view, visit https://gerrit.osmocom.org/12625
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51
Gerrit-Change-Number: 12625
Gerrit-PatchSet: 9
Gerrit-Owner: dexter
Gerrit-Assignee: Neels Hofmeyr
Gerrit-Reviewer: Harald Welte
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Max
Gerrit-Reviewer: Neels Hofmeyr
Gerrit-CC: Vadim Yanitskiy
Gerrit-CC: tnt
Gerrit-Comment-Date: Thu, 14 Feb 2019 15:57:19 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Hello Max, Neels Hofmeyr, Harald Welte, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/12625 to look at the new patch set (#10). Change subject: assignment_fsm: fix channel allocator preferences .. assignment_fsm: fix channel allocator preferences When the MSC allocates a channel through the ASSIGNMENT REQUEST, it may ask for a TCH/H and a TCH/F at the same time and tell which of the two types it prefers. The process of channel allocation currently selects, based on the BTS, MSC and MS capabilites exactly one apropriate codec/rate (e.g. TCH/H) and then tries to allocate it. If that allocation fails, there is no way to try the second choice and the assignment fails. For example: The MSC asks for TCH/F and TCH/H, prefering TCH/F, then the channel allocator will try TCH/F and if it fails (all TCH/F are currently in use), then TCH/H is never tried. Since the BSC currently only trys the first best codec/rate that is supported it also ignores the preference. Lets fix those problems by including the preference information and both possible codec/rate settings into the channel allocation decision. Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Related: OS#3503 --- M include/osmocom/bsc/codec_pref.h M include/osmocom/bsc/gsm_data.h M src/osmo-bsc/assignment_fsm.c M src/osmo-bsc/bsc_vty.c M src/osmo-bsc/codec_pref.c M src/osmo-bsc/handover_fsm.c M src/osmo-bsc/osmo_bsc_bssap.c M tests/codec_pref/codec_pref_test.c 8 files changed, 323 insertions(+), 117 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/25/12625/10 -- To view, visit https://gerrit.osmocom.org/12625 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Gerrit-Change-Number: 12625 Gerrit-PatchSet: 10 Gerrit-Owner: dexter Gerrit-Assignee: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-CC: Vadim Yanitskiy Gerrit-CC: tnt
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Harald Welte has posted comments on this change. ( https://gerrit.osmocom.org/12625 ) Change subject: assignment_fsm: fix channel allocator preferences .. Patch Set 10: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/12625 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Gerrit-Change-Number: 12625 Gerrit-PatchSet: 10 Gerrit-Owner: dexter Gerrit-Assignee: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-CC: Vadim Yanitskiy Gerrit-CC: tnt Gerrit-Comment-Date: Sun, 17 Feb 2019 12:24:54 + Gerrit-HasComments: No Gerrit-HasLabels: Yes
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Vadim Yanitskiy has posted comments on this change. ( https://gerrit.osmocom.org/12625 ) Change subject: assignment_fsm: fix channel allocator preferences .. Patch Set 10: (1 comment) https://gerrit.osmocom.org/#/c/12625/10/src/osmo-bsc/bsc_vty.c File src/osmo-bsc/bsc_vty.c: https://gerrit.osmocom.org/#/c/12625/10/src/osmo-bsc/bsc_vty.c@4805 PS10, Line 4805: I am wondering how the changes in this file related to "channel allocator preferences"? -- To view, visit https://gerrit.osmocom.org/12625 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Gerrit-Change-Number: 12625 Gerrit-PatchSet: 10 Gerrit-Owner: dexter Gerrit-Assignee: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-CC: Vadim Yanitskiy Gerrit-CC: tnt Gerrit-Comment-Date: Sun, 17 Feb 2019 17:15:36 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/12625 ) Change subject: assignment_fsm: fix channel allocator preferences .. Patch Set 10: (1 comment) https://gerrit.osmocom.org/#/c/12625/10/src/osmo-bsc/bsc_vty.c File src/osmo-bsc/bsc_vty.c: https://gerrit.osmocom.org/#/c/12625/10/src/osmo-bsc/bsc_vty.c@4805 PS10, Line 4805: > I am wondering how the changes in this file related to "channel allocator > preferences"? indeed, seems like whitespace tweaks related to the previous patch that allows disabling specific lchans -- To view, visit https://gerrit.osmocom.org/12625 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Gerrit-Change-Number: 12625 Gerrit-PatchSet: 10 Gerrit-Owner: dexter Gerrit-Assignee: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-CC: Vadim Yanitskiy Gerrit-CC: tnt Gerrit-Comment-Date: Sun, 17 Feb 2019 20:44:26 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Hello Max, Neels Hofmeyr, Harald Welte, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/12625 to look at the new patch set (#11). Change subject: assignment_fsm: fix channel allocator preferences .. assignment_fsm: fix channel allocator preferences When the MSC allocates a channel through the ASSIGNMENT REQUEST, it may ask for a TCH/H and a TCH/F at the same time and tell which of the two types it prefers. The process of channel allocation currently selects, based on the BTS, MSC and MS capabilites exactly one apropriate codec/rate (e.g. TCH/H) and then tries to allocate it. If that allocation fails, there is no way to try the second choice and the assignment fails. For example: The MSC asks for TCH/F and TCH/H, prefering TCH/F, then the channel allocator will try TCH/F and if it fails (all TCH/F are currently in use), then TCH/H is never tried. Since the BSC currently only trys the first best codec/rate that is supported it also ignores the preference. Lets fix those problems by including the preference information and both possible codec/rate settings into the channel allocation decision. Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Related: OS#3503 --- M include/osmocom/bsc/codec_pref.h M include/osmocom/bsc/gsm_data.h M src/osmo-bsc/assignment_fsm.c M src/osmo-bsc/bsc_vty.c M src/osmo-bsc/codec_pref.c M src/osmo-bsc/handover_fsm.c M src/osmo-bsc/osmo_bsc_bssap.c M tests/codec_pref/codec_pref_test.c 8 files changed, 319 insertions(+), 113 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/25/12625/11 -- To view, visit https://gerrit.osmocom.org/12625 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Gerrit-Change-Number: 12625 Gerrit-PatchSet: 11 Gerrit-Owner: dexter Gerrit-Assignee: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-CC: Vadim Yanitskiy Gerrit-CC: tnt
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Harald Welte has posted comments on this change. ( https://gerrit.osmocom.org/12625 ) Change subject: assignment_fsm: fix channel allocator preferences .. Patch Set 11: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/12625 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Gerrit-Change-Number: 12625 Gerrit-PatchSet: 11 Gerrit-Owner: dexter Gerrit-Assignee: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-CC: Vadim Yanitskiy Gerrit-CC: tnt Gerrit-Comment-Date: Mon, 18 Feb 2019 13:21:59 + Gerrit-HasComments: No Gerrit-HasLabels: Yes
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
tnt has posted comments on this change. ( https://gerrit.osmocom.org/12625 )
Change subject: assignment_fsm: fix channel allocator preferences
..
Patch Set 11:
(1 comment)
* See comment about the unsupported value, any reason for this ?
* The preference of codec expressed by the MSC in the assignement message is
AFAICT completely ignored in favor of the local preference data (i.e. it scans
msc->audio_support[i] in order and check for a match at any location in the
assignement message and doesn't take into account the order inside th e
message). I think the BSC is free to ignore that order, just wanted to make
sure it was a conscious decision.
Rest looks fine.
https://gerrit.osmocom.org/#/c/12625/11/src/osmo-bsc/osmo_bsc_bssap.c
File src/osmo-bsc/osmo_bsc_bssap.c:
https://gerrit.osmocom.org/#/c/12625/11/src/osmo-bsc/osmo_bsc_bssap.c@635
PS11, Line 635: switch (ct->ch_rate_type) {
GSM0808_SPEECH_PERM and GSM0808_SPEECH_PERM_NO_CHANGE are not handled ?
AFAICT, those are perfectly legal values, just indicating there is not
preference for the channel type and that all preferences are expressed via the
codec list.
--
To view, visit https://gerrit.osmocom.org/12625
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51
Gerrit-Change-Number: 12625
Gerrit-PatchSet: 11
Gerrit-Owner: dexter
Gerrit-Assignee: Neels Hofmeyr
Gerrit-Reviewer: Harald Welte
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Max
Gerrit-Reviewer: Neels Hofmeyr
Gerrit-CC: Vadim Yanitskiy
Gerrit-CC: tnt
Gerrit-Comment-Date: Mon, 18 Feb 2019 14:30:11 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
dexter has posted comments on this change. ( https://gerrit.osmocom.org/12625 )
Change subject: assignment_fsm: fix channel allocator preferences
..
Patch Set 11:
(38 comments)
> * See comment about the unsupported value, any reason for this ?
I have now added that value, it was indeed missing.
> * The preference of codec expressed by the MSC in the assignement
Yes thats true, when I was implementing it it was not clear to me how to
handle the preference and if there is any preference at all so I did not
take that into consideration. I also can not remember seeing anything in
the spec that dictates that the order of the codecs needs to be retained
when making the codec decision. I think thats also not in scope of the
spec since at the end of the day it will be up to the vendor how to
optimize the resource management at the BSC anyway.
However, I think that if we decide to analyze and possibly fix/optimize
the codec selection behavior we should do that in a separate patch.
https://gerrit.osmocom.org/#/c/12625/2/include/osmocom/bsc/codec_pref.h
File include/osmocom/bsc/codec_pref.h:
https://gerrit.osmocom.org/#/c/12625/2/include/osmocom/bsc/codec_pref.h@24
PS2, Line 24:const struct gsm_bts *bts, enum rate_pref
rate_pref);
> in ch_mode_rate, we add a 'bool full_rate', and we also add a 'rate_pref'? […]
The bool in ch_mode_rate and the enum rate_pref are two different things. The
bool symbols an absolute choice between FR and HR, the enum rate_pref members
symbolize a set of three different preference choices. I don't think that it is
a good idea to mix this together.
match_codec_pref() is used here to make an absolute choice based on a
preference. If the MSC asks for FR and HR (with a preference) we execute
match_codec_pref() two times to get the choices for the two situations. These
two are then handed over to the channel allocator which makes the final
decision based on the channel load.
There is also a problem with the FR/HR flag that not every codec is available
for FR and HR. So we can not first choose a codec with match_codec_pref() and
then say if the TCH/F version of the codec is not available, just pick the
TCH/H version. This does not work for EFR and even for AMR there are problems
with the S-Bits. Thats why we need the full information for both possible
situations.
https://gerrit.osmocom.org/#/c/12625/2/include/osmocom/bsc/codec_pref.h@24
PS2, Line 24:const struct gsm_bts *bts, enum rate_pref
rate_pref);
> Also talked about this in person, and obviously ch_mode_rate is an
> out-argument, while the rate_pref […]
Done
https://gerrit.osmocom.org/#/c/12625/8/include/osmocom/bsc/gsm_data.h
File include/osmocom/bsc/gsm_data.h:
https://gerrit.osmocom.org/#/c/12625/8/include/osmocom/bsc/gsm_data.h@104
PS8, Line 104: bool full_rate;
> Just wondering if it wouldn't be worth directly using GSM_LCHAN_xxx constant
> in there rather than fu […]
Done
https://gerrit.osmocom.org/#/c/12625/8/include/osmocom/bsc/gsm_data.h@104
PS8, Line 104: bool full_rate;
> We ignore the MS channel preference on purpose and do "late assignment"
> instead. […]
Done
https://gerrit.osmocom.org/#/c/12625/8/include/osmocom/bsc/gsm_data.h@104
PS8, Line 104: bool full_rate;
> I hope I understood this right, though. […]
Done
https://gerrit.osmocom.org/#/c/12625/4/include/osmocom/bsc/gsm_data.h
File include/osmocom/bsc/gsm_data.h:
https://gerrit.osmocom.org/#/c/12625/4/include/osmocom/bsc/gsm_data.h@121
PS4, Line 121:
> We talked in person, and there really are only two possible outcomes: one for
> an FR and one for an H […]
Done
https://gerrit.osmocom.org/#/c/12625/4/include/osmocom/bsc/gsm_data.h@300
PS4, Line 300: /* LCLS FSM */
> this ^ should probably go inside the assignment{} scope instead, see other
> comments
Done
https://gerrit.osmocom.org/#/c/12625/2/include/osmocom/bsc/gsm_data.h
File include/osmocom/bsc/gsm_data.h:
https://gerrit.osmocom.org/#/c/12625/2/include/osmocom/bsc/gsm_data.h@121
PS2, Line 121:
> Are there really only two possible mode,rate combinations?
Yes, the MSC can only ask for TCH/F and TCH/H at the same time. It may also ask
only for one of the two. The codec is a different thing. There are more
combinations possible, but thats a pure capability thing since a TCH may be
used with several different codecs.
https://gerrit.osmocom.org/#/c/12625/1/src/osmo-bsc/abis_rsl.c
File src/osmo-bsc/abis_rsl.c:
https://gerrit.osmocom.org/#/c/12625/1/src/osmo-bsc/abis_rsl.c@1378
PS1, Line 1378: a
> Spelling: 'to' is not needed here I think.
Done
https://gerrit.osmocom.org/#/c/12625/2/src/osmo-bsc/abis_rsl.c
File src/osmo-bsc/abis_rsl.c:
https://gerrit.osmocom.org/#/c/12625/2/src/osmo-bsc/abis_rsl.c@1391
PS2, Line 1391: if (!lchan) {
> These changes above make up a separate fix by themselves. […]
I have now separated this.
https://gerrit.osmocom.org/#/
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Hello Max, Neels Hofmeyr, Harald Welte, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/12625 to look at the new patch set (#12). Change subject: assignment_fsm: fix channel allocator preferences .. assignment_fsm: fix channel allocator preferences When the MSC allocates a channel through the ASSIGNMENT REQUEST, it may ask for a TCH/H and a TCH/F at the same time and tell which of the two types it prefers. The process of channel allocation currently selects, based on the BTS, MSC and MS capabilites exactly one apropriate codec/rate (e.g. TCH/H) and then tries to allocate it. If that allocation fails, there is no way to try the second choice and the assignment fails. For example: The MSC asks for TCH/F and TCH/H, prefering TCH/F, then the channel allocator will try TCH/F and if it fails (all TCH/F are currently in use), then TCH/H is never tried. Since the BSC currently only trys the first best codec/rate that is supported it also ignores the preference. Lets fix those problems by including the preference information and both possible codec/rate settings into the channel allocation decision. Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Related: OS#3503 --- M include/osmocom/bsc/codec_pref.h M include/osmocom/bsc/gsm_data.h M src/osmo-bsc/assignment_fsm.c M src/osmo-bsc/bsc_vty.c M src/osmo-bsc/codec_pref.c M src/osmo-bsc/handover_fsm.c M src/osmo-bsc/osmo_bsc_bssap.c M tests/codec_pref/codec_pref_test.c 8 files changed, 321 insertions(+), 113 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/25/12625/12 -- To view, visit https://gerrit.osmocom.org/12625 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Gerrit-Change-Number: 12625 Gerrit-PatchSet: 12 Gerrit-Owner: dexter Gerrit-Assignee: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: dexter Gerrit-CC: Vadim Yanitskiy Gerrit-CC: tnt
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
tnt has posted comments on this change. ( https://gerrit.osmocom.org/12625 ) Change subject: assignment_fsm: fix channel allocator preferences .. Patch Set 12: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/12625 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Gerrit-Change-Number: 12625 Gerrit-PatchSet: 12 Gerrit-Owner: dexter Gerrit-Assignee: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: dexter Gerrit-Reviewer: tnt Gerrit-CC: Vadim Yanitskiy Gerrit-Comment-Date: Thu, 21 Feb 2019 09:36:25 + Gerrit-HasComments: No Gerrit-HasLabels: Yes
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Max has posted comments on this change. ( https://gerrit.osmocom.org/12625 )
Change subject: assignment_fsm: fix channel allocator preferences
..
Patch Set 12:
(1 comment)
https://gerrit.osmocom.org/#/c/12625/8/src/osmo-bsc/osmo_bsc_bssap.c
File src/osmo-bsc/osmo_bsc_bssap.c:
https://gerrit.osmocom.org/#/c/12625/8/src/osmo-bsc/osmo_bsc_bssap.c@635
PS8, Line 635: switch (ct->ch_rate_type) {
> I can of course change it but I am not sure if this is wise, we might break
> the backward compatibility of the API.
You can always add wrapper to libosmocore's src/gsm/gsm0808_utils.c to avoid
dealing with gsm0808_chan_rate_type_speech values/masking directly. This would
hide the odd naming until we can deprecate it properly. It'd also help with
documenting/clarifying that naming confusion.
--
To view, visit https://gerrit.osmocom.org/12625
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51
Gerrit-Change-Number: 12625
Gerrit-PatchSet: 12
Gerrit-Owner: dexter
Gerrit-Assignee: Neels Hofmeyr
Gerrit-Reviewer: Harald Welte
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Max
Gerrit-Reviewer: Neels Hofmeyr
Gerrit-Reviewer: dexter
Gerrit-Reviewer: tnt
Gerrit-CC: Vadim Yanitskiy
Gerrit-Comment-Date: Thu, 21 Feb 2019 09:52:10 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
tnt has posted comments on this change. ( https://gerrit.osmocom.org/12625 )
Change subject: assignment_fsm: fix channel allocator preferences
..
Patch Set 12:
(1 comment)
https://gerrit.osmocom.org/#/c/12625/8/src/osmo-bsc/osmo_bsc_bssap.c
File src/osmo-bsc/osmo_bsc_bssap.c:
https://gerrit.osmocom.org/#/c/12625/8/src/osmo-bsc/osmo_bsc_bssap.c@635
PS8, Line 635: switch (ct->ch_rate_type) {
> > I can of course change it but I am not sure if this is wise, we might break
> > the backward compatibi […]
@Max the above comment is irrelevant. This wasn't a mask, naming is correct.
0xf is a valid value, so is 0x1f. This is now handled properly in the v12 of
this patch.
--
To view, visit https://gerrit.osmocom.org/12625
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51
Gerrit-Change-Number: 12625
Gerrit-PatchSet: 12
Gerrit-Owner: dexter
Gerrit-Assignee: Neels Hofmeyr
Gerrit-Reviewer: Harald Welte
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Max
Gerrit-Reviewer: Neels Hofmeyr
Gerrit-Reviewer: dexter
Gerrit-Reviewer: tnt
Gerrit-CC: Vadim Yanitskiy
Gerrit-Comment-Date: Thu, 21 Feb 2019 10:02:49 +
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Harald Welte has submitted this change and it was merged. (
https://gerrit.osmocom.org/12625 )
Change subject: assignment_fsm: fix channel allocator preferences
..
assignment_fsm: fix channel allocator preferences
When the MSC allocates a channel through the ASSIGNMENT REQUEST, it may
ask for a TCH/H and a TCH/F at the same time and tell which of the two
types it prefers.
The process of channel allocation currently selects, based on the BTS,
MSC and MS capabilites exactly one apropriate codec/rate (e.g. TCH/H)
and then tries to allocate it. If that allocation fails, there is no way
to try the second choice and the assignment fails.
For example: The MSC asks for TCH/F and TCH/H, prefering TCH/F, then the
channel allocator will try TCH/F and if it fails (all TCH/F are
currently in use), then TCH/H is never tried.
Since the BSC currently only trys the first best codec/rate that is
supported it also ignores the preference.
Lets fix those problems by including the preference information and both
possible codec/rate settings into the channel allocation decision.
Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51
Related: OS#3503
---
M include/osmocom/bsc/codec_pref.h
M include/osmocom/bsc/gsm_data.h
M src/osmo-bsc/assignment_fsm.c
M src/osmo-bsc/bsc_vty.c
M src/osmo-bsc/codec_pref.c
M src/osmo-bsc/handover_fsm.c
M src/osmo-bsc/osmo_bsc_bssap.c
M tests/codec_pref/codec_pref_test.c
8 files changed, 321 insertions(+), 113 deletions(-)
Approvals:
Jenkins Builder: Verified
tnt: Looks good to me, but someone else must approve
Harald Welte: Looks good to me, approved
diff --git a/include/osmocom/bsc/codec_pref.h b/include/osmocom/bsc/codec_pref.h
index 51340c1..adefe04 100644
--- a/include/osmocom/bsc/codec_pref.h
+++ b/include/osmocom/bsc/codec_pref.h
@@ -9,14 +9,19 @@
struct bts_codec_conf;
struct bsc_msc_data;
struct gsm_bts;
+struct channel_mode_and_rate;
-int match_codec_pref(enum gsm48_chan_mode *chan_mode,
-bool *full_rate,
-uint16_t *s15_s0,
+enum rate_pref {
+ RATE_PREF_NONE,
+ RATE_PREF_HR,
+ RATE_PREF_FR,
+};
+
+int match_codec_pref(struct channel_mode_and_rate *ch_mode_rate,
const struct gsm0808_channel_type *ct,
const struct gsm0808_speech_codec_list *scl,
const struct bsc_msc_data *msc,
-const struct gsm_bts *bts);
+const struct gsm_bts *bts, enum rate_pref rate_pref);
void gen_bss_supported_codec_list(struct gsm0808_speech_codec_list *scl,
const struct bsc_msc_data *msc,
diff --git a/include/osmocom/bsc/gsm_data.h b/include/osmocom/bsc/gsm_data.h
index f6c5129..4d27a2e 100644
--- a/include/osmocom/bsc/gsm_data.h
+++ b/include/osmocom/bsc/gsm_data.h
@@ -99,6 +99,12 @@
SUBSCR_SCCP_ST_CONNECTED
};
+struct channel_mode_and_rate {
+ enum gsm48_chan_mode chan_mode;
+ bool full_rate;
+ uint16_t s15_s0;
+};
+
/* Information retrieved during an Assignment Request from the MSC. This is
storage of the Assignment instructions
* parsed from the Assignment Request message, to pass on until the gscon and
assignment FSMs have decided whether an
* Assignment is actually going to be carried out. Should remain unchanged
after initial decoding. */
@@ -110,9 +116,12 @@
char msc_rtp_addr[INET_ADDRSTRLEN];
uint16_t msc_rtp_port;
- enum gsm48_chan_mode chan_mode;
- bool full_rate;
- uint16_t s15_s0;
+ /* Prefered rate/codec setting (mandatory) */
+ struct channel_mode_and_rate ch_mode_rate_pref;
+
+ /* Alternate rate/codec setting (optional) */
+ bool ch_mode_rate_alt_present;
+ struct channel_mode_and_rate ch_mode_rate_alt;
};
/* State of an ongoing Assignment, while the assignment_fsm is still busy.
This serves as state separation to keep the
@@ -629,6 +638,13 @@
struct gsm48_req_ref *rqd_ref;
struct gsm_subscriber_connection *conn;
+
+ /* Depending on the preferences that where submitted together with
+* the assignment and the current channel load, the BSC has to select
+* one of the offered codec/rates. The final selection by the BSC is
+* stored here and is used when sending the assignment complete or
+* when performing a handover procedure. */
+ struct channel_mode_and_rate ch_mode_rate;
};
/* One Timeslot in a TRX */
diff --git a/src/osmo-bsc/assignment_fsm.c b/src/osmo-bsc/assignment_fsm.c
index 2ad3959..67937d6 100644
--- a/src/osmo-bsc/assignment_fsm.c
+++ b/src/osmo-bsc/assignment_fsm.c
@@ -170,7 +170,7 @@
if (gscon_is_aoip(conn)) {
/* Extrapolate speech codec from speech mode */
gsm0808_speech_codec_from_chan_type(&sc, perm_spch);
- sc.cfg = conn->assignment.req.s15_s0;
+
Change in osmo-bsc[master]: assignment_fsm: fix channel allocator preferences
Harald Welte has posted comments on this change. ( https://gerrit.osmocom.org/12625 ) Change subject: assignment_fsm: fix channel allocator preferences .. Patch Set 12: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/12625 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5239e05c1cfbcb8af28f43373a58fa6c2d216c51 Gerrit-Change-Number: 12625 Gerrit-PatchSet: 12 Gerrit-Owner: dexter Gerrit-Assignee: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: dexter Gerrit-Reviewer: tnt Gerrit-CC: Vadim Yanitskiy Gerrit-Comment-Date: Sat, 23 Feb 2019 15:14:02 + Gerrit-HasComments: No Gerrit-HasLabels: Yes
