Messages in this thread | | | Date | Fri, 21 Mar 2008 02:17:57 -0700 | From | "Yinghai Lu" <> | Subject | Re: [patch 1/4] x86: vSMP: Fix is_vsmp_box() |
| |
On Fri, Mar 21, 2008 at 2:11 AM, Ingo Molnar <mingo@elte.hu> 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 - because it's such a simple check? > > OTOH, you are right in general, for example in mmconf-fam10h_64.c's > get_fam10h_pci_mmconf_base() function, we do this: > > id = read_pci_config(bus, slot, 0, PCI_VENDOR_ID); > > vendor = id & 0xffff; > device = (id>>16) & 0xffff; > if (pci_probes[i].vendor == vendor && > pci_probes[i].device == device) { > > here it would indeed be cleaner to simply do: > > vendor = read_pci_config_16(bus, slot, 0, PCI_VENDOR_ID); > device = read_pci_config_16(bus, slot, 0, PCI_DEVICE_ID); > > instead of open-coding the 16-bit splitting. > > Ingo > > ------------> > Subject: x86: vsmp fix x86 vsmp fix is vsmp box cleanup > From: Ingo Molnar <mingo@elte.hu> > Date: Fri Mar 21 09:55:06 CET 2008 > > 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
good evidence.
YH
| |