Re: [RFC PATCH 2/3] mm: Drop use of test_and_set_skip in favor of just setting skip
在 2020/8/13 下午12:02, Alexander Duyck 写道:
> From: Alexander Duyck
>
> The only user of test_and_set_skip was isolate_migratepages_block and it
> was using it after a call that was testing and clearing the LRU flag. As
> such it really didn't need to be behind the LRU lock anymore as it wasn't
> really fulfilling its purpose.
>
> With that being the case we can simply drop the bit and instead directly
> just call the set_pageblock_skip function if the page we are working on is
> the valid_page at the start of the pageblock. It shouldn't be possible for
> us to encounter the bit being set since we obtained the LRU flag for the
> first page in the pageblock which means we would have exclusive access to
> setting the skip bit. As such we don't need to worry about the abort case
> since no other thread will be able to call what used to be
> test_and_set_skip.
>
> Since we have dropped the late abort case we can drop the code that was
> clearing the LRU flag and calling page_put since the abort case will now
> not be holding a reference to a page.
>
> Signed-off-by: Alexander Duyck
After my false sharing remove on pageblock_flags, this patch looks fine with
a minor change
> ---
> mm/compaction.c | 50 +++---
> 1 file changed, 7 insertions(+), 43 deletions(-)
>
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 5021a18ef722..c1e9918f9dd4 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -399,29 +399,6 @@ void reset_isolation_suitable(pg_data_t *pgdat)
> }
> }
>
> -/*
> - * Sets the pageblock skip bit if it was clear. Note that this is a hint as
> - * locks are not required for read/writers. Returns true if it was already
> set.
> - */
> -static bool test_and_set_skip(struct compact_control *cc, struct page *page,
> - unsigned long pfn)
> -{
> - bool skip;
> -
> - /* Do no update if skip hint is being ignored */
> - if (cc->ignore_skip_hint)
> - return false;
> -
> - if (!IS_ALIGNED(pfn, pageblock_nr_pages))
> - return false;
> -
> - skip = get_pageblock_skip(page);
> - if (!skip && !cc->no_set_skip_hint)
> - skip = !set_pageblock_skip(page);
> -
> - return skip;
> -}
> -
> static void update_cached_migrate(struct compact_control *cc, unsigned long
> pfn)
> {
> struct zone *zone = cc->zone;
> @@ -480,12 +457,6 @@ static inline void update_pageblock_skip(struct
> compact_control *cc,
> static void update_cached_migrate(struct compact_control *cc, unsigned long
> pfn)
> {
> }
> -
> -static bool test_and_set_skip(struct compact_control *cc, struct page *page,
> - unsigned long pfn)
> -{
> - return false;
> -}
> #endif /* CONFIG_COMPACTION */
>
> /*
> @@ -895,7 +866,6 @@ static bool too_many_isolated(pg_data_t *pgdat)
> if (!valid_page && IS_ALIGNED(low_pfn, pageblock_nr_pages)) {
> if (!cc->ignore_skip_hint && get_pageblock_skip(page)) {
> low_pfn = end_pfn;
> - page = NULL;
> goto isolate_abort;
> }
> valid_page = page;
> @@ -991,6 +961,13 @@ static bool too_many_isolated(pg_data_t *pgdat)
> if (!TestClearPageLRU(page))
> goto isolate_fail_put;
>
> + /* Indicate that we want exclusive access to this pageblock */
> + if (page == valid_page) {
> + skip_updated = true;
> + if (!cc->ignore_skip_hint)
if (!cc->ignore_skip_hint && !cc->no_set_skip_hint)
no_set_skip_hint needs to add here.
Thanks
Alex
> + set_pageblock_skip(page);
> + }
> +
> /* If we already hold the lock, we can skip some rechecking */
> if (!lruvec || !lruvec_holds_page_lru_lock(page, lruvec)) {
> if (lruvec)
> @@ -1002,13 +979,6 @@ static bool too_many_isolated(pg_data_t *pgdat)
>
> lruvec_memcg_debug(lruvec, page);
>
> - /* Try get exclusive access under lock */
> - if (!skip_updated) {
> - skip_updated = true;
> - if (test_and_set_skip(cc, page, low_pfn))
> - goto isolate_abort;
> - }
> -
> /*
>* Page become compound since the non-locked check,
>* and it's on LRU. It can only be a THP so the order
> @@ -1094,15 +1064,9 @@ static bool too_many_isolated(pg_data_t *pgdat)
> if (unlikely(low_pfn > end_pfn))
> low_pfn = end_pfn;
>
> - page = NULL;
> -
> isolate_abort:
> if (lruvec)
> unlock_page_lruvec_irqrestore(lruv
Re: [RFC PATCH 2/3] mm: Drop use of test_and_set_skip in favor of just setting skip
On Sat, Aug 15, 2020 at 2:51 AM Alex Shi wrote: > > > > 在 2020/8/15 上午5:15, Alexander Duyck 写道: > > On Fri, Aug 14, 2020 at 7:24 AM Alexander Duyck > > wrote: > >> > >> On Fri, Aug 14, 2020 at 12:19 AM Alex Shi > >> wrote: > >>> > >>> > >>> > >>> 在 2020/8/13 下午12:02, Alexander Duyck 写道: > > Since we have dropped the late abort case we can drop the code that was > clearing the LRU flag and calling page_put since the abort case will now > not be holding a reference to a page. > > Signed-off-by: Alexander Duyck > >>> > >>> seems the case-lru-file-mmap-read case drop about 3% on this patch in a > >>> rough testing. > >>> on my 80 core machine. > >> > >> I'm not sure how it could have that much impact on the performance > >> since the total effect would just be dropping what should be a > >> redundant test since we tested the skip bit before we took the LRU > >> bit, so we shouldn't need to test it again after. > >> > >> I finally got my test setup working last night. I'll have to do some > >> testing in my environment and I can start trying to see what is going > >> on. > > > > So I ran the case-lru-file-mmap-read a few times and I don't see how > > it is supposed to be testing the compaction code. It doesn't seem like > > compaction is running at least on my system as a result of the test > > script. > > atteched my kernel config, it is used on mine machine, I'm just wondering what the margin of error is on the tests you are running. What is the variance between runs? I'm just wondering if 3% falls into the range of noise or possible changes due to just code shifting around? In order for the code to have shown any change it needs to be run and I didn't see the tests triggering compaction on my test system. I'm wondering how much memory you have available in the system you were testing on that the test was enough to trigger compaction? > > I wonder if testing this code wouldn't be better done using > > something like thpscale from the > > mmtests(https://github.com/gormanm/mmtests)? It seems past changes to > > the compaction code were tested using that, and the config script for > > the test explains that it is designed specifically to stress the > > compaction code. I have the test up and running now and hope to > > collect results over the weekend. > > I did the testing, but a awkward is that I failed to get result, > maybe leak some packages. So one thing I noticed is that if you have over 128GB of memory in the system it will fail unless you update the sysctl value vm.max_map_count. It defaulted to somewhere close to 64K, and I increased it 20X to 1280K in order for the test to run without failing on the mmap calls. The other edit I had to make was the config file as the test system I was on had about 1TB of RAM, and my home partition only had about 800GB to spare so I had to reduce the map size from 8/10 to 5/8. > # ../../compare-kernels.sh > > thpscale Fault Latencies > Can't locate List/BinarySearch.pm in @INC (@INC contains: > /root/mmtests/bin/lib /usr/local/lib64/perl5 /usr/local/share/perl5 > /usr/lib64/perl5/vendor_perl /usr/share/perl5/vend. > BEGIN failed--compilation aborted at /root/mmtests/bin/lib/MMTests/Stat.pm > line 13. > Compilation failed in require at > /root/mmtests/work/log/../../bin/compare-mmtests.pl line 13. > BEGIN failed--compilation aborted at > /root/mmtests/work/log/../../bin/compare-mmtests.pl line 13. I had to install List::BinarySearch.pm. It required installing the cpan perl libraries. > > > > There is one change I will probably make to this patch and that is to > > place the new code that is setting skip_updated where the old code was > > calling test_and_set_skip_bit. By doing that we can avoid extra checks > > and it should help to reduce possible collisions when setting the skip > > bit in the pageblock flags. > > the problem maybe on cmpchxg pb flags, that may involved other blocks changes. That is the only thing I can think of just based on code review. Although that would imply multiple compact threads are running, and as I said in my tests I never saw kcompactd wakeup so I don't think the tests you were mentioning were enough to stress compaction.
Re: [RFC PATCH 2/3] mm: Drop use of test_and_set_skip in favor of just setting skip
On Fri, Aug 14, 2020 at 7:24 AM Alexander Duyck wrote: > > On Fri, Aug 14, 2020 at 12:19 AM Alex Shi wrote: > > > > > > > > 在 2020/8/13 下午12:02, Alexander Duyck 写道: > > > > > > Since we have dropped the late abort case we can drop the code that was > > > clearing the LRU flag and calling page_put since the abort case will now > > > not be holding a reference to a page. > > > > > > Signed-off-by: Alexander Duyck > > > > seems the case-lru-file-mmap-read case drop about 3% on this patch in a > > rough testing. > > on my 80 core machine. > > I'm not sure how it could have that much impact on the performance > since the total effect would just be dropping what should be a > redundant test since we tested the skip bit before we took the LRU > bit, so we shouldn't need to test it again after. > > I finally got my test setup working last night. I'll have to do some > testing in my environment and I can start trying to see what is going > on. So I ran the case-lru-file-mmap-read a few times and I don't see how it is supposed to be testing the compaction code. It doesn't seem like compaction is running at least on my system as a result of the test script. I wonder if testing this code wouldn't be better done using something like thpscale from the mmtests(https://github.com/gormanm/mmtests)? It seems past changes to the compaction code were tested using that, and the config script for the test explains that it is designed specifically to stress the compaction code. I have the test up and running now and hope to collect results over the weekend. There is one change I will probably make to this patch and that is to place the new code that is setting skip_updated where the old code was calling test_and_set_skip_bit. By doing that we can avoid extra checks and it should help to reduce possible collisions when setting the skip bit in the pageblock flags. Thanks. - Alex
Re: [RFC PATCH 2/3] mm: Drop use of test_and_set_skip in favor of just setting skip
On Fri, Aug 14, 2020 at 12:19 AM Alex Shi wrote: > > > > 在 2020/8/13 下午12:02, Alexander Duyck 写道: > > > > Since we have dropped the late abort case we can drop the code that was > > clearing the LRU flag and calling page_put since the abort case will now > > not be holding a reference to a page. > > > > Signed-off-by: Alexander Duyck > > seems the case-lru-file-mmap-read case drop about 3% on this patch in a rough > testing. > on my 80 core machine. I'm not sure how it could have that much impact on the performance since the total effect would just be dropping what should be a redundant test since we tested the skip bit before we took the LRU bit, so we shouldn't need to test it again after. I finally got my test setup working last night. I'll have to do some testing in my environment and I can start trying to see what is going on. Thanks. - Alex
Re: [RFC PATCH 2/3] mm: Drop use of test_and_set_skip in favor of just setting skip
在 2020/8/13 下午12:02, Alexander Duyck 写道: > > Since we have dropped the late abort case we can drop the code that was > clearing the LRU flag and calling page_put since the abort case will now > not be holding a reference to a page. > > Signed-off-by: Alexander Duyck seems the case-lru-file-mmap-read case drop about 3% on this patch in a rough testing. on my 80 core machine. Thanks Alex
