| Message ID | 1661483530-11308-1-git-send-email-zhaoyang.huang@unisoc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: skip reserved page for kmem leak scanning | expand |
On Fri, Aug 26, 2022 at 11:13 AM zhaoyang.huang <zhaoyang.huang@unisoc.com> wrote: > > From: Zhaoyang Huang <zhaoyang.huang@unisoc.com> > > It is no need to scan reserved page, skip it. > > Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com> > --- > mm/kmemleak.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/kmemleak.c b/mm/kmemleak.c > index a182f5d..c546250 100644 > --- a/mm/kmemleak.c > +++ b/mm/kmemleak.c > @@ -1471,7 +1471,7 @@ static void kmemleak_scan(void) > if (page_zone(page) != zone) > continue; > /* only scan if page is in use */ > - if (page_count(page) == 0) > + if (page_count(page) == 0 || PageReserved(page)) Sorry for previous stupid code by my faint, correct it here > continue; > scan_block(page, page + 1, NULL); > if (!(pfn & 63)) > -- > 1.9.1 >
On 26.08.22 05:23, Zhaoyang Huang wrote: > On Fri, Aug 26, 2022 at 11:13 AM zhaoyang.huang > <zhaoyang.huang@unisoc.com> wrote: >> >> From: Zhaoyang Huang <zhaoyang.huang@unisoc.com> >> >> It is no need to scan reserved page, skip it. >> >> Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com> >> --- >> mm/kmemleak.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/mm/kmemleak.c b/mm/kmemleak.c >> index a182f5d..c546250 100644 >> --- a/mm/kmemleak.c >> +++ b/mm/kmemleak.c >> @@ -1471,7 +1471,7 @@ static void kmemleak_scan(void) >> if (page_zone(page) != zone) >> continue; >> /* only scan if page is in use */ >> - if (page_count(page) == 0) >> + if (page_count(page) == 0 || PageReserved(page)) > Sorry for previous stupid code by my faint, correct it here Did you even test the initial patch? I wonder why we should consider this change (a) I doubt it's a performance issue. If it is, please provide numbers before/after. (b) We'll stop scanning early allocations. As the memmap is usually allocated early during boot ... we'll stop scanning essentially the whole mmap and that whole loop would be dead code? What am i missing?
On Mon, Aug 29, 2022 at 8:19 PM David Hildenbrand <david@redhat.com> wrote: > > On 26.08.22 05:23, Zhaoyang Huang wrote: > > On Fri, Aug 26, 2022 at 11:13 AM zhaoyang.huang > > <zhaoyang.huang@unisoc.com> wrote: > >> > >> From: Zhaoyang Huang <zhaoyang.huang@unisoc.com> > >> > >> It is no need to scan reserved page, skip it. > >> > >> Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com> > >> --- > >> mm/kmemleak.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/mm/kmemleak.c b/mm/kmemleak.c > >> index a182f5d..c546250 100644 > >> --- a/mm/kmemleak.c > >> +++ b/mm/kmemleak.c > >> @@ -1471,7 +1471,7 @@ static void kmemleak_scan(void) > >> if (page_zone(page) != zone) > >> continue; > >> /* only scan if page is in use */ > >> - if (page_count(page) == 0) > >> + if (page_count(page) == 0 || PageReserved(page)) > > Sorry for previous stupid code by my faint, correct it here > > Did you even test the initial patch? > > I wonder why we should consider this change > > (a) I doubt it's a performance issue. If it is, please provide numbers > before/after. For Android-like SOC systems where AP(cpu runs linux) are one of the memory consumers which are composed of other processors such as modem, isp,wcn etc. The reserved memory occupies a certain number of memory(could be 30% of MemTotal) which makes scan reserved pages pointless. > (b) We'll stop scanning early allocations. As the memmap is usually > allocated early during boot ... we'll stop scanning essentially the > whole mmap and that whole loop would be dead code? What am i > missing? memmap refers to pages here? If we can surpass these as it exist permanently during life period. Besides, I wonder if PageLRU should also be skipped? - if (page_count(page) == 0) + if (page_count(page) == 0 || PageReserved(page) || PageLRU(page)) > > -- > Thanks, > > David / dhildenb >
On 30.08.22 04:41, Zhaoyang Huang wrote: > On Mon, Aug 29, 2022 at 8:19 PM David Hildenbrand <david@redhat.com> wrote: >> >> On 26.08.22 05:23, Zhaoyang Huang wrote: >>> On Fri, Aug 26, 2022 at 11:13 AM zhaoyang.huang >>> <zhaoyang.huang@unisoc.com> wrote: >>>> >>>> From: Zhaoyang Huang <zhaoyang.huang@unisoc.com> >>>> >>>> It is no need to scan reserved page, skip it. >>>> >>>> Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com> >>>> --- >>>> mm/kmemleak.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/mm/kmemleak.c b/mm/kmemleak.c >>>> index a182f5d..c546250 100644 >>>> --- a/mm/kmemleak.c >>>> +++ b/mm/kmemleak.c >>>> @@ -1471,7 +1471,7 @@ static void kmemleak_scan(void) >>>> if (page_zone(page) != zone) >>>> continue; >>>> /* only scan if page is in use */ >>>> - if (page_count(page) == 0) >>>> + if (page_count(page) == 0 || PageReserved(page)) >>> Sorry for previous stupid code by my faint, correct it here >> >> Did you even test the initial patch? >> >> I wonder why we should consider this change >> >> (a) I doubt it's a performance issue. If it is, please provide numbers >> before/after. > For Android-like SOC systems where AP(cpu runs linux) are one of the > memory consumers which are composed of other processors such as modem, > isp,wcn etc. The reserved memory occupies a certain number of > memory(could be 30% of MemTotal) which makes scan reserved pages > pointless. But we only scan the memmap (struct page) here and not the actual memory. Do you have any performance numbers showing that there is even an observable change? >> (b) We'll stop scanning early allocations. As the memmap is usually >> allocated early during boot ... we'll stop scanning essentially the >> whole mmap and that whole loop would be dead code? What am i >> missing? > memmap refers to pages here? If we can surpass these as it exist > permanently during life period. Besides, I wonder if PageLRU should > also be skipped? > - if (page_count(page) == 0) > + if (page_count(page) == 0 || > PageReserved(page) || PageLRU(page)) I think we need a really good justification to start poking holes into the memmap scanner. I'm no expert on this code (and under which circumstances we actually might find referenced objects in a memmap), though. But we should be careful with that.
On Tue, Aug 30, 2022 at 4:03 PM David Hildenbrand <david@redhat.com> wrote: > > On 30.08.22 04:41, Zhaoyang Huang wrote: > > On Mon, Aug 29, 2022 at 8:19 PM David Hildenbrand <david@redhat.com> wrote: > >> > >> On 26.08.22 05:23, Zhaoyang Huang wrote: > >>> On Fri, Aug 26, 2022 at 11:13 AM zhaoyang.huang > >>> <zhaoyang.huang@unisoc.com> wrote: > >>>> > >>>> From: Zhaoyang Huang <zhaoyang.huang@unisoc.com> > >>>> > >>>> It is no need to scan reserved page, skip it. > >>>> > >>>> Signed-off-by: Zhaoyang Huang <zhaoyang.huang@unisoc.com> > >>>> --- > >>>> mm/kmemleak.c | 2 +- > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>> > >>>> diff --git a/mm/kmemleak.c b/mm/kmemleak.c > >>>> index a182f5d..c546250 100644 > >>>> --- a/mm/kmemleak.c > >>>> +++ b/mm/kmemleak.c > >>>> @@ -1471,7 +1471,7 @@ static void kmemleak_scan(void) > >>>> if (page_zone(page) != zone) > >>>> continue; > >>>> /* only scan if page is in use */ > >>>> - if (page_count(page) == 0) > >>>> + if (page_count(page) == 0 || PageReserved(page)) > >>> Sorry for previous stupid code by my faint, correct it here > >> > >> Did you even test the initial patch? > >> > >> I wonder why we should consider this change > >> > >> (a) I doubt it's a performance issue. If it is, please provide numbers > >> before/after. > > For Android-like SOC systems where AP(cpu runs linux) are one of the > > memory consumers which are composed of other processors such as modem, > > isp,wcn etc. The reserved memory occupies a certain number of > > memory(could be 30% of MemTotal) which makes scan reserved pages > > pointless. > > But we only scan the memmap (struct page) here and not the actual > memory. Do you have any performance numbers showing that there is even > an observable change? > > >> (b) We'll stop scanning early allocations. As the memmap is usually > >> allocated early during boot ... we'll stop scanning essentially the > >> whole mmap and that whole loop would be dead code? What am i > >> missing? > > memmap refers to pages here? If we can surpass these as it exist > > permanently during life period. Besides, I wonder if PageLRU should > > also be skipped? > > - if (page_count(page) == 0) > > + if (page_count(page) == 0 || > > PageReserved(page) || PageLRU(page)) > > I think we need a really good justification to start poking holes into > the memmap scanner. I'm no expert on this code (and under which > circumstances we actually might find referenced objects in a memmap), > though. > > But we should be careful with that. Agree. It may be helpless as kmemleak is for debugging purposes. Nack this patch by myself. Sorry for interrupt. > > -- > Thanks, > > David / dhildenb >
diff --git a/mm/kmemleak.c b/mm/kmemleak.c index a182f5d..c546250 100644 --- a/mm/kmemleak.c +++ b/mm/kmemleak.c @@ -1471,7 +1471,7 @@ static void kmemleak_scan(void) if (page_zone(page) != zone) continue; /* only scan if page is in use */ - if (page_count(page) == 0) + if (page_count(page) == 0 && !PageReserved(page)) continue; scan_block(page, page + 1, NULL); if (!(pfn & 63))