This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
dexter gerrit-no-reply at lists.osmocom.orgdexter 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/#/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: sw
> Let's use bool.
Done
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:
> const struct ...
Done
https://gerrit.osmocom.org/#/c/12625/2/src/osmo-bsc/assignment_fsm.c@305
PS2, Line 305: case GSM48_CMODE_SPEECH_AMR:
> (we already have a flag around called "requires_voice_stream", so prefer if this is called "check_re […]
Done
https://gerrit.osmocom.org/#/c/12625/2/src/osmo-bsc/assignment_fsm.c@324
PS2, Line 324: bool result_requires_voice_pref;
> don't understand the name 'result_pref' ... […]
I have added 'requires_voice' to the variable names to make it more clear.
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. […]
Done
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@304
PS4, Line 304: case GSM48_CMODE_SPEECH_EFR:
> -1: code dup: please write one function that takes a chan_mode as input and returns a requires_voice […]
The requires voice stream must be identical for both possible choices. If it is not identical there is a protocol error since it is not possible mix signaling and voice here. There is no "Full rate TCH channel or Signalling channel, signalling prefered..."
This means if we request for voice/TCH, both choices must always be voice channels.
See also: 3GPP TS 48.008, chapter 3.2.2.11
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@377
PS4, Line 377: /* Check if the currently existing lchan is compatible with the
> mistake in my comment: should be […]
I don't get that. Why check the req->ch_mode_rate_pref and req->ch_mode_rate_alt in the same statement if it is not checked if req->ch_mode_rate_alt is even populated. One must first check the _pref, and only if an _alt exists one may move on.
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@443
PS4, Line 443: return;
> -1: code dup (same)
Its similar, but also different enough. I am not sure if it makes sense to split this up into a helper function, we wouldn't save much.
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@479
PS4, Line 479: conn->lchan->ch_mode_rate.full_rate,
> (FYI, I considered whether it is better to allocate the FSM instance only *after* we checked whether […]
Done
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@495
PS4, Line 495: static void assignment_fsm_wait_lchan(struct osmo_fsm_inst *fi, uint32_t event, void *data)
> ^ still valid, but don't worry about this one, see https://gerrit.osmocom. […]
Done
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@495
PS4, Line 495: static void assignment_fsm_wait_lchan(struct osmo_fsm_inst *fi, uint32_t event, void *data)
> ...maybe we can deallocate the assignment. […]
I see, however since it is allocated as a child we sill won't run into serious memory leaks. I have deallocated the fi now.
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@501
PS4, Line 501: if (data != conn->assignment.new_lchan)
> -1: the ch_mode_rate use must not be ambiguous: does it apply to the current conn->lchan or to the n […]
I have put it now into the assignment scope as you suggested. I am not sure if it makes sense to put it into the lchan. Probably worth to discuss this on Thursday?
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/assignment_fsm.c@503
PS4, Line 503:
> ("alternate". use line width. […]
Done
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 suppo […]
Done
https://gerrit.osmocom.org/#/c/12625/5/src/osmo-bsc/assignment_fsm.c@497
PS5, Line 497: struct gsm_subscriber_connection *conn = assignment_fi_conn(fi);
> instead of spraying CR comments I pushed a suggestion instead. […]
I have checked that out, looks good to me, also the tests pass fine.
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"?
Done
https://gerrit.osmocom.org/#/c/12625/10/src/osmo-bsc/bsc_vty.c@4805
PS10, Line 4805:
> indeed, seems like whitespace tweaks related to the previous patch that allows disabling specific lc […]
Done
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->lchan->ch_mode_rate.s15_s0;
> Noticing this problem that exists before this patch: […]
I see, then ch_mode_rate should be really seen as an lchan property. I have moved the struct now to gsm_lchan.
https://gerrit.osmocom.org/#/c/12625/5/src/osmo-bsc/handover_fsm.c@716
PS5, Line 716: sc.cfg = conn->lchan->ch_mode_rate.s15_s0;
> Noticing this problem that exists before this patch: […]
Then its
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: lcls_apply_config(conn);
> "select_codecs" ?
Done
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/osmo_bsc_bssap.c@632
PS4, Line 632: msc = conn->sccp.msc;
> (if full rate didn't work, then this must end up being half rate? […]
Yes, thats correct. Its possibly more logical to express it explicitly.
https://gerrit.osmocom.org/#/c/12625/4/src/osmo-bsc/osmo_bsc_bssap.c@647
PS4, Line 647: RATE_PREF_FR);
> (RATE_PREF_FR?)
Done
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) {
> do you really need the mask at all?
Yes, we need to mask off the bits that define that a later change is between the two rates is allowed or not. This information plays no role here, but is contained in the byte.
https://gerrit.osmocom.org/#/c/12625/8/src/osmo-bsc/osmo_bsc_bssap.c@635
PS8, Line 635: switch (ct->ch_rate_type) {
> Wait what ? GSM0808_SPEECH_PERM is not a mask, it's an actual possible value of the field defined in […]
I have looked up 3GPP TS 48.008 version 14.2.0 Release 14 3.2.2.11 and 0x0F is not a valid value, this is indeed a mask. The original author of the file seems to have forgotten to add a suffix to make that clear. If you look at enum gsm0808_chan_rate_type_data it should be more clear how the intension was. I know this is odd, but unfortunately its now the way it is. I can of course change it but I am not sure if this is wise, we might break the backward compatibility of the API.
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. […]
Done
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),
> agree
Done
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) {
> Still that weird masking ...
The purpose of the masking is to get rid of the bit that configures if later changes are allowed or not. As I said, I interpret GSM0808_SPEECH_PERM as a mask that is intended to do this.
However, I looked at 3GPP TS 48.008 again, the mask would also mask off bits that define TCH multislot configurations, so now I think that just masking the bits off is probably not a very good Idea. I have added the fall through cases instead.
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 ? […]
Now I finally understand this, i always thought that those were masks, but the spec indeed lists the constants for speech + CTM Text Telephony, but not for data. I thought that data and voice would have no difference in their options, but this is not the case.
I have never seen those constants anywhere in the wild. Presumably MSCs always seem to have a preference for the one or the other rate.
However, I have now programmed it that way that it is handled like GSM0808_SPEECH_FULL_PREF, so if the MSC has no preference at all we would internally assume a preference for FR. I think this makes sense since FR has better quality and if we have resources available, we should just use FR.
--
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 <pmaier at sysmocom.de>
Gerrit-Assignee: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Max <msuraev at sysmocom.de>
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: dexter <pmaier at sysmocom.de>
Gerrit-CC: Vadim Yanitskiy <axilirator at gmail.com>
Gerrit-CC: tnt <tnt at 246tNt.com>
Gerrit-Comment-Date: Thu, 21 Feb 2019 09:08:27 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190221/8278473e/attachment.htm>