| Message ID | 20180713062106.31114-2-mikey@neuling.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
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.
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
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.
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
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.
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 --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; }
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(-)