Attention is currently required from: neels, laforge, fixeria, pespin. 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 16:
(7 comments)
File include/osmocom/bsc/bts_setup_ramp.h:
https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/5881e245_9af48487 PS3, Line 31: BTS_SETUP_RAMP_INIT, /** initial state */
I am surprised it took so many iterations, but still does not match what Neels actually suggested. […]
One is just wrong. doxygen doesn't accept a single one. Or maybe you're using a different version or config of doxygen. It's either /*! (java doc) or /**< but not /*< according https://www.doxygen.nl/manual/docblocks.html#specialblock I've tested it with doxygen using the config from libosmocore.
I've switched to /*!, but honestly we should have a linter yelling here as well.
File src/osmo-bsc/bsc_vty.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/275d9f81_3c967533 PS4, Line 3100: bts-setup-ramping-step-interval
I am sorry, but no. This should be addressed before merging.
As pespin proposed I followed the example of access-control-class-ramping and be consistent with access-control-class-ramping. I don't want to do more review rounds with different taste and style between different reviewers. A proper solution for this would be a vty style guide including good and bad examples.
File src/osmo-bsc/bts_setup_ramp.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/4f4a3b8e_c9a0262f PS4, Line 98: && net->bts_setup_ramp.step_size > 0
Ack
Done
https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/a7d62928_386b6fd1 PS4, Line 101: net->bts_setup_ramp.active
Ack
Done
https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/225ab2d6_e9c3bcaa PS4, Line 201: INIT_LLIST_HEAD(&bts->bts_setup_ramp.list);
The Linux kernel llist API has always worked in the way that you cannot llist_del() a llist_entry th […]
Maybe I missed the point. But I don't see any issue with the current implementation nor do I understand why it needs to be an explicit comment in here.
File src/osmo-bsc/bts_setup_ramp.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/7d943aaf_9af8c4e2 PS15, Line 51: if (llist_empty(&bts->bts_setup_ramp.list))
Why don't you check the bts->bts_setup_ramp.state against BTS_SETUP_RAMP_WAIT here? […]
good point. only possible after patchset14
https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/4a93f6bd_846517e9 PS15, Line 74: net->bts_setup_ramp.count >= net->bts_setup_ramp.step_size)
Ack
Done