diff mbox series

[v2,2/2] main: Address by linux CPU number with PPC host backend

Message ID 20180713062106.31114-2-mikey@neuling.org
State Superseded
Headers show
Series None | expand

Commit Message

Michael Neuling July 13, 2018, 6:21 a.m. UTC
With the PPC host backend used for HTM it's difficult to match up the
hardware numbers used pdbg with linux CPU numbers that people want to
affinitise a workload against (ie. taskset -c <cpu number>).

This adds a new "-l <cpu>" options so users can address the CPU to
target using linux CPU numbers. This is only available when using the
host backend on POWER machines.

Signed-off-by: Michael Neuling <mikey@neuling.org>
--
v2:
  Fix POWER9
  rebase on upstream
  Reduce #ifdef ugliness
---
 src/main.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 100 insertions(+), 2 deletions(-)

Comments

Amitay Isaacs July 13, 2018, 6:34 a.m. UTC | #1
On Fri, 2018-07-13 at 16:21 +1000, Michael Neuling wrote:
> With the PPC host backend used for HTM it's difficult to match up the
> hardware numbers used pdbg with linux CPU numbers that people want to
> affinitise a workload against (ie. taskset -c <cpu number>).
> 
> This adds a new "-l <cpu>" options so users can address the CPU to
> target using linux CPU numbers. This is only available when using the
> host backend on POWER machines.

Is -l mutually exlusive with -p?

Or you expect "... -p 13 -l 1 ... " to work?

If they are mutually exclusive, we can just map l_list to p_list via
pir_map().


