| Message ID | 1583655036-5025-1-git-send-email-qiwuchen55@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/page_alloc: simplify page_is_buddy() for better code readability | expand |
On 03/08/20 at 04:10pm, qiwuchen55@gmail.com wrote: > From: chenqiwu <chenqiwu@xiaomi.com> > > Simplify page_is_buddy() to reduce the redundant code for better > code readability. > > Signed-off-by: chenqiwu <chenqiwu@xiaomi.com> > --- > mm/page_alloc.c | 13 +++---------- > 1 file changed, 3 insertions(+), 10 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 79e950d..c6eef38 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -797,16 +797,8 @@ static inline void set_page_order(struct page *page, unsigned int order) > static inline int page_is_buddy(struct page *page, struct page *buddy, > unsigned int order) > { > - if (page_is_guard(buddy) && page_order(buddy) == order) { > - if (page_zone_id(page) != page_zone_id(buddy)) > - return 0; > - > - VM_BUG_ON_PAGE(page_count(buddy) != 0, buddy); > - > - return 1; > - } This duplicated removing Looks good, not sure if there's any performance impact or other considering. FWIW, Reviewed-by: Baoquan He <bhe@redhat.com> > - > - if (PageBuddy(buddy) && page_order(buddy) == order) { > + if ((page_is_guard(buddy) || PageBuddy(buddy)) > + && page_order(buddy) == order) { > /* > * zone check is done late to avoid uselessly > * calculating zone/node ids for pages that could > @@ -819,6 +811,7 @@ static inline int page_is_buddy(struct page *page, struct page *buddy, > > return 1; > } > + > return 0; > } > > -- > 1.9.1 > >
On Sun, Mar 08, 2020 at 04:10:36PM +0800, qiwuchen55@gmail.com wrote: > From: chenqiwu <chenqiwu@xiaomi.com> > > Simplify page_is_buddy() to reduce the redundant code for better > code readability. > > Signed-off-by: chenqiwu <chenqiwu@xiaomi.com> Yes, this makes sense. It's fascinating looking at the git history for how we got here. Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
On Sun, Mar 8, 2020 at 12:10 AM <qiwuchen55@gmail.com> wrote: > > From: chenqiwu <chenqiwu@xiaomi.com> > > Simplify page_is_buddy() to reduce the redundant code for better > code readability. > > Signed-off-by: chenqiwu <chenqiwu@xiaomi.com> > --- > mm/page_alloc.c | 13 +++---------- > 1 file changed, 3 insertions(+), 10 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 79e950d..c6eef38 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -797,16 +797,8 @@ static inline void set_page_order(struct page *page, unsigned int order) > static inline int page_is_buddy(struct page *page, struct page *buddy, > unsigned int order) > { > - if (page_is_guard(buddy) && page_order(buddy) == order) { > - if (page_zone_id(page) != page_zone_id(buddy)) > - return 0; > - > - VM_BUG_ON_PAGE(page_count(buddy) != 0, buddy); > - > - return 1; > - } > - > - if (PageBuddy(buddy) && page_order(buddy) == order) { > + if ((page_is_guard(buddy) || PageBuddy(buddy)) > + && page_order(buddy) == order) { > /* > * zone check is done late to avoid uselessly > * calculating zone/node ids for pages that could Instead of keeping the if statement as is couldn't you flatten this out further by just returning 0 if !page_is_guard && !PageBuddy? So something like: if (!page_is_guard(buddy) && !PageBuddy(buddy)) return0; if (page_order(buddy) != order) return 0; I feel like this would be more readable than sorting out the parenthesis for the conditional statement. Then you can also just get rid of the indenting and braces for the rest of the statement. With that it would more closely match the description above as well as you are going through and checking a - d as separate tests.
> > From: chenqiwu <chenqiwu@xiaomi.com> > > Simplify page_is_buddy() to reduce the redundant code for better > code readability. > > Signed-off-by: chenqiwu <chenqiwu@xiaomi.com> > --- > mm/page_alloc.c | 13 +++---------- > 1 file changed, 3 insertions(+), 10 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 79e950d..c6eef38 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -797,16 +797,8 @@ static inline void set_page_order(struct page *page, unsigned int order) > static inline int page_is_buddy(struct page *page, struct page *buddy, > unsigned int order) > { > - if (page_is_guard(buddy) && page_order(buddy) == order) { > - if (page_zone_id(page) != page_zone_id(buddy)) > - return 0; > - > - VM_BUG_ON_PAGE(page_count(buddy) != 0, buddy); > - > - return 1; > - } > - > - if (PageBuddy(buddy) && page_order(buddy) == order) { > + if ((page_is_guard(buddy) || PageBuddy(buddy)) > + && page_order(buddy) == order) { > /* > * zone check is done late to avoid uselessly > * calculating zone/node ids for pages that could > @@ -819,6 +811,7 @@ static inline int page_is_buddy(struct page *page, struct page *buddy, > > return 1; > } > + > return 0; > } Looks good to me. Acked-by: Pankaj Gupta <pankaj.gupta.linux@gmail.com> > > -- > 1.9.1 > >
On Mon, Mar 09, 2020 at 09:53:08AM -0700, Alexander Duyck wrote: > On Sun, Mar 8, 2020 at 12:10 AM <qiwuchen55@gmail.com> wrote: > > > > From: chenqiwu <chenqiwu@xiaomi.com> > > > > Simplify page_is_buddy() to reduce the redundant code for better > > code readability. > > > > Signed-off-by: chenqiwu <chenqiwu@xiaomi.com> > > --- > > mm/page_alloc.c | 13 +++---------- > > 1 file changed, 3 insertions(+), 10 deletions(-) > > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > index 79e950d..c6eef38 100644 > > --- a/mm/page_alloc.c > > +++ b/mm/page_alloc.c > > @@ -797,16 +797,8 @@ static inline void set_page_order(struct page *page, unsigned int order) > > static inline int page_is_buddy(struct page *page, struct page *buddy, > > unsigned int order) > > { > > - if (page_is_guard(buddy) && page_order(buddy) == order) { > > - if (page_zone_id(page) != page_zone_id(buddy)) > > - return 0; > > - > > - VM_BUG_ON_PAGE(page_count(buddy) != 0, buddy); > > - > > - return 1; > > - } > > - > > - if (PageBuddy(buddy) && page_order(buddy) == order) { > > + if ((page_is_guard(buddy) || PageBuddy(buddy)) > > + && page_order(buddy) == order) { > > /* > > * zone check is done late to avoid uselessly > > * calculating zone/node ids for pages that could > > Instead of keeping the if statement as is couldn't you flatten this > out further by just returning 0 if !page_is_guard && !PageBuddy? > > So something like: > if (!page_is_guard(buddy) && !PageBuddy(buddy)) > return0; > > if (page_order(buddy) != order) > return 0; > > I feel like this would be more readable than sorting out the > parenthesis for the conditional statement. Then you can also just get > rid of the indenting and braces for the rest of the statement. With > that it would more closely match the description above as well as you > are going through and checking a - d as separate tests. I agree, for performance considering, I think the second conditional statement should be moved up. I will resend this as proper patch v2 for review.
diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 79e950d..c6eef38 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -797,16 +797,8 @@ static inline void set_page_order(struct page *page, unsigned int order) static inline int page_is_buddy(struct page *page, struct page *buddy, unsigned int order) { - if (page_is_guard(buddy) && page_order(buddy) == order) { - if (page_zone_id(page) != page_zone_id(buddy)) - return 0; - - VM_BUG_ON_PAGE(page_count(buddy) != 0, buddy); - - return 1; - } - - if (PageBuddy(buddy) && page_order(buddy) == order) { + if ((page_is_guard(buddy) || PageBuddy(buddy)) + && page_order(buddy) == order) { /* * zone check is done late to avoid uselessly * calculating zone/node ids for pages that could @@ -819,6 +811,7 @@ static inline int page_is_buddy(struct page *page, struct page *buddy, return 1; } + return 0; }