* [PATCH] mm: page_alloc: remove branch operation in free_pages_prepare() @ 2013-03-07 8:21 Joonsoo Kim 2013-03-07 18:54 ` Hugh Dickins 0 siblings, 1 reply; 6+ messages in thread From: Joonsoo Kim @ 2013-03-07 8:21 UTC (permalink / raw) To: Andrew Morton; +Cc: linux-kernel, linux-mm, Joonsoo Kim, Hugh Dickins When we found that the flag has a bit of PAGE_FLAGS_CHECK_AT_PREP, we reset the flag. If we always reset the flag, we can reduce one branch operation. So remove it. Cc: Hugh Dickins <hughd@google.com> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com> diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 8fcced7..778f2a9 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -614,8 +614,7 @@ static inline int free_pages_check(struct page *page) return 1; } page_nid_reset_last(page); - if (page->flags & PAGE_FLAGS_CHECK_AT_PREP) - page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP; + page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP; return 0; } -- 1.7.9.5 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mm: page_alloc: remove branch operation in free_pages_prepare() 2013-03-07 8:21 [PATCH] mm: page_alloc: remove branch operation in free_pages_prepare() Joonsoo Kim @ 2013-03-07 18:54 ` Hugh Dickins 2013-03-08 0:45 ` Joonsoo Kim 0 siblings, 1 reply; 6+ messages in thread From: Hugh Dickins @ 2013-03-07 18:54 UTC (permalink / raw) To: Joonsoo Kim; +Cc: Andrew Morton, linux-kernel, linux-mm On Thu, 7 Mar 2013, Joonsoo Kim wrote: > When we found that the flag has a bit of PAGE_FLAGS_CHECK_AT_PREP, > we reset the flag. If we always reset the flag, we can reduce one > branch operation. So remove it. > > Cc: Hugh Dickins <hughd@google.com> > Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com> I don't object to this patch. But certainly I would have written it that way in order not to dirty a cacheline unnecessarily. It may be obvious to you that the cacheline in question is almost always already dirty, and the branch almost always more expensive. But I'll leave that to you, and to those who know more about these subtle costs than I do. Hugh > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 8fcced7..778f2a9 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -614,8 +614,7 @@ static inline int free_pages_check(struct page *page) > return 1; > } > page_nid_reset_last(page); > - if (page->flags & PAGE_FLAGS_CHECK_AT_PREP) > - page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP; > + page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP; > return 0; > } > -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm: page_alloc: remove branch operation in free_pages_prepare() 2013-03-07 18:54 ` Hugh Dickins @ 2013-03-08 0:45 ` Joonsoo Kim 2013-03-08 2:01 ` Hugh Dickins 0 siblings, 1 reply; 6+ messages in thread From: Joonsoo Kim @ 2013-03-08 0:45 UTC (permalink / raw) To: Hugh Dickins; +Cc: Andrew Morton, linux-kernel, linux-mm Hello, Hugh. On Thu, Mar 07, 2013 at 10:54:15AM -0800, Hugh Dickins wrote: > On Thu, 7 Mar 2013, Joonsoo Kim wrote: > > > When we found that the flag has a bit of PAGE_FLAGS_CHECK_AT_PREP, > > we reset the flag. If we always reset the flag, we can reduce one > > branch operation. So remove it. > > > > Cc: Hugh Dickins <hughd@google.com> > > Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com> > > I don't object to this patch. But certainly I would have written it > that way in order not to dirty a cacheline unnecessarily. It may be > obvious to you that the cacheline in question is almost always already > dirty, and the branch almost always more expensive. But I'll leave that > to you, and to those who know more about these subtle costs than I do. Yes. I already think about that. I thought that even if a cacheline is not dirty at this time, we always touch the 'struct page' in set_freepage_migratetype() a little later, so dirtying is not the problem. But, now, I re-think this and decide to drop this patch. The reason is that 'struct page' of 'compound pages' may not be dirty at this time and will not be dirty at later time. So this patch is bad idea. Is there any comments? Thanks. > Hugh > > > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > index 8fcced7..778f2a9 100644 > > --- a/mm/page_alloc.c > > +++ b/mm/page_alloc.c > > @@ -614,8 +614,7 @@ static inline int free_pages_check(struct page *page) > > return 1; > > } > > page_nid_reset_last(page); > > - if (page->flags & PAGE_FLAGS_CHECK_AT_PREP) > > - page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP; > > + page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP; > > return 0; > > } > > > > -- > To unsubscribe, send a message with 'unsubscribe linux-mm' in > the body to majordomo@kvack.org. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm: page_alloc: remove branch operation in free_pages_prepare() 2013-03-08 0:45 ` Joonsoo Kim @ 2013-03-08 2:01 ` Hugh Dickins 2013-03-09 3:54 ` Will Huck 2013-03-13 8:08 ` Joonsoo Kim 0 siblings, 2 replies; 6+ messages in thread From: Hugh Dickins @ 2013-03-08 2:01 UTC (permalink / raw) To: Joonsoo Kim; +Cc: Andrew Morton, linux-kernel, linux-mm On Fri, 8 Mar 2013, Joonsoo Kim wrote: > On Thu, Mar 07, 2013 at 10:54:15AM -0800, Hugh Dickins wrote: > > On Thu, 7 Mar 2013, Joonsoo Kim wrote: > > > > > When we found that the flag has a bit of PAGE_FLAGS_CHECK_AT_PREP, > > > we reset the flag. If we always reset the flag, we can reduce one > > > branch operation. So remove it. > > > > > > Cc: Hugh Dickins <hughd@google.com> > > > Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com> > > > > I don't object to this patch. But certainly I would have written it > > that way in order not to dirty a cacheline unnecessarily. It may be > > obvious to you that the cacheline in question is almost always already > > dirty, and the branch almost always more expensive. But I'll leave that > > to you, and to those who know more about these subtle costs than I do. > > Yes. I already think about that. I thought that even if a cacheline is > not dirty at this time, we always touch the 'struct page' in > set_freepage_migratetype() a little later, so dirtying is not the problem. I expect that a very high proportion of user pages have PG_uptodate to be cleared here; and there's also the recently added page_nid_reset_last(), which will dirty the flags or a nearby field when CONFIG_NUMA_BALANCING. Those argue in favour of your patch. > > But, now, I re-think this and decide to drop this patch. > The reason is that 'struct page' of 'compound pages' may not be dirty > at this time and will not be dirty at later time. Actual compound pages would have PG_head or PG_tail or PG_compound to be cleared there, I believe (check if I'm right on that). The questionable case is the ordinary order>0 case without __GFP_COMP (and page_nid_reset_last() is applied to each subpage of those). > So this patch is bad idea. I'm not so sure. I doubt your patch will make a giant improvement in kernel performance! But it might make a little - maybe you just need to give some numbers from perf to justify it (but I'm easily dazzled by numbers - don't expect me to judge the result). Hugh > > Is there any comments? > > Thanks. > > > Hugh > > > > > > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > > index 8fcced7..778f2a9 100644 > > > --- a/mm/page_alloc.c > > > +++ b/mm/page_alloc.c > > > @@ -614,8 +614,7 @@ static inline int free_pages_check(struct page *page) > > > return 1; > > > } > > > page_nid_reset_last(page); > > > - if (page->flags & PAGE_FLAGS_CHECK_AT_PREP) > > > - page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP; > > > + page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP; > > > return 0; > > > } > > > > > > > -- > > To unsubscribe, send a message with 'unsubscribe linux-mm' in > > the body to majordomo@kvack.org. For more info on Linux MM, > > see: http://www.linux-mm.org/ . > > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> > -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm: page_alloc: remove branch operation in free_pages_prepare() 2013-03-08 2:01 ` Hugh Dickins @ 2013-03-09 3:54 ` Will Huck 2013-03-13 8:08 ` Joonsoo Kim 1 sibling, 0 replies; 6+ messages in thread From: Will Huck @ 2013-03-09 3:54 UTC (permalink / raw) To: Hugh Dickins; +Cc: Joonsoo Kim, Andrew Morton, linux-kernel, linux-mm Hi Hugh, On 03/08/2013 10:01 AM, Hugh Dickins wrote: > On Fri, 8 Mar 2013, Joonsoo Kim wrote: >> On Thu, Mar 07, 2013 at 10:54:15AM -0800, Hugh Dickins wrote: >>> On Thu, 7 Mar 2013, Joonsoo Kim wrote: >>> >>>> When we found that the flag has a bit of PAGE_FLAGS_CHECK_AT_PREP, >>>> we reset the flag. If we always reset the flag, we can reduce one >>>> branch operation. So remove it. >>>> >>>> Cc: Hugh Dickins <hughd@google.com> >>>> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com> >>> I don't object to this patch. But certainly I would have written it >>> that way in order not to dirty a cacheline unnecessarily. It may be >>> obvious to you that the cacheline in question is almost always already >>> dirty, and the branch almost always more expensive. But I'll leave that >>> to you, and to those who know more about these subtle costs than I do. >> Yes. I already think about that. I thought that even if a cacheline is >> not dirty at this time, we always touch the 'struct page' in >> set_freepage_migratetype() a little later, so dirtying is not the problem. > I expect that a very high proportion of user pages have > PG_uptodate to be cleared here; and there's also the recently added When PG_uptodate will be set? > page_nid_reset_last(), which will dirty the flags or a nearby field > when CONFIG_NUMA_BALANCING. Those argue in favour of your patch. > >> But, now, I re-think this and decide to drop this patch. >> The reason is that 'struct page' of 'compound pages' may not be dirty >> at this time and will not be dirty at later time. > Actual compound pages would have PG_head or PG_tail or PG_compound > to be cleared there, I believe (check if I'm right on that). The > questionable case is the ordinary order>0 case without __GFP_COMP > (and page_nid_reset_last() is applied to each subpage of those). > >> So this patch is bad idea. > I'm not so sure. I doubt your patch will make a giant improvement > in kernel performance! But it might make a little - maybe you just > need to give some numbers from perf to justify it (but I'm easily > dazzled by numbers - don't expect me to judge the result). > > Hugh > >> Is there any comments? >> >> Thanks. >> >>> Hugh >>> >>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c >>>> index 8fcced7..778f2a9 100644 >>>> --- a/mm/page_alloc.c >>>> +++ b/mm/page_alloc.c >>>> @@ -614,8 +614,7 @@ static inline int free_pages_check(struct page *page) >>>> return 1; >>>> } >>>> page_nid_reset_last(page); >>>> - if (page->flags & PAGE_FLAGS_CHECK_AT_PREP) >>>> - page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP; >>>> + page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP; >>>> return 0; >>>> } >>>> >>> -- >>> To unsubscribe, send a message with 'unsubscribe linux-mm' in >>> the body to majordomo@kvack.org. For more info on Linux MM, >>> see: http://www.linux-mm.org/ . >>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> > -- > To unsubscribe, send a message with 'unsubscribe linux-mm' in > the body to majordomo@kvack.org. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm: page_alloc: remove branch operation in free_pages_prepare() 2013-03-08 2:01 ` Hugh Dickins 2013-03-09 3:54 ` Will Huck @ 2013-03-13 8:08 ` Joonsoo Kim 1 sibling, 0 replies; 6+ messages in thread From: Joonsoo Kim @ 2013-03-13 8:08 UTC (permalink / raw) To: Hugh Dickins; +Cc: Andrew Morton, linux-kernel, linux-mm Hello, Hugh. On Thu, Mar 07, 2013 at 06:01:26PM -0800, Hugh Dickins wrote: > On Fri, 8 Mar 2013, Joonsoo Kim wrote: > > On Thu, Mar 07, 2013 at 10:54:15AM -0800, Hugh Dickins wrote: > > > On Thu, 7 Mar 2013, Joonsoo Kim wrote: > > > > > > > When we found that the flag has a bit of PAGE_FLAGS_CHECK_AT_PREP, > > > > we reset the flag. If we always reset the flag, we can reduce one > > > > branch operation. So remove it. > > > > > > > > Cc: Hugh Dickins <hughd@google.com> > > > > Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com> > > > > > > I don't object to this patch. But certainly I would have written it > > > that way in order not to dirty a cacheline unnecessarily. It may be > > > obvious to you that the cacheline in question is almost always already > > > dirty, and the branch almost always more expensive. But I'll leave that > > > to you, and to those who know more about these subtle costs than I do. > > > > Yes. I already think about that. I thought that even if a cacheline is > > not dirty at this time, we always touch the 'struct page' in > > set_freepage_migratetype() a little later, so dirtying is not the problem. > > I expect that a very high proportion of user pages have > PG_uptodate to be cleared here; and there's also the recently added > page_nid_reset_last(), which will dirty the flags or a nearby field > when CONFIG_NUMA_BALANCING. Those argue in favour of your patch. > Ah... I totally missed it. > > > > But, now, I re-think this and decide to drop this patch. > > The reason is that 'struct page' of 'compound pages' may not be dirty > > at this time and will not be dirty at later time. > > Actual compound pages would have PG_head or PG_tail or PG_compound > to be cleared there, I believe (check if I'm right on that). The > questionable case is the ordinary order>0 case without __GFP_COMP > (and page_nid_reset_last() is applied to each subpage of those). > Yes. > > So this patch is bad idea. > > I'm not so sure. I doubt your patch will make a giant improvement > in kernel performance! But it might make a little - maybe you just > need to give some numbers from perf to justify it (but I'm easily > dazzled by numbers - don't expect me to judge the result). Okay. Thanks for enlightening comment. Now, I don't have any idea to collect a performance result for this patch. When I have more time, I try to think it. Thanks. > > Hugh > > > > > Is there any comments? > > > > Thanks. > > > > > Hugh > > > > > > > > > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > > > index 8fcced7..778f2a9 100644 > > > > --- a/mm/page_alloc.c > > > > +++ b/mm/page_alloc.c > > > > @@ -614,8 +614,7 @@ static inline int free_pages_check(struct page *page) > > > > return 1; > > > > } > > > > page_nid_reset_last(page); > > > > - if (page->flags & PAGE_FLAGS_CHECK_AT_PREP) > > > > - page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP; > > > > + page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP; > > > > return 0; > > > > } > > > > > > > > > > -- > > > To unsubscribe, send a message with 'unsubscribe linux-mm' in > > > the body to majordomo@kvack.org. For more info on Linux MM, > > > see: http://www.linux-mm.org/ . > > > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> > > > > -- > To unsubscribe, send a message with 'unsubscribe linux-mm' in > the body to majordomo@kvack.org. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-03-13 8:08 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-03-07 8:21 [PATCH] mm: page_alloc: remove branch operation in free_pages_prepare() Joonsoo Kim 2013-03-07 18:54 ` Hugh Dickins 2013-03-08 0:45 ` Joonsoo Kim 2013-03-08 2:01 ` Hugh Dickins 2013-03-09 3:54 ` Will Huck 2013-03-13 8:08 ` Joonsoo Kim
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).