> 
> Signed-off-by: Michael Neuling <mikey@neuling.org>
> --
> v2:
>   Fix POWER9
>   rebase on upstream
>   Reduce #ifdef ugliness
> ---
>  src/main.c | 102
> +++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 100 insertions(+), 2 deletions(-)
> 
> diff --git a/src/main.c b/src/main.c
> index 2b348208fb..2dcb3359c1 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -139,6 +139,9 @@ static void print_usage(char *pname)
>  	printf("\t-p, --processor=<0-%d>|<range>|<list>\n",
> MAX_PROCESSORS-1);
>  	printf("\t-c, --chip=<0-%d>|<range>|<list>\n", MAX_CHIPS-1);
>  	printf("\t-t, --thread=<0-%d>|<range>|<list>\n",
> MAX_THREADS-1);
> +#ifdef TARGET_PPC
> +	printf("\t-l, --cpu=<0-%d>|<range>|<list>\n",
> MAX_PROCESSORS-1);
> +#endif
>  	printf("\t-a, --all\n");
>  	printf("\t\tRun command on all possible
> processors/chips/threads (default)\n");
>  	printf("\t-b, --backend=backend\n");
> @@ -242,6 +245,68 @@ static bool parse_list(const char *arg, int max,
> int *list, int *count)
>  	return true;
>  }
>  
> +#ifdef TARGET_PPC
> +int get_pir(int linux_cpu)
> +{
> +	char *filename;
> +	FILE *file;
> +	int pir = -1;
> +
> +	if(asprintf(&filename, "/sys/devices/system/cpu/cpu%i/pir",
> +		    linux_cpu) < 0)
> +		return -1;
> +
> +	file = fopen(filename, "r");
> +	if (!file) {
> +		PR_ERROR("Invalid Linux CPU number %" PRIi32 "\n",
> linux_cpu);
> +		goto out2;
> +	}
> +
> +	if(fscanf(file, "%" PRIx32 "\n", &pir) != 1) {
> +		PR_ERROR("fscanf() didn't match: %m\n");
> +		pir = -1;
> +		goto out1;
> +	}
> +
> +out1:
> +	fclose(file);
> +out2:
> +	free(filename);
> +	return pir;
> +}
> +
> +/* Stolen from skiboot */
> +#define P9_PIR2GCID(pir) (((pir) >> 8) & 0x7f)
> +#define P9_PIR2COREID(pir) (((pir) >> 2) & 0x3f)
> +#define P9_PIR2THREADID(pir) ((pir) & 0x3)
> +#define P8_PIR2GCID(pir) (((pir) >> 7) & 0x3f)
> +#define P8_PIR2COREID(pir) (((pir) >> 3) & 0xf)
> +#define P8_PIR2THREADID(pir) ((pir) & 0x7)
> +
> +void pir_map(int pir, int *chip, int *core, int *thread)
> +{
> +	assert(chip && core && thread);
> +
> +	if (!strncmp(device_node, "p9", 2)) {
> +		*chip = P9_PIR2GCID(pir);
> +		*core = P9_PIR2COREID(pir);
> +		*thread = P9_PIR2THREADID(pir);
> +	} else if (!strncmp(device_node, "p8", 2)) {
> +		*chip = P8_PIR2GCID(pir);
> +		*core = P8_PIR2COREID(pir);
> +		*thread = P8_PIR2THREADID(pir);
> +	} else
> +		assert(0);
> +
> +}
> +
> +#define PPC_OPTS "l:"
> +#else
> +int get_pir(int linux_cpu) { return -1; }
> +void pir_map(int pir, int *chip, int *core, int *thread) {}
> +#define PPC_OPTS
> +#endif
> +
>  static bool parse_options(int argc, char *argv[])
>  {
>  	int c;
> @@ -249,7 +314,8 @@ static bool parse_options(int argc, char *argv[])
>  	int p_list[MAX_PROCESSORS];
>  	int c_list[MAX_CHIPS];
>  	int t_list[MAX_THREADS];
> -	int p_count = 0, c_count = 0, t_count = 0;
> +	int l_list[MAX_PROCESSORS];
> +	int p_count = 0, c_count = 0, t_count = 0, l_count = 0;
>  	int i, j, k;
>  	struct option long_opts[] = {
>  		{"all",			no_argument,		
> NULL,	'a'},
> @@ -260,6 +326,9 @@ static bool parse_options(int argc, char *argv[])
>  		{"processor",		required_argument,	
> NULL,	'p'},
>  		{"slave-address",	required_argument,	N
> ULL,	's'},
>  		{"thread",		required_argument,	
> NULL,	't'},
> +#ifdef TARGET_PPC
> +		{"cpu",			required_argument,	
> NULL,	'l'},
> +#endif
>  		{"debug",		required_argument,	N
> ULL,	'D'},
>  		{"version",		no_argument,		
> NULL,	'V'},
>  		{NULL,			0,			
> NULL,     0}
> @@ -269,9 +338,11 @@ static bool parse_options(int argc, char
> *argv[])
>  	memset(p_list, 0, sizeof(p_list));
>  	memset(c_list, 0, sizeof(c_list));
>  	memset(t_list, 0, sizeof(t_list));
> +	memset(l_list, 0, sizeof(l_list));
>  
>  	do {
> -		c = getopt_long(argc, argv, "+ab:c:d:hp:s:t:D:V",
> long_opts, NULL);
> +		c = getopt_long(argc, argv, "+ab:c:d:hp:s:t:D:V"
> PPC_OPTS,
> +				long_opts, NULL);
>  		if (c == -1)
>  			break;
>  
> @@ -317,6 +388,13 @@ static bool parse_options(int argc, char
> *argv[])
>  			}
>  			break;
>  
> +		case 'l':
> +			if (!parse_list(optarg, MAX_PROCESSORS,
> l_list, &l_count)) {
> +				fprintf(stderr, "Failed to parse '-l 
> %s'\n", optarg);
> +				opt_error = true;
> +			}
> +			break;
> +
>  		case 'b':
>  			if (strcmp(optarg, "fsi") == 0) {
>  				backend = FSI;
> @@ -403,6 +481,26 @@ static bool parse_options(int argc, char
> *argv[])
>  		}
>  	}
>  
> +	if (l_count) {
> +		int pir = -1, i, chip, core, thread;
> +
> +		for (i = 0; i < MAX_PROCESSORS; i++) {
> +			if (l_list[i] == 1) {
> +				pir = get_pir(i);
> +				if (pir < 0)
> +					return true;
> +				break;
> +			}
> +		}
> +		if (pir < 0)
> +			return true;
> +
> +		pir_map(pir, &chip, &core, &thread);
> +
> +		threadsel[chip][core][thread] = 1;
> +		chipsel[chip][core] =
> &threadsel[chip][core][thread];
> +		processorsel[chip] = &chipsel[chip][core];
> +	}
>  	return true;
>  }
>  
> -- 
> 2.17.1
> 

Amitay.
Michael Neuling July 15, 2018, 6:42 a.m. UTC | #2
On Fri, 2018-07-13 at 16:34 +1000, Amitay Isaacs wrote:
> On Fri, 2018-07-13 at 16:21 +1000, Michael Neuling wrote:
> > With the PPC host backend used for HTM it's difficult to match up the
> > hardware numbers used pdbg with linux CPU numbers that people want to
> > affinitise a workload against (ie. taskset -c <cpu number>).
> > 
> > This adds a new "-l <cpu>" options so users can address the CPU to
> > target using linux CPU numbers. This is only available when using the
> > host backend on POWER machines.
> 
> Is -l mutually exlusive with -p?

Yes, they are mutually exclusive

> Or you expect "... -p 13 -l 1 ... " to work?

I would not expect that to work but I don't have an explicit check for it right
now.  We should probably add something.

> If they are mutually exclusive, we can just map l_list to p_list via
> pir_map().

I think it l_list would need to map to p_list, c_list and t_list... not justp_list.  

I end up just doing the mapping from l_list to thread/chip/processor sel.

Mikey
Amitay Isaacs July 16, 2018, 2:35 a.m. UTC | #3
On Sun, 2018-07-15 at 16:42 +1000, Michael Neuling wrote:
> On Fri, 2018-07-13 at 16:34 +1000, Amitay Isaacs wrote:
> > On Fri, 2018-07-13 at 16:21 +1000, Michael Neuling wrote:
> > > With the PPC host backend used for HTM it's difficult to match up
> > > the
> > > hardware numbers used pdbg with linux CPU numbers that people
> > > want to
> > > affinitise a workload against (ie. taskset -c <cpu number>).
> > > 
> > > This adds a new "-l <cpu>" options so users can address the CPU
> > > to
> > > target using linux CPU numbers. This is only available when using
> > > the
> > > host backend on POWER machines.
> > 
> > Is -l mutually exlusive with -p?
> 
> Yes, they are mutually exclusive
> 
> > Or you expect "... -p 13 -l 1 ... " to work?
> 
> I would not expect that to work but I don't have an explicit check
> for it right
> now.  We should probably add something.
> 
> > If they are mutually exclusive, we can just map l_list to p_list
> > via
> > pir_map().
> 
> I think it l_list would need to map to p_list, c_list and t_list...
> not justp_list.  
> 
> I end up just doing the mapping from l_list to thread/chip/processor
> sel.

I realized that "-l <cpu>" should really be mapping to hardware threads
and not just hardware processors.

So l_list should be declared as:

   int l_list[MAX_PROCESSORS * MAX_CHIPS * MAX_THREADS];

It should enable appropriately selected processors/cores/threads.

Amitay.
Michael Neuling July 16, 2018, 6:59 a.m. UTC | #4
On Mon, 2018-07-16 at 12:35 +1000, Amitay Isaacs wrote:
> On Sun, 2018-07-15 at 16:42 +1000, Michael Neuling wrote:
> > On Fri, 2018-07-13 at 16:34 +1000, Amitay Isaacs wrote:
> > > On Fri, 2018-07-13 at 16:21 +1000, Michael Neuling wrote:
> > > > With the PPC host backend used for HTM it's difficult to match up
> > > > the
> > > > hardware numbers used pdbg with linux CPU numbers that people
> > > > want to
> > > > affinitise a workload against (ie. taskset -c <cpu number>).
> > > > 
> > > > This adds a new "-l <cpu>" options so users can address the CPU
> > > > to
> > > > target using linux CPU numbers. This is only available when using
> > > > the
> > > > host backend on POWER machines.
> > > 
> > > Is -l mutually exlusive with -p?
> > 
> > Yes, they are mutually exclusive
> > 
> > > Or you expect "... -p 13 -l 1 ... " to work?
> > 
> > I would not expect that to work but I don't have an explicit check
> > for it right
> > now.  We should probably add something.
> > 
> > > If they are mutually exclusive, we can just map l_list to p_list
> > > via
> > > pir_map().
> > 
> > I think it l_list would need to map to p_list, c_list and t_list...
> > not justp_list.  
> > 
> > I end up just doing the mapping from l_list to thread/chip/processor
> > sel.
> 
> I realized that "-l <cpu>" should really be mapping to hardware threads
> and not just hardware processors.
> 
> So l_list should be declared as:
> 
>    int l_list[MAX_PROCESSORS * MAX_CHIPS * MAX_THREADS];

Agreed.

> It should enable appropriately selected processors/cores/threads.

Can we also rename MAX_CHIPS to MAX_CORES?

Mikey
Amitay Isaacs July 16, 2018, 7:06 a.m. UTC | #5
On Mon, 2018-07-16 at 16:59 +1000, Michael Neuling wrote:
> On Mon, 2018-07-16 at 12:35 +1000, Amitay Isaacs wrote:
> > On Sun, 2018-07-15 at 16:42 +1000, Michael Neuling wrote:
> > > On Fri, 2018-07-13 at 16:34 +1000, Amitay Isaacs wrote:
> > > > On Fri, 2018-07-13 at 16:21 +1000, Michael Neuling wrote:
> > > > > With the PPC host backend used for HTM it's difficult to
> > > > > match up
> > > > > the
> > > > > hardware numbers used pdbg with linux CPU numbers that people
> > > > > want to
> > > > > affinitise a workload against (ie. taskset -c <cpu number>).
> > > > > 
> > > > > This adds a new "-l <cpu>" options so users can address the
> > > > > CPU
> > > > > to
> > > > > target using linux CPU numbers. This is only available when
> > > > > using
> > > > > the
> > > > > host backend on POWER machines.
> > > > 
> > > > Is -l mutually exlusive with -p?
> > > 
> > > Yes, they are mutually exclusive
> > > 
> > > > Or you expect "... -p 13 -l 1 ... " to work?
> > > 
> > > I would not expect that to work but I don't have an explicit
> > > check
> > > for it right
> > > now.  We should probably add something.
> > > 
> > > > If they are mutually exclusive, we can just map l_list to
> > > > p_list
> > > > via
> > > > pir_map().
> > > 
> > > I think it l_list would need to map to p_list, c_list and
> > > t_list...
> > > not justp_list.  
> > > 
> > > I end up just doing the mapping from l_list to
> > > thread/chip/processor
> > > sel.
> > 
> > I realized that "-l <cpu>" should really be mapping to hardware
> > threads
> > and not just hardware processors.
> > 
> > So l_list should be declared as:
> > 
> >    int l_list[MAX_PROCESSORS * MAX_CHIPS * MAX_THREADS];
> 
> Agreed.
> 
> > It should enable appropriately selected processors/cores/threads.
> 
> Can we also rename MAX_CHIPS to MAX_CORES?

We definily should.  However...

There is horrible overloading of chiplet array to address chiplets.

			/* This is kinda broken as we're overloading what '-c'
			 * means - it's now up to each command to select targets
			 * based on core/chiplet. We really need a better
			 * solution to target selection. */
			pdbg_for_each_target("chiplet", pib, chip) {

We need to figure out a way to target any "chiplet" rather than adding letters of alphabet for each type of chiplet.

Maybe use -p/-c/-t or -l for main targets (processor/chip/thread) and create a different scheme for other chiplets?

Amitay.
Michael Neuling July 18, 2018, 4:57 a.m. UTC | #6
On Tue, 2018-07-17 at 13:48 +1000, Alistair Popple wrote:
> > > Can we also rename MAX_CHIPS to MAX_CORES?
> > 
> > We definily should.  However...
> > 
> > There is horrible overloading of chiplet array to address chiplets.
> > 
> > 			/* This is kinda broken as we're overloading what '-c'
> > 			 * means - it's now up to each command to select targets
> > 			 * based on core/chiplet. We really need a better
> > 			 * solution to target selection. */
> > 			pdbg_for_each_target("chiplet", pib, chip) {
> > 
> > We need to figure out a way to target any "chiplet" rather than adding letters of alphabet for each type of chiplet.
> 
> Agreed. I guess that's what your interactive path selection patches are going to
> provide.

Ok I'll let you guys fix that :-)

I'll repost with the 'int l_list[MAX_PROCESSORS * MAX_THREADS * MAX_CHIPS]' fix
in v3.

> > Maybe use -p/-c/-t or -l for main targets (processor/chip/thread) and create a different scheme for other chiplets?
> 
> Sounds good to me. I think we can drop the horrid overloading, I doubt anyone is
> using it. It crept in there mostly for testing purposes.

+1 

Mikey
diff mbox series

Patch

diff --git a/src/main.c b/src/main.c
index 2b348208fb..2dcb3359c1 100644
--- a/src/main.c
+++ b/src/main.c
@@ -139,6 +139,9 @@  static void print_usage(char *pname)
 	printf("\t-p, --processor=<0-%d>|<range>|<list>\n", MAX_PROCESSORS-1);
 	printf("\t-c, --chip=<0-%d>|<range>|<list>\n", MAX_CHIPS-1);
 	printf("\t-t, --thread=<0-%d>|<range>|<list>\n", MAX_THREADS-1);
+#ifdef TARGET_PPC
+	printf("\t-l, --cpu=<0-%d>|<range>|<list>\n", MAX_PROCESSORS-1);
+#endif
 	printf("\t-a, --all\n");
 	printf("\t\tRun command on all possible processors/chips/threads (default)\n");
 	printf("\t-b, --backend=backend\n");
@@ -242,6 +245,68 @@  static bool parse_list(const char *arg, int max, int *list, int *count)
 	return true;
 }
 
+#ifdef TARGET_PPC
+int get_pir(int linux_cpu)
+{
+	char *filename;
+	FILE *file;
+	int pir = -1;
+
+	if(asprintf(&filename, "/sys/devices/system/cpu/cpu%i/pir",
+		    linux_cpu) < 0)
+		return -1;
+
+	file = fopen(filename, "r");
+	if (!file) {
+		PR_ERROR("Invalid Linux CPU number %" PRIi32 "\n", linux_cpu);
+		goto out2;
+	}
+
+	if(fscanf(file, "%" PRIx32 "\n", &pir) != 1) {
+		PR_ERROR("fscanf() didn't match: %m\n");
+		pir = -1;
+		goto out1;
+	}
+
+out1:
+	fclose(file);
+out2:
+	free(filename);
+	return pir;
+}
+
+/* Stolen from skiboot */
+#define P9_PIR2GCID(pir) (((pir) >> 8) & 0x7f)
+#define P9_PIR2COREID(pir) (((pir) >> 2) & 0x3f)
+#define P9_PIR2THREADID(pir) ((pir) & 0x3)
+#define P8_PIR2GCID(pir) (((pir) >> 7) & 0x3f)
+#define P8_PIR2COREID(pir) (((pir) >> 3) & 0xf)
+#define P8_PIR2THREADID(pir) ((pir) & 0x7)
+
+void pir_map(int pir, int *chip, int *core, int *thread)
+{
+	assert(chip && core && thread);
+
+	if (!strncmp(device_node, "p9", 2)) {
+		*chip = P9_PIR2GCID(pir);
+		*core = P9_PIR2COREID(pir);
+		*thread = P9_PIR2THREADID(pir);
+	} else if (!strncmp(device_node, "p8", 2)) {
+		*chip = P8_PIR2GCID(pir);
+		*core = P8_PIR2COREID(pir);
+		*thread = P8_PIR2THREADID(pir);
+	} else
+		assert(0);
+
+}
+
+#define PPC_OPTS "l:"
+#else
+int get_pir(int linux_cpu) { return -1; }
+void pir_map(int pir, int *chip, int *core, int *thread) {}
+#define PPC_OPTS
+#endif
+
 static bool parse_options(int argc, char *argv[])
 {
 	int c;
@@ -249,7 +314,8 @@  static bool parse_options(int argc, char *argv[])
 	int p_list[MAX_PROCESSORS];
 	int c_list[MAX_CHIPS];
 	int t_list[MAX_THREADS];
-	int p_count = 0, c_count = 0, t_count = 0;
+	int l_list[MAX_PROCESSORS];
+	int p_count = 0, c_count = 0, t_count = 0, l_count = 0;
 	int i, j, k;
 	struct option long_opts[] = {
 		{"all",			no_argument,		NULL,	'a'},
@@ -260,6 +326,9 @@  static bool parse_options(int argc, char *argv[])
 		{"processor",		required_argument,	NULL,	'p'},
 		{"slave-address",	required_argument,	NULL,	's'},
 		{"thread",		required_argument,	NULL,	't'},
+#ifdef TARGET_PPC
+		{"cpu",			required_argument,	NULL,	'l'},
+#endif
 		{"debug",		required_argument,	NULL,	'D'},
 		{"version",		no_argument,		NULL,	'V'},
 		{NULL,			0,			NULL,     0}
@@ -269,9 +338,11 @@  static bool parse_options(int argc, char *argv[])
 	memset(p_list, 0, sizeof(p_list));
 	memset(c_list, 0, sizeof(c_list));
 	memset(t_list, 0, sizeof(t_list));
+	memset(l_list, 0, sizeof(l_list));
 
 	do {
-		c = getopt_long(argc, argv, "+ab:c:d:hp:s:t:D:V", long_opts, NULL);
+		c = getopt_long(argc, argv, "+ab:c:d:hp:s:t:D:V" PPC_OPTS,
+				long_opts, NULL);
 		if (c == -1)
 			break;
 
@@ -317,6 +388,13 @@  static bool parse_options(int argc, char *argv[])
 			}
 			break;
 
+		case 'l':
+			if (!parse_list(optarg, MAX_PROCESSORS, l_list, &l_count)) {
+				fprintf(stderr, "Failed to parse '-l %s'\n", optarg);
+				opt_error = true;
+			}
+			break;
+
 		case 'b':
 			if (strcmp(optarg, "fsi") == 0) {
 				backend = FSI;
@@ -403,6 +481,26 @@  static bool parse_options(int argc, char *argv[])
 		}
 	}
 
+	if (l_count) {
+		int pir = -1, i, chip, core, thread;
+
+		for (i = 0; i < MAX_PROCESSORS; i++) {
+			if (l_list[i] == 1) {
+				pir = get_pir(i);
+				if (pir < 0)
+					return true;
+				break;
+			}
+		}
+		if (pir < 0)
+			return true;
+
+		pir_map(pir, &chip, &core, &thread);
+
+		threadsel[chip][core][thread] = 1;
+		chipsel[chip][core] = &threadsel[chip][core][thread];
+		processorsel[chip] = &chipsel[chip][core];
+	}
 	return true;
 }