cpumask move patch revised - RFC
R Sharada
sharada at in.ibm.com
Fri Aug 13 14:30:00 EST 2004
Hello Joel,
Thanks for your feedback and comments. Responses inline
I shall send out a revised patch soon.
On Thu, Aug 12, 2004 at 02:05:03PM -0500, Joel Schopp wrote:
> This will surely conflict with Nathan's recent patch "[patch 4/4] Remove
> unnecessary cpu maps (available, present_at_boot)". I think Nathan's
> patch should go in first and yours reworked to match. Other comments
> inline below.
Yes, you are correct. I did see Nathan's patch on the removal of the
unnecessary cpu maps. And yes, I am waiting for his patch to go first
and then have this reworked to match that change.
>
> R Sharada wrote:
>
> >Hello,
> > Based on the feedback, here is the revised cpumask patch that
> >moves the cpumask initialization from prom_hold_cpus() to later boot, in
> >setup_system().
> >The patch is against the 2.6.8-rc2 linus bitkeeper tree.
> >
> >- The get_property call has been corrected to obtain the property size from
> >the correct argument.
> >- The unnecessary variable initializations have been removed
> >- check for NULL value of status incorporated.
> >
> >I have not removed the #ifdefs for SMP, as all the cpumask data structures,
> >as I see them in code now, are defined for SMP systems and does not seem
> >to be
> >defined for UP.
> >The merge of the POWERMAC and PSERIES #ifdefs is also deferred as I don't
> >know a lot about the POWERMAC initialization and startup to see if the two
> >cases can be merged.
> >
> >Please review and comment on the patch.
> >
> >Thanks and Regards,
> >Sharada
> >
> >
> >------------------------------------------------------------------------
> >
> >diff -Naur linux-2.6.8-rc2-org/arch/ppc64/kernel/chrp_setup.c
> >linux-2.6.8-rc2-chg/arch/ppc64/kernel/chrp_setup.c
> >--- linux-2.6.8-rc2-org/arch/ppc64/kernel/chrp_setup.c 2004-08-03
> >02:12:58.000000000 -0700
> >+++ linux-2.6.8-rc2-chg/arch/ppc64/kernel/chrp_setup.c 2004-08-13
> >06:02:22.808964544 -0700
> >@@ -77,6 +77,8 @@
> > void pSeries_calibrate_decr(void);
> > void fwnmi_init(void);
> > extern void SystemReset_FWNMI(void), MachineCheck_FWNMI(void); /*
> > from head.S */
> >+void cpumask_setup(void);
> >+
>
> Is this really necessary? Might it go better in a .h file somewhere?
Well, yes, perhaps it could be put in some .h file. However, the idea here
was that, I just followed the conventions for other functions in chrp_setup.c
file
>
> > int fwnmi_active; /* TRUE if an FWNMI handler is present */
> >
> > dev_t boot_dev;
> >@@ -468,3 +470,92 @@
> >
> > setup_default_decr();
> > }
> >+
> >+void cpumask_setup()
> >+{
> >+ unsigned long ind;
> >+ struct device_node *np = NULL;
> >+ int cpuid = 0;
> >+ unsigned int *reg;
> >+ char *statusp;
> >+ int prop;
> >+ int *propsize = ∝
> >+ unsigned int cpu_threads;
> >+
> >+ printk(KERN_INFO "cpumask_setup\n");
> >+ /* On pmac, we just fill out the various global bitmasks and
> >+ * arrays indicating our CPUs are here, they are actually started
> >+ * later on from pmac_smp
> >+ */
> >+ if (systemcfg->platform == PLATFORM_POWERMAC) {
> >+ while ((np = of_find_node_by_type(np, "cpu"))) {
> >+ reg = (unsigned int *)get_property(np, "reg", NULL);
> >+#ifdef CONFIG_SMP
> >+ cpu_set(cpuid, cpu_available_map);
> >+ cpu_set(cpuid, cpu_possible_map);
> >+ cpu_set(cpuid, cpu_present_at_boot);
> >+ if (*reg == 0)
> >+ cpu_set(cpuid, cpu_online_map);
> >+#endif /* CONFIG_SMP */
> >+ cpuid++;
> >+ }
>
> Shouldn't the whole while loop and of_node_put be in the #ifdef
> CONFIG_SMP, as otherwise all we do is iterate over the cpus not doing
> anything?
Hmm.. Well, yes, I suppose we could move all of the while loop under #ifdef
SMP. The idea as I understood it was that the cpumask data structures are
defined for SMP alone and hencel flanked within the #ifdef SMP.
But looking at it again, perhaps what you are suggesting is a good idea. I
don't see the while loop doing anything else, anyways, in the non-SMP case.
As regards the of_node_put, discussing with Nathan, I realized that it isn't
really necessary, even for the last cpu node data structure in the while
loop. So, this of_node_put will be gone soon, in the next patch.
>
> >+ of_node_put(np);
> >+ return;
> >+ }
> >+
> >+ while ((np = of_find_node_by_type(np, "cpu"))) {
> >+
> >+ statusp = (char *)get_property(np, "status", NULL);
> >+ if ((statusp == NULL) || (statusp && strcmp(statusp, "okay")
> >!= 0))
> >+ continue;
> >+
> >+ reg = (unsigned int *)get_property(np, "reg", NULL);
> >+
> >+ get_property(np, "ibm,ppc-interrupt-server#s", propsize);
> >+ if (*propsize < 0) {
> >+ /* no property. old hardware has no SMT */
> >+ cpu_threads = 1;
> >+ } else {
> >+ /* We have a threaded processor */
> >+ cpu_threads = *propsize / sizeof(u32);
> >+ if (cpu_threads > 2)
> >+ cpu_threads = 1; /* ToDo: panic? */
>
> I think it is about time we start making code that will deal with more
> than 2 cpu_threads, as the processors seem inevitable and not too far off.
>
So, can SMT/HMT have more than 2 threads now? or planned in the near future?
> >+ }
> >+
> >+#ifdef CONFIG_SMP
> >+ cpu_set(cpuid, cpu_available_map);
> >+ cpu_set(cpuid, cpu_possible_map);
> >+ cpu_set(cpuid, cpu_present_at_boot);
> >+ if (cpuid == boot_cpuid)
> >+ cpu_set(cpuid, cpu_online_map);
> >+
> >+ /* set the secondary threads into the cpuid mask */
> >+ for (ind=1; ind < cpu_threads; ind++) {
> >+ cpuid++;
> >+ if (cpuid >= NR_CPUS)
> >+ continue;
> >+ if (naca->smt_state) {
> >+ cpu_set(cpuid, cpu_available_map);
> >+ cpu_set(cpuid, cpu_present_at_boot);
> >+ }
> >+ }
> >+#endif /* CONFIG_SMP */
>
> Again I'd have the CONFIG_SMP cover more. The whole while loop and the
> of_node_put.
>
However, here we still need to be able to check cpu node status and
interrupt-server#s property, etc. for non-SMP (UP) systems as well,
is it not? In that case, we can't really move the while loop inside the
#ifdef SMP, can we?
The case that you are talking about ( iterating over the cpus and not doing
anything ) would occur only in the case of a SMP machine running a UP
kernel, is it not? That seems unlikely? Or are there other scenarios?
> >+ cpuid++;
> >+ }
> >+ of_node_put(np);
> >+
> >+#ifdef CONFIG_HMT
> >+ /* Only enable HMT on processors that provide support. */
> >+ if (__is_processor(PV_PULSAR) ||
> >+ __is_processor(PV_ICESTAR) ||
> >+ __is_processor(PV_SSTAR)) {
> >+
> >+ for (ind = 0; ind < NR_CPUS; ind += 2) {
> >+ if (!cpu_online(ind))
> >+ continue;
> >+ cpu_set(ind+1, cpu_possible_map);
> >+ }
> >+ }
> >+#endif
> >+ return;
> >+}
> >diff -Naur linux-2.6.8-rc2-org/arch/ppc64/kernel/prom.c
> >linux-2.6.8-rc2-chg/arch/ppc64/kernel/prom.c
> >--- linux-2.6.8-rc2-org/arch/ppc64/kernel/prom.c 2004-08-04
> >06:10:30.000000000 -0700
> >+++ linux-2.6.8-rc2-chg/arch/ppc64/kernel/prom.c 2004-08-12
> >23:52:47.000000000 -0700
> >@@ -939,13 +939,6 @@
> > prom_getprop(node, "reg", ®, sizeof(reg));
> > lpaca[cpuid].hw_cpu_id = reg;
> >
> >-#ifdef CONFIG_SMP
> >- cpu_set(cpuid, RELOC(cpu_available_map));
> >- cpu_set(cpuid, RELOC(cpu_possible_map));
> >- cpu_set(cpuid, RELOC(cpu_present_at_boot));
> >- if (reg == 0)
> >- cpu_set(cpuid, RELOC(cpu_online_map));
> >-#endif /* CONFIG_SMP */
> > cpuid++;
> > }
> > return;
> >@@ -1042,9 +1035,6 @@
> > #ifdef CONFIG_SMP
> > /* Set the number of active processors. */
> > _systemcfg->processorCount++;
> >- cpu_set(cpuid, RELOC(cpu_available_map));
> >- cpu_set(cpuid, RELOC(cpu_possible_map));
> >- cpu_set(cpuid, RELOC(cpu_present_at_boot));
> > #endif
> > } else {
> > prom_printf("... failed: %x\n",
> > *acknowledge);
> >@@ -1053,10 +1043,6 @@
> > #ifdef CONFIG_SMP
> > else {
> > prom_printf("%x : booting cpu %s\n", cpuid, path);
> >- cpu_set(cpuid, RELOC(cpu_available_map));
> >- cpu_set(cpuid, RELOC(cpu_possible_map));
> >- cpu_set(cpuid, RELOC(cpu_online_map));
> >- cpu_set(cpuid, RELOC(cpu_present_at_boot));
> > }
> > #endif
> > next:
> >@@ -1069,13 +1055,6 @@
> > lpaca[cpuid].hw_cpu_id = interrupt_server[i];
> > prom_printf("%x : preparing thread ... ",
> > interrupt_server[i]);
> >- if (_naca->smt_state) {
> >- cpu_set(cpuid, RELOC(cpu_available_map));
> >- cpu_set(cpuid, RELOC(cpu_present_at_boot));
> >- prom_printf("available\n");
> >- } else {
> >- prom_printf("not available\n");
> >- }
> > }
> > #endif
> > cpuid++;
> >@@ -1101,8 +1080,6 @@
> > pir & 0x3ff;
> > }
> > }
> >-/* cpu_set(i+1, cpu_online_map); */
> >- cpu_set(i+1, RELOC(cpu_possible_map));
> > }
> > _systemcfg->processorCount *= 2;
> > } else {
> >diff -Naur linux-2.6.8-rc2-org/arch/ppc64/kernel/setup.c
> >linux-2.6.8-rc2-chg/arch/ppc64/kernel/setup.c
> >--- linux-2.6.8-rc2-org/arch/ppc64/kernel/setup.c 2004-08-03
> >02:12:59.000000000 -0700
> >+++ linux-2.6.8-rc2-chg/arch/ppc64/kernel/setup.c 2004-08-04
> >06:15:27.000000000 -0700
> >@@ -76,6 +76,7 @@
> > extern void pseries_secondary_smp_init(unsigned long);
> > extern int idle_setup(void);
> > extern void vpa_init(int cpu);
> >+extern void cpumask_setup(void);
>
> Could this go in a .h file somewhere?
Again, the conventions used for other functions in the file was used for this
function declaration.
Was it the idea behind declaring this way, that we declare the functions only
in the required source files and do away stacking up too many declarations
of functions (that are not called from many places anyways) in common .h
files?
>
> >
> > unsigned long decr_overclock = 1;
> > unsigned long decr_overclock_proc0 = 1;
> >@@ -229,6 +230,7 @@
> > register_console(&udbg_console);
> > __irq_offset_value = NUM_ISA_INTERRUPTS;
> > finish_device_tree();
> >+ cpumask_setup();
> > chrp_init(r3, r4, r5, r6, r7);
> >
> > #ifdef CONFIG_SMP
> >@@ -251,6 +253,7 @@
> > #ifdef CONFIG_PPC_PMAC
> > if (systemcfg->platform == PLATFORM_POWERMAC) {
> > finish_device_tree();
> >+ cpumask_setup();
> > pmac_init(r3, r4, r5, r6, r7);
> > }
> > #endif /* CONFIG_PPC_PMAC */
Thanks and Regards,
Sharada
** Sent via the linuxppc64-dev mail list. See http://lists.linuxppc.org/
More information about the Linuxppc64-dev
mailing list