Date:   Wed, 1 Feb 2023 11:07:21 +0100
From:   Peter Zijlstra <peterz@infradead.org>
To:     Song Liu <song@kernel.org>
Cc:     linux-modules@vger.kernel.org, linux-kernel@vger.kernel.org,
        hch@lst.de, kernel-team@meta.com,
        Luis Chamberlain <mcgrof@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Guenter Roeck <linux@roeck-us.net>,
        Christophe Leroy <christophe.leroy@csgroup.eu>
Subject: Re: [PATCH v5] module: replace module_layout with module_memory
Message-ID: <Y9o52aAC33YlRueI@hirez.programming.kicks-ass.net>
References: <20230201064720.1949224-1-song@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230201064720.1949224-1-song@kernel.org>
Precedence: bulk

On Tue, Jan 31, 2023 at 10:47:20PM -0800, Song Liu wrote:
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 8c5909c0076c..3429d354fec0 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -320,17 +320,50 @@ struct mod_tree_node {
>  	struct latch_tree_node node;
>  };
>  
> +enum mod_mem_type {
	MOD_TEXT = 0,

(just paranoia, and you explicitly rely on that below)

> +	MOD_DATA,
> +	MOD_RODATA,
> +	MOD_RO_AFTER_INIT,
> +	MOD_INIT_TEXT,
> +	MOD_INIT_DATA,
> +	MOD_INIT_RODATA,
> +
> +	MOD_MEM_NUM_TYPES,
> +	MOD_INVALID = -1,
> +};
> +
> +#define mod_mem_type_is_core_data(type)	\
> +	((type) == MOD_DATA ||		\
> +	 (type) == MOD_RODATA ||	\
> +	 (type) == MOD_RO_AFTER_INIT)
> +
> +#define mod_mem_type_is_core(type)	\
> +	((type) == MOD_TEXT ||		\
> +	 mod_mem_type_is_core_data(type))
> +
> +#define mod_mem_type_is_init(type)	\
> +	((type) == MOD_INIT_TEXT ||	\
> +	 (type) == MOD_INIT_DATA ||	\
> +	 (type) == MOD_INIT_RODATA)

Note that, per definition:

  core := !init
  data := !text

(and vice-versa ofcourse, so pick the smallest set) also ISTR you
explicitly needing is_text somewhere.... ah yes, module_enable_nx().

That is; I'd write something like:

#define mod_mem_type_is_core(type) !mod_mem_type_is_init(type)

#define mod_mem_type_is_text(type)	\
	((type) == MOD_TEXT ||		\
	 (type) == MOD_INIT_TEXT)

#define mod_mem_type_is_data(type) !mod_mem_type_is_text(type)

and then possibly additional helpers like is_core_data etc.. where
needed.

#define mod_mem_type_is_core_data(type)	\
	(mod_mem_type_is_core(type) &&	\
	 mod_mem_type_is_data(type))

> +#define for_each_mod_mem_type(type)		\
> +	for (enum mod_mem_type (type) = 0;	\
> +	     (type) < MOD_MEM_NUM_TYPES; (type)++)

So how about instead of this ...

> +#define for_core_mod_mem_type(type)			\
> +	for (enum mod_mem_type (type) = 0;		\
> +	     (type) < MOD_MEM_NUM_TYPES; (type)++)	\
> +		if (mod_mem_type_is_core(type))
> +
> +#define for_init_mod_mem_type(type)			\
> +	for (enum mod_mem_type (type) = 0;		\
> +	     (type) < MOD_MEM_NUM_TYPES; (type)++)	\
> +		if (mod_mem_type_is_init(type))

... you write something like:

#define for_class_mod_mem_type(type, class)		\
	for_each_mod_mem_type(type)			\
		if (mod_mem_type_is_##class(type))

Then we can write things like:

	for_class_mod_mem_type(type, init)
	for_class_mod_mem_type(type, data)

and

	for_class_mod_mem_type(type, core_data)

(this last could be used in show_datasize() for example).

Does that make sense?