ACPI: Remove a redundant call to acpi_hw_register_read(). Signed-off-by: Wei Gang <gang.wei@intel.com> diff -r fc0b0c64246d xen/drivers/acpi/hwregs.c --- a/xen/drivers/acpi/hwregs.c Thu Aug 21 16:15:30 2008 +0800 +++ b/xen/drivers/acpi/hwregs.c Mon Aug 25 15:24:32 2008 +0800 @@ -238,12 +238,6 @@ acpi_status acpi_set_register(u32 regist break; case ACPI_REGISTER_PM2_CONTROL: - - status = acpi_hw_register_read(ACPI_REGISTER_PM2_CONTROL, - ®ister_value); - if (ACPI_FAILURE(status)) { - goto unlock_and_exit; - } ACPI_DEBUG_PRINT((ACPI_DB_IO, "PM2 control: Read %X from %8.8X%8.8X\n", _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
acpi_set_register() is doing a read-modify-write of PM2_CONTROL. It''s only safe to skip the read if the modification is going to obliterate all old bits. Is the read really that expensive (and equally, in patch 3, is the write of ARB_DIS really that expensive)? -- Keir On 3/9/08 02:52, "Wei, Gang" <gang.wei@intel.com> wrote:> ACPI: Remove a redundant call to acpi_hw_register_read(). > > Signed-off-by: Wei Gang <gang.wei@intel.com> > > diff -r fc0b0c64246d xen/drivers/acpi/hwregs.c > --- a/xen/drivers/acpi/hwregs.c Thu Aug 21 16:15:30 2008 +0800 > +++ b/xen/drivers/acpi/hwregs.c Mon Aug 25 15:24:32 2008 +0800 > @@ -238,12 +238,6 @@ acpi_status acpi_set_register(u32 regist > break; > > case ACPI_REGISTER_PM2_CONTROL: > - > - status = acpi_hw_register_read(ACPI_REGISTER_PM2_CONTROL, > - ®ister_value); > - if (ACPI_FAILURE(status)) { > - goto unlock_and_exit; > - } > > ACPI_DEBUG_PRINT((ACPI_DB_IO, > "PM2 control: Read %X from %8.8X%8.8X\n", > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
acpi_set_register() has already done a read before switch-case block, this read with in switch-case is a unnecessary repeat. The non-continuous PIO/MMIO access will spent far longer time than imagination because of the chipset link ASPM features. One single PIO/MMIO read may spent up to tens of thousands cpu cycles, although the fastest read may only cause less than one thousand cycles. I tried to measure the cost via TSC. Jimmy On Wednesday, September 03, 2008 4:43 PM, Keir Fraser wrote:> acpi_set_register() is doing a read-modify-write of PM2_CONTROL. It''s only > safe to skip the read if the modification is going to obliterate all old > bits. Is the read really that expensive (and equally, in patch 3, is the > write of ARB_DIS really that expensive)? > > -- Keir > > On 3/9/08 02:52, "Wei, Gang" <gang.wei@intel.com> wrote: > >> ACPI: Remove a redundant call to acpi_hw_register_read(). >> >> Signed-off-by: Wei Gang <gang.wei@intel.com> >> >> diff -r fc0b0c64246d xen/drivers/acpi/hwregs.c >> --- a/xen/drivers/acpi/hwregs.c Thu Aug 21 16:15:30 2008 +0800 >> +++ b/xen/drivers/acpi/hwregs.c Mon Aug 25 15:24:32 2008 +0800 >> @@ -238,12 +238,6 @@ acpi_status acpi_set_register(u32 regist >> break; >> >> case ACPI_REGISTER_PM2_CONTROL: >> - >> - status = acpi_hw_register_read(ACPI_REGISTER_PM2_CONTROL, >> - ®ister_value); >> - if (ACPI_FAILURE(status)) { >> - goto unlock_and_exit; >> - } >> >> ACPI_DEBUG_PRINT((ACPI_DB_IO, >> "PM2 control: Read %X from %8.8X%8.8X\n", >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xensource.com >> http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Agreed. Linux has the same double-read by the way, as recently as 2.6.26-rc4. I hope it''s not for a particular reason, and is just a typo. You might send your patch upstream to Linux if it is still not fixed there. -- Keir On 3/9/08 11:44, "Wei, Gang" <gang.wei@intel.com> wrote:> acpi_set_register() has already done a read before switch-case block, this > read with in switch-case is a unnecessary repeat. > > The non-continuous PIO/MMIO access will spent far longer time than imagination > because of the chipset link ASPM features. One single PIO/MMIO read may spent > up to tens of thousands cpu cycles, although the fastest read may only cause > less than one thousand cycles. I tried to measure the cost via TSC. > > Jimmy > > On Wednesday, September 03, 2008 4:43 PM, Keir Fraser wrote: >> acpi_set_register() is doing a read-modify-write of PM2_CONTROL. It''s only >> safe to skip the read if the modification is going to obliterate all old >> bits. Is the read really that expensive (and equally, in patch 3, is the >> write of ARB_DIS really that expensive)? >> >> -- Keir >> >> On 3/9/08 02:52, "Wei, Gang" <gang.wei@intel.com> wrote: >> >>> ACPI: Remove a redundant call to acpi_hw_register_read(). >>> >>> Signed-off-by: Wei Gang <gang.wei@intel.com> >>> >>> diff -r fc0b0c64246d xen/drivers/acpi/hwregs.c >>> --- a/xen/drivers/acpi/hwregs.c Thu Aug 21 16:15:30 2008 +0800 >>> +++ b/xen/drivers/acpi/hwregs.c Mon Aug 25 15:24:32 2008 +0800 >>> @@ -238,12 +238,6 @@ acpi_status acpi_set_register(u32 regist >>> break; >>> >>> case ACPI_REGISTER_PM2_CONTROL: >>> - >>> - status = acpi_hw_register_read(ACPI_REGISTER_PM2_CONTROL, >>> - ®ister_value); >>> - if (ACPI_FAILURE(status)) { >>> - goto unlock_and_exit; >>> - } >>> >>> ACPI_DEBUG_PRINT((ACPI_DB_IO, >>> "PM2 control: Read %X from %8.8X%8.8X\n", >>> _______________________________________________ >>> Xen-devel mailing list >>> Xen-devel@lists.xensource.com >>> http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Good suggestion. I will try it. Jimmy On Wednesday, September 03, 2008 7:44 PM, Keir Fraser wrote:> Agreed. Linux has the same double-read by the way, as recently as > 2.6.26-rc4. I hope it''s not for a particular reason, and is just a typo. You > might send your patch upstream to Linux if it is still not fixed there. > > -- Keir > > On 3/9/08 11:44, "Wei, Gang" <gang.wei@intel.com> wrote: > >> acpi_set_register() has already done a read before switch-case block, this >> read with in switch-case is a unnecessary repeat. >> >> The non-continuous PIO/MMIO access will spent far longer time than >> imagination because of the chipset link ASPM features. One single PIO/MMIO >> read may spent up to tens of thousands cpu cycles, although the fastest read >> may only cause less than one thousand cycles. I tried to measure the cost >> via TSC. >> >> Jimmy >> >> On Wednesday, September 03, 2008 4:43 PM, Keir Fraser wrote: >>> acpi_set_register() is doing a read-modify-write of PM2_CONTROL. It''s only >>> safe to skip the read if the modification is going to obliterate all old >>> bits. Is the read really that expensive (and equally, in patch 3, is the >>> write of ARB_DIS really that expensive)? >>> >>> -- Keir >>> >>> On 3/9/08 02:52, "Wei, Gang" <gang.wei@intel.com> wrote: >>> >>>> ACPI: Remove a redundant call to acpi_hw_register_read(). >>>> >>>> Signed-off-by: Wei Gang <gang.wei@intel.com> >>>> >>>> diff -r fc0b0c64246d xen/drivers/acpi/hwregs.c >>>> --- a/xen/drivers/acpi/hwregs.c Thu Aug 21 16:15:30 2008 +0800 >>>> +++ b/xen/drivers/acpi/hwregs.c Mon Aug 25 15:24:32 2008 +0800 >>>> @@ -238,12 +238,6 @@ acpi_status acpi_set_register(u32 regist >>>> break; >>>> >>>> case ACPI_REGISTER_PM2_CONTROL: >>>> - >>>> - status = acpi_hw_register_read(ACPI_REGISTER_PM2_CONTROL, >>>> - ®ister_value); >>>> - if (ACPI_FAILURE(status)) { >>>> - goto unlock_and_exit; >>>> - } >>>> >>>> ACPI_DEBUG_PRINT((ACPI_DB_IO, >>>> "PM2 control: Read %X from %8.8X%8.8X\n", >>>> _______________________________________________ >>>> Xen-devel mailing list >>>> Xen-devel@lists.xensource.com >>>> http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel