* Re: [PATCH] mm/page_alloc: simplify page_is_buddy() for better code readability
2020-03-08 8:10 [PATCH] mm/page_alloc: simplify page_is_buddy() for better code readability qiwuchen55
@ 2020-03-08 8:44 ` Baoquan He
2020-03-08 12:23 ` Matthew Wilcox
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Baoquan He @ 2020-03-08 8:44 UTC (permalink / raw)
To: qiwuchen55; +Cc: akpm, linux-mm, chenqiwu
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
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] mm/page_alloc: simplify page_is_buddy() for better code readability
2020-03-08 8:10 [PATCH] mm/page_alloc: simplify page_is_buddy() for better code readability qiwuchen55
2020-03-08 8:44 ` Baoquan He
@ 2020-03-08 12:23 ` Matthew Wilcox
2020-03-09 16:53 ` Alexander Duyck
2020-03-09 19:18 ` Pankaj Gupta
3 siblings, 0 replies; 6+ messages in thread
From: Matthew Wilcox @ 2020-03-08 12:23 UTC (permalink / raw)
To: qiwuchen55; +Cc: akpm, linux-mm, chenqiwu
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm/page_alloc: simplify page_is_buddy() for better code readability
2020-03-08 8:10 [PATCH] mm/page_alloc: simplify page_is_buddy() for better code readability qiwuchen55
2020-03-08 8:44 ` Baoquan He
2020-03-08 12:23 ` Matthew Wilcox
@ 2020-03-09 16:53 ` Alexander Duyck
2020-03-10 2:15 ` chenqiwu
2020-03-09 19:18 ` Pankaj Gupta
3 siblings, 1 reply; 6+ messages in thread
From: Alexander Duyck @ 2020-03-09 16:53 UTC (permalink / raw)
To: qiwuchen55; +Cc: Andrew Morton, linux-mm, chenqiwu
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] mm/page_alloc: simplify page_is_buddy() for better code readability
2020-03-09 16:53 ` Alexander Duyck
@ 2020-03-10 2:15 ` chenqiwu
0 siblings, 0 replies; 6+ messages in thread
From: chenqiwu @ 2020-03-10 2:15 UTC (permalink / raw)
To: Alexander Duyck; +Cc: Andrew Morton, linux-mm, chenqiwu
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm/page_alloc: simplify page_is_buddy() for better code readability
2020-03-08 8:10 [PATCH] mm/page_alloc: simplify page_is_buddy() for better code readability qiwuchen55
` (2 preceding siblings ...)
2020-03-09 16:53 ` Alexander Duyck
@ 2020-03-09 19:18 ` Pankaj Gupta
3 siblings, 0 replies; 6+ messages in thread
From: Pankaj Gupta @ 2020-03-09 19:18 UTC (permalink / raw)
To: qiwuchen55; +Cc: Andrew Morton, linux-mm, chenqiwu
>
> 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
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread