Attention is currently required from: iedemam, fixeria, pespin.

View Change

11 comments:

• Patchset:

  • Patch Set #18:

    looks like we will merge this soon. Let's sort out these remaining issues:

    • milliseconds vs tenth seconds, various places marked (1):
    • The time_cc updates the rate_ctr every tenth of a second now, which is nice, thumbs up.
    • However, the naming and doc still suggest that the rate_ctrs are counting milliseconds.
    • reporting on vty the average activity duration, see the place marked (2)
    • reporting on vty how long a single active lchan has been active, marked (3)
    • minor style

• File include/osmocom/bsc/bts.h:

  • Patch Set #18, Line 61: BTS_CTR_CHAN_SDCCH_ACTIVE_MILLISECONDS_TOTAL,

    (1)

• File include/osmocom/bsc/gsm_data.h:

  • Patch Set #18, Line 890: /* Interval timing to capture duration per activation and cummulative active time */

    "cumulative"

  • Patch Set #18, Line 891: struct osmo_time_cc active_ms;

    (1)

    I think it was named active_ms due to my code review ... maybe just 'active' or 'active_cc' is better after all? sorry about that.

• File src/osmo-bsc/bts.c:

  • Patch Set #18, Line 1038: "Cumulative number of milliseconds of SDCCH channel activity" },

    (1)

• File src/osmo-bsc/bts_trx_vty.c:

  • Patch Set #18, Line 576: VTY_NEWLINE);

    (3)
    Before, I gave code review to remove the osmo_time_cc_reset(), saying it was not necessary. Which is true, when thinking in terms of rate counters and stats; the normal / intended way to use osmo_time_cc is to always just carry on. I realize there is a "but" that I didn't see before: of course without a reset the total_sum accumulates across all activations of an lchan, and we cannot use it to report how long ago the currently active lchan has been activated. Sorry for not seeing that before.

    ways to resolve: one of

    a) drop this vty output and the gsm_lchan_active_duration_ms() function. Then it is fine to not do osmo_time_cc_reset().

    b) use a separate simple timestamp to count a single lchan's active time, orthogonal to the osmo_time_cc that does the "magic" rate_ctr stuff.

    c) re-add the osmo_time_cc_reset() with a comment indicating
    "/* This reset allows showing on vty how long ago a single active lchan was activated. This reset does not affect rate_ctr statistics. */".
    The osmo_time_cc still works fine when it is reset at a time when the flag is false.

    I think either way it would be nicer to first do this patch about rate_ctr and averages only, and add the "activated x seconds ago" in a separate patch -- it is a separate feature. We can then discuss there whether that x should also be exposed to statistics somehow.

• File src/osmo-bsc/bts_vty.c:

  • Patch Set #18, Line 3844: vty_out(vty, "%s", VTY_NEWLINE);

    (2)
    the usefulness of this output is disputable because it averages over the entire lifetime of the cell. so if we have a month of normal operation followed by an hour of short-lived lchans, the avg lifespan reported here does not reflect reality in a useful way.

    We can instead (or in addition to above output?) use the rate in the last interval instead: in rate_ctr->intv, we keep values for the last second, minute, hour and day. For example:

      const struct rate_ctr *active_time_tch = rate_ctr_group_get_ctr(bts->bts_ctrs, BTS_CTR_CHAN_TCH_ACTIVE_TENTH_SECONDS);
      const struct rate_ctr *activations_tch = rate_ctr_group_get_ctr(bts->bts_ctrs, BTS_CTR_CHAN_ACT_TCH);

      vty_out(vty, "avg active duration in the last hour: TCH: %s",
           /* active_time_tch counts tenths of seconds, so *100 gives an avg in ms */
           osmo_int_to_float_str_c(OTC_SELECT,
               100 * active_time_tch->intv[RATE_CTR_INTV_HOUR].rate
               / activations_tch->[RATE_CTR_INTV_HOUR].rate, 3));

    Same can be done per minute, per day, and per each lchan type, as you see fit...

• File src/osmo-bsc/lchan_fsm.c:

  • Patch Set #18, Line 208: struct rate_ctr *rate_ctr;

    osmocom style requires that this local variable be defined at the start of the scope, at line 203 above

  • Patch Set #18, Line 224: if (lchan->active_ms.cfg.rate_ctr) {

    (could just set the flag no matter if rate_ctr is set or not.)

  • Patch Set #18, Line 546: .active_ms = lchan->active_ms

    please add a trailing comma. it allows future patches to add a single line, less merge-conflict prone

  • Patch Set #18, Line 556: if (lchan->active_ms.cfg.rate_ctr) {

    (could just set the flag no matter if rate_ctr is set or not.)

To view, visit change 27081. To unsubscribe, or for help writing mail filters, visit settings.