| Message ID | 20180809042845.16580-1-npiggin@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | libpdbg: ensure thread state validity, only print requested targets | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | master/apply_patch Successfully applied |
| snowpatch_ozlabs/build-multiarch | success | Test build-multiarch on branch master |
Thanks Nick, On Thursday, 9 August 2018 2:28:45 PM AEST Nicholas Piggin wrote: > Thread state is not queried from the target each time, but cached. > This patch adds a valid flag in the thread state cache to ensure > this is being probed proprly before use. I really need to add some comments/documentation but there is already a pdbg_target_status() function which is supposed to indicate if a particular target has been initialised or not (and therefore if thread_status() is initialised). So rather than adding a target specific valid flag I think it would be best to just check that. There is also an implicit (ie. undocumented :-) rule that target functions other than target_probe() should only be called on targets with status == PDBG_TARGET_ENABLED. > This also skips threads that were not targetted when printing thread > status. This should already have been dealt with by the for_each_target calls in src/main.c. Ie: > @@ -28,6 +29,7 @@ static int print_thread_status(struct pdbg_target *target, uint32_t index, uint6 > struct thread_state *status = (struct thread_state *) arg; > > status[index] = thread_status(target); This should only get called for selected targets, so really the problem is that some elements of status[] may not be initialised due to the target not being selected/probed/found. > return 1; > } > > @@ -36,12 +38,19 @@ static int print_core_thread_status(struct pdbg_target *core_target, uint32_t in > struct thread_state status[8]; > int i, rc; > > + memset(status, 0, sizeof(status)); > + > printf("c%02d: ", index); > > /* TODO: This cast is gross. Need to rewrite for_each_child_target as an iterator. */ > rc = for_each_child_target("thread", core_target, print_thread_status, (uint64_t *) &status[0], NULL); > for (i = 0; i <= *maxindex; i++) { I think the correct fix is probably to use the second argument in the callback to detect if status[i] is initialised/selected/etc. Yes that TODO statement still needs to done :-) It would likely make this code much clearer. - Alistair > + if (!status[i].valid) { > + printf(" "); > + continue; > + } > + > if (status[i].active) > printf("A"); > else >
On Thu, 09 Aug 2018 16:46:09 +1000 Alistair Popple <alistair@popple.id.au> wrote: > Thanks Nick, > > On Thursday, 9 August 2018 2:28:45 PM AEST Nicholas Piggin wrote: > > Thread state is not queried from the target each time, but cached. > > This patch adds a valid flag in the thread state cache to ensure > > this is being probed proprly before use. > > I really need to add some comments/documentation but there is already a > pdbg_target_status() function which is supposed to indicate if a particular > target has been initialised or not (and therefore if thread_status() is > initialised). > > So rather than adding a target specific valid flag I think it would be best to > just check that. There is also an implicit (ie. undocumented :-) rule that > target functions other than target_probe() should only be called on targets with > status == PDBG_TARGET_ENABLED. Oh it's not the target status field but the thread_state one (which is of course target specific), so I think it's okay. Using the target status maybe works, but we pass around that thread_state structure without the target in some places, also could target status be used with things like ramming that wants to check status of non-target siblings? (not that I've added the valid check there, but I should) Thanks, Nick
On Thursday, 9 August 2018 5:53:14 PM AEST Nicholas Piggin wrote: > On Thu, 09 Aug 2018 16:46:09 +1000 > Alistair Popple <alistair@popple.id.au> wrote: > > > Thanks Nick, > > > > On Thursday, 9 August 2018 2:28:45 PM AEST Nicholas Piggin wrote: > > > Thread state is not queried from the target each time, but cached. > > > This patch adds a valid flag in the thread state cache to ensure > > > this is being probed proprly before use. > > > > I really need to add some comments/documentation but there is already a > > pdbg_target_status() function which is supposed to indicate if a particular > > target has been initialised or not (and therefore if thread_status() is > > initialised). > > > > So rather than adding a target specific valid flag I think it would be best to > > just check that. There is also an implicit (ie. undocumented :-) rule that > > target functions other than target_probe() should only be called on targets with > > status == PDBG_TARGET_ENABLED. > > Oh it's not the target status field but the thread_state one (which is > of course target specific), so I think it's okay. Using the target > status maybe works, but we pass around that thread_state structure > without the target in some places, also could target status be used > with things like ramming that wants to check status of non-target > siblings? (not that I've added the valid check there, but I should) Not quite sure I follow, but what I meant is we should assume: 1) If pdbg_target_status(target) == PDBG_TARGET_ENABLED then thread_status(target) will always return a valid thread_state. 2) If pdbg_target_status(target) != PDBG_TARGET_ENABLED then we shouldn't be calling thread_status(target) as the target hasn't been initialised and doesn't even exist so it can't have a status. Therefore I didn't quite understand why we needed a seperate bit tracking thread_state validity as it should always be valid or non-existant. For checking status of all thread siblings on a given core you could do something like: pdbg_for_each_target("thread", core, sibling_thread) { assert(pdbg_target_probe(sibling_thread) == PDBG_TARGET_ENABLED); thread_state = thread_status(sibling_thread); } This is effectively the check we do in p9_ram_setup() today, although that code is a little old and needs some cleanup to make it look more like the above which is what I'm doing now. Thanks again, Alistair > Thanks, > Nick >
On Thu, 09 Aug 2018 18:39:46 +1000 Alistair Popple <alistair@popple.id.au> wrote: > On Thursday, 9 August 2018 5:53:14 PM AEST Nicholas Piggin wrote: > > On Thu, 09 Aug 2018 16:46:09 +1000 > > Alistair Popple <alistair@popple.id.au> wrote: > > > > > Thanks Nick, > > > > > > On Thursday, 9 August 2018 2:28:45 PM AEST Nicholas Piggin wrote: > > > > Thread state is not queried from the target each time, but cached. > > > > This patch adds a valid flag in the thread state cache to ensure > > > > this is being probed proprly before use. > > > > > > I really need to add some comments/documentation but there is already a > > > pdbg_target_status() function which is supposed to indicate if a particular > > > target has been initialised or not (and therefore if thread_status() is > > > initialised). > > > > > > So rather than adding a target specific valid flag I think it would be best to > > > just check that. There is also an implicit (ie. undocumented :-) rule that > > > target functions other than target_probe() should only be called on targets with > > > status == PDBG_TARGET_ENABLED. > > > > Oh it's not the target status field but the thread_state one (which is > > of course target specific), so I think it's okay. Using the target > > status maybe works, but we pass around that thread_state structure > > without the target in some places, also could target status be used > > with things like ramming that wants to check status of non-target > > siblings? (not that I've added the valid check there, but I should) > > Not quite sure I follow, but what I meant is we should assume: > > 1) If pdbg_target_status(target) == PDBG_TARGET_ENABLED then > thread_status(target) will always return a valid thread_state. > > 2) If pdbg_target_status(target) != PDBG_TARGET_ENABLED then we shouldn't be > calling thread_status(target) as the target hasn't been initialised and > doesn't even exist so it can't have a status. > > Therefore I didn't quite understand why we needed a seperate bit tracking > thread_state validity as it should always be valid or non-existant. Well we don't need it if we can do it another way. It's not a target specific bit through, it's specifically part of the thread status which So I don't think that should exclude this approach. Actually a positive point considering we pass that structure around a bit (although arguably it's a bit ugly when we do that). > > For checking status of all thread siblings on a given core you could do > something like: > > pdbg_for_each_target("thread", core, sibling_thread) { > assert(pdbg_target_probe(sibling_thread) == PDBG_TARGET_ENABLED); > thread_state = thread_status(sibling_thread); > } > > This is effectively the check we do in p9_ram_setup() today, although that code > is a little old and needs some cleanup to make it look more like the above which > is what I'm doing now. I suppose so. I wonder if we should go the other way though and take thread status out of probing. Do it on demand the first time we're asked for a status. Maybe that doesn't save many scoms in most cases. Taking a step back I wonder if we shouldn't query status with scoms every time rather than cache it. I wonder how much the cache buys, it certainly makes it more complex and error prone to ensure it is up to date properly. Thanks, Nick
> > > Oh it's not the target status field but the thread_state one (which is > > > of course target specific), so I think it's okay. Using the target > > > status maybe works, but we pass around that thread_state structure > > > without the target in some places, also could target status be used > > > with things like ramming that wants to check status of non-target > > > siblings? (not that I've added the valid check there, but I should) > > > > Not quite sure I follow, but what I meant is we should assume: > > > > 1) If pdbg_target_status(target) == PDBG_TARGET_ENABLED then > > thread_status(target) will always return a valid thread_state. > > > > 2) If pdbg_target_status(target) != PDBG_TARGET_ENABLED then we shouldn't be > > calling thread_status(target) as the target hasn't been initialised and > > doesn't even exist so it can't have a status. > > > > Therefore I didn't quite understand why we needed a seperate bit tracking > > thread_state validity as it should always be valid or non-existant. > > Well we don't need it if we can do it another way. It's not a target > specific bit through, it's specifically part of the thread status which > So I don't think that should exclude this approach. Actually a > positive point considering we pass that structure around a bit > (although arguably it's a bit ugly when we do that). I guess my point is that the structure is part of the library API, and if a thread_state is obtained from the library it must be valid as I don't think it makes sense to have an API that would return an invalid thread_state. If the application (src/*) wants to pass it around in ugly ways that's fine - but it's up to the application to track if it has initialised a particular instance of thread_state or not rather than embedding application concerns in the library API. > > > > For checking status of all thread siblings on a given core you could do > > something like: > > > > pdbg_for_each_target("thread", core, sibling_thread) { > > assert(pdbg_target_probe(sibling_thread) == PDBG_TARGET_ENABLED); > > thread_state = thread_status(sibling_thread); > > } > > > > This is effectively the check we do in p9_ram_setup() today, although that code > > is a little old and needs some cleanup to make it look more like the above which > > is what I'm doing now. > > I suppose so. I wonder if we should go the other way though and > take thread status out of probing. Do it on demand the first time > we're asked for a status. Maybe that doesn't save many scoms in > most cases. > > Taking a step back I wonder if we shouldn't query status with scoms > every time rather than cache it. I wonder how much the cache buys, > it certainly makes it more complex and error prone to ensure it is up > to date properly. I was wondering that as well :-) Looking back I think historically it was being used to track some other private thread state during instruction ramming prior to cleaning up targetting, etc. I certainly can't come up with a good reason for it to exist today so will happily look at ripping it out. With regards to the issue here I am happy to come up with an alternate. Thanks, Alistair > Thanks, > Nick > > >
On Fri, 10 Aug 2018 13:05:14 +1000 Alistair Popple <alistair@popple.id.au> wrote: > > > > Oh it's not the target status field but the thread_state one (which is > > > > of course target specific), so I think it's okay. Using the target > > > > status maybe works, but we pass around that thread_state structure > > > > without the target in some places, also could target status be used > > > > with things like ramming that wants to check status of non-target > > > > siblings? (not that I've added the valid check there, but I should) > > > > > > Not quite sure I follow, but what I meant is we should assume: > > > > > > 1) If pdbg_target_status(target) == PDBG_TARGET_ENABLED then > > > thread_status(target) will always return a valid thread_state. > > > > > > 2) If pdbg_target_status(target) != PDBG_TARGET_ENABLED then we shouldn't be > > > calling thread_status(target) as the target hasn't been initialised and > > > doesn't even exist so it can't have a status. > > > > > > Therefore I didn't quite understand why we needed a seperate bit tracking > > > thread_state validity as it should always be valid or non-existant. > > > > Well we don't need it if we can do it another way. It's not a target > > specific bit through, it's specifically part of the thread status which > > So I don't think that should exclude this approach. Actually a > > positive point considering we pass that structure around a bit > > (although arguably it's a bit ugly when we do that). > > I guess my point is that the structure is part of the library API, and if a > thread_state is obtained from the library it must be valid as I don't think it > makes sense to have an API that would return an invalid thread_state. If the > application (src/*) wants to pass it around in ugly ways that's fine - but it's > up to the application to track if it has initialised a particular instance of > thread_state or not rather than embedding application concerns in the library > API. Yeah that may be a fair point. > > > > > > > For checking status of all thread siblings on a given core you could do > > > something like: > > > > > > pdbg_for_each_target("thread", core, sibling_thread) { > > > assert(pdbg_target_probe(sibling_thread) == PDBG_TARGET_ENABLED); > > > thread_state = thread_status(sibling_thread); > > > } > > > > > > This is effectively the check we do in p9_ram_setup() today, although that code > > > is a little old and needs some cleanup to make it look more like the above which > > > is what I'm doing now. > > > > I suppose so. I wonder if we should go the other way though and > > take thread status out of probing. Do it on demand the first time > > we're asked for a status. Maybe that doesn't save many scoms in > > most cases. > > > > Taking a step back I wonder if we shouldn't query status with scoms > > every time rather than cache it. I wonder how much the cache buys, > > it certainly makes it more complex and error prone to ensure it is up > > to date properly. > > I was wondering that as well :-) > > Looking back I think historically it was being used to track some other private > thread state during instruction ramming prior to cleaning up targetting, etc. I > certainly can't come up with a good reason for it to exist today so will happily > look at ripping it out. > > With regards to the issue here I am happy to come up with an alternate. Yeah my vote is rip out the cache and make it a call, that makes things easier and less error prone. We can think about how to set validity for the state printing some other way (another array or something would be fine). Thanks, Nick
diff --git a/libpdbg/chip.c b/libpdbg/chip.c index 079592c..2103b2e 100644 --- a/libpdbg/chip.c +++ b/libpdbg/chip.c @@ -98,6 +98,8 @@ struct thread_state thread_status(struct pdbg_target *target) assert(!strcmp(target->class, "thread")); thread = target_to_thread(target); + assert(thread->status.valid); + return thread->status; } diff --git a/libpdbg/libpdbg.h b/libpdbg/libpdbg.h index 1c345cb..39d12e8 100644 --- a/libpdbg/libpdbg.h +++ b/libpdbg/libpdbg.h @@ -156,6 +156,7 @@ enum pdbg_sleep_state {PDBG_THREAD_STATE_RUN, PDBG_THREAD_STATE_DOZE, enum pdbg_smt_state {PDBG_SMT_UNKNOWN, PDBG_SMT_1, PDBG_SMT_2, PDBG_SMT_4, PDBG_SMT_8}; struct thread_state { + bool valid; bool active; bool quiesced; enum pdbg_sleep_state sleep_state; diff --git a/libpdbg/p8chip.c b/libpdbg/p8chip.c index 3e90c8d..453a6f4 100644 --- a/libpdbg/p8chip.c +++ b/libpdbg/p8chip.c @@ -125,6 +125,9 @@ static struct thread_state get_thread_status(struct thread *thread) uint64_t val, mode_reg; struct thread_state thread_status; + memset(&thread_status, 0, sizeof(thread_status)); + thread_status.valid = true; + /* Need to activete debug mode to get complete status */ pib_read(&thread->target, RAS_MODE_REG, &mode_reg); pib_write(&thread->target, RAS_MODE_REG, mode_reg | MR_THREAD_IN_DEBUG); diff --git a/libpdbg/p9chip.c b/libpdbg/p9chip.c index 1433d19..f1e22cf 100644 --- a/libpdbg/p9chip.c +++ b/libpdbg/p9chip.c @@ -97,6 +97,9 @@ static struct thread_state p9_get_thread_status(struct thread *thread) uint64_t value; struct thread_state thread_state; + memset(&thread_state, 0, sizeof(thread_state)); + thread_state.valid = true; + thread_read(thread, P9_RAS_STATUS, &value); thread_state.quiesced = (GETFIELD(PPC_BITMASK(8*thread->id, 3 + 8*thread->id), value) == 0xf); @@ -121,6 +124,7 @@ static int p9_thread_probe(struct pdbg_target *target) thread->id = dt_prop_get_u32(target, "tid"); thread->status = p9_get_thread_status(thread); + assert(thread->status.valid); return 0; } diff --git a/src/thread.c b/src/thread.c index 4b95636..2625874 100644 --- a/src/thread.c +++ b/src/thread.c @@ -15,6 +15,7 @@ */ #include <errno.h> #include <inttypes.h> +#include <string.h> #include <stdio.h> #include <stdlib.h> @@ -28,6 +29,7 @@ static int print_thread_status(struct pdbg_target *target, uint32_t index, uint6 struct thread_state *status = (struct thread_state *) arg; status[index] = thread_status(target); + return 1; } @@ -36,12 +38,19 @@ static int print_core_thread_status(struct pdbg_target *core_target, uint32_t in struct thread_state status[8]; int i, rc; + memset(status, 0, sizeof(status)); + printf("c%02d: ", index); /* TODO: This cast is gross. Need to rewrite for_each_child_target as an iterator. */ rc = for_each_child_target("thread", core_target, print_thread_status, (uint64_t *) &status[0], NULL); for (i = 0; i <= *maxindex; i++) { + if (!status[i].valid) { + printf(" "); + continue; + } + if (status[i].active) printf("A"); else
Thread state is not queried from the target each time, but cached. This patch adds a valid flag in the thread state cache to ensure this is being probed proprly before use. This also skips threads that were not targetted when printing thread status. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- libpdbg/chip.c | 2 ++ libpdbg/libpdbg.h | 1 + libpdbg/p8chip.c | 3 +++ libpdbg/p9chip.c | 4 ++++ src/thread.c | 9 +++++++++ 5 files changed, 19 insertions(+)