Re: [PATCH 05/11] x86 topology: export die_siblings

[Liang, Kan]

On 2/19/2019 1:43 PM, Brown, Len wrote:
> Thanks for the comments, Kan,
>
> > > diff --git a/Documentation/cputopology.txt
> > > b/Documentation/cputopology.txt index 287213b4517b..7dd2ae3df233
> > > 100644
> > > --- a/Documentation/cputopology.txt
> > > +++ b/Documentation/cputopology.txt
> > > @@ -56,6 +56,16 @@ core_siblings_list:
> > >    	human-readable list of cpuX's hardware threads within the same
> > >    	die_id.
> > >    
> > > +die_siblings:
> > > +
> > > +	internal kernel map of cpuX's hardware threads within the same
> > > +	physical_package_id.
> > > +
> > > +die_siblings_list:
> > > +
> > > +	human-readable list of cpuX's hardware threads within the same
> > > +	physical_package_id.
> > > +
> > >    book_siblings:
> > >    
> > >    	internal kernel map of cpuX's hardware threads within the same diff
>
> > Could you please update the document regarding to topology_die_cpumask and topology_core_cpumask in Documentation/x86/topology.txt
>   
> I agree that the top part of this file, as updated above, should document the external sysfs interface...
>
> I'm less excited about the center of the file trying to document the internal implementation -- as the source code
> is actually more clear than the document, but here is an update, consistent with the existing file.
> Let me know if it does not fully address your comment.

Besides the generic document, I think we should update x86 document as 
well, which is in Documentation/x86/topology.txt.

The definition of topology_core_cpumask has to be changed to per die, right?

diff --git a/Documentation/x86/topology.txt b/Documentation/x86/topology.txt
index 8107b6c..8698a41 100644
--- a/Documentation/x86/topology.txt
+++ b/Documentation/x86/topology.txt
@@ -105,11 +105,16 @@ The topology of a system is described in the units of:

   Thread-related topology information in the kernel:

-  - topology_core_cpumask():
+  - topology_die_cpumask():

     The cpumask contains all online threads in the package to which a 
thread
     belongs.

+  - topology_core_cpumask():
+
+    The cpumask contains all online threads in the die to which a thread
+    belongs.
+
     The number of online threads is also printed in /proc/cpuinfo 
"siblings."

   - topology_sibling_cpumask():


Thanks,
Kan

> Thanks,
> -Len
> ---
>
> diff --git a/Documentation/cputopology.txt b/Documentation/cputopology.txt
> index 7dd2ae3..f39c759 100644
> --- a/Documentation/cputopology.txt
> +++ b/Documentation/cputopology.txt
> @@ -102,6 +102,7 @@ these macros in include/asm-XXX/topology.h::
>          #define topology_drawer_id(cpu)
>          #define topology_sibling_cpumask(cpu)
>          #define topology_core_cpumask(cpu)
> +       #define topology_die_cpumask(cpu)
>          #define topology_book_cpumask(cpu)
>          #define topology_drawer_cpumask(cpu)
>
> @@ -114,10 +115,11 @@ To be consistent on all architectures, include/linux/topology.h
>   provides default definitions for any of the above macros that are
>   not defined by include/asm-XXX/topology.h:
>
> -1) physical_package_id: -1
> -2) core_id: 0
> -3) sibling_cpumask: just the given CPU
> -4) core_cpumask: just the given CPU
> +1) topology_physical_package_id: -1
> +2) topology_core_id: 0
> +3) topology_sibling_cpumask: just the given CPU
> +4) topology_core_cpumask: just the given CPU
> +5) topology_die_cpumask: just the given CPU
>
>   For architectures that don't support books (CONFIG_SCHED_BOOK) there are no
>   default definitions for topology_book_id() and topology_book_cpumask().