[Pdbg] [PATCH] libpdbg: ensure thread state validity, only print requested targets

Nicholas Piggin npiggin at gmail.com
Thu Aug 9 19:57:20 AEST 2018


On Thu, 09 Aug 2018 18:39:46 +1000
Alistair Popple <alistair at 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 at 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




More information about the Pdbg mailing list