On Fri, Aug 26, 2022 at 11:13 AM zhaoyang.huang
<[email protected]> wrote:
>
> From: Zhaoyang Huang <[email protected]>
>
> It is no need to scan reserved page, skip it.
>
> Signed-off-by: Zhaoyang Huang <[email protected]>
> ---
> 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
> <[email protected]> wrote:
>>
>> From: Zhaoyang Huang <[email protected]>
>>
>> It is no need to scan reserved page, skip it.
>>
>> Signed-off-by: Zhaoyang Huang <[email protected]>
>> ---
>> 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?
--
Thanks,
David / dhildenb
On Mon, Aug 29, 2022 at 8:19 PM David Hildenbrand <[email protected]> wrote:
>
> On 26.08.22 05:23, Zhaoyang Huang wrote:
> > On Fri, Aug 26, 2022 at 11:13 AM zhaoyang.huang
> > <[email protected]> wrote:
> >>
> >> From: Zhaoyang Huang <[email protected]>
> >>
> >> It is no need to scan reserved page, skip it.
> >>
> >> Signed-off-by: Zhaoyang Huang <[email protected]>
> >> ---
> >> 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 <[email protected]> wrote:
>>
>> On 26.08.22 05:23, Zhaoyang Huang wrote:
>>> On Fri, Aug 26, 2022 at 11:13 AM zhaoyang.huang
>>> <[email protected]> wrote:
>>>>
>>>> From: Zhaoyang Huang <[email protected]>
>>>>
>>>> It is no need to scan reserved page, skip it.
>>>>
>>>> Signed-off-by: Zhaoyang Huang <[email protected]>
>>>> ---
>>>> 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.
--
Thanks,
David / dhildenb
On Tue, Aug 30, 2022 at 4:03 PM David Hildenbrand <[email protected]> wrote:
>
> On 30.08.22 04:41, Zhaoyang Huang wrote:
> > On Mon, Aug 29, 2022 at 8:19 PM David Hildenbrand <[email protected]> wrote:
> >>
> >> On 26.08.22 05:23, Zhaoyang Huang wrote:
> >>> On Fri, Aug 26, 2022 at 11:13 AM zhaoyang.huang
> >>> <[email protected]> wrote:
> >>>>
> >>>> From: Zhaoyang Huang <[email protected]>
> >>>>
> >>>> It is no need to scan reserved page, skip it.
> >>>>
> >>>> Signed-off-by: Zhaoyang Huang <[email protected]>
> >>>> ---
> >>>> 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
>