On Thu 19-05-22 13:23:56, Kent Overstreet wrote:
[...]
> The goal is to replace most of our %p format extensions with this
> interface, and to move pretty-printers out of the core vsprintf.c code -
> this will get us better organization and better discoverability (you'll
> be able to cscope to pretty printer calls!), as well as eliminate a lot
> of dispatch code in vsprintf.c.
Is this really something that we want? While I do see arguments about
our existing %p$FOO mess there is at least one good argument to have all
those "pretty printers" at a single location. That approach allows to do
a proper review whether those printers are safe from the printk point of
view. If we allow any random callback to be called from the printk
context we just give a free ticket to anybody to do whatever from there
without understanding of all potential consequences.
Maybe that is less of a concern these days when printk locking has been
reworked a lot but I still do remember how frustrating it is to debug
issues related to printk getting stuck...
--
Michal Hocko
SUSE Labs
On Fri, May 20, 2022 at 09:40:24AM +0200, Michal Hocko wrote:
> On Thu 19-05-22 13:23:56, Kent Overstreet wrote:
> [...]
> > The goal is to replace most of our %p format extensions with this
> > interface, and to move pretty-printers out of the core vsprintf.c code -
> > this will get us better organization and better discoverability (you'll
> > be able to cscope to pretty printer calls!), as well as eliminate a lot
> > of dispatch code in vsprintf.c.
>
> Is this really something that we want? While I do see arguments about
> our existing %p$FOO mess there is at least one good argument to have all
> those "pretty printers" at a single location. That approach allows to do
> a proper review whether those printers are safe from the printk point of
> view. If we allow any random callback to be called from the printk
> context we just give a free ticket to anybody to do whatever from there
> without understanding of all potential consequences.
>
> Maybe that is less of a concern these days when printk locking has been
> reworked a lot but I still do remember how frustrating it is to debug
> issues related to printk getting stuck...
So for now, I added a note in the documentation that pretty-printers may not
sleep if passed to printk() - but if they're just passed to pr_buf() or
sprintf() it's completely fine.
On Fri, May 20, 2022 at 09:40:24AM +0200, Michal Hocko wrote:
> On Thu 19-05-22 13:23:56, Kent Overstreet wrote:
> [...]
> > The goal is to replace most of our %p format extensions with this
> > interface, and to move pretty-printers out of the core vsprintf.c code -
> > this will get us better organization and better discoverability (you'll
> > be able to cscope to pretty printer calls!), as well as eliminate a lot
> > of dispatch code in vsprintf.c.
>
> Is this really something that we want? While I do see arguments about
> our existing %p$FOO mess there is at least one good argument to have all
> those "pretty printers" at a single location. That approach allows to do
> a proper review whether those printers are safe from the printk point of
> view. If we allow any random callback to be called from the printk
> context we just give a free ticket to anybody to do whatever from there
> without understanding of all potential consequences.
>
> Maybe that is less of a concern these days when printk locking has been
> reworked a lot but I still do remember how frustrating it is to debug
> issues related to printk getting stuck...
Thanks for raising this, but I see this as an argument for documenting and
perhaps improving printk locking rules. Curiously, there was no such
documentation in vsprintf.c, nor anywhere else I've looked yet.
We may want to consider double buffering - it would get rid of any locking
concerns if we wrote to a separate buffer while building up the printk message.
However, this would require allocating memory, so it'd require MM folks to get
over their fear of allocating memory... :)