| Message ID | 20230607090734.1259-1-haifeng.xu@shopee.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/mm_init.c: add debug messsge for dma zone | expand |
On Wed 07-06-23 09:07:34, Haifeng Xu wrote: > If freesize is less than dma_reserve, print warning message to report > this case. Why? > Signed-off-by: Haifeng Xu <haifeng.xu@shopee.com> > --- > mm/mm_init.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/mm/mm_init.c b/mm/mm_init.c > index 232efac9a929..9a9d6a52471c 100644 > --- a/mm/mm_init.c > +++ b/mm/mm_init.c > @@ -1561,9 +1561,14 @@ static void __init free_area_init_core(struct pglist_data *pgdat) > } > > /* Account for reserved pages */ > - if (j == 0 && freesize > dma_reserve) { > - freesize -= dma_reserve; > - pr_debug(" %s zone: %lu pages reserved\n", zone_names[0], dma_reserve); > + if (j == 0) { > + if (freesize >= dma_reserve) { > + freesize -= dma_reserve; > + pr_debug(" %s zone: %lu pages reserved\n", > + zone_names[0], dma_reserve); > + } else > + pr_warn(" %s zone: %lu reserved pages exceeds freesize %lu\n", > + zone_names[0], dma_reserve, freesize); > } > > if (!is_highmem_idx(j)) > -- > 2.25.1
On 07.06.23 12:16, Michal Hocko wrote: > On Wed 07-06-23 09:07:34, Haifeng Xu wrote: >> If freesize is less than dma_reserve, print warning message to report >> this case. > > Why? I'd like to second that question, and add a) Did you run into that scenario? b) What can an admin do in that case with that error messages? If it reveals a buggy situation, maybe a WARN_ON_ONCE() is warranted ... but maybe only if anybody actually ran into that issue.
On 2023/6/7 18:22, David Hildenbrand wrote: > On 07.06.23 12:16, Michal Hocko wrote: >> On Wed 07-06-23 09:07:34, Haifeng Xu wrote: >>> If freesize is less than dma_reserve, print warning message to report >>> this case. >> >> Why? > > I'd like to second that question, and add > > a) Did you run into that scenario? > b) What can an admin do in that case with that error messages? > > If it reveals a buggy situation, maybe a WARN_ON_ONCE() is warranted ... but maybe only if anybody actually ran into that issue. > I didn't run into that scenario, just from code review. I think the account for reserved pages is similar to memmap pages, if freesize is less than memmap pages, it print a warning message, so for dma_reserve, it can also does the same thing.
On 2023/6/7 18:22, David Hildenbrand wrote: > On 07.06.23 12:16, Michal Hocko wrote: >> On Wed 07-06-23 09:07:34, Haifeng Xu wrote: >>> If freesize is less than dma_reserve, print warning message to report >>> this case. >> >> Why? > > I'd like to second that question, and add > > a) Did you run into that scenario? > b) What can an admin do in that case with that error messages? In theory，dma_reserve shouldn't exceed freesize, so the error messages can remind us to verify whether the configuration of reserved memory is correct. > > If it reveals a buggy situation, maybe a WARN_ON_ONCE() is warranted ... but maybe only if anybody actually ran into that issue. >
On Thu 08-06-23 15:38:48, Haifeng Xu wrote: > > > On 2023/6/7 18:22, David Hildenbrand wrote: > > On 07.06.23 12:16, Michal Hocko wrote: > >> On Wed 07-06-23 09:07:34, Haifeng Xu wrote: > >>> If freesize is less than dma_reserve, print warning message to report > >>> this case. > >> > >> Why? > > > > I'd like to second that question, and add > > > > a) Did you run into that scenario? > > b) What can an admin do in that case with that error messages? > > In theory，dma_reserve shouldn't exceed freesize, so the error messages can remind us > to verify whether the configuration of reserved memory is correct. I am not really convinced this is worth touching the code TBH.
On Thu, Jun 08, 2023 at 11:18:02AM +0200, Michal Hocko wrote: > On Thu 08-06-23 15:38:48, Haifeng Xu wrote: > > > > > > On 2023/6/7 18:22, David Hildenbrand wrote: > > > On 07.06.23 12:16, Michal Hocko wrote: > > >> On Wed 07-06-23 09:07:34, Haifeng Xu wrote: > > >>> If freesize is less than dma_reserve, print warning message to report > > >>> this case. > > >> > > >> Why? > > > > > > I'd like to second that question, and add > > > > > > a) Did you run into that scenario? > > > b) What can an admin do in that case with that error messages? > > > > In theory，dma_reserve shouldn't exceed freesize, so the error messages can remind us > > to verify whether the configuration of reserved memory is correct. > > I am not really convinced this is worth touching the code TBH. The only architecture that sets the dma_reserve is x86_64 and it sets it to the number of reserved pages in DMA zone. There is no way freesize will be less than dma_reserve. I'm not sure that in general dma_reserve has some value now, but that's a completely different story. > -- > Michal Hocko > SUSE Labs
On 2023/6/8 18:13, Mike Rapoport wrote: > On Thu, Jun 08, 2023 at 11:18:02AM +0200, Michal Hocko wrote: >> On Thu 08-06-23 15:38:48, Haifeng Xu wrote: >>> >>> >>> On 2023/6/7 18:22, David Hildenbrand wrote: >>>> On 07.06.23 12:16, Michal Hocko wrote: >>>>> On Wed 07-06-23 09:07:34, Haifeng Xu wrote: >>>>>> If freesize is less than dma_reserve, print warning message to report >>>>>> this case. >>>>> >>>>> Why? >>>> >>>> I'd like to second that question, and add >>>> >>>> a) Did you run into that scenario? >>>> b) What can an admin do in that case with that error messages? >>> >>> In theory，dma_reserve shouldn't exceed freesize, so the error messages can remind us >>> to verify whether the configuration of reserved memory is correct. >> >> I am not really convinced this is worth touching the code TBH. > > The only architecture that sets the dma_reserve is x86_64 and it sets it to > the number of reserved pages in DMA zone. There is no way freesize will be > less than dma_reserve. Yes. From the comments, x86_64 calculates the dma_reserve in order to set zone watermarks more accurately. But berfore init_per_zone_wmark_min(), memblock_free_all() has already recalculated the managed pages. It seems that the dma_reserve is not really helpful to this. > > I'm not sure that in general dma_reserve has some value now, but that's a > completely different story. > >> -- >> Michal Hocko >> SUSE Labs >
diff --git a/mm/mm_init.c b/mm/mm_init.c index 232efac9a929..9a9d6a52471c 100644 --- a/mm/mm_init.c +++ b/mm/mm_init.c @@ -1561,9 +1561,14 @@ static void __init free_area_init_core(struct pglist_data *pgdat) } /* Account for reserved pages */ - if (j == 0 && freesize > dma_reserve) { - freesize -= dma_reserve; - pr_debug(" %s zone: %lu pages reserved\n", zone_names[0], dma_reserve); + if (j == 0) { + if (freesize >= dma_reserve) { + freesize -= dma_reserve; + pr_debug(" %s zone: %lu pages reserved\n", + zone_names[0], dma_reserve); + } else + pr_warn(" %s zone: %lu reserved pages exceeds freesize %lu\n", + zone_names[0], dma_reserve, freesize); } if (!is_highmem_idx(j))
If freesize is less than dma_reserve, print warning message to report this case. Signed-off-by: Haifeng Xu <haifeng.xu@shopee.com> --- mm/mm_init.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)