| Message ID | 20240325121905.2869271-1-john.g.garry@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blk-throttle: Fix W=1 build issue in tg_prfill_limit() | expand |
On Mon, Mar 25, 2024 at 12:19:05PM +0000, John Garry wrote: > For when using gcc 8 and above, the following warnings can be seen when > compiling blk-throttle.c with W=1: Why is this function even using these local buffers vs a sequence of separate seq_printf calls that would get rid of these pointless on-stack buffers?
On 26/03/2024 07:03, Christoph Hellwig wrote: > On Mon, Mar 25, 2024 at 12:19:05PM +0000, John Garry wrote: >> For when using gcc 8 and above, the following warnings can be seen when >> compiling blk-throttle.c with W=1: > > Why is this function even using these local buffers vs a sequence > of separate seq_printf calls that would get rid of these pointless > on-stack buffers? Currently a combo of snprintf and seq_printf is used, a strategy which seems to go as far back as 2ee867dcfa2ea (2015), when it was a much simpler print. All the other code in this area only uses seq_printf, so it seems that the author(s) prefer this way here. Here's how the current code could look (using only seq_printf): static u64 tg_prfill_limit(struct seq_file *sf, struct blkg_policy_data *pd, int off) { struct throtl_grp *tg = pd_to_tg(pd); const char *dname = blkg_dev_name(pd->blkg); u64 bps_dft; unsigned int iops_dft; if (!dname) return 0; if (off == LIMIT_LOW) { bps_dft = 0; iops_dft = 0; } else { bps_dft = U64_MAX; iops_dft = UINT_MAX; } if (tg->bps_conf[READ][off] == bps_dft && tg->bps_conf[WRITE][off] == bps_dft && tg->iops_conf[READ][off] == iops_dft && tg->iops_conf[WRITE][off] == iops_dft && (off != LIMIT_LOW || (tg->idletime_threshold_conf == DFL_IDLE_THRESHOLD && tg->latency_target_conf == DFL_LATENCY_TARGET))) return 0; seq_printf(sf, "%s rbps=", dname); if (tg->bps_conf[READ][off] == U64_MAX) seq_printf(sf, "max"); else seq_printf(sf, "%llu", tg->bps_conf[READ][off]); if (tg->bps_conf[WRITE][off] == U64_MAX) seq_printf(sf, " wbps=max "); else seq_printf(sf, " wbps=%llu ", tg->bps_conf[WRITE][off]); if (tg->iops_conf[READ][off] == UINT_MAX) seq_printf(sf, " riops=max"); else seq_printf(sf, " riops=%u", tg->iops_conf[READ][off]); if (tg->iops_conf[WRITE][off] == UINT_MAX) seq_printf(sf, " wiops=max"); else seq_printf(sf, " wiops=%u", tg->iops_conf[WRITE][off]); if (off == LIMIT_LOW) { if (tg->idletime_threshold_conf == ULONG_MAX) seq_printf(sf, " idle=max"); else seq_printf(sf, " idle=%lu", tg->idletime_threshold_conf); if (tg->latency_target_conf == ULONG_MAX) seq_printf(sf, " latency=max"); else seq_printf(sf, " latency=%lu", tg->latency_target_conf); } return 0; } better? Thanks, John
On Tue, Mar 26, 2024 at 08:46:24AM +0000, John Garry wrote: > On 26/03/2024 07:03, Christoph Hellwig wrote: > > On Mon, Mar 25, 2024 at 12:19:05PM +0000, John Garry wrote: > > > For when using gcc 8 and above, the following warnings can be seen when > > > compiling blk-throttle.c with W=1: > > > > Why is this function even using these local buffers vs a sequence > > of separate seq_printf calls that would get rid of these pointless > > on-stack buffers? > > Currently a combo of snprintf and seq_printf is used, a strategy which seems > to go as far back as 2ee867dcfa2ea (2015), when it was a much simpler print. > All the other code in this area only uses seq_printf, so it seems that the > author(s) prefer this way here. > > Here's how the current code could look (using only seq_printf): That looks much better, thanks.
On 26/03/2024 14:49, Christoph Hellwig wrote: >> Here's how the current code could look (using only seq_printf): > That looks much better, thanks. ok, I'll put it in a formal patch. It would be nice to put purely a "fix" for stable, but no one else has complained about this old warning AFAIK. Thanks, John
diff --git a/block/blk-throttle.c b/block/blk-throttle.c index f4850a6f860b..ece272937792 100644 --- a/block/blk-throttle.c +++ b/block/blk-throttle.c @@ -1497,8 +1497,8 @@ static u64 tg_prfill_limit(struct seq_file *sf, struct blkg_policy_data *pd, char bufs[4][21] = { "max", "max", "max", "max" }; u64 bps_dft; unsigned int iops_dft; - char idle_time[26] = ""; - char latency_time[26] = ""; + char idle_time[27] = ""; + char latency_time[30] = ""; if (!dname) return 0;
For when using gcc 8 and above, the following warnings can be seen when compiling blk-throttle.c with W=1: block/blk-throttle.c: In function ‘tg_prfill_limit’: block/blk-throttle.c:1539:53: error: ‘snprintf’ output may be truncated before the last format character [-Werror=format-truncation=] snprintf(idle_time, sizeof(idle_time), " idle=%lu", ^ block/blk-throttle.c:1539:4: note: ‘snprintf’ output between 8 and 27 bytes into a destination of size 26 snprintf(idle_time, sizeof(idle_time), " idle=%lu", ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ tg->idletime_threshold_conf); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ block/blk-throttle.c:1546:15: error: ‘%lu’ directive output may be truncated writing between 1 and 20 bytes into a region of size 17 [-Werror=format-truncation=] " latency=%lu", tg->latency_target_conf); ^~~ block/blk-throttle.c:1546:5: note: directive argument in the range [0, 18446744073709551614] " latency=%lu", tg->latency_target_conf); ^~~~~~~~~~~~~~ block/blk-throttle.c:1545:4: note: ‘snprintf’ output between 11 and 30 bytes into a destination of size 26 snprintf(latency_time, sizeof(latency_time), ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ " latency=%lu", tg->latency_target_conf); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors make[3]: *** [scripts/Makefile.build:244: block/blk-throttle.o] Error 1 Increase idle_time[] and latency_time[] sizes to clear the warnings. Fixes: ec80991d6fc2c ("blk-throttle: add interface for per-cgroup target latency") Signed-off-by: John Garry <john.g.garry@oracle.com> --- block/blk-throttle.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)