| Message ID | 20231204085735.4112882-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] net: hns3: reduce stack usage in hclge_dbg_dump_tm_pri() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
on 2023/12/4 16:57, Arnd Bergmann wrote: > s already allocated by debugfs, > but that is a much larger change. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > v2: fix error handling leak > --- > .../hisilicon/hns3/hns3pf/hclge_debugfs.c | 21 ++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c > index ff3f8f424ad9..8f94e13c1edf 100644 > --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c > +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c > @@ -981,7 +981,7 @@ static const struct hclge_dbg_item tm_pri_items[] = { > > static int hclge_dbg_dump_tm_pri(struct hclge_dev *hdev, char *buf, int len) > { > - char data_str[ARRAY_SIZE(tm_pri_items)][HCLGE_DBG_DATA_STR_LEN]; > + char *data_str; We want to define variables in an inverted triangle based on the code length. so, "char *data_str" should move four lines down. struct hclge_tm_shaper_para c_shaper_para, p_shaper_para; char *result[ARRAY_SIZE(tm_pri_items)], *sch_mode_str; char content[HCLGE_DBG_TM_INFO_LEN]; u8 pri_num, sch_mode, weight, i, j; char *data_str; int pos, ret; > struct hclge_tm_shaper_para c_shaper_para, p_shaper_para; > char *result[ARRAY_SIZE(tm_pri_items)], *sch_mode_str; > char content[HCLGE_DBG_TM_INFO_LEN]; > @@ -992,8 +992,13 @@ static int hclge_dbg_dump_tm_pri(struct hclge_dev *hdev, char *buf, int len) > if (ret) > return ret; >
Le 04/12/2023 à 09:57, Arnd Bergmann a écrit : > From: Arnd Bergmann <arnd@arndb.de> > > This function exceeds the stack frame warning limit: > > drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c: In function 'hclge_dbg_dump_tm_pri': > drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c:1039:1: error: the frame size of 1408 bytes is larger than 1024 bytes [-Werror=frame-larger-than=] > > Use dynamic allocation for the largest stack object instead. It > would be nice to rewrite this file to completely avoid the extra > buffer and just use the one that was already allocated by debugfs, > but that is a much larger change. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > v2: fix error handling leak > --- > .../hisilicon/hns3/hns3pf/hclge_debugfs.c | 21 ++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c > index ff3f8f424ad9..8f94e13c1edf 100644 > --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c > +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c > @@ -981,7 +981,7 @@ static const struct hclge_dbg_item tm_pri_items[] = { > > static int hclge_dbg_dump_tm_pri(struct hclge_dev *hdev, char *buf, int len) > { > - char data_str[ARRAY_SIZE(tm_pri_items)][HCLGE_DBG_DATA_STR_LEN]; > + char *data_str; > struct hclge_tm_shaper_para c_shaper_para, p_shaper_para; > char *result[ARRAY_SIZE(tm_pri_items)], *sch_mode_str; > char content[HCLGE_DBG_TM_INFO_LEN]; > @@ -992,8 +992,13 @@ static int hclge_dbg_dump_tm_pri(struct hclge_dev *hdev, char *buf, int len) > if (ret) > return ret; > > + data_str = kcalloc(ARRAY_SIZE(tm_pri_items), HCLGE_DBG_DATA_STR_LEN, > + GFP_KERNEL); > + if (!data_str) > + return -ENOMEM; > + > for (i = 0; i < ARRAY_SIZE(tm_pri_items); i++) > - result[i] = &data_str[i][0]; > + result[i] = &data_str[i * HCLGE_DBG_DATA_STR_LEN]; > > hclge_dbg_fill_content(content, sizeof(content), tm_pri_items, > NULL, ARRAY_SIZE(tm_pri_items)); > @@ -1002,23 +1007,23 @@ static int hclge_dbg_dump_tm_pri(struct hclge_dev *hdev, char *buf, int len) > for (i = 0; i < pri_num; i++) { > ret = hclge_tm_get_pri_sch_mode(hdev, i, &sch_mode); > if (ret) > - return ret; > + goto out; > > ret = hclge_tm_get_pri_weight(hdev, i, &weight); > if (ret) > - return ret; > + goto out; > > ret = hclge_tm_get_pri_shaper(hdev, i, > HCLGE_OPC_TM_PRI_C_SHAPPING, > &c_shaper_para); > if (ret) > - return ret; > + goto out; > > ret = hclge_tm_get_pri_shaper(hdev, i, > HCLGE_OPC_TM_PRI_P_SHAPPING, > &p_shaper_para); > if (ret) > - return ret; > + goto out; > > sch_mode_str = sch_mode & HCLGE_TM_TX_SCHD_DWRR_MSK ? "dwrr" : > "sp"; > @@ -1035,7 +1040,9 @@ static int hclge_dbg_dump_tm_pri(struct hclge_dev *hdev, char *buf, int len) > pos += scnprintf(buf + pos, len - pos, "%s", content); > } > > - return 0; > +out: > + kfree(data_str); > + return ret; > } > > static const struct hclge_dbg_item tm_qset_items[] = { Hi, could : pos += scnprintf(buf + pos, len - pos, "%s", <something>); be more widely used to avoid the alloc()/free() + copy of strings? CJ
On Mon, Dec 4, 2023, at 19:54, Christophe JAILLET wrote: > Le 04/12/2023 à 09:57, Arnd Bergmann a écrit : >> >> Use dynamic allocation for the largest stack object instead. It >> would be nice to rewrite this file to completely avoid the extra >> buffer and just use the one that was already allocated by debugfs, >> but that is a much larger change. >> > could : > pos += scnprintf(buf + pos, len - pos, "%s", <something>); > be more widely used to avoid the alloc()/free() + copy of strings? Yes, I think that would help, but this is beyond what I trust myself to do blindly. Arnd
On Mon, 4 Dec 2023 22:50:55 +0800 Jijie Shao wrote: > > static int hclge_dbg_dump_tm_pri(struct hclge_dev *hdev, char *buf, int len) > > { > > - char data_str[ARRAY_SIZE(tm_pri_items)][HCLGE_DBG_DATA_STR_LEN]; > > + char *data_str; > > We want to define variables in an inverted triangle based on the code length. > so, "char *data_str" should move four lines down. > > struct hclge_tm_shaper_para c_shaper_para, p_shaper_para; > char *result[ARRAY_SIZE(tm_pri_items)], *sch_mode_str; > char content[HCLGE_DBG_TM_INFO_LEN]; > u8 pri_num, sch_mode, weight, i, j; > char *data_str; > int pos, ret; I took the liberty of fixing this when applying. Don't want this to fall thru the cracks. Applied to net-next now, thanks!
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c index ff3f8f424ad9..8f94e13c1edf 100644 --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c @@ -981,7 +981,7 @@ static const struct hclge_dbg_item tm_pri_items[] = { static int hclge_dbg_dump_tm_pri(struct hclge_dev *hdev, char *buf, int len) { - char data_str[ARRAY_SIZE(tm_pri_items)][HCLGE_DBG_DATA_STR_LEN]; + char *data_str; struct hclge_tm_shaper_para c_shaper_para, p_shaper_para; char *result[ARRAY_SIZE(tm_pri_items)], *sch_mode_str; char content[HCLGE_DBG_TM_INFO_LEN]; @@ -992,8 +992,13 @@ static int hclge_dbg_dump_tm_pri(struct hclge_dev *hdev, char *buf, int len) if (ret) return ret; + data_str = kcalloc(ARRAY_SIZE(tm_pri_items), HCLGE_DBG_DATA_STR_LEN, + GFP_KERNEL); + if (!data_str) + return -ENOMEM; + for (i = 0; i < ARRAY_SIZE(tm_pri_items); i++) - result[i] = &data_str[i][0]; + result[i] = &data_str[i * HCLGE_DBG_DATA_STR_LEN]; hclge_dbg_fill_content(content, sizeof(content), tm_pri_items, NULL, ARRAY_SIZE(tm_pri_items)); @@ -1002,23 +1007,23 @@ static int hclge_dbg_dump_tm_pri(struct hclge_dev *hdev, char *buf, int len) for (i = 0; i < pri_num; i++) { ret = hclge_tm_get_pri_sch_mode(hdev, i, &sch_mode); if (ret) - return ret; + goto out; ret = hclge_tm_get_pri_weight(hdev, i, &weight); if (ret) - return ret; + goto out; ret = hclge_tm_get_pri_shaper(hdev, i, HCLGE_OPC_TM_PRI_C_SHAPPING, &c_shaper_para); if (ret) - return ret; + goto out; ret = hclge_tm_get_pri_shaper(hdev, i, HCLGE_OPC_TM_PRI_P_SHAPPING, &p_shaper_para); if (ret) - return ret; + goto out; sch_mode_str = sch_mode & HCLGE_TM_TX_SCHD_DWRR_MSK ? "dwrr" : "sp"; @@ -1035,7 +1040,9 @@ static int hclge_dbg_dump_tm_pri(struct hclge_dev *hdev, char *buf, int len) pos += scnprintf(buf + pos, len - pos, "%s", content); } - return 0; +out: + kfree(data_str); + return ret; } static const struct hclge_dbg_item tm_qset_items[] = {