Messages in this thread | | | Date | Tue, 19 Jul 2022 13:48:43 -0700 | Subject | Re: [PATCH v2 09/13] notifier: Show function names on notifier routines if DEBUG_NOTIFIERS is set | From | Arjan van de Ven <> |
| |
On 7/19/2022 1:44 PM, Guilherme G. Piccoli wrote: > On 19/07/2022 17:33, Arjan van de Ven wrote: >> On 7/19/2022 12:53 PM, Guilherme G. Piccoli wrote: >>> Currently we have a debug infrastructure in the notifiers file, but >>> it's very simple/limited. Extend it by: >>> >>> (a) Showing all registered/unregistered notifiers' callback names; >> >> >> I'm not yet convinced that this is the right direction. >> The original intent for this "debug" feature was to be lightweight enough that it could run in production, since at the time, rootkits >> liked to clobber/hijack notifiers and there were also some other signs of corruption at the time. >> >> By making something print (even at pr_info) for what are probably frequent non-error operations, you turn something that is light >> into something that's a lot more heavy and generally that's not a great idea... it'll be a performance surprise. >> >> > > Is registering/un-registering notifiers a hot path, or performance > sensitive usually? For me, this patch proved to be very useful, and once > enabled, shows relatively few entries in dmesg, these operations aren't > so common thing it seems. > > Also, if this Kconfig option was meant to run in production, maybe the > first thing would be have some sysfs tuning or anything able to turn it > on - I've worked with a variety of customers and the most terrifying > thing in servers is to install a new kernel and reboot heh > > My understanding is that this debug infrastructure would be useful for > notifiers writers and people playing with the core notifiers > code...tracing would be much more useful in the context of checking if > some specific notifier got registered/executed in production environment > I guess.
I would totally support an approach where instead of pr_info, there's a tracepoint for these events (and that shouldnt' need to be conditional on a config option)
that's not what the patch does though.
| |