Re: [PATCH 02/58] serial: Move sysrq members above

[Greg Kroah-Hartman]

On Fri, Dec 13, 2019 at 12:06:01AM +0000, Dmitry Safonov wrote:
> At the current place members those follow are:
> :	upf_t			flags;
> :	upstat_t		status;
> :	int			hw_stopped;
> :	unsigned int		mctrl;
> :	unsigned int		timeout;
> :	unsigned int		type;
> :	const struct uart_ops	*ops;
> 
> Together, they give (*ops) 8-byte align on 64-bit platforms.
> And `sysrq_ch` introduces 4-byte padding.
> 
> On the other side, above:
> :	struct device		*dev;
> :	unsigned char		hub6;
> :	unsigned char		suspended;
> :	unsigned char		unused[2];
> :	const char		*name;
> 
> Adds another 4-byte padding.
> 
> Moving sysrq members just before `hub6` allows to save 8 bytes
> per-uart_port on 64-bit platforms:
> On my gcc, x86_64 sizeof(struct uart_port) goes from 528 to 520.
> 
> Signed-off-by: Dmitry Safonov <dima@xxxxxxxxxx>
> ---
>  include/linux/serial_core.h | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 2b78cc734719..bbbe57bf5163 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -161,11 +161,6 @@ struct uart_port {
>  	struct uart_icount	icount;			/* statistics */
>  
>  	struct console		*cons;			/* struct console, if any */
> -#if defined(CONFIG_SERIAL_CORE_CONSOLE) || defined(SUPPORT_SYSRQ)
> -	unsigned long		sysrq;			/* sysrq timeout */
> -	unsigned int		sysrq_ch;		/* char for sysrq */
> -#endif
> -
>  	/* flags must be updated while holding port mutex */
>  	upf_t			flags;
>  
> @@ -244,6 +239,12 @@ struct uart_port {
>  	resource_size_t		mapbase;		/* for ioremap */
>  	resource_size_t		mapsize;
>  	struct device		*dev;			/* parent device */
> +
> +#if defined(CONFIG_SERIAL_CORE_CONSOLE) || defined(SUPPORT_SYSRQ)
> +	unsigned long		sysrq;			/* sysrq timeout */
> +	unsigned int		sysrq_ch;		/* char for sysrq */
> +#endif

Let's just always have these 2 fields in the structure, no need for a
#ifdef at all, right?  There is no real "savings" from removing them (8
bytes for a structure that is maybe created 2-4 times per machine?)

That way it would keep any of this mess of if the SUPPORT_SYSRQ is
enabled or not to keep everything in sync.

thanks,

greg k-h