All of lore.kernel.org
 help / color / mirror / Atom feed
From: Lorenzo Stoakes <ljs@kernel.org>
To: Thorsten Blum <thorsten.blum@linux.dev>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	 David Hildenbrand <david@kernel.org>,
	"Liam R. Howlett" <liam@infradead.org>,
	 Vlastimil Babka <vbabka@kernel.org>,
	Mike Rapoport <rppt@kernel.org>,
	 Suren Baghdasaryan <surenb@google.com>,
	Michal Hocko <mhocko@suse.com>,
	 Yury Norov <yury.norov@gmail.com>,
	Rasmus Villemoes <linux@rasmusvillemoes.dk>,
	 Andy Shevchenko <andriy.shevchenko@intel.com>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [PATCH 2/3] mm: add bytes_to_page_end() helper
Date: Mon, 18 May 2026 11:24:23 +0100	[thread overview]
Message-ID: <agrjr91mgYGVpixV@lucifer> (raw)
In-Reply-To: <20260517123428.1181981-5-thorsten.blum@linux.dev>

Why is this a separate commit? It's tiny, I don't think it needs to be separated
at all.

On Sun, May 17, 2026 at 02:34:30PM +0200, Thorsten Blum wrote:
> Add bytes_to_page_end() for the common PAGE_SIZE - offset_in_page()
> calculation.

This is a totally useless commit message. Again you're not indicating why it's
needed. Please look around mm to get a sense of how commit messages should look.

Also it's so common that you have exactly zero callers updated here or
elsewhere?

I see uses which are all pretty specific. I wonder if it's really useful to make
this its own thing?

And it's untyped and etc. etc. yeah, it's a no really to this.

>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>

Was there a previous discussion that led to this series? You should really give
some background somewhere (like a cover letter, and if previous revisions were
sent, a history of them below the ---)

> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>

Again I'm really unconvinced you need to do any of this.

> ---
>  include/linux/page_helpers.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/include/linux/page_helpers.h b/include/linux/page_helpers.h
> index 102a4f3c3868..981731faa1fc 100644
> --- a/include/linux/page_helpers.h
> +++ b/include/linux/page_helpers.h
> @@ -6,5 +6,6 @@
>  #include <asm/page.h>
>
>  #define offset_in_page(p)	((unsigned long)(p) & ~PAGE_MASK)

I mean another thing about this (and yes this was existing but since you're
proposing making this rather special it's worth raising) is that it's enormously
un-typesafe and 'p' is vague, what if somebody put a (struct page *) here?

It'd be better if it was a static inline function, maybe there's places which
use this already that require it to be a macro but perhaps not.

> +#define bytes_to_page_end(p)	(PAGE_SIZE - offset_in_page(p))

Here, since, you're adding it, there's no excuse at all for it being a
macro. And you're implementing something just as horribly vague as the other
form, it's not obvious 'p' is meant to be a pointer. Page begins with p too :)

Anyway I'm not really convinced this is useful.

>
>  #endif /* _LINUX_PAGE_HELPERS_H */

Thanks, Lorenzo


  parent reply	other threads:[~2026-05-18 10:24 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-05-17 12:34 [PATCH 1/3] mm: move offset_in_page() to page_helpers.h Thorsten Blum
2026-05-17 12:34 ` [PATCH 2/3] mm: add bytes_to_page_end() helper Thorsten Blum
2026-05-17 15:28   ` Yury Norov
2026-05-18  6:48     ` Andy Shevchenko
2026-05-18 10:23       ` Lorenzo Stoakes
2026-05-18 11:33         ` William Kucharski
2026-05-18 11:53         ` William Kucharski
2026-05-18  9:09     ` David Laight
2026-05-18 10:24       ` Lorenzo Stoakes
2026-05-18 13:06         ` David Hildenbrand (Arm)
2026-05-18 13:15           ` Lorenzo Stoakes
2026-05-18 13:24             ` David Hildenbrand (Arm)
2026-05-18 13:38               ` Lorenzo Stoakes
2026-05-18 14:29             ` Thomas Weißschuh
2026-05-18 14:41               ` David Hildenbrand (Arm)
2026-05-18 14:52                 ` Thomas Weißschuh
2026-05-18 15:32                   ` David Hildenbrand (Arm)
2026-05-18  6:49   ` Andy Shevchenko
2026-05-18 10:24   ` Lorenzo Stoakes [this message]
2026-05-17 12:34 ` [PATCH 3/3] lib/bitmap: use " Thorsten Blum
2026-05-18 10:33   ` Lorenzo Stoakes
2026-05-18 15:29   ` Yury Norov
2026-05-18  6:51 ` [PATCH 1/3] mm: move offset_in_page() to page_helpers.h Andy Shevchenko
2026-05-18 10:02 ` Lorenzo Stoakes
2026-05-18 12:13   ` Thorsten Blum
2026-05-18 12:33     ` Lorenzo Stoakes
2026-05-18 13:36       ` Thorsten Blum
2026-05-18 14:03         ` Lorenzo Stoakes
2026-05-18 12:58     ` David Hildenbrand (Arm)

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=agrjr91mgYGVpixV@lucifer \
    --to=ljs@kernel.org \
    --cc=akpm@linux-foundation.org \
    --cc=andriy.shevchenko@intel.com \
    --cc=david@kernel.org \
    --cc=liam@infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux@rasmusvillemoes.dk \
    --cc=mhocko@suse.com \
    --cc=rppt@kernel.org \
    --cc=surenb@google.com \
    --cc=thorsten.blum@linux.dev \
    --cc=vbabka@kernel.org \
    --cc=yury.norov@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.