diff mbox series

[printk,v2,04/38] printk: introduce console_is_enabled() wrapper

Message ID 20221019145600.1282823-5-john.ogness@linutronix.de
State New
Headers
Series reduce console_lock scope |

Commit Message

John Ogness Oct. 19, 2022, 2:55 p.m. UTC 
  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(+)

Comments

Greg KH Oct. 19, 2022, 4:01 p.m. UTC | #1 
On Wed, Oct 19, 2022 at 05:01:26PM +0206, 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>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Petr Mladek Oct. 21, 2022, 8:57 a.m. UTC | #2 
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
Petr Mladek Oct. 21, 2022, 9:37 a.m. UTC | #3 
On Fri 2022-10-21 10:57:58, Petr Mladek wrote:
> 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.

Still thinking about it.

It is possible that console_is_enabled_safe() variant won't be
needed because all the callers will be either naturally serialized
or can be racy.

By other words, it makes sense to use data_race() because there are
used racy checks. And there probably is not any caller that would
strictly require explicit synchronization when reading this flag.

Anyway, if there is any caller that would require explicit
synchronization than we need a variant without data_race().

It would be great to somehow explain this in the commit message.

Best Regards,
Petr
diff mbox series

Patch

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.
+	 */
+	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