Change in osmo-bsc[master]: Avoid switching dyn ts to sdcch8 if it starves later TCH

[neels]

neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/24876 )

Change subject: Avoid switching dyn ts to sdcch8 if it starves later TCH
......................................................................


Patch Set 4:

(8 comments)

I know this patch is already merged, sorry for being late here.
Maybe some of my comments could warrant a follow-up patch?

https://gerrit.osmocom.org/c/osmo-bsc/+/24876/4/src/osmo-bsc/abis_rsl.c 
File src/osmo-bsc/abis_rsl.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/24876/4/src/osmo-bsc/abis_rsl.c@1921 
PS4, Line 1921: 
(2 blank lines)


https://gerrit.osmocom.org/c/osmo-bsc/+/24876/4/src/osmo-bsc/abis_rsl.c@1927 
PS4, Line 1927: 					lchan->ts->pchan_is != GSM_PCHAN_SDCCH8_SACCH8C;
i'd favor a bunch of braces


https://gerrit.osmocom.org/c/osmo-bsc/+/24876/4/src/osmo-bsc/abis_rsl.c@1930 
PS4, Line 1930: 	free_tcch = bts_count_free_ts(bts, GSM_PCHAN_TCH_H);
"tchh"


https://gerrit.osmocom.org/c/osmo-bsc/+/24876/4/src/osmo-bsc/abis_rsl.c@1938 
PS4, Line 1938: 	/* There's a TCH available and we'll not switch any of them, so we are fine */
"switch any dyn ts"


https://gerrit.osmocom.org/c/osmo-bsc/+/24876/4/src/osmo-bsc/abis_rsl.c@1942 
PS4, Line 1942: 	/* We need to switch, but there's at least 2 TCH TS available so we are fine: */
"so we can switch one of them to SDCCH8 and still have one left"


https://gerrit.osmocom.org/c/osmo-bsc/+/24876/4/src/osmo-bsc/abis_rsl.c@1951 
PS4, Line 1951: 	 * If condition [C] is met, it means there's 1 dynamic TS and it's the
"because a dyn TS is counted both as 1 free TCH/F and 2 free TCH/H at the same time"


https://gerrit.osmocom.org/c/osmo-bsc/+/24876/4/src/osmo-bsc/abis_rsl.c@1955 
PS4, Line 1955: 	 * different TS than the one we want to switch, so we are fine selecting
"A: the free TCH/F must be a static timeslot; B1: half occupied TCH/H TS; B2: half occupied TCH/H TS plus one static TCH/F; C: exactly one dyn TS available"

But for C, what if there are two half occupied TCH/H plus one static TCH/F?
Wouldn't you make the wrong assumption that it is a dyn TS based on the counts?

In the end I think your logic is correct, but the explanation becomes a lot simpler if you do

  if (needs_dyn_switch) {
      free_tchf--;
      free_tchh -= 2;
  }

and see if any tch are left after that.


https://gerrit.osmocom.org/c/osmo-bsc/+/24876/4/src/osmo-bsc/abis_rsl.c@1967 
PS4, Line 1967: 	lchan = lchan_select_by_type(bts, GSM_LCHAN_SDCCH);
doing 'type = SDCCH' would be all that's needed indeed, and add a logging 'LOG_LCHAN(lchan, LOGL_INFO, "Selected\n");'



-- 
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/24876
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I3b32968949a7bdcbebf5a823359295bac51d8e08
Gerrit-Change-Number: 24876
Gerrit-PatchSet: 4
Gerrit-Owner: pespin <pespin at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann at sysmocom.de>
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Wed, 14 Jul 2021 21:15:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210714/51edf8e7/attachment.htm>