Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 21 Oct 2022 10:57:57 +0200
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>
Subject: Re: [PATCH printk v2 04/38] printk: introduce console_is_enabled()
 wrapper
Message-ID: <Y1JfFTAIbcFOrPtD@alley>
References: <20221019145600.1282823-1-john.ogness@linutronix.de>
 <20221019145600.1282823-5-john.ogness@linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221019145600.1282823-5-john.ogness@linutronix.de>
Precedence: bulk

On Wed 2022-10-19 17:01:26, John Ogness wrote:
> After switching to SRCU for console list iteration, some readers
> will begin accessing console->flags as a data race. This is safe
> because there is at most one CPU modifying console->flags and
> using rmw operations.
> 
> The primary reason for readers to access console->flags is to
> check if the console is enabled. Introduce console_is_enabled()
> to mark such access as a data race.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  include/linux/console.h | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/include/linux/console.h b/include/linux/console.h
> index cff86cc615f8..60195cd086dc 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -172,6 +172,26 @@ extern void console_srcu_read_unlock(int cookie);
>  
>  extern struct hlist_head console_list;
>  
> +/**
> + * console_is_enabled - Check if the console is enabled
> + * @con:	struct console pointer of console to check
> + *
> + * This should be used instead of manually testing for the CON_ENABLED
> + * bit in the console->flags.
> + *
> + * Context: Any context.
> + */
> +static inline bool console_is_enabled(const struct console *con)
> +{
> +	/*
> +	 * If SRCU is used, reading of console->flags can be a data
> +	 * race. However, this is safe because there is at most one
> +	 * CPU modifying console->flags and it is using only
> +	 * read-modify-write operations to do so.

Hmm, I somehow do not understand the explanation. How does
read-modify-write operation make this safe, please?

We are interested into one bit. IMHO, it is not important
if the flags variable is modified atomically or byte by byte.
The important thing is if the reading is synchronized against
modifications.

This function does not do any synchronization on its own.
So, it depends on the caller.


I would personally do two variants. for example:

    console_is_enabled()
    console_is_enabled_safe()

The first variant would be called in situations where the race
does not matter and the other when it matters.


> +	 */
> +	return (data_race(READ_ONCE(con->flags)) & CON_ENABLED);
> +}
> +
>  /**
>   * for_each_console_srcu() - Iterator over registered consoles
>   * @con:	struct console pointer used as loop cursor

Best Regards,
Petr