Message-ID: <1342725950.5599.7.camel@lorien2>
Date:	Thu, 19 Jul 2012 13:25:50 -0600
From:	Shuah Khan <shuah.khan@...com>
To:	Toshi Kani <toshi.kani@...com>
Cc:	lenb@...nel.org, linux-acpi@...r.kernel.org,
	linux-kernel@...r.kernel.org, bhelgaas@...gle.com,
	isimatu.yasuaki@...fujitsu.com, liuj97@...il.com,
	srivatsa.bhat@...ux.vnet.ibm.com, prarit@...hat.com,
	imammedo@...hat.com, vijaymohan.pandarathil@...com,
	shuahkhan@...il.com
Subject: Re: [PATCH 1/4] ACPI: Add acpi_pr_<level>() interfaces

On Thu, 2012-07-19 at 11:28 -0600, Toshi Kani wrote:
> On Thu, 2012-07-19 at 10:15 -0600, Shuah Khan wrote:
> > On Wed, 2012-07-18 at 18:38 -0600, Toshi Kani wrote:
> > 
> > > 
> > > This interface is defined in acpi/acpi_bus.h, which is intended for ACPI
> > > drivers which make many ACPI calls to proceed when they are called at
> > > run-time today.  This interface does not change that, and I believe
> > > acpi_get_name() is much faster compared to ACPI method calls these ACPI
> > > drivers make in their normal code path.  The extra work to call
> > > acpi_get_name() is simply a noise in this case (if you try to measure),
> > > and the use of this interface is limited in error paths of such ACPI
> > > drivers.
> > 
> > I understand the scope of the usage of this new interface. I don't think
> > I am able to explain the problem I see with this interface as it gets
> > used more and more from acpi drivers. Let me try another way.
> > 
> > If understand the this patch set, if and when acpi drivers that
> > currently use pr_* interfaces switch to using acpi_pr_*, the execution
> > path goes from a what printk() does to the following:
> > 
> > acpi_pr_*
> > - setup static buffer
> > - calls acpi_get_name()
> > - acpi_get_name() calls acpi_ut_validate_buffer() and then calls
> >   acpi_ns_handle_to_pathname()
> > - acpi_ns_handle_to_pathname() calls acpi_ns_validate_handle() followed
> >   by acpi_ns_get_pathname_length() and so on.
> > 
> > I think this should give you a good idea of my concern. I think
> > acpi_pr_* full functionality should be enabled under special cases such
> > as some acpi_debug level setting or some other way, and not for default
> > case. I propose the following:
> > 
> > Make acpi_pr_* versions execute the full path to do acpi_get_name()
> > conditionally and not as a default case.
> > 
> > To illustrate my point further, I currently see the following ACPI
> > messages in my dmesg buffer on my laptop. I haven't taken the time to
> > evaluate how many of them originate from acpi drivers, however I would
> > not want to see all of these becoming acpi_pr_* versions that do more
> > than what pr_* does today. I hope this explains my concern clearly.
> 
> I agree that there are many ACPI messages at boot, and I understand that
> you concerned with them, but that is a different issue.  It requires a
> different project to address them.  Changing my patchset won't make any
> difference.

On the contrary, your patch set could make the existing problems worse
by introducing lot of complexity (makes the execution path very long for
each and every one these messages) in the path that prints messages. As
more and more acpi code paths start using the new interfaces, it will
keep getting worse.

I am not questioning the usefulness of the additional information, I am
questioning the complexity your patch set adds. The added complexity
isn't desirable.

The design in this patch set needs refinement so it doesn't add to the
execution path.

-- Shuah


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists