linux-kernel - Re: preffer_ofload param: was: Re: [PATCH printk v3 11/19] printk: nbcon: Rely on kthreads for normal operation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZqyyovKfUfI1t6Nz@pathway.suse.cz>
Date: Fri, 2 Aug 2024 12:19:14 +0200
From: Petr Mladek <pmladek@...e.com>
To: John Ogness <john.ogness@...utronix.de>
Cc: Sergey Senozhatsky <senozhatsky@...omium.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Thomas Gleixner <tglx@...utronix.de>, linux-kernel@...r.kernel.org
Subject: Re: preffer_ofload param: was: Re: [PATCH printk v3 11/19] printk:
 nbcon: Rely on kthreads for normal operation

On Fri 2024-08-02 09:35:17, John Ogness wrote:
> On 2024-08-01, Petr Mladek <pmladek@...e.com> wrote:
> > I believe that the parameter "prefer_offload" adds more harm than good
> > because:
> >
> >    + It is a non-sense for nbcon consoles. They always prefer offload
> >      except for emergency and panic. But emergency and panic is
> >      handled transparently by nbcon_get_default_prio().
> >
> >    + It is confusing even for legacy consoles after introducing the
> >      kthread. There will be three types of offload:
> >
> > 	+ do console_lock()/unlock() in IRQ work
> > 	+ wake kthread
> > 	+ wake kthread in IRQ work
> 
> I think the confusion comes from my intention of the function. I wanted
> a caller to use it as:
> 
> "Tell me how to flush."

Yes, I understand it the same way.

> This requires input from the caller to know some information about what
> the caller's intentions are.

Does it really need it?
Where?

Please, show me two examples where the function is not able to make
the right decision by global variables?

I know about __wake_up_klogd(). And it is only because
printk_deferred() requires the offload via @level parameter
instead of the per-CPU @printk_context.

Is there any other?

Let me repeat:

    + The parameter makes a difference only when it is "true".
    + The parameter affects only legacy loop.

Let me check where the parameter is true in the current code:

 + nbcon_atomic_flush_pending_con[1548]
    + No effect! The function ignores legacy consoles.

 + nbcon_device_release[1848]
    + No effect! The function ignores legacy consoles.

 + resume_console[2706]
    + Wrong! The function should flush the legacy
	consoles directly by default. Otherwise, we would
	change the legacy behavior.

 + console_flush_all[3102]        printk_get_console_flush_type(&ft, true);
    + No effect! The function ignores legacy consoles.
    + Confusing! The function flushes legacy consoles directly!

  + console_start[3453]
    + Wrong! The function should flush the legacy
      consoles directly by default. Otherwise, we would
	change the legacy behavior.

  + legacy_kthread_should_wakeup[3484]
    + Not needed! The function is called only when
	force_legacy_kthread() == true.

  + __wake_up_klogd[4466]
    + This is the only location where it makes a difference.
    + We could simply check both legacy_direct && legacy_offload
      here.
    + Yes, it is a hack. But it is needed only because of
      printk_deferred() which is already hacky by using
      LOGLEVEL_SCHED.

  + console_try_replay_all[4851]
    + Wrong! It is called under console_lock() => the function will
	flush legacy consoles directly anyway.


> If I change the function so that a caller uses it as:
> 
> "Tell me what flush mechanisms are available to me."

No, this will make the code complicated again.

> Then the function does not need to know the caller's intentions. It only
> needs to know the caller's state, and that information is readily
> available via global/per-cpu variables.
>
> I will drop the @prefer_offload argument, simplifying the function to
> only provide a list of available flush options. The caller will then
> decide itself which option it wants to use. I believe this aligns with
> your intentions as well.

No. Please, keep the original meaning.

Best Regards,
Petr