mm/page_alloc: simplify page_is_buddy() for better code readability
diff mbox series

Message ID 1583655036-5025-1-git-send-email-qiwuchen55@gmail.com
State New
Headers show
Series
  • mm/page_alloc: simplify page_is_buddy() for better code readability
Related show

Commit Message

Qiwu Chen March 8, 2020, 8:10 a.m. UTC
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(-)

Comments

Baoquan He March 8, 2020, 8:44 a.m. UTC | #1
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
> 
>
Matthew Wilcox March 8, 2020, 12:23 p.m. UTC | #2
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>
Alexander Duyck March 9, 2020, 4:53 p.m. UTC | #3
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.
Pankaj Gupta March 9, 2020, 7:18 p.m. UTC | #4
>
> 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
>
>
Qiwu Chen March 10, 2020, 2:15 a.m. UTC | #5
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.

Patch
diff mbox series

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;
 }