Date:   Tue, 21 Mar 2023 17:04:33 +0100
From:   Petr Mladek <pmladek@suse.com>
To:     John Ogness <john.ogness@linutronix.de>
Cc:     Sergey Senozhatsky <senozhatsky@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        linux-kernel@vger.kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-fsdevel@vger.kernel.org
Subject: union: was: Re: [PATCH printk v1 05/18] printk: Add non-BKL console
 basic infrastructure
Message-ID: <ZBnVkarywpyWlDWW@alley>
References: <20230302195618.156940-1-john.ogness@linutronix.de>
 <20230302195618.156940-6-john.ogness@linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230302195618.156940-6-john.ogness@linutronix.de>
Precedence: bulk

On Thu 2023-03-02 21:02:05, John Ogness wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> The current console/printk subsystem is protected by a Big Kernel Lock,
> (aka console_lock) which has ill defined semantics and is more or less
> stateless. This puts severe limitations on the console subsystem and
> makes forced takeover and output in emergency and panic situations a
> fragile endavour which is based on try and pray.
> 
> The goal of non-BKL consoles is to break out of the console lock jail
> and to provide a new infrastructure that avoids the pitfalls and
> allows console drivers to be gradually converted over.
> 
> The proposed infrastructure aims for the following properties:
> 
>   - Per console locking instead of global locking
>   - Per console state which allows to make informed decisions
>   - Stateful handover and takeover
> 
> As a first step state is added to struct console. The per console state
> is an atomic_long_t with a 32bit bit field and on 64bit also a 32bit
> sequence for tracking the last printed ringbuffer sequence number. On
> 32bit the sequence is separate from state for obvious reasons which
> requires handling a few extra race conditions.
>

It is not completely clear that that this struct is stored
as atomic_long_t atomic_state[2] in struct console.

What about adding?

		atomic_long_t atomic;

> +		unsigned long	atom;
> +		struct {
> +#ifdef CONFIG_64BIT
> +			u32	seq;
> +#endif
> +			union {
> +				u32	bits;
> +				struct {
> +				};
> +			};
> +		};
> +	};
>  };
>  
>  /**
> @@ -186,6 +214,8 @@ enum cons_flags {
>   * @dropped:		Number of unreported dropped ringbuffer records
>   * @data:		Driver private data
>   * @node:		hlist node for the console list
> + *
> + * @atomic_state:	State array for NOBKL consoles; real and handover
>   */
>  struct console {
>  	char			name[16];
> @@ -205,6 +235,9 @@ struct console {
>  	unsigned long		dropped;
>  	void			*data;
>  	struct hlist_node	node;
> +
> +	/* NOBKL console specific members */
> +	atomic_long_t		__private atomic_state[2];

and using here

	struct cons_state	__private cons_state[2];

Then we could use cons_state[which].atomic to access it as
the atomic type.

Or was this on purpose? It is true that only the variable
in struct console has to be accessed the atomic way.

Anyway, we should at least add a comment into struct console
about that atomic_state[2] is used to store and access
struct cons_state an atomic way. Also add a compilation
check that the size is the same.

Best Regards,
Petr