| Message ID | 20231201145936.5ddfdb50@gandalf.local.home (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/rmap: Fix misplaced parenthesis of a likely() | expand |
On 01.12.23 20:59, Steven Rostedt wrote: > From: Steven Rostedt (Google) <rostedt@goodmis.org> > > Running my yearly branch profiler to see where likely/unlikely annotation > may be added or removed, I discovered this: > > correct incorrect % Function File Line > ------- --------- - -------- ---- ---- > 0 457918 100 page_try_dup_anon_rmap rmap.h 264 > [..] > 458021 0 0 page_try_dup_anon_rmap rmap.h 265 > That looks like a handy tool! > I thought it was interesting that line 264 of rmap.h had a 100% incorrect > annotation, but the line directly below it was 100% correct. Looking at the > code: > > if (likely(!is_device_private_page(page) && > unlikely(page_needs_cow_for_dma(vma, page)))) > > It didn't make sense. The "likely()" was around the entire if statement > (not just the "!is_device_private_page(page)"), which also included the > "unlikely()" portion of that if condition. Yes, that was clearly misplaced. > > If the unlikely portion is unlikely to be true, that would make the entire > if condition unlikely to be true, so it made no sense at all to say the > entire if condition is true. > > What is more likely to be likely is just the first part of the if statement > before the && operation. It's likely to be a misplaced parenthesis. And > after making the if condition broken into a likely() && unlikely(), both > now appear to be correct! > Reviewed-by: David Hildenbrand <david@redhat.com> But > Cc: stable@vger.kernel.org stable, really? Why? > Fixes:fb3d824d1a46c ("mm/rmap: split page_dup_rmap() into page_dup_file_rmap() and page_try_dup_anon_rmap()") and does it even fix a real bug?
On Fri, 1 Dec 2023 21:06:22 +0100 David Hildenbrand <david@redhat.com> wrote: > But > > > Cc: stable@vger.kernel.org > > stable, really? Why? > > > Fixes:fb3d824d1a46c ("mm/rmap: split page_dup_rmap() into page_dup_file_rmap() and page_try_dup_anon_rmap()") > > and does it even fix a real bug? As a performance person, who measures likely and unlikely results (the ftrace ring buffer was sped up by over 50% with strategically placed likely/unlikely annotation). I find this to be a real bug, and something I would want backported to the kernels we maintain in ChromeOS (which uses upstream stable kernels). -- Steve
On 01.12.23 21:15, Steven Rostedt wrote: > On Fri, 1 Dec 2023 21:06:22 +0100 > David Hildenbrand <david@redhat.com> wrote: > >> But >> >>> Cc: stable@vger.kernel.org >> >> stable, really? Why? >> >>> Fixes:fb3d824d1a46c ("mm/rmap: split page_dup_rmap() into page_dup_file_rmap() and page_try_dup_anon_rmap()") >> >> and does it even fix a real bug? > > As a performance person, who measures likely and unlikely results (the > ftrace ring buffer was sped up by over 50% with strategically placed > likely/unlikely annotation). I find this to be a real bug, and something I > would want backported to the kernels we maintain in ChromeOS (which uses > upstream stable kernels). Okay, I don't care that much about a "Fixes" annotation. But I am *pretty* sure that this is not stable material, looking once again at the stable rules. Anyhow, thanks for catching this!
On 12/1/23 20:59, Steven Rostedt wrote: > From: Steven Rostedt (Google) <rostedt@goodmis.org> > > Running my yearly branch profiler to see where likely/unlikely annotation > may be added or removed, I discovered this: > > correct incorrect % Function File Line > ------- --------- - -------- ---- ---- > 0 457918 100 page_try_dup_anon_rmap rmap.h 264 > [..] > 458021 0 0 page_try_dup_anon_rmap rmap.h 265 > > I thought it was interesting that line 264 of rmap.h had a 100% incorrect > annotation, but the line directly below it was 100% correct. Looking at the > code: > > if (likely(!is_device_private_page(page) && > unlikely(page_needs_cow_for_dma(vma, page)))) > > It didn't make sense. The "likely()" was around the entire if statement > (not just the "!is_device_private_page(page)"), which also included the > "unlikely()" portion of that if condition. > > If the unlikely portion is unlikely to be true, that would make the entire > if condition unlikely to be true, so it made no sense at all to say the > entire if condition is true. > > What is more likely to be likely is just the first part of the if statement > before the && operation. It's likely to be a misplaced parenthesis. And > after making the if condition broken into a likely() && unlikely(), both > now appear to be correct! > > Cc: stable@vger.kernel.org > Fixes:fb3d824d1a46c ("mm/rmap: split page_dup_rmap() into page_dup_file_rmap() and page_try_dup_anon_rmap()") > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> Acked-by: Vlastimil Babka <vbabka@suse.cz> Pragmatically speaking, stable maintainers haven't been following the stable rules for a long time, and a commit with Fixes and without Cc: stable is often backported on the assumption people forget Cc: stable, and "Fixes:" implies there's a bug to fix, and it's good to have bugs fixed in stable... We have (repeatedly...) had mm extempted from this and Cc: stable is required, which is good. So if Steven thinks there are reasons to backport, then I'd rather let him keep the Cc: stable, instead of this later becoming an argument to question the mm extemption again :) > --- > diff --git a/include/linux/rmap.h b/include/linux/rmap.h > index b26fe858fd44..3c2fc291b071 100644 > --- a/include/linux/rmap.h > +++ b/include/linux/rmap.h > @@ -261,8 +261,8 @@ static inline int page_try_dup_anon_rmap(struct page *page, bool compound, > * guarantee the pinned page won't be randomly replaced in the > * future on write faults. > */ > - if (likely(!is_device_private_page(page) && > - unlikely(page_needs_cow_for_dma(vma, page)))) > + if (likely(!is_device_private_page(page)) && > + unlikely(page_needs_cow_for_dma(vma, page))) > return -EBUSY; > > ClearPageAnonExclusive(page);
diff --git a/include/linux/rmap.h b/include/linux/rmap.h index b26fe858fd44..3c2fc291b071 100644 --- a/include/linux/rmap.h +++ b/include/linux/rmap.h @@ -261,8 +261,8 @@ static inline int page_try_dup_anon_rmap(struct page *page, bool compound, * guarantee the pinned page won't be randomly replaced in the * future on write faults. */ - if (likely(!is_device_private_page(page) && - unlikely(page_needs_cow_for_dma(vma, page)))) + if (likely(!is_device_private_page(page)) && + unlikely(page_needs_cow_for_dma(vma, page))) return -EBUSY; ClearPageAnonExclusive(page);