Change in osmo-bts[master]: measurement: make sure measurement interval end is detected

[Harald Welte]

Harald Welte has posted comments on this change. ( https://gerrit.osmocom.org/10492 )

Change subject: measurement: make sure measurement interval end is detected
......................................................................


Patch Set 1: Code-Review-1

(6 comments)

sorry, there are unfortunately some situations not taken in consideration :(

https://gerrit.osmocom.org/#/c/10492/1/include/osmo-bts/gsm_data_shared.h
File include/osmo-bts/gsm_data_shared.h:

https://gerrit.osmocom.org/#/c/10492/1/include/osmo-bts/gsm_data_shared.h@263
PS1, Line 263: mesaurement
measurement


https://gerrit.osmocom.org/#/c/10492/1/include/osmo-bts/gsm_data_shared.h@264
PS1, Line 264: last_fn
I can see you are writing to this field, but I cannot see that you ever reset/reinitialize it after a lchan is closed.


https://gerrit.osmocom.org/#/c/10492/1/src/common/measurement.c
File src/common/measurement.c:

https://gerrit.osmocom.org/#/c/10492/1/src/common/measurement.c@280
PS1, Line 280: is_meas_overdue
what about the start condition? lchan->meas.last_fn will be zero, but the current frame number will be e.g. 200000.  Then your current logic would consider a measurement overdue at the very first measurement, as there's a distance of 200000.  I think lchan->meas.last_fn needs to be turned into a signed integer and initialized to -1 every time the lchan is reset.  This way you can check for -1 here and if that's the case, assume that nothing is overdue.
Or you keep it unsigned and use the magic value 0xffffffff as initializer.  This is much larger than the largest permitted framenumber.


https://gerrit.osmocom.org/#/c/10492/1/src/common/measurement.c@285
PS1, Line 285: abs(fn - lchan->meas.last_fn)
I think you need to do this in modulo-arithmetic of the GSM hyperframe duration.  Keep in mind frame numbers wrap at that point, and arithemtically (in "unsigned int" math), without the modulo, your "distance" is way larger than in reality is is (think of last_fn = HYPERFRAME-1 and fn = HYPERFRAME+1.  Then the distance is 2, but only in modulo-hyperframe math.

Unless I'm wrong, this must be fixed before merge.


https://gerrit.osmocom.org/#/c/10492/1/src/common/measurement.c@286
PS1, Line 286: for (i = 1; i < distance; i++) {
             : 		if (is_meas_complete(lchan, i + lchan->meas.last_fn)) {
             : 			*fn_missed_end = i + lchan->meas.last_fn;
             : 			return true;
             : 		}
             : 	}
this is a potentially quite expensive / time-consuming loop, particularly if the distance is quite large. I wish there was some obvious way to solve this without the expensive and lengthy iteration.

If the distance is larger than the sacch reporting interval, it is for sure overdue, So that could be a short-cut, limiting the maximum number of iterations here to 104.  That would be a good start.  Please add this and a comment as rationale.

I think even for distance <= 102/104, it can even be done more efficiently by determining if the end of the measurement period for this lchan (see lookup tables) is within the interval [ lchan->meas.last_fn .. fn ]. So two if statement should be sufficient to avoid the iteration.  Feel free to do this optimization as a follow-up patch, to not delay this any further.


https://gerrit.osmocom.org/#/c/10492/1/src/common/measurement.c@294
PS1, Line 294: *fn_missed_end = 0;
"0" is a valid frame number like anything else below the value of the maximum hyperframe number.  I think setting it to "0" is no different than simply leaving it at whatever random value it was before calling the function.  A caller must never use fn_missed_end when this function returns false.



-- 
To view, visit https://gerrit.osmocom.org/10492
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3a86cd8185cc6b94258373fe929f0c2f1cf27cfa
Gerrit-Change-Number: 10492
Gerrit-PatchSet: 1
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Comment-Date: Fri, 17 Aug 2018 16:24:19 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20180817/0e865592/attachment.htm>