Attention is currently required from: neels, pespin, fixeria. lynxis lazus has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/29788 )

Change subject: Add BTS setup ramping to prevent BSC overloading ......................................................................

Patch Set 4:

(14 comments)

Patchset:

PS4: I hope I've addressed all your comments. I'll revisited all your comments tomorrow again to ensure I'ven't missed something.

File include/osmocom/bsc/bts_setup_ramp.h:

https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/8a675608_bbf435fb PS3, Line 31: BTS_SETUP_RAMP_INIT, /** initial state */

(osmo-bsc doesn't generate doxygen, but just saying: […]

Thank you!

File include/osmocom/bsc/nm_bts_ramp.h:

https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/56dc7feb_49f0ff6a PS1, Line 40: /*!

Because there can be multiple declarations of the function, but usually only one implementation.

Thank you for the explanation.

File src/osmo-bsc/bsc_vty.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/d011f286_c5c9da30 PS1, Line 1284: "allow-bts-configuration <0-65535>",

(i also prefer 'bts <0-255> foo')

Done

https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/de0aeb4d_77c496a8 PS1, Line 2659: vty_out(vty, " bts ramp limit %d within %d seconds%s",

I've changed it as fixeria suggested.

Done

https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/00d3fe9e_888f6435 PS1, Line 3087: "bts ramp limit <0-999> within <0-999> seconds",

It may not have a complex configuration NOW, but it may LATER, because we add more features and ways […]

Done

File src/osmo-bsc/bsc_vty.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/7dcd39db_17a2c54a PS3, Line 3091: "Number of BTS"

missing dot.

Done

File src/osmo-bsc/bts_setup_ramp.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/a0b26dd7_d6b938e2 PS3, Line 42: /* inform all MOs */

My comment regarding moving this to a generic address was not applied nor got feedback.

Done

https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/cc1d31d5_70734628 PS3, Line 62: * \brief Unblock a BTS from BTS setup ramping to allow it to setup and configure.

please don't use '\brief', our doxygen config has AUTOBRIEF set

Done

https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/3d28daaf_728e2118 PS3, Line 77: * \brief Timer callback and called by bts_setup_ramp_deactivate

drop \brief and end the line with a period (same below)

Done

https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/55349534_d146c047 PS3, Line 201: llist_del(&bts->bts_setup_ramp.list);

what's the content of &bts->bts_setup_ramp. […]

It's empty afterwards.

File src/osmo-bsc/nm_bb_transc_fsm.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/5334a6c9_a3034a82 PS1, Line 345: case NM_EV_RAMP_GO:

This has not yet been applied.

I've implemented it together with a libosmocore (ignore_invalid_event_mask). Without the libosmocore patch I don't really see the point because it would require to add NM_EV_RAMP_GO in every fsm or it would need to check the state of the fsm before dispatching the signal.

File src/osmo-bsc/nm_bts_fsm.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/c547ca0b_15126586 PS1, Line 92: struct gsm_bts *bts,

Nope, not addressed.

IMHO this is fine. last example of 6.2) looks the same. https://www.kernel.org/doc/html/latest/process/coding-style.html

File src/osmo-bsc/nm_bts_ramp.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/f3dd7853_f0068953 PS1, Line 42: /* inform all MOs */

It may make sense to have this moved into a function in nm_common_fsm. […]

Done