linux-kernel - Re: [PATCH v1 1/5] mm/page_alloc: convert "report" flag of __free_one

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201002134118.GA20115@casper.infradead.org>
Date:   Fri, 2 Oct 2020 14:41:18 +0100
From:   Matthew Wilcox <willy@...radead.org>
To:     David Hildenbrand <david@...hat.com>
Cc:     linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        linux-hyperv@...r.kernel.org, xen-devel@...ts.xenproject.org,
        linux-acpi@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Alexander Duyck <alexander.h.duyck@...ux.intel.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        Oscar Salvador <osalvador@...e.de>,
        Mel Gorman <mgorman@...hsingularity.net>,
        Michal Hocko <mhocko@...nel.org>,
        Dave Hansen <dave.hansen@...el.com>,
        Wei Yang <richard.weiyang@...ux.alibaba.com>,
        Mike Rapoport <rppt@...nel.org>
Subject: Re: [PATCH v1 1/5] mm/page_alloc: convert "report" flag of
 __free_one_page() to a proper flag

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?

> +/*
> + * 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"?

*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?

> -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.