* Re: [PATCH v4 3/5] [RFC] arm64: Add support for idle bit in swap PTE
From: Michal Hocko @ 2019-08-06 11:57 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, Robin Murphy, Alexey Dobriyan, Andrew Morton,
Borislav Petkov, Brendan Gregg, Catalin Marinas, Christian Hansen,
dancol, fmayer, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
Kees Cook, kernel-team, linux-api, linux-doc, linux-fsdevel,
linux-mm, Mike Rapoport, minchan, namhyung, paulmck,
Roman Gushchin, Stephen Rothwell, surenb, Thomas Gleixner, tkjos,
Vladimir Davydov, Vlastimil Babka, Will Deacon
In-Reply-To: <20190806111446.GA117316@google.com>
On Tue 06-08-19 07:14:46, Joel Fernandes wrote:
> On Tue, Aug 06, 2019 at 12:47:55PM +0200, Michal Hocko wrote:
> > On Tue 06-08-19 06:36:27, Joel Fernandes wrote:
> > > On Tue, Aug 06, 2019 at 10:42:03AM +0200, Michal Hocko wrote:
> > > > On Mon 05-08-19 13:04:49, Joel Fernandes (Google) wrote:
> > > > > This bit will be used by idle page tracking code to correctly identify
> > > > > if a page that was swapped out was idle before it got swapped out.
> > > > > Without this PTE bit, we lose information about if a page is idle or not
> > > > > since the page frame gets unmapped.
> > > >
> > > > And why do we need that? Why cannot we simply assume all swapped out
> > > > pages to be idle? They were certainly idle enough to be reclaimed,
> > > > right? Or what does idle actualy mean here?
> > >
> > > Yes, but other than swapping, in Android a page can be forced to be swapped
> > > out as well using the new hints that Minchan is adding?
> >
> > Yes and that is effectivelly making them idle, no?
>
> That depends on how you think of it.
I would much prefer to have it documented so that I do not have to guess ;)
> If you are thinking of a monitoring
> process like a heap profiler, then from the heap profiler's (that only cares
> about the process it is monitoring) perspective it will look extremely odd if
> pages that are recently accessed by the process appear to be idle which would
> falsely look like those processes are leaking memory. The reality being,
> Android forced those pages into swap because of other reasons. I would like
> for the swapping mechanism, whether forced swapping or memory reclaim, not to
> interfere with the idle detection.
Hmm, but how are you going to handle situation when the page is unmapped
and refaulted again (e.g. a normal reclaim of a pagecache)? You are
losing that information same was as in the swapout case, no? Or am I
missing something?
> This is just an effort to make the idle tracking a little bit better. We
> would like to not lose the 'accessed' information of the pages.
>
> Initially, I had proposed what you are suggesting as well however the above
> reasons made me to do it like this. Also Minchan and Konstantin suggested
> this, so there are more people interested in the swap idle bit. Minchan, can
> you provide more thoughts here? (He is on 2-week vacation from today so
> hopefully replies before he vanishes ;-)).
We can move on with the rest of the series in the mean time but I would
like to see a proper justification for the swap entries and why they
should be handled special.
> Also assuming all swap pages as idle has other "semantic" issues. It is quite
> odd if a swapped page is automatically marked as idle without userspace
> telling it to. Consider the following set of events: 1. Userspace marks only
> a certain memory region as idle. 2. Userspace reads back the bits
> corresponding to a bigger region. Part of this bigger region is swapped.
> Userspace expects all of the pages it did not mark, to have idle bit set to
> '0' because it never marked them as idle. However if it is now surprised by
> what it read back (not all '0' read back). Since a page is swapped, it will
> be now marked "automatically" as idle as per your proposal, even if userspace
> never marked it explicity before. This would be quite confusing/ambiguous.
OK, I see. I guess the primary question I have is how do you distinguish
Idle page which got unmapped and faulted in again from swapped out page
and refaulted - including the time the pte is not present.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v4 4/5] page_idle: Drain all LRU pagevec before idle tracking
From: Michal Hocko @ 2019-08-06 11:44 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, Alexey Dobriyan, Andrew Morton, Borislav Petkov,
Brendan Gregg, Catalin Marinas, Christian Hansen, dancol, fmayer,
H. Peter Anvin, Ingo Molnar, Jonathan Corbet, Kees Cook,
kernel-team, linux-api, linux-doc, linux-fsdevel, linux-mm,
Mike Rapoport, minchan, namhyung, paulmck, Robin Murphy,
Roman Gushchin, Stephen Rothwell, surenb, Thomas Gleixner, tkjos,
Vladimir Davydov, Vlastimil Babka, Will Deacon
In-Reply-To: <20190806111921.GB117316@google.com>
On Tue 06-08-19 07:19:21, Joel Fernandes wrote:
> On Tue, Aug 06, 2019 at 12:51:49PM +0200, Michal Hocko wrote:
> > On Tue 06-08-19 06:45:54, Joel Fernandes wrote:
> > > On Tue, Aug 06, 2019 at 10:43:57AM +0200, Michal Hocko wrote:
> > > > On Mon 05-08-19 13:04:50, Joel Fernandes (Google) wrote:
> > > > > During idle tracking, we see that sometimes faulted anon pages are in
> > > > > pagevec but are not drained to LRU. Idle tracking considers pages only
> > > > > on LRU. Drain all CPU's LRU before starting idle tracking.
> > > >
> > > > Please expand on why does this matter enough to introduce a potentially
> > > > expensinve draining which has to schedule a work on each CPU and wait
> > > > for them to finish.
> > >
> > > Sure, I can expand. I am able to find multiple issues involving this. One
> > > issue looks like idle tracking is completely broken. It shows up in my
> > > testing as if a page that is marked as idle is always "accessed" -- because
> > > it was never marked as idle (due to not draining of pagevec).
> > >
> > > The other issue shows up as a failure in my "swap test", with the following
> > > sequence:
> > > 1. Allocate some pages
> > > 2. Write to them
> > > 3. Mark them as idle <--- fails
> > > 4. Introduce some memory pressure to induce swapping.
> > > 5. Check the swap bit I introduced in this series. <--- fails to set idle
> > > bit in swap PTE.
> > >
> > > Draining the pagevec in advance fixes both of these issues.
> >
> > This belongs to the changelog.
>
> Sure, will add.
>
>
> > > This operation even if expensive is only done once during the access of the
> > > page_idle file. Did you have a better fix in mind?
> >
> > Can we set the idle bit also for non-lru pages as long as they are
> > reachable via pte?
>
> Not at the moment with the current page idle tracking code. PageLRU(page)
> flag is checked in page_idle_get_page().
yes, I am aware of the current code. I strongly suspect that the PageLRU
check was there to not mark arbitrary page looked up by pfn with the
idle bit because that would be unexpected. But I might be easily wrong
here.
> Even if we could set it for non-LRU, the idle bit (page flag) would not be
> cleared if page is not on LRU because page-reclaim code (page_referenced() I
> believe) would not clear it.
Yes, it is either reclaim when checking references as you say but also
mark_page_accessed. I believe the later might still have the page on the
pcp LRU add cache. Maybe I am missing something something but it seems
that there is nothing fundamentally requiring the user mapped page to be
on the LRU list when seting the idle bit.
That being said, your big hammer approach will work more reliable but if
you do not feel like changing the underlying PageLRU assumption then
document that draining should be removed longterm.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v4 3/5] [RFC] arm64: Add support for idle bit in swap PTE
From: Joel Fernandes @ 2019-08-06 11:26 UTC (permalink / raw)
To: Michal Hocko
Cc: Minchan Kim, linux-kernel, Robin Murphy, Alexey Dobriyan,
Andrew Morton, Borislav Petkov, Brendan Gregg, Catalin Marinas,
Christian Hansen, dancol, fmayer, H. Peter Anvin, Ingo Molnar,
Jonathan Corbet, Kees Cook, kernel-team, linux-api, linux-doc,
linux-fsdevel, linux-mm, Mike Rapoport, namhyung, paulmck,
Roman Gushchin, Stephen Rothwell, surenb, Thomas Gleixner, tkjos,
Vladimir Davydov, Vlastimil Babka, Will Deacon
In-Reply-To: <20190806111452.GW11812@dhcp22.suse.cz>
On Tue, Aug 06, 2019 at 01:14:52PM +0200, Michal Hocko wrote:
> On Tue 06-08-19 20:07:37, Minchan Kim wrote:
> > On Tue, Aug 06, 2019 at 12:47:55PM +0200, Michal Hocko wrote:
> > > On Tue 06-08-19 06:36:27, Joel Fernandes wrote:
> > > > On Tue, Aug 06, 2019 at 10:42:03AM +0200, Michal Hocko wrote:
> > > > > On Mon 05-08-19 13:04:49, Joel Fernandes (Google) wrote:
> > > > > > This bit will be used by idle page tracking code to correctly identify
> > > > > > if a page that was swapped out was idle before it got swapped out.
> > > > > > Without this PTE bit, we lose information about if a page is idle or not
> > > > > > since the page frame gets unmapped.
> > > > >
> > > > > And why do we need that? Why cannot we simply assume all swapped out
> > > > > pages to be idle? They were certainly idle enough to be reclaimed,
> > > > > right? Or what does idle actualy mean here?
> > > >
> > > > Yes, but other than swapping, in Android a page can be forced to be swapped
> > > > out as well using the new hints that Minchan is adding?
> > >
> > > Yes and that is effectivelly making them idle, no?
> >
> > 1. mark page-A idle which was present at that time.
> > 2. run workload
> > 3. page-A is touched several times
> > 4. *sudden* memory pressure happen so finally page A is finally swapped out
> > 5. now see the page A idle - but it's incorrect.
>
> Could you expand on what you mean by idle exactly? Why pageout doesn't
> really qualify as "mark-idle and reclaim"? Also could you describe a
> usecase where the swapout distinction really matters and it would lead
> to incorrect behavior?
Michal,
Did you read this post ? :
https://lore.kernel.org/lkml/20190806104715.GC218260@google.com/T/#m4ece68ceaf6e54d4d29e974f5f4c1080e733f6c1
Just wanted to be sure you did not miss it.
thanks,
- Joel
^ permalink raw reply
* Re: [PATCH v4 4/5] page_idle: Drain all LRU pagevec before idle tracking
From: Joel Fernandes @ 2019-08-06 11:19 UTC (permalink / raw)
To: Michal Hocko
Cc: linux-kernel, Alexey Dobriyan, Andrew Morton, Borislav Petkov,
Brendan Gregg, Catalin Marinas, Christian Hansen, dancol, fmayer,
H. Peter Anvin, Ingo Molnar, Jonathan Corbet, Kees Cook,
kernel-team, linux-api, linux-doc, linux-fsdevel, linux-mm,
Mike Rapoport, minchan, namhyung, paulmck, Robin Murphy,
Roman Gushchin, Stephen Rothwell, surenb, Thomas Gleixner, tkjos,
Vladimir Davydov, Vlastimil Babka, Will Deacon
In-Reply-To: <20190806105149.GT11812@dhcp22.suse.cz>
On Tue, Aug 06, 2019 at 12:51:49PM +0200, Michal Hocko wrote:
> On Tue 06-08-19 06:45:54, Joel Fernandes wrote:
> > On Tue, Aug 06, 2019 at 10:43:57AM +0200, Michal Hocko wrote:
> > > On Mon 05-08-19 13:04:50, Joel Fernandes (Google) wrote:
> > > > During idle tracking, we see that sometimes faulted anon pages are in
> > > > pagevec but are not drained to LRU. Idle tracking considers pages only
> > > > on LRU. Drain all CPU's LRU before starting idle tracking.
> > >
> > > Please expand on why does this matter enough to introduce a potentially
> > > expensinve draining which has to schedule a work on each CPU and wait
> > > for them to finish.
> >
> > Sure, I can expand. I am able to find multiple issues involving this. One
> > issue looks like idle tracking is completely broken. It shows up in my
> > testing as if a page that is marked as idle is always "accessed" -- because
> > it was never marked as idle (due to not draining of pagevec).
> >
> > The other issue shows up as a failure in my "swap test", with the following
> > sequence:
> > 1. Allocate some pages
> > 2. Write to them
> > 3. Mark them as idle <--- fails
> > 4. Introduce some memory pressure to induce swapping.
> > 5. Check the swap bit I introduced in this series. <--- fails to set idle
> > bit in swap PTE.
> >
> > Draining the pagevec in advance fixes both of these issues.
>
> This belongs to the changelog.
Sure, will add.
> > This operation even if expensive is only done once during the access of the
> > page_idle file. Did you have a better fix in mind?
>
> Can we set the idle bit also for non-lru pages as long as they are
> reachable via pte?
Not at the moment with the current page idle tracking code. PageLRU(page)
flag is checked in page_idle_get_page().
Even if we could set it for non-LRU, the idle bit (page flag) would not be
cleared if page is not on LRU because page-reclaim code (page_referenced() I
believe) would not clear it. This whole mechanism depends on page-reclaim. Or
did I miss your point?
thanks,
- Joel
^ permalink raw reply
* Re: [PATCH v4 3/5] [RFC] arm64: Add support for idle bit in swap PTE
From: Michal Hocko @ 2019-08-06 11:14 UTC (permalink / raw)
To: Minchan Kim
Cc: Joel Fernandes, linux-kernel, Robin Murphy, Alexey Dobriyan,
Andrew Morton, Borislav Petkov, Brendan Gregg, Catalin Marinas,
Christian Hansen, dancol, fmayer, H. Peter Anvin, Ingo Molnar,
Jonathan Corbet, Kees Cook, kernel-team, linux-api, linux-doc,
linux-fsdevel, linux-mm, Mike Rapoport, namhyung, paulmck,
Roman Gushchin, Stephen Rothwell, surenb, Thomas Gleixner, tkjos,
Vladimir Davydov, Vlastimil Babka, Will Deacon
In-Reply-To: <20190806110737.GB32615@google.com>
On Tue 06-08-19 20:07:37, Minchan Kim wrote:
> On Tue, Aug 06, 2019 at 12:47:55PM +0200, Michal Hocko wrote:
> > On Tue 06-08-19 06:36:27, Joel Fernandes wrote:
> > > On Tue, Aug 06, 2019 at 10:42:03AM +0200, Michal Hocko wrote:
> > > > On Mon 05-08-19 13:04:49, Joel Fernandes (Google) wrote:
> > > > > This bit will be used by idle page tracking code to correctly identify
> > > > > if a page that was swapped out was idle before it got swapped out.
> > > > > Without this PTE bit, we lose information about if a page is idle or not
> > > > > since the page frame gets unmapped.
> > > >
> > > > And why do we need that? Why cannot we simply assume all swapped out
> > > > pages to be idle? They were certainly idle enough to be reclaimed,
> > > > right? Or what does idle actualy mean here?
> > >
> > > Yes, but other than swapping, in Android a page can be forced to be swapped
> > > out as well using the new hints that Minchan is adding?
> >
> > Yes and that is effectivelly making them idle, no?
>
> 1. mark page-A idle which was present at that time.
> 2. run workload
> 3. page-A is touched several times
> 4. *sudden* memory pressure happen so finally page A is finally swapped out
> 5. now see the page A idle - but it's incorrect.
Could you expand on what you mean by idle exactly? Why pageout doesn't
really qualify as "mark-idle and reclaim"? Also could you describe a
usecase where the swapout distinction really matters and it would lead
to incorrect behavior?
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v4 3/5] [RFC] arm64: Add support for idle bit in swap PTE
From: Joel Fernandes @ 2019-08-06 11:14 UTC (permalink / raw)
To: Michal Hocko
Cc: linux-kernel, Robin Murphy, Alexey Dobriyan, Andrew Morton,
Borislav Petkov, Brendan Gregg, Catalin Marinas, Christian Hansen,
dancol, fmayer, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
Kees Cook, kernel-team, linux-api, linux-doc, linux-fsdevel,
linux-mm, Mike Rapoport, minchan, namhyung, paulmck,
Roman Gushchin, Stephen Rothwell, surenb, Thomas Gleixner, tkjos,
Vladimir Davydov, Vlastimil Babka, Will Deacon
In-Reply-To: <20190806104755.GR11812@dhcp22.suse.cz>
On Tue, Aug 06, 2019 at 12:47:55PM +0200, Michal Hocko wrote:
> On Tue 06-08-19 06:36:27, Joel Fernandes wrote:
> > On Tue, Aug 06, 2019 at 10:42:03AM +0200, Michal Hocko wrote:
> > > On Mon 05-08-19 13:04:49, Joel Fernandes (Google) wrote:
> > > > This bit will be used by idle page tracking code to correctly identify
> > > > if a page that was swapped out was idle before it got swapped out.
> > > > Without this PTE bit, we lose information about if a page is idle or not
> > > > since the page frame gets unmapped.
> > >
> > > And why do we need that? Why cannot we simply assume all swapped out
> > > pages to be idle? They were certainly idle enough to be reclaimed,
> > > right? Or what does idle actualy mean here?
> >
> > Yes, but other than swapping, in Android a page can be forced to be swapped
> > out as well using the new hints that Minchan is adding?
>
> Yes and that is effectivelly making them idle, no?
That depends on how you think of it. If you are thinking of a monitoring
process like a heap profiler, then from the heap profiler's (that only cares
about the process it is monitoring) perspective it will look extremely odd if
pages that are recently accessed by the process appear to be idle which would
falsely look like those processes are leaking memory. The reality being,
Android forced those pages into swap because of other reasons. I would like
for the swapping mechanism, whether forced swapping or memory reclaim, not to
interfere with the idle detection.
This is just an effort to make the idle tracking a little bit better. We
would like to not lose the 'accessed' information of the pages.
Initially, I had proposed what you are suggesting as well however the above
reasons made me to do it like this. Also Minchan and Konstantin suggested
this, so there are more people interested in the swap idle bit. Minchan, can
you provide more thoughts here? (He is on 2-week vacation from today so
hopefully replies before he vanishes ;-)).
Also assuming all swap pages as idle has other "semantic" issues. It is quite
odd if a swapped page is automatically marked as idle without userspace
telling it to. Consider the following set of events: 1. Userspace marks only
a certain memory region as idle. 2. Userspace reads back the bits
corresponding to a bigger region. Part of this bigger region is swapped.
Userspace expects all of the pages it did not mark, to have idle bit set to
'0' because it never marked them as idle. However if it is now surprised by
what it read back (not all '0' read back). Since a page is swapped, it will
be now marked "automatically" as idle as per your proposal, even if userspace
never marked it explicity before. This would be quite confusing/ambiguous.
I will include this and other information in future commit messages.
thanks,
- Joel
^ permalink raw reply
* Re: [PATCH v4 3/5] [RFC] arm64: Add support for idle bit in swap PTE
From: Minchan Kim @ 2019-08-06 11:07 UTC (permalink / raw)
To: Michal Hocko
Cc: Joel Fernandes, linux-kernel, Robin Murphy, Alexey Dobriyan,
Andrew Morton, Borislav Petkov, Brendan Gregg, Catalin Marinas,
Christian Hansen, dancol, fmayer, H. Peter Anvin, Ingo Molnar,
Jonathan Corbet, Kees Cook, kernel-team, linux-api, linux-doc,
linux-fsdevel, linux-mm, Mike Rapoport, namhyung, paulmck,
Roman Gushchin, Stephen Rothwell, surenb, Thomas Gleixner, tkjos,
Vladimir Davydov, Vlastimil Babka, Will Deacon
In-Reply-To: <20190806104755.GR11812@dhcp22.suse.cz>
On Tue, Aug 06, 2019 at 12:47:55PM +0200, Michal Hocko wrote:
> On Tue 06-08-19 06:36:27, Joel Fernandes wrote:
> > On Tue, Aug 06, 2019 at 10:42:03AM +0200, Michal Hocko wrote:
> > > On Mon 05-08-19 13:04:49, Joel Fernandes (Google) wrote:
> > > > This bit will be used by idle page tracking code to correctly identify
> > > > if a page that was swapped out was idle before it got swapped out.
> > > > Without this PTE bit, we lose information about if a page is idle or not
> > > > since the page frame gets unmapped.
> > >
> > > And why do we need that? Why cannot we simply assume all swapped out
> > > pages to be idle? They were certainly idle enough to be reclaimed,
> > > right? Or what does idle actualy mean here?
> >
> > Yes, but other than swapping, in Android a page can be forced to be swapped
> > out as well using the new hints that Minchan is adding?
>
> Yes and that is effectivelly making them idle, no?
1. mark page-A idle which was present at that time.
2. run workload
3. page-A is touched several times
4. *sudden* memory pressure happen so finally page A is finally swapped out
5. now see the page A idle - but it's incorrect.
^ permalink raw reply
* Re: [PATCH v4 4/5] page_idle: Drain all LRU pagevec before idle tracking
From: Michal Hocko @ 2019-08-06 10:51 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, Alexey Dobriyan, Andrew Morton, Borislav Petkov,
Brendan Gregg, Catalin Marinas, Christian Hansen, dancol, fmayer,
H. Peter Anvin, Ingo Molnar, Jonathan Corbet, Kees Cook,
kernel-team, linux-api, linux-doc, linux-fsdevel, linux-mm,
Mike Rapoport, minchan, namhyung, paulmck, Robin Murphy,
Roman Gushchin, Stephen Rothwell, surenb, Thomas Gleixner, tkjos,
Vladimir Davydov, Vlastimil Babka, Will Deacon
In-Reply-To: <20190806104554.GB218260@google.com>
On Tue 06-08-19 06:45:54, Joel Fernandes wrote:
> On Tue, Aug 06, 2019 at 10:43:57AM +0200, Michal Hocko wrote:
> > On Mon 05-08-19 13:04:50, Joel Fernandes (Google) wrote:
> > > During idle tracking, we see that sometimes faulted anon pages are in
> > > pagevec but are not drained to LRU. Idle tracking considers pages only
> > > on LRU. Drain all CPU's LRU before starting idle tracking.
> >
> > Please expand on why does this matter enough to introduce a potentially
> > expensinve draining which has to schedule a work on each CPU and wait
> > for them to finish.
>
> Sure, I can expand. I am able to find multiple issues involving this. One
> issue looks like idle tracking is completely broken. It shows up in my
> testing as if a page that is marked as idle is always "accessed" -- because
> it was never marked as idle (due to not draining of pagevec).
>
> The other issue shows up as a failure in my "swap test", with the following
> sequence:
> 1. Allocate some pages
> 2. Write to them
> 3. Mark them as idle <--- fails
> 4. Introduce some memory pressure to induce swapping.
> 5. Check the swap bit I introduced in this series. <--- fails to set idle
> bit in swap PTE.
>
> Draining the pagevec in advance fixes both of these issues.
This belongs to the changelog.
> This operation even if expensive is only done once during the access of the
> page_idle file. Did you have a better fix in mind?
Can we set the idle bit also for non-lru pages as long as they are
reachable via pte?
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v4 3/5] [RFC] arm64: Add support for idle bit in swap PTE
From: Michal Hocko @ 2019-08-06 10:47 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, Robin Murphy, Alexey Dobriyan, Andrew Morton,
Borislav Petkov, Brendan Gregg, Catalin Marinas, Christian Hansen,
dancol, fmayer, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
Kees Cook, kernel-team, linux-api, linux-doc, linux-fsdevel,
linux-mm, Mike Rapoport, minchan, namhyung, paulmck,
Roman Gushchin, Stephen Rothwell, surenb, Thomas Gleixner, tkjos,
Vladimir Davydov, Vlastimil Babka, Will Deacon
In-Reply-To: <20190806103627.GA218260@google.com>
On Tue 06-08-19 06:36:27, Joel Fernandes wrote:
> On Tue, Aug 06, 2019 at 10:42:03AM +0200, Michal Hocko wrote:
> > On Mon 05-08-19 13:04:49, Joel Fernandes (Google) wrote:
> > > This bit will be used by idle page tracking code to correctly identify
> > > if a page that was swapped out was idle before it got swapped out.
> > > Without this PTE bit, we lose information about if a page is idle or not
> > > since the page frame gets unmapped.
> >
> > And why do we need that? Why cannot we simply assume all swapped out
> > pages to be idle? They were certainly idle enough to be reclaimed,
> > right? Or what does idle actualy mean here?
>
> Yes, but other than swapping, in Android a page can be forced to be swapped
> out as well using the new hints that Minchan is adding?
Yes and that is effectivelly making them idle, no?
> Also, even if they were idle enough to be swapped, there is a chance that they
> were marked as idle and *accessed* before the swapping. Due to swapping, the
> "page was accessed since we last marked it as idle" information is lost. I am
> able to verify this.
>
> Idle in this context means the same thing as in page idle tracking terms, the
> page was not accessed by userspace since we last marked it as idle (using
> /proc/<pid>/page_idle).
Please describe a usecase and why that information might be useful.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v4 1/5] mm/page_idle: Add per-pid idle page tracking using virtual indexing
From: Joel Fernandes @ 2019-08-06 10:47 UTC (permalink / raw)
To: Michal Hocko
Cc: linux-kernel, Alexey Dobriyan, Andrew Morton, Borislav Petkov,
Brendan Gregg, Catalin Marinas, Christian Hansen, dancol, fmayer,
H. Peter Anvin, Ingo Molnar, Jonathan Corbet, Kees Cook,
kernel-team, linux-api, linux-doc, linux-fsdevel, linux-mm,
Mike Rapoport, minchan, namhyung, paulmck, Robin Murphy,
Roman Gushchin, Stephen Rothwell, surenb, Thomas Gleixner, tkjos,
Vladimir Davydov, Vlastimil Babka, Will Deacon
In-Reply-To: <20190806085605.GL11812@dhcp22.suse.cz>
On Tue, Aug 06, 2019 at 10:56:05AM +0200, Michal Hocko wrote:
> On Mon 05-08-19 13:04:47, Joel Fernandes (Google) wrote:
> > The page_idle tracking feature currently requires looking up the pagemap
> > for a process followed by interacting with /sys/kernel/mm/page_idle.
> > Looking up PFN from pagemap in Android devices is not supported by
> > unprivileged process and requires SYS_ADMIN and gives 0 for the PFN.
> >
> > This patch adds support to directly interact with page_idle tracking at
> > the PID level by introducing a /proc/<pid>/page_idle file. It follows
> > the exact same semantics as the global /sys/kernel/mm/page_idle, but now
> > looking up PFN through pagemap is not needed since the interface uses
> > virtual frame numbers, and at the same time also does not require
> > SYS_ADMIN.
> >
> > In Android, we are using this for the heap profiler (heapprofd) which
> > profiles and pin points code paths which allocates and leaves memory
> > idle for long periods of time. This method solves the security issue
> > with userspace learning the PFN, and while at it is also shown to yield
> > better results than the pagemap lookup, the theory being that the window
> > where the address space can change is reduced by eliminating the
> > intermediate pagemap look up stage. In virtual address indexing, the
> > process's mmap_sem is held for the duration of the access.
>
> As already mentioned in one of the previous versions. The interface
> seems sane and the usecase as well. So I do not really have high level
> objections.
That is great to know.
> From a quick look at the patch I would just object to pulling swap idle
> tracking into this patch because it makes the review harder and it is
> essentially a dead code until a later patch. I am also not sure whether
> that is really necessary and it really begs for an explicit
> justification.
Ok I will split it out, and also expand on the need for it a bit more.
>
> I will try to go through the patch more carefully later as time allows.
Thanks a lot.
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> --
> Michal Hocko
> SUSE Labs
- Joel
^ permalink raw reply
* Re: [PATCH v4 4/5] page_idle: Drain all LRU pagevec before idle tracking
From: Joel Fernandes @ 2019-08-06 10:45 UTC (permalink / raw)
To: Michal Hocko
Cc: linux-kernel, Alexey Dobriyan, Andrew Morton, Borislav Petkov,
Brendan Gregg, Catalin Marinas, Christian Hansen, dancol, fmayer,
H. Peter Anvin, Ingo Molnar, Jonathan Corbet, Kees Cook,
kernel-team, linux-api, linux-doc, linux-fsdevel, linux-mm,
Mike Rapoport, minchan, namhyung, paulmck, Robin Murphy,
Roman Gushchin, Stephen Rothwell, surenb, Thomas Gleixner, tkjos,
Vladimir Davydov, Vlastimil Babka, Will Deacon
In-Reply-To: <20190806084357.GK11812@dhcp22.suse.cz>
On Tue, Aug 06, 2019 at 10:43:57AM +0200, Michal Hocko wrote:
> On Mon 05-08-19 13:04:50, Joel Fernandes (Google) wrote:
> > During idle tracking, we see that sometimes faulted anon pages are in
> > pagevec but are not drained to LRU. Idle tracking considers pages only
> > on LRU. Drain all CPU's LRU before starting idle tracking.
>
> Please expand on why does this matter enough to introduce a potentially
> expensinve draining which has to schedule a work on each CPU and wait
> for them to finish.
Sure, I can expand. I am able to find multiple issues involving this. One
issue looks like idle tracking is completely broken. It shows up in my
testing as if a page that is marked as idle is always "accessed" -- because
it was never marked as idle (due to not draining of pagevec).
The other issue shows up as a failure in my "swap test", with the following
sequence:
1. Allocate some pages
2. Write to them
3. Mark them as idle <--- fails
4. Introduce some memory pressure to induce swapping.
5. Check the swap bit I introduced in this series. <--- fails to set idle
bit in swap PTE.
Draining the pagevec in advance fixes both of these issues.
This operation even if expensive is only done once during the access of the
page_idle file. Did you have a better fix in mind?
thanks,
- Joel
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > ---
> > mm/page_idle.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/mm/page_idle.c b/mm/page_idle.c
> > index a5b00d63216c..2972367a599f 100644
> > --- a/mm/page_idle.c
> > +++ b/mm/page_idle.c
> > @@ -180,6 +180,8 @@ static ssize_t page_idle_bitmap_read(struct file *file, struct kobject *kobj,
> > unsigned long pfn, end_pfn;
> > int bit, ret;
> >
> > + lru_add_drain_all();
> > +
> > ret = page_idle_get_frames(pos, count, NULL, &pfn, &end_pfn);
> > if (ret == -ENXIO)
> > return 0; /* Reads beyond max_pfn do nothing */
> > @@ -211,6 +213,8 @@ static ssize_t page_idle_bitmap_write(struct file *file, struct kobject *kobj,
> > unsigned long pfn, end_pfn;
> > int bit, ret;
> >
> > + lru_add_drain_all();
> > +
> > ret = page_idle_get_frames(pos, count, NULL, &pfn, &end_pfn);
> > if (ret)
> > return ret;
> > @@ -428,6 +432,8 @@ ssize_t page_idle_proc_generic(struct file *file, char __user *ubuff,
> > walk.private = &priv;
> > walk.mm = mm;
> >
> > + lru_add_drain_all();
> > +
> > down_read(&mm->mmap_sem);
> >
> > /*
> > --
> > 2.22.0.770.g0f2c4a37fd-goog
>
> --
> Michal Hocko
> SUSE Labs
^ permalink raw reply
* Re: [PATCH v4 3/5] [RFC] arm64: Add support for idle bit in swap PTE
From: Joel Fernandes @ 2019-08-06 10:36 UTC (permalink / raw)
To: Michal Hocko
Cc: linux-kernel, Robin Murphy, Alexey Dobriyan, Andrew Morton,
Borislav Petkov, Brendan Gregg, Catalin Marinas, Christian Hansen,
dancol, fmayer, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
Kees Cook, kernel-team, linux-api, linux-doc, linux-fsdevel,
linux-mm, Mike Rapoport, minchan, namhyung, paulmck,
Roman Gushchin, Stephen Rothwell, surenb, Thomas Gleixner, tkjos,
Vladimir Davydov, Vlastimil Babka, Will Deacon
In-Reply-To: <20190806084203.GJ11812@dhcp22.suse.cz>
On Tue, Aug 06, 2019 at 10:42:03AM +0200, Michal Hocko wrote:
> On Mon 05-08-19 13:04:49, Joel Fernandes (Google) wrote:
> > This bit will be used by idle page tracking code to correctly identify
> > if a page that was swapped out was idle before it got swapped out.
> > Without this PTE bit, we lose information about if a page is idle or not
> > since the page frame gets unmapped.
>
> And why do we need that? Why cannot we simply assume all swapped out
> pages to be idle? They were certainly idle enough to be reclaimed,
> right? Or what does idle actualy mean here?
Yes, but other than swapping, in Android a page can be forced to be swapped
out as well using the new hints that Minchan is adding?
Also, even if they were idle enough to be swapped, there is a chance that they
were marked as idle and *accessed* before the swapping. Due to swapping, the
"page was accessed since we last marked it as idle" information is lost. I am
able to verify this.
Idle in this context means the same thing as in page idle tracking terms, the
page was not accessed by userspace since we last marked it as idle (using
/proc/<pid>/page_idle).
thanks,
- Joel
> > In this patch we reuse PTE_DEVMAP bit since idle page tracking only
> > works on user pages in the LRU. Device pages should not consitute those
> > so it should be unused and safe to use.
> >
> > Cc: Robin Murphy <robin.murphy@arm.com>
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > ---
> > arch/arm64/Kconfig | 1 +
> > arch/arm64/include/asm/pgtable-prot.h | 1 +
> > arch/arm64/include/asm/pgtable.h | 15 +++++++++++++++
> > 3 files changed, 17 insertions(+)
> >
> > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > index 3adcec05b1f6..9d1412c693d7 100644
> > --- a/arch/arm64/Kconfig
> > +++ b/arch/arm64/Kconfig
> > @@ -128,6 +128,7 @@ config ARM64
> > select HAVE_ARCH_MMAP_RND_BITS
> > select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT
> > select HAVE_ARCH_PREL32_RELOCATIONS
> > + select HAVE_ARCH_PTE_SWP_PGIDLE
> > select HAVE_ARCH_SECCOMP_FILTER
> > select HAVE_ARCH_STACKLEAK
> > select HAVE_ARCH_THREAD_STRUCT_WHITELIST
> > diff --git a/arch/arm64/include/asm/pgtable-prot.h b/arch/arm64/include/asm/pgtable-prot.h
> > index 92d2e9f28f28..917b15c5d63a 100644
> > --- a/arch/arm64/include/asm/pgtable-prot.h
> > +++ b/arch/arm64/include/asm/pgtable-prot.h
> > @@ -18,6 +18,7 @@
> > #define PTE_SPECIAL (_AT(pteval_t, 1) << 56)
> > #define PTE_DEVMAP (_AT(pteval_t, 1) << 57)
> > #define PTE_PROT_NONE (_AT(pteval_t, 1) << 58) /* only when !PTE_VALID */
> > +#define PTE_SWP_PGIDLE PTE_DEVMAP /* for idle page tracking during swapout */
> >
> > #ifndef __ASSEMBLY__
> >
> > diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> > index 3f5461f7b560..558f5ebd81ba 100644
> > --- a/arch/arm64/include/asm/pgtable.h
> > +++ b/arch/arm64/include/asm/pgtable.h
> > @@ -212,6 +212,21 @@ static inline pte_t pte_mkdevmap(pte_t pte)
> > return set_pte_bit(pte, __pgprot(PTE_DEVMAP));
> > }
> >
> > +static inline int pte_swp_page_idle(pte_t pte)
> > +{
> > + return 0;
> > +}
> > +
> > +static inline pte_t pte_swp_mkpage_idle(pte_t pte)
> > +{
> > + return set_pte_bit(pte, __pgprot(PTE_SWP_PGIDLE));
> > +}
> > +
> > +static inline pte_t pte_swp_clear_page_idle(pte_t pte)
> > +{
> > + return clear_pte_bit(pte, __pgprot(PTE_SWP_PGIDLE));
> > +}
> > +
> > static inline void set_pte(pte_t *ptep, pte_t pte)
> > {
> > WRITE_ONCE(*ptep, pte);
> > --
> > 2.22.0.770.g0f2c4a37fd-goog
>
> --
> Michal Hocko
> SUSE Labs
^ permalink raw reply
* [PATCH] Documentation: fs: Convert xfs-delayed-logging-design.txt to ReSt
From: Sheriff Esseson @ 2019-08-06 9:03 UTC (permalink / raw)
To: skhan
Cc: linux-kernel-mentees, Darrick J. Wong, supporter:XFS FILESYSTEM,
Jonathan Corbet, open list:DOCUMENTATION, open list
Convert xfs-delayed-logging-design.txt to ReST and fix broken references.
The enumerations at "Lifecycle Changes" breaks because of lines begining with
"<", treat as diagrams.
Signed-off-by: Sheriff Esseson <sheriffesseson@gmail.com>
---
Documentation/filesystems/index.rst | 1 +
...ign.txt => xfs-delayed-logging-design.rst} | 69 +++++++++++--------
MAINTAINERS | 2 +-
3 files changed, 44 insertions(+), 28 deletions(-)
rename Documentation/filesystems/{xfs-delayed-logging-design.txt => xfs-delayed-logging-design.rst} (96%)
diff --git a/Documentation/filesystems/index.rst b/Documentation/filesystems/index.rst
index 2de2fe2ab078..0b94ff710b67 100644
--- a/Documentation/filesystems/index.rst
+++ b/Documentation/filesystems/index.rst
@@ -32,3 +32,4 @@ filesystem implementations.
journalling
fscrypt
+ xfs-delayed-logging-design
diff --git a/Documentation/filesystems/xfs-delayed-logging-design.txt b/Documentation/filesystems/xfs-delayed-logging-design.rst
similarity index 96%
rename from Documentation/filesystems/xfs-delayed-logging-design.txt
rename to Documentation/filesystems/xfs-delayed-logging-design.rst
index 9a6dd289b17b..a85ca00d4221 100644
--- a/Documentation/filesystems/xfs-delayed-logging-design.txt
+++ b/Documentation/filesystems/xfs-delayed-logging-design.rst
@@ -1,8 +1,11 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+==========================
XFS Delayed Logging Design
---------------------------
+==========================
Introduction to Re-logging in XFS
----------------------------------
+=================================
XFS logging is a combination of logical and physical logging. Some objects,
such as inodes and dquots, are logged in logical format where the details
@@ -27,14 +30,18 @@ written to disk after change D, we would see in the log the following series
of transactions, their contents and the log sequence number (LSN) of the
transaction:
+ ============ ========= ==============
Transaction Contents LSN
+ ============ ========= ==============
A A X
B A+B X+n
C A+B+C X+n+m
D A+B+C+D X+n+m+o
<object written to disk>
- E E Y (> X+n+m+o)
+ ------------------------------------------------------
+ E E Y (> X+n+m+o)
F E+F Y+p
+ ============ ========= ==============
In other words, each time an object is relogged, the new transaction contains
the aggregation of all the previous changes currently held only in the log.
@@ -85,7 +92,7 @@ IO permanently. Hence the XFS journalling subsystem can be considered to be IO
bound.
Delayed Logging: Concepts
--------------------------
+=========================
The key thing to note about the asynchronous logging combined with the
relogging technique XFS uses is that we can be relogging changed objects
@@ -154,9 +161,10 @@ The fundamental requirements for delayed logging in XFS are simple:
6. No performance regressions for synchronous transaction workloads.
Delayed Logging: Design
------------------------
+=======================
Storing Changes
+---------------
The problem with accumulating changes at a logical level (i.e. just using the
existing log item dirty region tracking) is that when it comes to writing the
@@ -194,30 +202,30 @@ asynchronous transactions to the log. The differences between the existing
formatting method and the delayed logging formatting can be seen in the
diagram below.
-Current format log vector:
+Current format log vector::
-Object +---------------------------------------------+
-Vector 1 +----+
-Vector 2 +----+
-Vector 3 +----------+
+ Object +---------------------------------------------+
+ Vector 1 +----+
+ Vector 2 +----+
+ Vector 3 +----------+
-After formatting:
+After formatting::
-Log Buffer +-V1-+-V2-+----V3----+
+ Log Buffer +-V1-+-V2-+----V3----+
-Delayed logging vector:
+Delayed logging vector::
-Object +---------------------------------------------+
-Vector 1 +----+
-Vector 2 +----+
-Vector 3 +----------+
+ Object +---------------------------------------------+
+ Vector 1 +----+
+ Vector 2 +----+
+ Vector 3 +----------+
-After formatting:
+After formatting::
-Memory Buffer +-V1-+-V2-+----V3----+
-Vector 1 +----+
-Vector 2 +----+
-Vector 3 +----------+
+ Memory Buffer +-V1-+-V2-+----V3----+
+ Vector 1 +----+
+ Vector 2 +----+
+ Vector 3 +----------+
The memory buffer and associated vector need to be passed as a single object,
but still need to be associated with the parent object so if the object is
@@ -242,6 +250,7 @@ relogged in memory.
Tracking Changes
+----------------
Now that we can record transactional changes in memory in a form that allows
them to be used without limitations, we need to be able to track and accumulate
@@ -278,6 +287,7 @@ done for convenience/sanity of the developers.
Delayed Logging: Checkpoints
+============================
When we have a log synchronisation event, commonly known as a "log force",
all the items in the CIL must be written into the log via the log buffers.
@@ -341,7 +351,7 @@ Hence log vectors need to be able to be chained together to allow them to be
detached from the log items. That is, when the CIL is flushed the memory
buffer and log vector attached to each log item needs to be attached to the
checkpoint context so that the log item can be released. In diagrammatic form,
-the CIL would look like this before the flush:
+the CIL would look like this before the flush::
CIL Head
|
@@ -362,7 +372,7 @@ the CIL would look like this before the flush:
-> vector array
And after the flush the CIL head is empty, and the checkpoint context log
-vector list would look like:
+vector list would look like::
Checkpoint Context
|
@@ -411,6 +421,7 @@ compare" situation that can be done after a working and reviewed implementation
is in the dev tree....
Delayed Logging: Checkpoint Sequencing
+======================================
One of the key aspects of the XFS transaction subsystem is that it tags
committed transactions with the log sequence number of the transaction commit.
@@ -474,6 +485,7 @@ force the log at the LSN of that transaction) and so the higher level code
behaves the same regardless of whether delayed logging is being used or not.
Delayed Logging: Checkpoint Log Space Accounting
+================================================
The big issue for a checkpoint transaction is the log space reservation for the
transaction. We don't know how big a checkpoint transaction is going to be
@@ -491,7 +503,7 @@ the size of the transaction and the number of regions being logged (the number
of log vectors in the transaction).
An example of the differences would be logging directory changes versus logging
-inode changes. If you modify lots of inode cores (e.g. chmod -R g+w *), then
+inode changes. If you modify lots of inode cores e.g. ``$ chmod -R g+w *``, then
there are lots of transactions that only contain an inode core and an inode log
format structure. That is, two vectors totaling roughly 150 bytes. If we modify
10,000 inodes, we have about 1.5MB of metadata to write in 20,000 vectors. Each
@@ -565,6 +577,7 @@ which is once every 30s.
Delayed Logging: Log Item Pinning
+=================================
Currently log items are pinned during transaction commit while the items are
still locked. This happens just after the items are formatted, though it could
@@ -605,6 +618,7 @@ object, we have a race with CIL being flushed between the check and the pin
lock to guarantee that we pin the items correctly.
Delayed Logging: Concurrent Scalability
+=======================================
A fundamental requirement for the CIL is that accesses through transaction
commits must scale to many concurrent commits. The current transaction commit
@@ -683,8 +697,9 @@ woken by the wrong event.
Lifecycle Changes
+=================
-The existing log item life cycle is as follows:
+The existing log item life cycle is as follows::
1. Transaction allocate
2. Transaction reserve
@@ -729,7 +744,7 @@ at the same time. If the log item is in the AIL or between steps 6 and 7
and steps 1-6 are re-entered, then the item is relogged. Only when steps 8-9
are entered and completed is the object considered clean.
-With delayed logging, there are new steps inserted into the life cycle:
+With delayed logging, there are new steps inserted into the life cycle::
1. Transaction allocate
2. Transaction reserve
diff --git a/MAINTAINERS b/MAINTAINERS
index 6c49b48cfd69..acbce11c3d49 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17664,7 +17664,7 @@ T: git git://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git
S: Supported
F: Documentation/admin-guide/xfs.rst
F: Documentation/ABI/testing/sysfs-fs-xfs
-F: Documentation/filesystems/xfs-delayed-logging-design.txt
+F: Documentation/filesystems/xfs-delayed-logging-design.rst
F: Documentation/filesystems/xfs-self-describing-metadata.txt
F: fs/xfs/
F: include/uapi/linux/dqblk_xfs.h
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v4 1/5] mm/page_idle: Add per-pid idle page tracking using virtual indexing
From: Michal Hocko @ 2019-08-06 8:56 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Alexey Dobriyan, Andrew Morton, Borislav Petkov,
Brendan Gregg, Catalin Marinas, Christian Hansen, dancol, fmayer,
H. Peter Anvin, Ingo Molnar, joelaf, Jonathan Corbet, Kees Cook,
kernel-team, linux-api, linux-doc, linux-fsdevel, linux-mm,
Mike Rapoport, minchan, namhyung, paulmck, Robin Murphy,
Roman Gushchin, Stephen Rothwell, surenb, Thomas Gleixner, tkjos,
Vladimir Davydov, Vlastimil Babka, Will Deacon
In-Reply-To: <20190805170451.26009-1-joel@joelfernandes.org>
On Mon 05-08-19 13:04:47, Joel Fernandes (Google) wrote:
> The page_idle tracking feature currently requires looking up the pagemap
> for a process followed by interacting with /sys/kernel/mm/page_idle.
> Looking up PFN from pagemap in Android devices is not supported by
> unprivileged process and requires SYS_ADMIN and gives 0 for the PFN.
>
> This patch adds support to directly interact with page_idle tracking at
> the PID level by introducing a /proc/<pid>/page_idle file. It follows
> the exact same semantics as the global /sys/kernel/mm/page_idle, but now
> looking up PFN through pagemap is not needed since the interface uses
> virtual frame numbers, and at the same time also does not require
> SYS_ADMIN.
>
> In Android, we are using this for the heap profiler (heapprofd) which
> profiles and pin points code paths which allocates and leaves memory
> idle for long periods of time. This method solves the security issue
> with userspace learning the PFN, and while at it is also shown to yield
> better results than the pagemap lookup, the theory being that the window
> where the address space can change is reduced by eliminating the
> intermediate pagemap look up stage. In virtual address indexing, the
> process's mmap_sem is held for the duration of the access.
As already mentioned in one of the previous versions. The interface
seems sane and the usecase as well. So I do not really have high level
objections.
From a quick look at the patch I would just object to pulling swap idle
tracking into this patch because it makes the review harder and it is
essentially a dead code until a later patch. I am also not sure whether
that is really necessary and it really begs for an explicit
justification.
I will try to go through the patch more carefully later as time allows.
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v4 4/5] page_idle: Drain all LRU pagevec before idle tracking
From: Michal Hocko @ 2019-08-06 8:43 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Alexey Dobriyan, Andrew Morton, Borislav Petkov,
Brendan Gregg, Catalin Marinas, Christian Hansen, dancol, fmayer,
H. Peter Anvin, Ingo Molnar, joelaf, Jonathan Corbet, Kees Cook,
kernel-team, linux-api, linux-doc, linux-fsdevel, linux-mm,
Mike Rapoport, minchan, namhyung, paulmck, Robin Murphy,
Roman Gushchin, Stephen Rothwell, surenb, Thomas Gleixner, tkjos,
Vladimir Davydov, Vlastimil Babka, Will Deacon
In-Reply-To: <20190805170451.26009-4-joel@joelfernandes.org>
On Mon 05-08-19 13:04:50, Joel Fernandes (Google) wrote:
> During idle tracking, we see that sometimes faulted anon pages are in
> pagevec but are not drained to LRU. Idle tracking considers pages only
> on LRU. Drain all CPU's LRU before starting idle tracking.
Please expand on why does this matter enough to introduce a potentially
expensinve draining which has to schedule a work on each CPU and wait
for them to finish.
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
> mm/page_idle.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/mm/page_idle.c b/mm/page_idle.c
> index a5b00d63216c..2972367a599f 100644
> --- a/mm/page_idle.c
> +++ b/mm/page_idle.c
> @@ -180,6 +180,8 @@ static ssize_t page_idle_bitmap_read(struct file *file, struct kobject *kobj,
> unsigned long pfn, end_pfn;
> int bit, ret;
>
> + lru_add_drain_all();
> +
> ret = page_idle_get_frames(pos, count, NULL, &pfn, &end_pfn);
> if (ret == -ENXIO)
> return 0; /* Reads beyond max_pfn do nothing */
> @@ -211,6 +213,8 @@ static ssize_t page_idle_bitmap_write(struct file *file, struct kobject *kobj,
> unsigned long pfn, end_pfn;
> int bit, ret;
>
> + lru_add_drain_all();
> +
> ret = page_idle_get_frames(pos, count, NULL, &pfn, &end_pfn);
> if (ret)
> return ret;
> @@ -428,6 +432,8 @@ ssize_t page_idle_proc_generic(struct file *file, char __user *ubuff,
> walk.private = &priv;
> walk.mm = mm;
>
> + lru_add_drain_all();
> +
> down_read(&mm->mmap_sem);
>
> /*
> --
> 2.22.0.770.g0f2c4a37fd-goog
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v4 3/5] [RFC] arm64: Add support for idle bit in swap PTE
From: Michal Hocko @ 2019-08-06 8:42 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Robin Murphy, Alexey Dobriyan, Andrew Morton,
Borislav Petkov, Brendan Gregg, Catalin Marinas, Christian Hansen,
dancol, fmayer, H. Peter Anvin, Ingo Molnar, joelaf,
Jonathan Corbet, Kees Cook, kernel-team, linux-api, linux-doc,
linux-fsdevel, linux-mm, Mike Rapoport, minchan, namhyung,
paulmck, Roman Gushchin, Stephen Rothwell, surenb,
Thomas Gleixner, tkjos, Vladimir Davydov, Vlastimil Babka,
Will Deacon
In-Reply-To: <20190805170451.26009-3-joel@joelfernandes.org>
On Mon 05-08-19 13:04:49, Joel Fernandes (Google) wrote:
> This bit will be used by idle page tracking code to correctly identify
> if a page that was swapped out was idle before it got swapped out.
> Without this PTE bit, we lose information about if a page is idle or not
> since the page frame gets unmapped.
And why do we need that? Why cannot we simply assume all swapped out
pages to be idle? They were certainly idle enough to be reclaimed,
right? Or what does idle actualy mean here?
> In this patch we reuse PTE_DEVMAP bit since idle page tracking only
> works on user pages in the LRU. Device pages should not consitute those
> so it should be unused and safe to use.
>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
> arch/arm64/Kconfig | 1 +
> arch/arm64/include/asm/pgtable-prot.h | 1 +
> arch/arm64/include/asm/pgtable.h | 15 +++++++++++++++
> 3 files changed, 17 insertions(+)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 3adcec05b1f6..9d1412c693d7 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -128,6 +128,7 @@ config ARM64
> select HAVE_ARCH_MMAP_RND_BITS
> select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT
> select HAVE_ARCH_PREL32_RELOCATIONS
> + select HAVE_ARCH_PTE_SWP_PGIDLE
> select HAVE_ARCH_SECCOMP_FILTER
> select HAVE_ARCH_STACKLEAK
> select HAVE_ARCH_THREAD_STRUCT_WHITELIST
> diff --git a/arch/arm64/include/asm/pgtable-prot.h b/arch/arm64/include/asm/pgtable-prot.h
> index 92d2e9f28f28..917b15c5d63a 100644
> --- a/arch/arm64/include/asm/pgtable-prot.h
> +++ b/arch/arm64/include/asm/pgtable-prot.h
> @@ -18,6 +18,7 @@
> #define PTE_SPECIAL (_AT(pteval_t, 1) << 56)
> #define PTE_DEVMAP (_AT(pteval_t, 1) << 57)
> #define PTE_PROT_NONE (_AT(pteval_t, 1) << 58) /* only when !PTE_VALID */
> +#define PTE_SWP_PGIDLE PTE_DEVMAP /* for idle page tracking during swapout */
>
> #ifndef __ASSEMBLY__
>
> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> index 3f5461f7b560..558f5ebd81ba 100644
> --- a/arch/arm64/include/asm/pgtable.h
> +++ b/arch/arm64/include/asm/pgtable.h
> @@ -212,6 +212,21 @@ static inline pte_t pte_mkdevmap(pte_t pte)
> return set_pte_bit(pte, __pgprot(PTE_DEVMAP));
> }
>
> +static inline int pte_swp_page_idle(pte_t pte)
> +{
> + return 0;
> +}
> +
> +static inline pte_t pte_swp_mkpage_idle(pte_t pte)
> +{
> + return set_pte_bit(pte, __pgprot(PTE_SWP_PGIDLE));
> +}
> +
> +static inline pte_t pte_swp_clear_page_idle(pte_t pte)
> +{
> + return clear_pte_bit(pte, __pgprot(PTE_SWP_PGIDLE));
> +}
> +
> static inline void set_pte(pte_t *ptep, pte_t pte)
> {
> WRITE_ONCE(*ptep, pte);
> --
> 2.22.0.770.g0f2c4a37fd-goog
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* RE: [PATCH 0/6] hwspinlock: allow sharing of hwspinlocks
From: Fabien DESSENNE @ 2019-08-06 7:43 UTC (permalink / raw)
To: s-anna@ti.com
Cc: Ohad Ben-Cohen, Rob Herring, Mark Rutland, Maxime Coquelin,
Alexandre TORGUE, Jonathan Corbet,
linux-remoteproc@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-stm32@st-md-mailman.stormreply.com,
linux-arm-kernel@lists.infradead.org, linux-doc@vger.kernel.org,
Benjamin GAIGNARD, Bjorn Andersson
In-Reply-To: <20190805174659.GA23928@tuxbook-pro>
Hi Suman,
Could you please let us know your thoughts or comments?
BR
Fabien
> -----Original Message-----
> From: Bjorn Andersson <bjorn.andersson@linaro.org>
> Sent: lundi 5 août 2019 19:47
> To: Fabien DESSENNE <fabien.dessenne@st.com>
> Cc: Ohad Ben-Cohen <ohad@wizery.com>; Rob Herring <robh+dt@kernel.org>;
> Mark Rutland <mark.rutland@arm.com>; Maxime Coquelin
> <mcoquelin.stm32@gmail.com>; Alexandre TORGUE
> <alexandre.torgue@st.com>; Jonathan Corbet <corbet@lwn.net>; linux-
> remoteproc@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-stm32@st-md-mailman.stormreply.com; linux-arm-
> kernel@lists.infradead.org; linux-doc@vger.kernel.org; Benjamin GAIGNARD
> <benjamin.gaignard@st.com>
> Subject: Re: [PATCH 0/6] hwspinlock: allow sharing of hwspinlocks
>
> On Mon 05 Aug 01:48 PDT 2019, Fabien DESSENNE wrote:
>
> >
> > On 01/08/2019 9:14 PM, Bjorn Andersson wrote:
> > > On Wed 13 Mar 08:50 PDT 2019, Fabien Dessenne wrote:
> > >
> > >> The current implementation does not allow two different devices to
> > >> use a common hwspinlock. This patch set proposes to have, as an
> > >> option, some hwspinlocks shared between several users.
> > >>
> > >> Below is an example that explain the need for this:
> > >> exti: interrupt-controller@5000d000 {
> > >> compatible = "st,stm32mp1-exti", "syscon";
> > >> interrupt-controller;
> > >> #interrupt-cells = <2>;
> > >> reg = <0x5000d000 0x400>;
> > >> hwlocks = <&hsem 1>;
> > >> };
> > >> The two drivers (stm32mp1-exti and syscon) refer to the same hwlock.
> > >> With the current hwspinlock implementation, only the first driver
> > >> succeeds in requesting (hwspin_lock_request_specific) the hwlock.
> > >> The second request fails.
> > >>
> > >>
> > >> The proposed approach does not modify the API, but extends the DT
> 'hwlocks'
> > >> property with a second optional parameter (the first one identifies
> > >> an
> > >> hwlock) that specifies whether an hwlock is requested for exclusive
> > >> usage (current behavior) or can be shared between several users.
> > >> Examples:
> > >> hwlocks = <&hsem 8>; Ref to hwlock #8 for exclusive usage
> > >> hwlocks = <&hsem 8 0>; Ref to hwlock #8 for exclusive (0) usage
> > >> hwlocks = <&hsem 8 1>; Ref to hwlock #8 for shared (1) usage
> > >>
> > >> As a constraint, the #hwlock-cells value must be 1 or 2.
> > >> In the current implementation, this can have theorically any value but:
> > >> - all of the exisiting drivers use the same value : 1.
> > >> - the framework supports only one value : 1 (see implementation of
> > >> of_hwspin_lock_simple_xlate())
> > >> Hence, it shall not be a problem to restrict this value to 1 or 2
> > >> since it won't break any driver.
> > >>
> > > Hi Fabien,
> > >
> > > Your series looks good, but it makes me wonder why the hardware
> > > locks should be an exclusive resource.
> > >
> > > How about just making all (specific) locks shared?
> >
> > Hi Bjorn,
> >
> > Making all locks shared is a possible implementation (my first
> > implementation was going this way) but there are some drawbacks we
> > must be aware of:
> >
> > A/ This theoretically break the legacy behavior (the legacy works with
> > exclusive (UNUSED radix tag) usage). As a consequence, an existing
> > driver that is currently failing to request a lock (already claimed by
> > another
> > user) would now work fine. Not sure that there are such drivers, so
> > this point is probably not a real issue.
> >
>
> Right, it's possible that a previously misconfigured system now successfully
> probes more than one device that uses a particular spinlock. But such system
> would be suffering from issues related to e.g.
> probe ordering.
>
> So I think we should ignore this issue.
>
> > B/ This would introduce some inconsistency between the two 'request'
> > API which are hwspin_lock_request() and hwspin_lock_request_specific().
> > hwspin_lock_request() looks for an unused lock, so requests for an
> > exclusive usage. On the other side, request_specific() would request shared
> locks.
> > Worst the following sequence can transform an exclusive usage into a
> > shared
> >
>
> There is already an inconsistency in between these; as with above any system
> that uses both request() and request_specific() will be suffering from intermittent
> failures due to probe ordering.
>
> > one:
> > -hwspin_lock_request() -> returns Id#0 (exclusive)
> > -hwspin_lock_request() -> returns Id#1 (exclusive)
> > -hwspin_lock_request_specific(0) -> returns Id#0 and makes Id#0
> > shared Honestly I am not sure that this is a real issue, but it's
> > better to have it in mind before we take ay decision
>
> The case where I can see a
> problem with this would be if the two clients somehow would nest their locking
> regions.
>
> But generally I think this could consider this an improvement, because the
> request_specific() would now be able to acquire its hwlock, with some additional
> contention due to the multiple use.
>
> > I could not find any driver using the hwspin_lock_request() API, we
> > may decide to remove (or to make deprecated) this API, having
> > everything 'shared without any conditions'.
> >
>
> It would be nice to have an upstream user of this API.
>
> >
> > I can see three options:
> > 1- Keep my initial proposition
> > 2- Have hwspin_lock_request_specific() using shared locks and
> > hwspin_lock_request() using unused (so 'initially' exclusive) locks.
> > 3- Have hwspin_lock_request_specific() using shared locks and
> > remove/make deprecated hwspin_lock_request().
> >
> > Just let me know what is your preference.
> >
>
> I think we should start with #2 and would like input from e.g. Suman regarding #3.
>
> Regards,
> Bjorn
>
> > BR
> >
> > Fabien
> >
> > >
> > > Regards,
> > > Bjorn
> > >
> > >> Fabien Dessenne (6):
> > >> dt-bindings: hwlock: add support of shared locks
> > >> hwspinlock: allow sharing of hwspinlocks
> > >> dt-bindings: hwlock: update STM32 #hwlock-cells value
> > >> ARM: dts: stm32: Add hwspinlock node for stm32mp157 SoC
> > >> ARM: dts: stm32: Add hwlock for irqchip on stm32mp157
> > >> ARM: dts: stm32: hwlocks for GPIO for stm32mp157
> > >>
> > >> .../devicetree/bindings/hwlock/hwlock.txt | 27 +++++--
> > >> .../bindings/hwlock/st,stm32-hwspinlock.txt | 6 +-
> > >> Documentation/hwspinlock.txt | 10 ++-
> > >> arch/arm/boot/dts/stm32mp157-pinctrl.dtsi | 2 +
> > >> arch/arm/boot/dts/stm32mp157c.dtsi | 10 +++
> > >> drivers/hwspinlock/hwspinlock_core.c | 82 +++++++++++++++++-
> ----
> > >> drivers/hwspinlock/hwspinlock_internal.h | 2 +
> > >> 7 files changed, 108 insertions(+), 31 deletions(-)
> > >>
> > >> --
> > >> 2.7.4
> > >>
^ permalink raw reply
* Re: [PATCH v8 0/2] fTPM: firmware TPM running in TEE
From: Jarkko Sakkinen @ 2019-08-05 22:51 UTC (permalink / raw)
To: Sasha Levin
Cc: peterhuewe, jgg, corbet, linux-kernel, linux-doc, linux-integrity,
linux-kernel, thiruan, bryankel, tee-dev, ilias.apalodimas,
sumit.garg, rdunlap
In-Reply-To: <20190805180518.GC17747@sasha-vm>
On Mon, Aug 05, 2019 at 02:05:18PM -0400, Sasha Levin wrote:
> On Mon, Aug 05, 2019 at 12:44:28AM +0300, Jarkko Sakkinen wrote:
> > On Thu, Jul 11, 2019 at 11:08:58PM +0300, Jarkko Sakkinen wrote:
> > > On Fri, Jul 05, 2019 at 04:47:44PM -0400, Sasha Levin wrote:
> > > > Changes from v7:
> > > >
> > > > - Address Jarkko's comments.
> > > >
> > > > Sasha Levin (2):
> > > > fTPM: firmware TPM running in TEE
> > > > fTPM: add documentation for ftpm driver
> > > >
> > > > Documentation/security/tpm/index.rst | 1 +
> > > > Documentation/security/tpm/tpm_ftpm_tee.rst | 27 ++
> > > > drivers/char/tpm/Kconfig | 5 +
> > > > drivers/char/tpm/Makefile | 1 +
> > > > drivers/char/tpm/tpm_ftpm_tee.c | 350 ++++++++++++++++++++
> > > > drivers/char/tpm/tpm_ftpm_tee.h | 40 +++
> > > > 6 files changed, 424 insertions(+)
> > > > create mode 100644 Documentation/security/tpm/tpm_ftpm_tee.rst
> > > > create mode 100644 drivers/char/tpm/tpm_ftpm_tee.c
> > > > create mode 100644 drivers/char/tpm/tpm_ftpm_tee.h
> > > >
> > > > --
> > > > 2.20.1
> > > >
> > >
> > > I applied the patches now. Appreciate a lot the patience with these.
> > > Thank you.
> >
> > Hi, can you possibly fix these:
>
> Any objection to sending you a patch on top of your tree instead?
Go ahead. Added the previous patches to my master.
/Jarkko
^ permalink raw reply
* Re: [PATCH v8 0/2] fTPM: firmware TPM running in TEE
From: Sasha Levin @ 2019-08-05 18:05 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: peterhuewe, jgg, corbet, linux-kernel, linux-doc, linux-integrity,
linux-kernel, thiruan, bryankel, tee-dev, ilias.apalodimas,
sumit.garg, rdunlap
In-Reply-To: <20190804214218.vdv2sn4oc4cityy2@linux.intel.com>
On Mon, Aug 05, 2019 at 12:44:28AM +0300, Jarkko Sakkinen wrote:
>On Thu, Jul 11, 2019 at 11:08:58PM +0300, Jarkko Sakkinen wrote:
>> On Fri, Jul 05, 2019 at 04:47:44PM -0400, Sasha Levin wrote:
>> > Changes from v7:
>> >
>> > - Address Jarkko's comments.
>> >
>> > Sasha Levin (2):
>> > fTPM: firmware TPM running in TEE
>> > fTPM: add documentation for ftpm driver
>> >
>> > Documentation/security/tpm/index.rst | 1 +
>> > Documentation/security/tpm/tpm_ftpm_tee.rst | 27 ++
>> > drivers/char/tpm/Kconfig | 5 +
>> > drivers/char/tpm/Makefile | 1 +
>> > drivers/char/tpm/tpm_ftpm_tee.c | 350 ++++++++++++++++++++
>> > drivers/char/tpm/tpm_ftpm_tee.h | 40 +++
>> > 6 files changed, 424 insertions(+)
>> > create mode 100644 Documentation/security/tpm/tpm_ftpm_tee.rst
>> > create mode 100644 drivers/char/tpm/tpm_ftpm_tee.c
>> > create mode 100644 drivers/char/tpm/tpm_ftpm_tee.h
>> >
>> > --
>> > 2.20.1
>> >
>>
>> I applied the patches now. Appreciate a lot the patience with these.
>> Thank you.
>
>Hi, can you possibly fix these:
Any objection to sending you a patch on top of your tree instead?
--
Thanks,
Sasha
^ permalink raw reply
* Re: [PATCH 0/6] hwspinlock: allow sharing of hwspinlocks
From: Bjorn Andersson @ 2019-08-05 17:46 UTC (permalink / raw)
To: Fabien DESSENNE
Cc: Ohad Ben-Cohen, Rob Herring, Mark Rutland, Maxime Coquelin,
Alexandre TORGUE, Jonathan Corbet,
linux-remoteproc@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-stm32@st-md-mailman.stormreply.com,
linux-arm-kernel@lists.infradead.org, linux-doc@vger.kernel.org,
Benjamin GAIGNARD
In-Reply-To: <1a057176-81ab-e302-4375-2717ceef6924@st.com>
On Mon 05 Aug 01:48 PDT 2019, Fabien DESSENNE wrote:
>
> On 01/08/2019 9:14 PM, Bjorn Andersson wrote:
> > On Wed 13 Mar 08:50 PDT 2019, Fabien Dessenne wrote:
> >
> >> The current implementation does not allow two different devices to use
> >> a common hwspinlock. This patch set proposes to have, as an option, some
> >> hwspinlocks shared between several users.
> >>
> >> Below is an example that explain the need for this:
> >> exti: interrupt-controller@5000d000 {
> >> compatible = "st,stm32mp1-exti", "syscon";
> >> interrupt-controller;
> >> #interrupt-cells = <2>;
> >> reg = <0x5000d000 0x400>;
> >> hwlocks = <&hsem 1>;
> >> };
> >> The two drivers (stm32mp1-exti and syscon) refer to the same hwlock.
> >> With the current hwspinlock implementation, only the first driver succeeds
> >> in requesting (hwspin_lock_request_specific) the hwlock. The second request
> >> fails.
> >>
> >>
> >> The proposed approach does not modify the API, but extends the DT 'hwlocks'
> >> property with a second optional parameter (the first one identifies an
> >> hwlock) that specifies whether an hwlock is requested for exclusive usage
> >> (current behavior) or can be shared between several users.
> >> Examples:
> >> hwlocks = <&hsem 8>; Ref to hwlock #8 for exclusive usage
> >> hwlocks = <&hsem 8 0>; Ref to hwlock #8 for exclusive (0) usage
> >> hwlocks = <&hsem 8 1>; Ref to hwlock #8 for shared (1) usage
> >>
> >> As a constraint, the #hwlock-cells value must be 1 or 2.
> >> In the current implementation, this can have theorically any value but:
> >> - all of the exisiting drivers use the same value : 1.
> >> - the framework supports only one value : 1 (see implementation of
> >> of_hwspin_lock_simple_xlate())
> >> Hence, it shall not be a problem to restrict this value to 1 or 2 since
> >> it won't break any driver.
> >>
> > Hi Fabien,
> >
> > Your series looks good, but it makes me wonder why the hardware locks
> > should be an exclusive resource.
> >
> > How about just making all (specific) locks shared?
>
> Hi Bjorn,
>
> Making all locks shared is a possible implementation (my first
> implementation
> was going this way) but there are some drawbacks we must be aware of:
>
> A/ This theoretically break the legacy behavior (the legacy works with
> exclusive (UNUSED radix tag) usage). As a consequence, an existing driver
> that is currently failing to request a lock (already claimed by another
> user) would now work fine. Not sure that there are such drivers, so this
> point is probably not a real issue.
>
Right, it's possible that a previously misconfigured system now
successfully probes more than one device that uses a particular
spinlock. But such system would be suffering from issues related to e.g.
probe ordering.
So I think we should ignore this issue.
> B/ This would introduce some inconsistency between the two 'request' API
> which are hwspin_lock_request() and hwspin_lock_request_specific().
> hwspin_lock_request() looks for an unused lock, so requests for an exclusive
> usage. On the other side, request_specific() would request shared locks.
> Worst the following sequence can transform an exclusive usage into a shared
>
There is already an inconsistency in between these; as with above any
system that uses both request() and request_specific() will be suffering
from intermittent failures due to probe ordering.
> one:
> -hwspin_lock_request() -> returns Id#0 (exclusive)
> -hwspin_lock_request() -> returns Id#1 (exclusive)
> -hwspin_lock_request_specific(0) -> returns Id#0 and makes Id#0 shared
> Honestly I am not sure that this is a real issue, but it's better to have it
> in mind before we take ay decision
The case where I can see a
problem with this would be if the two clients somehow would nest their
locking regions.
But generally I think this could consider this an improvement, because
the request_specific() would now be able to acquire its hwlock, with
some additional contention due to the multiple use.
> I could not find any driver using the hwspin_lock_request() API, we
> may decide to remove (or to make deprecated) this API, having
> everything 'shared without any conditions'.
>
It would be nice to have an upstream user of this API.
>
> I can see three options:
> 1- Keep my initial proposition
> 2- Have hwspin_lock_request_specific() using shared locks and
> hwspin_lock_request() using unused (so 'initially' exclusive) locks.
> 3- Have hwspin_lock_request_specific() using shared locks and
> remove/make deprecated hwspin_lock_request().
>
> Just let me know what is your preference.
>
I think we should start with #2 and would like input from e.g. Suman
regarding #3.
Regards,
Bjorn
> BR
>
> Fabien
>
> >
> > Regards,
> > Bjorn
> >
> >> Fabien Dessenne (6):
> >> dt-bindings: hwlock: add support of shared locks
> >> hwspinlock: allow sharing of hwspinlocks
> >> dt-bindings: hwlock: update STM32 #hwlock-cells value
> >> ARM: dts: stm32: Add hwspinlock node for stm32mp157 SoC
> >> ARM: dts: stm32: Add hwlock for irqchip on stm32mp157
> >> ARM: dts: stm32: hwlocks for GPIO for stm32mp157
> >>
> >> .../devicetree/bindings/hwlock/hwlock.txt | 27 +++++--
> >> .../bindings/hwlock/st,stm32-hwspinlock.txt | 6 +-
> >> Documentation/hwspinlock.txt | 10 ++-
> >> arch/arm/boot/dts/stm32mp157-pinctrl.dtsi | 2 +
> >> arch/arm/boot/dts/stm32mp157c.dtsi | 10 +++
> >> drivers/hwspinlock/hwspinlock_core.c | 82 +++++++++++++++++-----
> >> drivers/hwspinlock/hwspinlock_internal.h | 2 +
> >> 7 files changed, 108 insertions(+), 31 deletions(-)
> >>
> >> --
> >> 2.7.4
> >>
^ permalink raw reply
* [PATCH v4 2/5] [RFC] x86: Add support for idle bit in swap PTE
From: Joel Fernandes (Google) @ 2019-08-05 17:04 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Alexey Dobriyan, Andrew Morton,
Borislav Petkov, Brendan Gregg, Catalin Marinas, Christian Hansen,
dancol, fmayer, H. Peter Anvin, Ingo Molnar, joelaf,
Jonathan Corbet, Kees Cook, kernel-team, linux-api, linux-doc,
linux-fsdevel, linux-mm, Michal Hocko, Mike Rapoport, minchan,
namhyung, paulmck, Robin Murphy, Roman Gushchin, Stephen Rothwell,
surenb, Thomas Gleixner, tkjos, Vladimir Davydov, Vlastimil Babka,
Will Deacon
In-Reply-To: <20190805170451.26009-1-joel@joelfernandes.org>
This bit will be used by idle page tracking code to correctly identify
if a page that was swapped out was idle before it got swapped out.
Without this PTE bit, we lose information about if a page is idle or not
since the page frame gets unmapped and the page gets freed.
Bits 2-6 are unused in the swap PTE (see the comment in
arch/x86/include/asm/pgtable_64.h). Bit 2 corresponds to _PAGE_USER. Use
it for swap PTE purposes.
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
arch/x86/Kconfig | 1 +
arch/x86/include/asm/pgtable.h | 15 +++++++++++++++
arch/x86/include/asm/pgtable_types.h | 6 ++++++
3 files changed, 22 insertions(+)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 222855cc0158..728f22370f17 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -139,6 +139,7 @@ config X86
select HAVE_ARCH_MMAP_RND_COMPAT_BITS if MMU && COMPAT
select HAVE_ARCH_COMPAT_MMAP_BASES if MMU && COMPAT
select HAVE_ARCH_PREL32_RELOCATIONS
+ select HAVE_ARCH_PTE_SWP_PGIDLE
select HAVE_ARCH_SECCOMP_FILTER
select HAVE_ARCH_THREAD_STRUCT_WHITELIST
select HAVE_ARCH_STACKLEAK
diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
index 0bc530c4eb13..ef3e662cee4a 100644
--- a/arch/x86/include/asm/pgtable.h
+++ b/arch/x86/include/asm/pgtable.h
@@ -1371,6 +1371,21 @@ static inline pmd_t pmd_swp_clear_soft_dirty(pmd_t pmd)
#endif
#endif
+static inline pte_t pte_swp_mkpage_idle(pte_t pte)
+{
+ return pte_set_flags(pte, _PAGE_SWP_PGIDLE);
+}
+
+static inline int pte_swp_page_idle(pte_t pte)
+{
+ return pte_flags(pte) & _PAGE_SWP_PGIDLE;
+}
+
+static inline pte_t pte_swp_clear_mkpage_idle(pte_t pte)
+{
+ return pte_clear_flags(pte, _PAGE_SWP_PGIDLE);
+}
+
#define PKRU_AD_BIT 0x1
#define PKRU_WD_BIT 0x2
#define PKRU_BITS_PER_PKEY 2
diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
index b5e49e6bac63..6739cba4c900 100644
--- a/arch/x86/include/asm/pgtable_types.h
+++ b/arch/x86/include/asm/pgtable_types.h
@@ -100,6 +100,12 @@
#define _PAGE_SWP_SOFT_DIRTY (_AT(pteval_t, 0))
#endif
+#ifdef CONFIG_IDLE_PAGE_TRACKING
+#define _PAGE_SWP_PGIDLE _PAGE_USER
+#else
+#define _PAGE_SWP_PGIDLE (_AT(pteval_t, 0))
+#endif
+
#if defined(CONFIG_X86_64) || defined(CONFIG_X86_PAE)
#define _PAGE_NX (_AT(pteval_t, 1) << _PAGE_BIT_NX)
#define _PAGE_DEVMAP (_AT(u64, 1) << _PAGE_BIT_DEVMAP)
--
2.22.0.770.g0f2c4a37fd-goog
^ permalink raw reply related
* [PATCH v4 5/5] doc: Update documentation for page_idle virtual address indexing
From: Joel Fernandes (Google) @ 2019-08-05 17:04 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Mike Rapoport, Sandeep Patil,
Alexey Dobriyan, Andrew Morton, Borislav Petkov, Brendan Gregg,
Catalin Marinas, Christian Hansen, dancol, fmayer, H. Peter Anvin,
Ingo Molnar, joelaf, Jonathan Corbet, Kees Cook, kernel-team,
linux-api, linux-doc, linux-fsdevel, linux-mm, Michal Hocko,
minchan, namhyung, paulmck, Robin Murphy, Roman Gushchin,
Stephen Rothwell, surenb, Thomas Gleixner, tkjos,
Vladimir Davydov, Vlastimil Babka, Will Deacon
In-Reply-To: <20190805170451.26009-1-joel@joelfernandes.org>
This patch updates the documentation with the new page_idle tracking
feature which uses virtual address indexing.
Reviewed-by: Mike Rapoport <rppt@linux.ibm.com>
Reviewed-by: Sandeep Patil <sspatil@google.com>
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
.../admin-guide/mm/idle_page_tracking.rst | 43 ++++++++++++++++---
1 file changed, 36 insertions(+), 7 deletions(-)
diff --git a/Documentation/admin-guide/mm/idle_page_tracking.rst b/Documentation/admin-guide/mm/idle_page_tracking.rst
index df9394fb39c2..9eef32000f5e 100644
--- a/Documentation/admin-guide/mm/idle_page_tracking.rst
+++ b/Documentation/admin-guide/mm/idle_page_tracking.rst
@@ -19,10 +19,14 @@ It is enabled by CONFIG_IDLE_PAGE_TRACKING=y.
User API
========
+There are 2 ways to access the idle page tracking API. One uses physical
+address indexing, another uses a simpler virtual address indexing scheme.
-The idle page tracking API is located at ``/sys/kernel/mm/page_idle``.
-Currently, it consists of the only read-write file,
-``/sys/kernel/mm/page_idle/bitmap``.
+Physical address indexing
+-------------------------
+The idle page tracking API for physical address indexing using page frame
+numbers (PFN) is located at ``/sys/kernel/mm/page_idle``. Currently, it
+consists of the only read-write file, ``/sys/kernel/mm/page_idle/bitmap``.
The file implements a bitmap where each bit corresponds to a memory page. The
bitmap is represented by an array of 8-byte integers, and the page at PFN #i is
@@ -74,6 +78,31 @@ See :ref:`Documentation/admin-guide/mm/pagemap.rst <pagemap>` for more
information about ``/proc/pid/pagemap``, ``/proc/kpageflags``, and
``/proc/kpagecgroup``.
+Virtual address indexing
+------------------------
+The idle page tracking API for virtual address indexing using virtual frame
+numbers (VFN) for a process ``<pid>`` is located at ``/proc/<pid>/page_idle``.
+It is a bitmap that follows the same semantics as
+``/sys/kernel/mm/page_idle/bitmap`` except that it uses virtual instead of
+physical frame numbers.
+
+This idle page tracking API does not deal with PFN so it does not require prior
+lookups of ``pagemap``. This is an advantage on some systems where looking up
+PFN is considered a security issue. Also in some cases, this interface could
+be slightly more reliable to use than physical address indexing, since in
+physical address indexing, address space changes can occur between reading the
+``pagemap`` and reading the ``bitmap``, while in virtual address indexing, the
+process's ``mmap_sem`` is held for the duration of the access.
+
+To estimate the amount of pages that are not used by a workload one should:
+
+ 1. Mark all the workload's pages as idle by setting corresponding bits in
+ ``/proc/<pid>/page_idle``.
+
+ 2. Wait until the workload accesses its working set.
+
+ 3. Read ``/proc/<pid>/page_idle`` and count the number of bits set.
+
.. _impl_details:
Implementation Details
@@ -99,10 +128,10 @@ When a dirty page is written to swap or disk as a result of memory reclaim or
exceeding the dirty memory limit, it is not marked referenced.
The idle memory tracking feature adds a new page flag, the Idle flag. This flag
-is set manually, by writing to ``/sys/kernel/mm/page_idle/bitmap`` (see the
-:ref:`User API <user_api>`
-section), and cleared automatically whenever a page is referenced as defined
-above.
+is set manually, by writing to ``/sys/kernel/mm/page_idle/bitmap`` for physical
+addressing or by writing to ``/proc/<pid>/page_idle`` for virtual
+addressing (see the :ref:`User API <user_api>` section), and cleared
+automatically whenever a page is referenced as defined above.
When a page is marked idle, the Accessed bit must be cleared in all PTEs it is
mapped to, otherwise we will not be able to detect accesses to the page coming
--
2.22.0.770.g0f2c4a37fd-goog
^ permalink raw reply related
* [PATCH v4 4/5] page_idle: Drain all LRU pagevec before idle tracking
From: Joel Fernandes (Google) @ 2019-08-05 17:04 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Alexey Dobriyan, Andrew Morton,
Borislav Petkov, Brendan Gregg, Catalin Marinas, Christian Hansen,
dancol, fmayer, H. Peter Anvin, Ingo Molnar, joelaf,
Jonathan Corbet, Kees Cook, kernel-team, linux-api, linux-doc,
linux-fsdevel, linux-mm, Michal Hocko, Mike Rapoport, minchan,
namhyung, paulmck, Robin Murphy, Roman Gushchin, Stephen Rothwell,
surenb, Thomas Gleixner, tkjos, Vladimir Davydov, Vlastimil Babka,
Will Deacon
In-Reply-To: <20190805170451.26009-1-joel@joelfernandes.org>
During idle tracking, we see that sometimes faulted anon pages are in
pagevec but are not drained to LRU. Idle tracking considers pages only
on LRU. Drain all CPU's LRU before starting idle tracking.
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
mm/page_idle.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/mm/page_idle.c b/mm/page_idle.c
index a5b00d63216c..2972367a599f 100644
--- a/mm/page_idle.c
+++ b/mm/page_idle.c
@@ -180,6 +180,8 @@ static ssize_t page_idle_bitmap_read(struct file *file, struct kobject *kobj,
unsigned long pfn, end_pfn;
int bit, ret;
+ lru_add_drain_all();
+
ret = page_idle_get_frames(pos, count, NULL, &pfn, &end_pfn);
if (ret == -ENXIO)
return 0; /* Reads beyond max_pfn do nothing */
@@ -211,6 +213,8 @@ static ssize_t page_idle_bitmap_write(struct file *file, struct kobject *kobj,
unsigned long pfn, end_pfn;
int bit, ret;
+ lru_add_drain_all();
+
ret = page_idle_get_frames(pos, count, NULL, &pfn, &end_pfn);
if (ret)
return ret;
@@ -428,6 +432,8 @@ ssize_t page_idle_proc_generic(struct file *file, char __user *ubuff,
walk.private = &priv;
walk.mm = mm;
+ lru_add_drain_all();
+
down_read(&mm->mmap_sem);
/*
--
2.22.0.770.g0f2c4a37fd-goog
^ permalink raw reply related
* [PATCH v4 3/5] [RFC] arm64: Add support for idle bit in swap PTE
From: Joel Fernandes (Google) @ 2019-08-05 17:04 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Robin Murphy, Alexey Dobriyan,
Andrew Morton, Borislav Petkov, Brendan Gregg, Catalin Marinas,
Christian Hansen, dancol, fmayer, H. Peter Anvin, Ingo Molnar,
joelaf, Jonathan Corbet, Kees Cook, kernel-team, linux-api,
linux-doc, linux-fsdevel, linux-mm, Michal Hocko, Mike Rapoport,
minchan, namhyung, paulmck, Roman Gushchin, Stephen Rothwell,
surenb, Thomas Gleixner, tkjos, Vladimir Davydov, Vlastimil Babka,
Will Deacon
In-Reply-To: <20190805170451.26009-1-joel@joelfernandes.org>
This bit will be used by idle page tracking code to correctly identify
if a page that was swapped out was idle before it got swapped out.
Without this PTE bit, we lose information about if a page is idle or not
since the page frame gets unmapped.
In this patch we reuse PTE_DEVMAP bit since idle page tracking only
works on user pages in the LRU. Device pages should not consitute those
so it should be unused and safe to use.
Cc: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
arch/arm64/Kconfig | 1 +
arch/arm64/include/asm/pgtable-prot.h | 1 +
arch/arm64/include/asm/pgtable.h | 15 +++++++++++++++
3 files changed, 17 insertions(+)
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 3adcec05b1f6..9d1412c693d7 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -128,6 +128,7 @@ config ARM64
select HAVE_ARCH_MMAP_RND_BITS
select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT
select HAVE_ARCH_PREL32_RELOCATIONS
+ select HAVE_ARCH_PTE_SWP_PGIDLE
select HAVE_ARCH_SECCOMP_FILTER
select HAVE_ARCH_STACKLEAK
select HAVE_ARCH_THREAD_STRUCT_WHITELIST
diff --git a/arch/arm64/include/asm/pgtable-prot.h b/arch/arm64/include/asm/pgtable-prot.h
index 92d2e9f28f28..917b15c5d63a 100644
--- a/arch/arm64/include/asm/pgtable-prot.h
+++ b/arch/arm64/include/asm/pgtable-prot.h
@@ -18,6 +18,7 @@
#define PTE_SPECIAL (_AT(pteval_t, 1) << 56)
#define PTE_DEVMAP (_AT(pteval_t, 1) << 57)
#define PTE_PROT_NONE (_AT(pteval_t, 1) << 58) /* only when !PTE_VALID */
+#define PTE_SWP_PGIDLE PTE_DEVMAP /* for idle page tracking during swapout */
#ifndef __ASSEMBLY__
diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
index 3f5461f7b560..558f5ebd81ba 100644
--- a/arch/arm64/include/asm/pgtable.h
+++ b/arch/arm64/include/asm/pgtable.h
@@ -212,6 +212,21 @@ static inline pte_t pte_mkdevmap(pte_t pte)
return set_pte_bit(pte, __pgprot(PTE_DEVMAP));
}
+static inline int pte_swp_page_idle(pte_t pte)
+{
+ return 0;
+}
+
+static inline pte_t pte_swp_mkpage_idle(pte_t pte)
+{
+ return set_pte_bit(pte, __pgprot(PTE_SWP_PGIDLE));
+}
+
+static inline pte_t pte_swp_clear_page_idle(pte_t pte)
+{
+ return clear_pte_bit(pte, __pgprot(PTE_SWP_PGIDLE));
+}
+
static inline void set_pte(pte_t *ptep, pte_t pte)
{
WRITE_ONCE(*ptep, pte);
--
2.22.0.770.g0f2c4a37fd-goog
^ permalink raw reply related
* [PATCH v4 1/5] mm/page_idle: Add per-pid idle page tracking using virtual indexing
From: Joel Fernandes (Google) @ 2019-08-05 17:04 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Alexey Dobriyan, Andrew Morton,
Borislav Petkov, Brendan Gregg, Catalin Marinas, Christian Hansen,
dancol, fmayer, H. Peter Anvin, Ingo Molnar, joelaf,
Jonathan Corbet, Kees Cook, kernel-team, linux-api, linux-doc,
linux-fsdevel, linux-mm, Michal Hocko, Mike Rapoport, minchan,
namhyung, paulmck, Robin Murphy, Roman Gushchin, Stephen Rothwell,
surenb, Thomas Gleixner, tkjos, Vladimir Davydov, Vlastimil Babka,
Will Deacon
The page_idle tracking feature currently requires looking up the pagemap
for a process followed by interacting with /sys/kernel/mm/page_idle.
Looking up PFN from pagemap in Android devices is not supported by
unprivileged process and requires SYS_ADMIN and gives 0 for the PFN.
This patch adds support to directly interact with page_idle tracking at
the PID level by introducing a /proc/<pid>/page_idle file. It follows
the exact same semantics as the global /sys/kernel/mm/page_idle, but now
looking up PFN through pagemap is not needed since the interface uses
virtual frame numbers, and at the same time also does not require
SYS_ADMIN.
In Android, we are using this for the heap profiler (heapprofd) which
profiles and pin points code paths which allocates and leaves memory
idle for long periods of time. This method solves the security issue
with userspace learning the PFN, and while at it is also shown to yield
better results than the pagemap lookup, the theory being that the window
where the address space can change is reduced by eliminating the
intermediate pagemap look up stage. In virtual address indexing, the
process's mmap_sem is held for the duration of the access.
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
v3->v4: Minor fixups (Minchan)
Add swap pte handling (Konstantin, Minchan)
v2->v3:
Fixed a bug where I was doing a kfree that is not needed due to not
needing to do GFP_ATOMIC allocations.
v1->v2:
Mark swap ptes as idle (Minchan)
Avoid need for GFP_ATOMIC (Andrew)
Get rid of idle_page_list lock by moving list to stack
Internal review -> v1:
Fixes from Suren.
Corrections to change log, docs (Florian, Sandeep)
arch/Kconfig | 3 +
fs/proc/base.c | 3 +
fs/proc/internal.h | 1 +
fs/proc/task_mmu.c | 43 ++++
include/asm-generic/pgtable.h | 6 +
include/linux/page_idle.h | 4 +
mm/page_idle.c | 359 +++++++++++++++++++++++++++++-----
mm/rmap.c | 2 +
8 files changed, 376 insertions(+), 45 deletions(-)
diff --git a/arch/Kconfig b/arch/Kconfig
index a7b57dd42c26..3aa121ce824e 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -575,6 +575,9 @@ config ARCH_WANT_HUGE_PMD_SHARE
config HAVE_ARCH_SOFT_DIRTY
bool
+config HAVE_ARCH_PTE_SWP_PGIDLE
+ bool
+
config HAVE_MOD_ARCH_SPECIFIC
bool
help
diff --git a/fs/proc/base.c b/fs/proc/base.c
index ebea9501afb8..fd2f74bd4e35 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -3039,6 +3039,9 @@ static const struct pid_entry tgid_base_stuff[] = {
REG("smaps", S_IRUGO, proc_pid_smaps_operations),
REG("smaps_rollup", S_IRUGO, proc_pid_smaps_rollup_operations),
REG("pagemap", S_IRUSR, proc_pagemap_operations),
+#ifdef CONFIG_IDLE_PAGE_TRACKING
+ REG("page_idle", S_IRUSR|S_IWUSR, proc_page_idle_operations),
+#endif
#endif
#ifdef CONFIG_SECURITY
DIR("attr", S_IRUGO|S_IXUGO, proc_attr_dir_inode_operations, proc_attr_dir_operations),
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index cd0c8d5ce9a1..bc9371880c63 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -293,6 +293,7 @@ extern const struct file_operations proc_pid_smaps_operations;
extern const struct file_operations proc_pid_smaps_rollup_operations;
extern const struct file_operations proc_clear_refs_operations;
extern const struct file_operations proc_pagemap_operations;
+extern const struct file_operations proc_page_idle_operations;
extern unsigned long task_vsize(struct mm_struct *);
extern unsigned long task_statm(struct mm_struct *,
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 582c5e680176..a9003fe8d267 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1650,6 +1650,49 @@ const struct file_operations proc_pagemap_operations = {
.open = pagemap_open,
.release = pagemap_release,
};
+
+#ifdef CONFIG_IDLE_PAGE_TRACKING
+static ssize_t proc_page_idle_read(struct file *file, char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ return page_idle_proc_read(file, buf, count, ppos);
+}
+
+static ssize_t proc_page_idle_write(struct file *file, const char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ return page_idle_proc_write(file, (char __user *)buf, count, ppos);
+}
+
+static int proc_page_idle_open(struct inode *inode, struct file *file)
+{
+ struct mm_struct *mm;
+
+ mm = proc_mem_open(inode, PTRACE_MODE_READ);
+ if (IS_ERR(mm))
+ return PTR_ERR(mm);
+ file->private_data = mm;
+ return 0;
+}
+
+static int proc_page_idle_release(struct inode *inode, struct file *file)
+{
+ struct mm_struct *mm = file->private_data;
+
+ if (mm)
+ mmdrop(mm);
+ return 0;
+}
+
+const struct file_operations proc_page_idle_operations = {
+ .llseek = mem_lseek, /* borrow this */
+ .read = proc_page_idle_read,
+ .write = proc_page_idle_write,
+ .open = proc_page_idle_open,
+ .release = proc_page_idle_release,
+};
+#endif /* CONFIG_IDLE_PAGE_TRACKING */
+
#endif /* CONFIG_PROC_PAGE_MONITOR */
#ifdef CONFIG_NUMA
diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pgtable.h
index 75d9d68a6de7..6d51d0a355a7 100644
--- a/include/asm-generic/pgtable.h
+++ b/include/asm-generic/pgtable.h
@@ -712,6 +712,12 @@ static inline void ptep_modify_prot_commit(struct vm_area_struct *vma,
#define arch_start_context_switch(prev) do {} while (0)
#endif
+#ifndef CONFIG_HAVE_ARCH_PTE_SWP_PGIDLE
+static inline pte_t pte_swp_mkpage_idle(pte_t pte) { return pte; }
+static inline int pte_swp_page_idle(pte_t pte) { return 0; }
+static inline pte_t pte_swp_clear_mkpage_idle(pte_t pte) { return pte; }
+#endif
+
#ifdef CONFIG_HAVE_ARCH_SOFT_DIRTY
#ifndef CONFIG_ARCH_ENABLE_THP_MIGRATION
static inline pmd_t pmd_swp_mksoft_dirty(pmd_t pmd)
diff --git a/include/linux/page_idle.h b/include/linux/page_idle.h
index 1e894d34bdce..f1bc2640d85e 100644
--- a/include/linux/page_idle.h
+++ b/include/linux/page_idle.h
@@ -106,6 +106,10 @@ static inline void clear_page_idle(struct page *page)
}
#endif /* CONFIG_64BIT */
+ssize_t page_idle_proc_write(struct file *file,
+ char __user *buf, size_t count, loff_t *ppos, struct task_struct *tsk);
+ssize_t page_idle_proc_read(struct file *file,
+ char __user *buf, size_t count, loff_t *ppos, struct task_struct *tsk);
#else /* !CONFIG_IDLE_PAGE_TRACKING */
static inline bool page_is_young(struct page *page)
diff --git a/mm/page_idle.c b/mm/page_idle.c
index 295512465065..a5b00d63216c 100644
--- a/mm/page_idle.c
+++ b/mm/page_idle.c
@@ -5,17 +5,22 @@
#include <linux/sysfs.h>
#include <linux/kobject.h>
#include <linux/mm.h>
-#include <linux/mmzone.h>
-#include <linux/pagemap.h>
-#include <linux/rmap.h>
#include <linux/mmu_notifier.h>
+#include <linux/mmzone.h>
#include <linux/page_ext.h>
#include <linux/page_idle.h>
+#include <linux/pagemap.h>
+#include <linux/rmap.h>
+#include <linux/sched/mm.h>
+#include <linux/swap.h>
+#include <linux/swapops.h>
#define BITMAP_CHUNK_SIZE sizeof(u64)
#define BITMAP_CHUNK_BITS (BITMAP_CHUNK_SIZE * BITS_PER_BYTE)
/*
+ * Get a reference to a page for idle tracking purposes, with additional checks.
+ *
* Idle page tracking only considers user memory pages, for other types of
* pages the idle flag is always unset and an attempt to set it is silently
* ignored.
@@ -25,18 +30,13 @@
* page tracking. With such an indicator of user pages we can skip isolated
* pages, but since there are not usually many of them, it will hardly affect
* the overall result.
- *
- * This function tries to get a user memory page by pfn as described above.
*/
-static struct page *page_idle_get_page(unsigned long pfn)
+static struct page *page_idle_get_page(struct page *page_in)
{
struct page *page;
pg_data_t *pgdat;
- if (!pfn_valid(pfn))
- return NULL;
-
- page = pfn_to_page(pfn);
+ page = page_in;
if (!page || !PageLRU(page) ||
!get_page_unless_zero(page))
return NULL;
@@ -51,6 +51,18 @@ static struct page *page_idle_get_page(unsigned long pfn)
return page;
}
+/*
+ * This function tries to get a user memory page by pfn as described above.
+ */
+static struct page *page_idle_get_page_pfn(unsigned long pfn)
+{
+
+ if (!pfn_valid(pfn))
+ return NULL;
+
+ return page_idle_get_page(pfn_to_page(pfn));
+}
+
static bool page_idle_clear_pte_refs_one(struct page *page,
struct vm_area_struct *vma,
unsigned long addr, void *arg)
@@ -118,6 +130,47 @@ static void page_idle_clear_pte_refs(struct page *page)
unlock_page(page);
}
+/* Helper to get the start and end frame given a pos and count */
+static int page_idle_get_frames(loff_t pos, size_t count, struct mm_struct *mm,
+ unsigned long *start, unsigned long *end)
+{
+ unsigned long max_frame;
+
+ /* If an mm is not given, assume we want physical frames */
+ max_frame = mm ? (mm->task_size >> PAGE_SHIFT) : max_pfn;
+
+ if (pos % BITMAP_CHUNK_SIZE || count % BITMAP_CHUNK_SIZE)
+ return -EINVAL;
+
+ *start = pos * BITS_PER_BYTE;
+ if (*start >= max_frame)
+ return -ENXIO;
+
+ *end = *start + count * BITS_PER_BYTE;
+ if (*end > max_frame)
+ *end = max_frame;
+ return 0;
+}
+
+static bool page_idle_pte_check(struct page *page)
+{
+ if (!page)
+ return false;
+
+ if (page_is_idle(page)) {
+ /*
+ * The page might have been referenced via a
+ * pte, in which case it is not idle. Clear
+ * refs and recheck.
+ */
+ page_idle_clear_pte_refs(page);
+ if (page_is_idle(page))
+ return true;
+ }
+
+ return false;
+}
+
static ssize_t page_idle_bitmap_read(struct file *file, struct kobject *kobj,
struct bin_attribute *attr, char *buf,
loff_t pos, size_t count)
@@ -125,35 +178,21 @@ static ssize_t page_idle_bitmap_read(struct file *file, struct kobject *kobj,
u64 *out = (u64 *)buf;
struct page *page;
unsigned long pfn, end_pfn;
- int bit;
+ int bit, ret;
- if (pos % BITMAP_CHUNK_SIZE || count % BITMAP_CHUNK_SIZE)
- return -EINVAL;
-
- pfn = pos * BITS_PER_BYTE;
- if (pfn >= max_pfn)
- return 0;
-
- end_pfn = pfn + count * BITS_PER_BYTE;
- if (end_pfn > max_pfn)
- end_pfn = max_pfn;
+ ret = page_idle_get_frames(pos, count, NULL, &pfn, &end_pfn);
+ if (ret == -ENXIO)
+ return 0; /* Reads beyond max_pfn do nothing */
+ else if (ret)
+ return ret;
for (; pfn < end_pfn; pfn++) {
bit = pfn % BITMAP_CHUNK_BITS;
if (!bit)
*out = 0ULL;
- page = page_idle_get_page(pfn);
- if (page) {
- if (page_is_idle(page)) {
- /*
- * The page might have been referenced via a
- * pte, in which case it is not idle. Clear
- * refs and recheck.
- */
- page_idle_clear_pte_refs(page);
- if (page_is_idle(page))
- *out |= 1ULL << bit;
- }
+ page = page_idle_get_page_pfn(pfn);
+ if (page && page_idle_pte_check(page)) {
+ *out |= 1ULL << bit;
put_page(page);
}
if (bit == BITMAP_CHUNK_BITS - 1)
@@ -170,23 +209,16 @@ static ssize_t page_idle_bitmap_write(struct file *file, struct kobject *kobj,
const u64 *in = (u64 *)buf;
struct page *page;
unsigned long pfn, end_pfn;
- int bit;
-
- if (pos % BITMAP_CHUNK_SIZE || count % BITMAP_CHUNK_SIZE)
- return -EINVAL;
+ int bit, ret;
- pfn = pos * BITS_PER_BYTE;
- if (pfn >= max_pfn)
- return -ENXIO;
-
- end_pfn = pfn + count * BITS_PER_BYTE;
- if (end_pfn > max_pfn)
- end_pfn = max_pfn;
+ ret = page_idle_get_frames(pos, count, NULL, &pfn, &end_pfn);
+ if (ret)
+ return ret;
for (; pfn < end_pfn; pfn++) {
bit = pfn % BITMAP_CHUNK_BITS;
if ((*in >> bit) & 1) {
- page = page_idle_get_page(pfn);
+ page = page_idle_get_page_pfn(pfn);
if (page) {
page_idle_clear_pte_refs(page);
set_page_idle(page);
@@ -224,6 +256,243 @@ struct page_ext_operations page_idle_ops = {
};
#endif
+/* page_idle tracking for /proc/<pid>/page_idle */
+
+struct page_node {
+ struct page *page;
+ unsigned long addr;
+ struct list_head list;
+};
+
+struct page_idle_proc_priv {
+ unsigned long start_addr;
+ char *buffer;
+ int write;
+
+ /* Pre-allocate and provide nodes to pte_page_idle_proc_add() */
+ struct page_node *page_nodes;
+ int cur_page_node;
+ struct list_head *idle_page_list;
+};
+
+/*
+ * Set a page as idle or add it to a list to be set as idle later.
+ */
+static void pte_page_idle_proc_add(struct page *page,
+ unsigned long addr, struct mm_walk *walk)
+{
+ struct page *page_get = NULL;
+ struct page_node *pn;
+ int bit;
+ unsigned long frames;
+ struct page_idle_proc_priv *priv = walk->private;
+ u64 *chunk = (u64 *)priv->buffer;
+
+ if (priv->write) {
+ VM_BUG_ON(!page);
+
+ /* Find whether this page was asked to be marked */
+ frames = (addr - priv->start_addr) >> PAGE_SHIFT;
+ bit = frames % BITMAP_CHUNK_BITS;
+ chunk = &chunk[frames / BITMAP_CHUNK_BITS];
+ if (((*chunk >> bit) & 1) == 0)
+ return;
+ }
+
+ if (page) {
+ page_get = page_idle_get_page(page);
+ if (!page_get)
+ return;
+ } else {
+ /* For swapped pages, set output bit as idle */
+ frames = (addr - priv->start_addr) >> PAGE_SHIFT;
+ bit = frames % BITMAP_CHUNK_BITS;
+ chunk = &chunk[frames / BITMAP_CHUNK_BITS];
+ *chunk |= (1 << bit);
+ return;
+ }
+
+ /*
+ * For all other pages, add it to a list since we have to walk rmap,
+ * which acquires ptlock, and we cannot walk rmap right now.
+ */
+ pn = &(priv->page_nodes[priv->cur_page_node++]);
+ pn->page = page_get;
+ pn->addr = addr;
+ list_add(&pn->list, priv->idle_page_list);
+}
+
+static int pte_page_idle_proc_range(pmd_t *pmd, unsigned long addr,
+ unsigned long end,
+ struct mm_walk *walk)
+{
+ pte_t *pte;
+ spinlock_t *ptl;
+ struct page *page;
+ struct vm_area_struct *vma = walk->vma;
+ struct page_idle_proc_priv *priv = walk->private;
+
+ ptl = pmd_trans_huge_lock(pmd, vma);
+ if (ptl) {
+ if (pmd_present(*pmd)) {
+ page = follow_trans_huge_pmd(vma, addr, pmd,
+ FOLL_DUMP|FOLL_WRITE);
+ if (!IS_ERR_OR_NULL(page))
+ pte_page_idle_proc_add(page, addr, walk);
+ }
+ spin_unlock(ptl);
+ return 0;
+ }
+
+ if (pmd_trans_unstable(pmd))
+ return 0;
+
+ pte = pte_offset_map_lock(vma->vm_mm, pmd, addr, &ptl);
+ for (; addr != end; pte++, addr += PAGE_SIZE) {
+ /* For swap_pte handling, we use an idle bit in the swap pte. */
+ if (is_swap_pte(*pte)) {
+ if (priv->write) {
+ set_pte_at(walk->mm, addr, pte,
+ pte_swp_mkpage_idle(*pte));
+ } else {
+ /* If swap pte has idle bit set, report it as idle */
+ if (pte_swp_page_idle(*pte))
+ pte_page_idle_proc_add(NULL, addr, walk);
+ }
+ continue;
+ }
+
+ if (!pte_present(*pte))
+ continue;
+
+ page = vm_normal_page(vma, addr, *pte);
+ if (page)
+ pte_page_idle_proc_add(page, addr, walk);
+ }
+
+ pte_unmap_unlock(pte - 1, ptl);
+ return 0;
+}
+
+ssize_t page_idle_proc_generic(struct file *file, char __user *ubuff,
+ size_t count, loff_t *pos, int write)
+{
+ int ret;
+ char *buffer;
+ u64 *out;
+ unsigned long start_addr, end_addr, start_frame, end_frame;
+ struct mm_struct *mm = file->private_data;
+ struct mm_walk walk = { .pmd_entry = pte_page_idle_proc_range, };
+ struct page_node *cur;
+ struct page_idle_proc_priv priv;
+ bool walk_error = false;
+ LIST_HEAD(idle_page_list);
+
+ if (!mm || !mmget_not_zero(mm))
+ return -EINVAL;
+
+ if (count > PAGE_SIZE)
+ count = PAGE_SIZE;
+
+ buffer = kzalloc(PAGE_SIZE, GFP_KERNEL);
+ if (!buffer) {
+ ret = -ENOMEM;
+ goto out_mmput;
+ }
+ out = (u64 *)buffer;
+
+ if (write && copy_from_user(buffer, ubuff, count)) {
+ ret = -EFAULT;
+ goto out;
+ }
+
+ ret = page_idle_get_frames(*pos, count, mm, &start_frame, &end_frame);
+ if (ret)
+ goto out;
+
+ start_addr = (start_frame << PAGE_SHIFT);
+ end_addr = (end_frame << PAGE_SHIFT);
+ priv.buffer = buffer;
+ priv.start_addr = start_addr;
+ priv.write = write;
+
+ priv.idle_page_list = &idle_page_list;
+ priv.cur_page_node = 0;
+ priv.page_nodes = kzalloc(sizeof(struct page_node) *
+ (end_frame - start_frame), GFP_KERNEL);
+ if (!priv.page_nodes) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ walk.private = &priv;
+ walk.mm = mm;
+
+ down_read(&mm->mmap_sem);
+
+ /*
+ * idle_page_list is needed because walk_page_vma() holds ptlock which
+ * deadlocks with page_idle_clear_pte_refs(). So we have to collect all
+ * pages first, and then call page_idle_clear_pte_refs().
+ */
+ ret = walk_page_range(start_addr, end_addr, &walk);
+ if (ret)
+ walk_error = true;
+
+ list_for_each_entry(cur, &idle_page_list, list) {
+ int bit, index;
+ unsigned long off;
+ struct page *page = cur->page;
+
+ if (unlikely(walk_error))
+ goto remove_page;
+
+ if (write) {
+ if (page) {
+ page_idle_clear_pte_refs(page);
+ set_page_idle(page);
+ }
+ } else {
+ /* If page is NULL, it was swapped out */
+ if (!page || page_idle_pte_check(page)) {
+ off = ((cur->addr) >> PAGE_SHIFT) - start_frame;
+ bit = off % BITMAP_CHUNK_BITS;
+ index = off / BITMAP_CHUNK_BITS;
+ out[index] |= 1ULL << bit;
+ }
+ }
+remove_page:
+ if (page)
+ put_page(page);
+ }
+
+ if (!write && !walk_error)
+ ret = copy_to_user(ubuff, buffer, count);
+
+ up_read(&mm->mmap_sem);
+ kfree(priv.page_nodes);
+out:
+ kfree(buffer);
+out_mmput:
+ mmput(mm);
+ if (!ret)
+ ret = count;
+ return ret;
+
+}
+
+ssize_t page_idle_proc_read(struct file *file, char __user *ubuff,
+ size_t count, loff_t *pos)
+{
+ return page_idle_proc_generic(file, ubuff, count, pos, 0);
+}
+
+ssize_t page_idle_proc_write(struct file *file, char __user *ubuff,
+ size_t count, loff_t *pos, struct mm_struct *mm)
+{
+ return page_idle_proc_generic(file, ubuff, count, pos, 1);
+}
+
static int __init page_idle_init(void)
{
int err;
diff --git a/mm/rmap.c b/mm/rmap.c
index e5dfe2ae6b0d..4bd618aab402 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1629,6 +1629,8 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
swp_pte = swp_entry_to_pte(entry);
if (pte_soft_dirty(pteval))
swp_pte = pte_swp_mksoft_dirty(swp_pte);
+ if (page_is_idle(page))
+ swp_pte = pte_swp_mkpage_idle(swp_pte);
set_pte_at(mm, address, pvmw.pte, swp_pte);
/* Invalidate as we cleared the pte */
mmu_notifier_invalidate_range(mm, address,
--
2.22.0.770.g0f2c4a37fd-goog
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox