Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1360619 > unrolled thread

Re: Suspicious error for CMA stress test

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: Suspicious error for CMA stress test Lucas Stach <l.stach@pengutronix.de> - 2016-03-18 14:40 +0100
    Re: Suspicious error for CMA stress test Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-03-21 05:50 +0100
      Re: Suspicious error for CMA stress test Lucas Stach <l.stach@pengutronix.de> - 2016-03-22 16:00 +0100
        Re: Suspicious error for CMA stress test Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-03-23 05:50 +0100

#1360619 — Re: Suspicious error for CMA stress test

Hi Vlastimil, Joonsoo,

Am Freitag, den 18.03.2016, 00:52 +0900 schrieb Joonsoo Kim:
> 2016-03-18 0:43 GMT+09:00 Vlastimil Babka <vbabka@suse.cz>:
> > On 03/17/2016 10:24 AM, Hanjun Guo wrote:
> >>
> >> On 2016/3/17 14:54, Joonsoo Kim wrote:
> >>>
> >>> On Wed, Mar 16, 2016 at 05:44:28PM +0800, Hanjun Guo wrote:
> >>>>
> >>>> On 2016/3/14 15:18, Joonsoo Kim wrote:
> >>>>>
> >>>>> On Mon, Mar 14, 2016 at 08:06:16AM +0100, Vlastimil Babka wrote:
> >>>>>>
> >>>>>> On 03/14/2016 07:49 AM, Joonsoo Kim wrote:
> >>>>>>>
> >>>>>>> On Fri, Mar 11, 2016 at 06:07:40PM +0100, Vlastimil Babka wrote:
> >>>>>>>>
> >>>>>>>> On 03/11/2016 04:00 PM, Joonsoo Kim wrote:
> >>>>>>>>
> >>>>>>>> How about something like this? Just and idea, probably buggy
> >>>>>>>> (off-by-one etc.).
> >>>>>>>> Should keep away cost from <pageblock_order iterations at the
> >>>>>>>> expense of the
> >>>>>>>> relatively fewer >pageblock_order iterations.
> >>>>>>>
> >>>>>>> Hmm... I tested this and found that it's code size is a little bit
> >>>>>>> larger than mine. I'm not sure why this happens exactly but I guess
> >>>>>>> it would be
> >>>>>>> related to compiler optimization. In this case, I'm in favor of my
> >>>>>>> implementation because it looks like well abstraction. It adds one
> >>>>>>> unlikely branch to the merge loop but compiler would optimize it to
> >>>>>>> check it once.
> >>>>>>
> >>>>>> I would be surprised if compiler optimized that to check it once, as
> >>>>>> order increases with each loop iteration. But maybe it's smart
> >>>>>> enough to do something like I did by hand? Guess I'll check the
> >>>>>> disassembly.
> >>>>>
> >>>>> Okay. I used following slightly optimized version and I need to
> >>>>> add 'max_order = min_t(unsigned int, MAX_ORDER, pageblock_order + 1)'
> >>>>> to yours. Please consider it, too.
> >>>>
> >>>> Hmm, this one is not work, I still can see the bug is there after
> >>>> applying
> >>>> this patch, did I miss something?
> >>>
> >>> I may find that there is a bug which was introduced by me some time
> >>> ago. Could you test following change in __free_one_page() on top of
> >>> Vlastimil's patch?
> >>>
> >>> -page_idx = pfn & ((1 << max_order) - 1);
> >>> +page_idx = pfn & ((1 << MAX_ORDER) - 1);
> >>
> >>
> >> I tested Vlastimil's patch + your change with stress for more than half
> >> hour, the bug
> >> I reported is gone :)
> >
> >
> > Oh, ok, will try to send proper patch, once I figure out what to write in
> > the changelog :)
> 
> Thanks in advance!

After digging into the "PFN busy" race in CMA (see [1]), I believe we
should just prevent any buddy merging in isolated ranges. This fixes the
race I'm seeing without the need to hold the zone lock for extend
periods of time.
Also any merging done in an isolated range is likely to be completely
wasted work, as higher order buddy pages are broken up again into single
pages in isolate_freepages.

If we do that the patch to fix the bug in question for this report would
boil down to a check if the current pages buddy is isolated and abort
merging at that point, right? undo_isolate_page_range will then do all
necessary merging that has been skipped while the range was isolated.

Do you see issues with this approach?

Regards,
Lucas

[1] http://thread.gmane.org/gmane.linux.kernel.mm/148383

[toc] | [next] | [standalone]

#1361518

On Fri, Mar 18, 2016 at 02:32:35PM +0100, Lucas Stach wrote:
> Hi Vlastimil, Joonsoo,
> 
> Am Freitag, den 18.03.2016, 00:52 +0900 schrieb Joonsoo Kim:
> > 2016-03-18 0:43 GMT+09:00 Vlastimil Babka <vbabka@suse.cz>:
> > > On 03/17/2016 10:24 AM, Hanjun Guo wrote:
> > >>
> > >> On 2016/3/17 14:54, Joonsoo Kim wrote:
> > >>>
> > >>> On Wed, Mar 16, 2016 at 05:44:28PM +0800, Hanjun Guo wrote:
> > >>>>
> > >>>> On 2016/3/14 15:18, Joonsoo Kim wrote:
> > >>>>>
> > >>>>> On Mon, Mar 14, 2016 at 08:06:16AM +0100, Vlastimil Babka wrote:
> > >>>>>>
> > >>>>>> On 03/14/2016 07:49 AM, Joonsoo Kim wrote:
> > >>>>>>>
> > >>>>>>> On Fri, Mar 11, 2016 at 06:07:40PM +0100, Vlastimil Babka wrote:
> > >>>>>>>>
> > >>>>>>>> On 03/11/2016 04:00 PM, Joonsoo Kim wrote:
> > >>>>>>>>
> > >>>>>>>> How about something like this? Just and idea, probably buggy
> > >>>>>>>> (off-by-one etc.).
> > >>>>>>>> Should keep away cost from <pageblock_order iterations at the
> > >>>>>>>> expense of the
> > >>>>>>>> relatively fewer >pageblock_order iterations.
> > >>>>>>>
> > >>>>>>> Hmm... I tested this and found that it's code size is a little bit
> > >>>>>>> larger than mine. I'm not sure why this happens exactly but I guess
> > >>>>>>> it would be
> > >>>>>>> related to compiler optimization. In this case, I'm in favor of my
> > >>>>>>> implementation because it looks like well abstraction. It adds one
> > >>>>>>> unlikely branch to the merge loop but compiler would optimize it to
> > >>>>>>> check it once.
> > >>>>>>
> > >>>>>> I would be surprised if compiler optimized that to check it once, as
> > >>>>>> order increases with each loop iteration. But maybe it's smart
> > >>>>>> enough to do something like I did by hand? Guess I'll check the
> > >>>>>> disassembly.
> > >>>>>
> > >>>>> Okay. I used following slightly optimized version and I need to
> > >>>>> add 'max_order = min_t(unsigned int, MAX_ORDER, pageblock_order + 1)'
> > >>>>> to yours. Please consider it, too.
> > >>>>
> > >>>> Hmm, this one is not work, I still can see the bug is there after
> > >>>> applying
> > >>>> this patch, did I miss something?
> > >>>
> > >>> I may find that there is a bug which was introduced by me some time
> > >>> ago. Could you test following change in __free_one_page() on top of
> > >>> Vlastimil's patch?
> > >>>
> > >>> -page_idx = pfn & ((1 << max_order) - 1);
> > >>> +page_idx = pfn & ((1 << MAX_ORDER) - 1);
> > >>
> > >>
> > >> I tested Vlastimil's patch + your change with stress for more than half
> > >> hour, the bug
> > >> I reported is gone :)
> > >
> > >
> > > Oh, ok, will try to send proper patch, once I figure out what to write in
> > > the changelog :)
> > 
> > Thanks in advance!
> 
> After digging into the "PFN busy" race in CMA (see [1]), I believe we
> should just prevent any buddy merging in isolated ranges. This fixes the
> race I'm seeing without the need to hold the zone lock for extend
> periods of time.

"PFNs busy" can be caused by other type of race, too. I guess that
other cases happens more than buddy merging. Do you have any test case for
your problem?

If it is indeed a problem, you can avoid it with simple retry
MAX_ORDER times on alloc_contig_range(). This is a rather dirty but
the reason I suggest it is that there are other type of race in
__alloc_contig_range() and retry could help them, too. For example,
if some of pages in the requested range isn't attached to the LRU yet
or detached from the LRU but not freed to buddy,
test_pages_isolated() can be failed.

Thanks.

[toc] | [prev] | [next] | [standalone]

#1362800

Am Montag, den 21.03.2016, 13:42 +0900 schrieb Joonsoo Kim:
> On Fri, Mar 18, 2016 at 02:32:35PM +0100, Lucas Stach wrote:
> > Hi Vlastimil, Joonsoo,
> > 
> > Am Freitag, den 18.03.2016, 00:52 +0900 schrieb Joonsoo Kim:
> > > 2016-03-18 0:43 GMT+09:00 Vlastimil Babka <vbabka@suse.cz>:
> > > > On 03/17/2016 10:24 AM, Hanjun Guo wrote:
> > > >>
> > > >> On 2016/3/17 14:54, Joonsoo Kim wrote:
> > > >>>
> > > >>> On Wed, Mar 16, 2016 at 05:44:28PM +0800, Hanjun Guo wrote:
> > > >>>>
> > > >>>> On 2016/3/14 15:18, Joonsoo Kim wrote:
> > > >>>>>
> > > >>>>> On Mon, Mar 14, 2016 at 08:06:16AM +0100, Vlastimil Babka wrote:
> > > >>>>>>
> > > >>>>>> On 03/14/2016 07:49 AM, Joonsoo Kim wrote:
> > > >>>>>>>
> > > >>>>>>> On Fri, Mar 11, 2016 at 06:07:40PM +0100, Vlastimil Babka wrote:
> > > >>>>>>>>
> > > >>>>>>>> On 03/11/2016 04:00 PM, Joonsoo Kim wrote:
> > > >>>>>>>>
> > > >>>>>>>> How about something like this? Just and idea, probably buggy
> > > >>>>>>>> (off-by-one etc.).
> > > >>>>>>>> Should keep away cost from <pageblock_order iterations at the
> > > >>>>>>>> expense of the
> > > >>>>>>>> relatively fewer >pageblock_order iterations.
> > > >>>>>>>
> > > >>>>>>> Hmm... I tested this and found that it's code size is a little bit
> > > >>>>>>> larger than mine. I'm not sure why this happens exactly but I guess
> > > >>>>>>> it would be
> > > >>>>>>> related to compiler optimization. In this case, I'm in favor of my
> > > >>>>>>> implementation because it looks like well abstraction. It adds one
> > > >>>>>>> unlikely branch to the merge loop but compiler would optimize it to
> > > >>>>>>> check it once.
> > > >>>>>>
> > > >>>>>> I would be surprised if compiler optimized that to check it once, as
> > > >>>>>> order increases with each loop iteration. But maybe it's smart
> > > >>>>>> enough to do something like I did by hand? Guess I'll check the
> > > >>>>>> disassembly.
> > > >>>>>
> > > >>>>> Okay. I used following slightly optimized version and I need to
> > > >>>>> add 'max_order = min_t(unsigned int, MAX_ORDER, pageblock_order + 1)'
> > > >>>>> to yours. Please consider it, too.
> > > >>>>
> > > >>>> Hmm, this one is not work, I still can see the bug is there after
> > > >>>> applying
> > > >>>> this patch, did I miss something?
> > > >>>
> > > >>> I may find that there is a bug which was introduced by me some time
> > > >>> ago. Could you test following change in __free_one_page() on top of
> > > >>> Vlastimil's patch?
> > > >>>
> > > >>> -page_idx = pfn & ((1 << max_order) - 1);
> > > >>> +page_idx = pfn & ((1 << MAX_ORDER) - 1);
> > > >>
> > > >>
> > > >> I tested Vlastimil's patch + your change with stress for more than half
> > > >> hour, the bug
> > > >> I reported is gone :)
> > > >
> > > >
> > > > Oh, ok, will try to send proper patch, once I figure out what to write in
> > > > the changelog :)
> > > 
> > > Thanks in advance!
> > 
> > After digging into the "PFN busy" race in CMA (see [1]), I believe we
> > should just prevent any buddy merging in isolated ranges. This fixes the
> > race I'm seeing without the need to hold the zone lock for extend
> > periods of time.
> 
> "PFNs busy" can be caused by other type of race, too. I guess that
> other cases happens more than buddy merging. Do you have any test case for
> your problem?
> 
I don't have any specific test case, but the etnaviv driver manages to
hit this race quite often. That's because we allocate/free a large
number of relatively small buffer from CMA, where allocation and free
regularly happen on different CPUs.

So while we also have cases where the "PFN busy" is triggered by other
factors, like pages locked for get_user_pages(), this race is the number
one source of CMA retries in my workload.

> If it is indeed a problem, you can avoid it with simple retry
> MAX_ORDER times on alloc_contig_range(). This is a rather dirty but
> the reason I suggest it is that there are other type of race in
> __alloc_contig_range() and retry could help them, too. For example,
> if some of pages in the requested range isn't attached to the LRU yet
> or detached from the LRU but not freed to buddy,
> test_pages_isolated() can be failed.

While a retry makes sense (if at all just to avoid a CMA allocation
failure under CMA pressure), I would like to avoid the associated
overhead for the common path where CMA is just racing with itself. The
retry should only be needed in situations where we don't have any means
to control the race, like a concurrent GUP.

Regards,
Lucas

[toc] | [prev] | [next] | [standalone]

#1363198

On Tue, Mar 22, 2016 at 03:56:46PM +0100, Lucas Stach wrote:
> Am Montag, den 21.03.2016, 13:42 +0900 schrieb Joonsoo Kim:
> > On Fri, Mar 18, 2016 at 02:32:35PM +0100, Lucas Stach wrote:
> > > Hi Vlastimil, Joonsoo,
> > > 
> > > Am Freitag, den 18.03.2016, 00:52 +0900 schrieb Joonsoo Kim:
> > > > 2016-03-18 0:43 GMT+09:00 Vlastimil Babka <vbabka@suse.cz>:
> > > > > On 03/17/2016 10:24 AM, Hanjun Guo wrote:
> > > > >>
> > > > >> On 2016/3/17 14:54, Joonsoo Kim wrote:
> > > > >>>
> > > > >>> On Wed, Mar 16, 2016 at 05:44:28PM +0800, Hanjun Guo wrote:
> > > > >>>>
> > > > >>>> On 2016/3/14 15:18, Joonsoo Kim wrote:
> > > > >>>>>
> > > > >>>>> On Mon, Mar 14, 2016 at 08:06:16AM +0100, Vlastimil Babka wrote:
> > > > >>>>>>
> > > > >>>>>> On 03/14/2016 07:49 AM, Joonsoo Kim wrote:
> > > > >>>>>>>
> > > > >>>>>>> On Fri, Mar 11, 2016 at 06:07:40PM +0100, Vlastimil Babka wrote:
> > > > >>>>>>>>
> > > > >>>>>>>> On 03/11/2016 04:00 PM, Joonsoo Kim wrote:
> > > > >>>>>>>>
> > > > >>>>>>>> How about something like this? Just and idea, probably buggy
> > > > >>>>>>>> (off-by-one etc.).
> > > > >>>>>>>> Should keep away cost from <pageblock_order iterations at the
> > > > >>>>>>>> expense of the
> > > > >>>>>>>> relatively fewer >pageblock_order iterations.
> > > > >>>>>>>
> > > > >>>>>>> Hmm... I tested this and found that it's code size is a little bit
> > > > >>>>>>> larger than mine. I'm not sure why this happens exactly but I guess
> > > > >>>>>>> it would be
> > > > >>>>>>> related to compiler optimization. In this case, I'm in favor of my
> > > > >>>>>>> implementation because it looks like well abstraction. It adds one
> > > > >>>>>>> unlikely branch to the merge loop but compiler would optimize it to
> > > > >>>>>>> check it once.
> > > > >>>>>>
> > > > >>>>>> I would be surprised if compiler optimized that to check it once, as
> > > > >>>>>> order increases with each loop iteration. But maybe it's smart
> > > > >>>>>> enough to do something like I did by hand? Guess I'll check the
> > > > >>>>>> disassembly.
> > > > >>>>>
> > > > >>>>> Okay. I used following slightly optimized version and I need to
> > > > >>>>> add 'max_order = min_t(unsigned int, MAX_ORDER, pageblock_order + 1)'
> > > > >>>>> to yours. Please consider it, too.
> > > > >>>>
> > > > >>>> Hmm, this one is not work, I still can see the bug is there after
> > > > >>>> applying
> > > > >>>> this patch, did I miss something?
> > > > >>>
> > > > >>> I may find that there is a bug which was introduced by me some time
> > > > >>> ago. Could you test following change in __free_one_page() on top of
> > > > >>> Vlastimil's patch?
> > > > >>>
> > > > >>> -page_idx = pfn & ((1 << max_order) - 1);
> > > > >>> +page_idx = pfn & ((1 << MAX_ORDER) - 1);
> > > > >>
> > > > >>
> > > > >> I tested Vlastimil's patch + your change with stress for more than half
> > > > >> hour, the bug
> > > > >> I reported is gone :)
> > > > >
> > > > >
> > > > > Oh, ok, will try to send proper patch, once I figure out what to write in
> > > > > the changelog :)
> > > > 
> > > > Thanks in advance!
> > > 
> > > After digging into the "PFN busy" race in CMA (see [1]), I believe we
> > > should just prevent any buddy merging in isolated ranges. This fixes the
> > > race I'm seeing without the need to hold the zone lock for extend
> > > periods of time.
> > 
> > "PFNs busy" can be caused by other type of race, too. I guess that
> > other cases happens more than buddy merging. Do you have any test case for
> > your problem?
> > 
> I don't have any specific test case, but the etnaviv driver manages to
> hit this race quite often. That's because we allocate/free a large
> number of relatively small buffer from CMA, where allocation and free
> regularly happen on different CPUs.
> 
> So while we also have cases where the "PFN busy" is triggered by other
> factors, like pages locked for get_user_pages(), this race is the number
> one source of CMA retries in my workload.
> 
> > If it is indeed a problem, you can avoid it with simple retry
> > MAX_ORDER times on alloc_contig_range(). This is a rather dirty but
> > the reason I suggest it is that there are other type of race in
> > __alloc_contig_range() and retry could help them, too. For example,
> > if some of pages in the requested range isn't attached to the LRU yet
> > or detached from the LRU but not freed to buddy,
> > test_pages_isolated() can be failed.
> 
> While a retry makes sense (if at all just to avoid a CMA allocation
> failure under CMA pressure), I would like to avoid the associated
> overhead for the common path where CMA is just racing with itself. The
> retry should only be needed in situations where we don't have any means
> to control the race, like a concurrent GUP.

Make sense. When I tried to fix merging issue previously, I worried
about side-effect of unmerged buddy so I tried to reduce unmerged
buddy as much as possible. But, your problem can be solved by
implementation that makes unmerged buddy in isolated pageblock and
it is a real problem. I think that now we can bear uncertain side-effect
of unmerged buddy for solving real problem. Please make a patch
and send to the list.

Thanks.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web