From: David Hildenbrand <david@redhat.com>
To: Matthew Wilcox <willy@infradead.org>
Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org,
linux-hyperv@vger.kernel.org, xen-devel@lists.xenproject.org,
linux-acpi@vger.kernel.org,
Andrew Morton <akpm@linux-foundation.org>,
Alexander Duyck <alexander.h.duyck@linux.intel.com>,
Vlastimil Babka <vbabka@suse.cz>,
Oscar Salvador <osalvador@suse.de>,
Mel Gorman <mgorman@techsingularity.net>,
Michal Hocko <mhocko@kernel.org>,
Dave Hansen <dave.hansen@intel.com>,
Wei Yang <richard.weiyang@linux.alibaba.com>,
Mike Rapoport <rppt@kernel.org>
Subject: Re: [PATCH v1 1/5] mm/page_alloc: convert "report" flag of __free_one_page() to a proper flag
Date: Fri, 2 Oct 2020 16:48:04 +0200 [thread overview]
Message-ID: <2b1baab8-861d-06a3-8eab-75c4e9e1b19d@redhat.com> (raw)
In-Reply-To: <20201002134118.GA20115@casper.infradead.org>
On 02.10.20 15:41, Matthew Wilcox wrote:
> On Mon, Sep 28, 2020 at 08:21:06PM +0200, David Hildenbrand wrote:
>> Let's prepare for additional flags and avoid long parameter lists of bools.
>> Follow-up patches will also make use of the flags in __free_pages_ok(),
>> however, I wasn't able to come up with a better name for the type - should
>> be good enough for internal purposes.
>
>> +/* Free One Page flags: for internal, non-pcp variants of free_pages(). */
>> +typedef int __bitwise fop_t;
>
> That invites confusion with f_op. There's no reason to use _t as a suffix
> here ... why not free_f?
git grep "bitwise" | grep typedef | grep include/linux
indicates that "_t" it the right thing to do.
I want a name that highlights that is is for the internal variants of
free_page(), free_f / free_t is too generic.
fpi_t (Free Page Internal) ?
>
>> +/*
>> + * Skip free page reporting notification for the (possibly merged) page. (will
>> + * *not* mark the page reported, only skip the notification).
>
> ... Don't you mean "will not skip marking the page as reported, only
> skip the notification"?
Yeah, I can use that.
The way free page reporting works is that
1. Free page reporting infrastructure will get notified after buddy
merging about a newly freed page.
2. Once a certain threshold of free pages is reached, it will pull pages
from the freelist, report them, and mark them as reported. (see
mm/page_reporting.c)
During 2., we didn't actually free a "new page", we only temporarily
removed it from the list, that's why we have to skip the notification.
What we do here is skip 1., not 2.
>
> *reads code*
>
> No, I'm still confused. What does this sentence mean?
>
> Would it help to have a FOP_DEFAULT that has FOP_REPORT_NOTIFY set and
> then a FOP_SKIP_REPORT_NOTIFY define that is 0?
Hmm, I'm not entirely sure if that improves the situation. Then, I need
3 defines instead of two, and an "inverse" documentation for
FOP_REPORT_NOTIFY.
>
>> -static inline void __free_one_page(struct page *page,
>> - unsigned long pfn,
>> - struct zone *zone, unsigned int order,
>> - int migratetype, bool report)
>> +static inline void __free_one_page(struct page *page, unsigned long pfn,
>> + struct zone *zone, unsigned int order,
>> + int migratetype, fop_t fop_flags)
>
> Please don't over-indent like this.
>
> static inline void __free_one_page(struct page *page, unsigned long pfn,
> struct zone *zone, unsigned int order, int migratetype,
> fop_t fop_flags)
>
> reads just as well and then if someone needs to delete the 'static'
> later, they don't need to fiddle around with subsequent lines getting
> the whitespace to line up again.
>
I don't care too much about this specific instance and can fix it up.
(this is clearly a matter of personal taste)
Thanks!
--
Thanks,
David / dhildenb
next prev parent reply other threads:[~2020-10-02 14:48 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-28 18:21 [PATCH v1 0/5] mm: place pages to the freelist tail when onling and undoing isolation David Hildenbrand
2020-09-28 18:21 ` [PATCH v1 1/5] mm/page_alloc: convert "report" flag of __free_one_page() to a proper flag David Hildenbrand
2020-09-28 20:11 ` Pankaj Gupta
2020-09-29 8:58 ` Wei Yang
2020-10-02 13:17 ` Michal Hocko
2020-10-02 13:41 ` Matthew Wilcox
2020-10-02 14:48 ` David Hildenbrand [this message]
2020-10-02 14:57 ` David Hildenbrand
2020-09-28 18:21 ` [PATCH v1 2/5] mm/page_alloc: place pages to tail in __putback_isolated_page() David Hildenbrand
2020-09-28 20:38 ` Pankaj Gupta
2020-09-29 9:10 ` Wei Yang
2020-10-02 13:19 ` Michal Hocko
2020-09-28 18:21 ` [PATCH v1 3/5] mm/page_alloc: always move pages to the tail of the freelist in unset_migratetype_isolate() David Hildenbrand
2020-09-28 20:55 ` Pankaj Gupta
2020-09-29 9:18 ` Wei Yang
2020-09-29 10:12 ` David Hildenbrand
2020-09-30 7:48 ` Wei Yang
2020-10-02 13:24 ` Michal Hocko
2020-10-02 15:20 ` David Hildenbrand
2020-10-05 6:56 ` Michal Hocko
2020-10-05 8:20 ` Mel Gorman
2020-10-05 9:11 ` David Hildenbrand
2020-09-28 18:21 ` [PATCH v1 4/5] mm/page_alloc: place pages to tail in __free_pages_core() David Hildenbrand
2020-09-28 20:33 ` Pankaj Gupta
2020-09-29 9:36 ` Wei Yang
2020-09-29 10:14 ` David Hildenbrand
2020-10-02 13:41 ` Michal Hocko
2020-10-02 15:10 ` David Hildenbrand
2020-09-28 18:21 ` [PATCH v1 5/5] mm/memory_hotplug: update comment regarding zone shuffling David Hildenbrand
2020-09-29 9:40 ` Wei Yang
2020-10-02 13:41 ` Michal Hocko
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=2b1baab8-861d-06a3-8eab-75c4e9e1b19d@redhat.com \
--to=david@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=alexander.h.duyck@linux.intel.com \
--cc=dave.hansen@intel.com \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-hyperv@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mgorman@techsingularity.net \
--cc=mhocko@kernel.org \
--cc=osalvador@suse.de \
--cc=richard.weiyang@linux.alibaba.com \
--cc=rppt@kernel.org \
--cc=vbabka@suse.cz \
--cc=willy@infradead.org \
--cc=xen-devel@lists.xenproject.org \
/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 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).