Make hard-coded values available as sysctl tunables.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
| sys/dev/dpaa2/dpaa2_ni.c | ||
|---|---|---|
| 455 | We've the only budget here, so "budget" would be shorter. Besides, consider making it read-only with "const int". It doesn't look like the variable itself is written in the function. | |
| 456 | same here | |
| 1810 | Could you re-write with SYSCTL_ADD_PROC as for "buf_num" and "buf_free"? You already know how not only to read, but to write a value from sysctl ;) | |
| 1813 | Should be "&sc->tx_budget", I guess | |
| 1815 | Should be "&sc->rx_budget", I guess | |
| 2890 | Please, declare local variables for the budgets like "uint32_t buf_num = DPAA2_ATOMIC_READ(&sc->buf_num);" and use them instead of those kept in the sc. It'd help to avoid inconsistency when someone modifies sysctl while the cleanup task is running. | |
| sys/dev/dpaa2/dpaa2_ni.h | ||
| 488 | There's a sysctl(9) section below in the same sc with "buf_num" and "buf_free". Put those guys there, please. Besides, I'd still prefer "struct dpaa2_atomic" for the budget variables. It might not be necessary for the 32-bit signed variables which will probably by read/written atomically on ARM64, but I just want to be sure. P.S. This is exactly the reason why full -U999999 context is needed ;) | |
| sys/dev/dpaa2/dpaa2_ni.c | ||
|---|---|---|
| 455 | The implementation contains a local variable called budget. I can of course rename the parameter, but then I have to rename the local variable, too. | |
| 1810 | If I understood the documentation correctly, the SYSCTL_ADD_INT() functions do support reading and writing the value, but they do nothing else, like calling update functions. But since I will change the variable type to dpaa2_atomic, I have to use custom handlers anyway. | |
| sys/dev/dpaa2/dpaa2_ni.h | ||
|---|---|---|
| 488 | Extra context added with this patch. | |
| sys/dev/dpaa2/dpaa2_ni.c | ||
|---|---|---|
| 2895 | Budgets aren't written in the rest of the function, i.e. "const int" please. | |
| 3605 | Please, declare the function at the top of the file. And a short comment what it does would be handy as I struggled a bit initially. | |
| 3616 | I'd move hardcoded values to the macros. Something like "...BUDGET_MIN" and "...BUDGET_MAX". | |
| sys/dev/dpaa2/dpaa2_ni.c | ||
|---|---|---|
| 3616 | Are these plaubsible and useful limits? | |
| sys/dev/dpaa2/dpaa2_ni.c | ||
|---|---|---|
| 3616 | Yes, I think so. At the moment, at least. | |
Comment Actions
Could you rebase to the latest main as well as it doesn't apply cleanly with https://reviews.freebsd.org/D59463 pushed?
Comment Actions
- Make local variables read from atomics const.
- Extract magic numbers to preprocessor defines.
- Add declaration and comment to sysctl helper function.