Sparc64, cpumask_t and struct arguments (was: [PATCH] Introducenodemask_t ADT)

[Paul Jackson]

David,

I understand that you are the Sparc64 expert.  I am doing some more
work refining the cpumask_t (and nodemask_t) implementation, in an
effort to obtain a common "mask_t" underlying ADT that can be useful
for both cpu and node masks.

As part of this, I'm looking at having only one data representation
for these masks - an array of unsigned longs encapsulated inside a
struct.  For the processors and compilation environment that I am
most focused on - gcc 3.2 and above on Intel CPUs, this generates
just as good code as the other variants.

However, as I recall, you recommend against passing structs on the
Sparc64 processor and compilation environment.  Am I recalling
correctly?

The two places that I see sparc64 passing a cpumask_t across a real
function call boundary (as opposed to an inline function) is in the file
arch/sparc64/kernel/smp.c, by the functions cheetah_xcall_deliver() and
smp_cross_call_masked().

Could you describe to me the requirements here.

 1) Are the above the two places of interest, currently, for sparc64 work?

 2) What sizes of cpumask_t are needed?

 3) Which of passing by pointer, passing as a struct, and/or passing by
    simple value, are required or acceptable or unacceptable?

 4) Does your answer to (3) vary with the sizes needed in (2)?

 5) The cpu_clear(i, mask) on about line 534 of smp.c confuses me, as it
    seems to be changing a local copy of 'mask' that no one will examine
    later.  What purpose does it serve?  See this line annotated with
    "No affect??" in the changes, below.

 6) I'm not done yet, but so far I have the following changes in my
    workarea for sparc64 (along with larger changes elsewhere).  These
    changes are untested, unbuilt, incomplete and unreviewed.  They
    were generated on a 2.6.4 base, with Matthew Dobson's nodemask_t
    patch of this thread applied.  Do you see anything that looks
    wrong in the following so far?

===== smp.c 1.66 vs edited =====
--- 1.66/arch/sparc64/kernel/smp.c      Tue Feb 24 16:11:02 2004
+++ edited/smp.c        Fri Mar 26 14:01:23 2004
@@ -409,14 +409,8 @@
        int i;

        __asm__ __volatile__("rdpr %%pstate, %0" : "=r" (pstate));
-       for (i = 0; i < NR_CPUS; i++) {
-               if (cpu_isset(i, mask)) {
-                       spitfire_xcall_helper(data0, data1, data2, pstate, i);
-                       cpu_clear(i, mask);
-                       if (cpus_empty(mask))
-                               break;
-               }
-       }
+       for_each_cpu_mask(i, mask)
+               spitfire_xcall_helper(data0, data1, data2, pstate, i);
 }

 /* Cheetah now allows to send the whole 64-bytes of data in the interrupt
@@ -459,25 +453,19 @@

        nack_busy_id = 0;
        {
-               cpumask_t work_mask = mask;
                int i;

-               for (i = 0; i < NR_CPUS; i++) {
-                       if (cpu_isset(i, work_mask)) {
-                               u64 target = (i << 14) | 0x70;
-
-                               if (!is_jalapeno)
-                                       target |= (nack_busy_id << 24);
-                               __asm__ __volatile__(
-                                       "stxa   %%g0, [%0] %1\n\t"
-                                       "membar #Sync\n\t"
-                                       : /* no outputs */
-                                       : "r" (target), "i" (ASI_INTR_W));
-                               nack_busy_id++;
-                               cpu_clear(i, work_mask);
-                               if (cpus_empty(work_mask))
-                                       break;
-                       }
+               for_each_cpu_mask(i, mask) {
+                       u64 target = (i << 14) | 0x70;
+
+                       if (!is_jalapeno)
+                               target |= (nack_busy_id << 24);
+                       __asm__ __volatile__(
+                               "stxa   %%g0, [%0] %1\n\t"
+                               "membar #Sync\n\t"
+                               : /* no outputs */
+                               : "r" (target), "i" (ASI_INTR_W));
+                       nack_busy_id++;
                }
        }

@@ -521,6 +509,8 @@
                        /* Clear out the mask bits for cpus which did not
                         * NACK us.
                         */
+                       /* Could replace with for_each_cpu_mask(i, mask) loop,
+                        * were it not for the cpu_clear(i, mask) below. -pj */
                        for (i = 0; i < NR_CPUS; i++) {
                                if (cpu_isset(i, work_mask)) {
                                        u64 check_mask;
@@ -531,7 +521,7 @@
                                                check_mask = (0x2UL <<
                                                              this_busy_nack);
                                        if ((dispatch_stat & check_mask) == 0)
-                                               cpu_clear(i, mask);
+                                               cpu_clear(i, mask); /* No affect?? -pj */
                                        this_busy_nack += 2;
                                        cpu_clear(i, work_mask);
                                        if (cpus_empty(work_mask))


Thank-you for your considerations.

-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@xxxxxxx> 1.650.933.1373
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/