Messages in this thread | | | Date | Fri, 21 Mar 2008 10:59:27 -0700 | From | Ravikiran G Thirumalai <> | Subject | Re: [patch 1/4] x86: vSMP: Fix is_vsmp_box() |
| |
On Fri, Mar 21, 2008 at 10:11:39AM +0100, Ingo Molnar wrote: > >* Ravikiran G Thirumalai <kiran@scalex86.org> wrote: > >> >> /* Check if we are running on a ScaleMP vSMP box */ >> >> - if (read_pci_config(0, 0x1f, 0, PCI_VENDOR_ID) == >> >> - (PCI_VENDOR_ID_SCALEMP || (PCI_DEVICE_ID_SCALEMP_VSMP_CTL << 16))) >> >> + if ((read_pci_config_16(0, 0x1f, 0, PCI_VENDOR_ID) == >> >> + PCI_VENDOR_ID_SCALEMP) && >> >> + (read_pci_config_16(0, 0x1f, 0, PCI_DEVICE_ID) == >> >> + PCI_DEVICE_ID_SCALEMP_VSMP_CTL)) >> >> vsmp = 1; >> >> >> >> return vsmp; >> > >> >why read two times >> > >> >> Well, the pci cfg space read happens just _once_ during the boot, as >> the result is cached in a static flag. The above code is better >> readable. So readability is better than micro-optimization here. > >i think the patch below results in even more readable and a bit smaller > >code got a bit smaller: > >arch/x86/kernel/vsmp_64.o: > > text data bss dec hex filename > 205 4 0 209 d1 vsmp_64.o.before > 181 4 0 185 b9 vsmp_64.o.after
Wow! good to know that avoiding one call shaved so many bytes. Agreed, the below patch is better.
Thanks, Kiran
> >Signed-off-by: Ingo Molnar <mingo@elte.hu> >--- > arch/x86/kernel/vsmp_64.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > >Index: linux-x86.q/arch/x86/kernel/vsmp_64.c >=================================================================== >--- linux-x86.q.orig/arch/x86/kernel/vsmp_64.c >+++ linux-x86.q/arch/x86/kernel/vsmp_64.c >@@ -120,10 +120,8 @@ int is_vsmp_box(void) > return vsmp; > > /* Check if we are running on a ScaleMP vSMP box */ >- if ((read_pci_config_16(0, 0x1f, 0, PCI_VENDOR_ID) == >- PCI_VENDOR_ID_SCALEMP) && >- (read_pci_config_16(0, 0x1f, 0, PCI_DEVICE_ID) == >- PCI_DEVICE_ID_SCALEMP_VSMP_CTL)) >+ if (read_pci_config(0, 0x1f, 0, PCI_VENDOR_ID) == >+ (PCI_VENDOR_ID_SCALEMP | (PCI_DEVICE_ID_SCALEMP_VSMP_CTL << 16))) > vsmp = 1; > > return vsmp;
| |