Source-Changes-D archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: CVS commit: src/sys



Hello,

Forgot to comment on this after your commit, but here we go..

David Young <dyoung%pobox.com@localhost> wrote:
> > > 'options SPLDEBUG' adds instrumentation to spllower() and splraise() as
> > > well as routines to start/stop debugging and to record IPL transitions:
> > > spldebug_start(), spldebug_stop(), spldebug_raise(), spldebug_lower().
> > 
> > this seems too ad-hoc to me.
> 
> If you have suggestions for providing a comparable function that is less
> ad-hoc, let me know.
> 
> > - please don't put MD code like this in kern/.  IPL_ values can't be
> >   compared with >= in MI code.  return_address.9 is in man.i386.

I agree with yamt that it should not belong to sys/kern.  Also, that it is
quite ad-hoc.  Perhaps it was better to teach LOCKDEBUG to do IPL tracking,
since most cases are via mutex(9) and in future there should not be many
direct spl() calls?

In fact, I am not convinced that such IPL tracking is very useful - we have
reduced amount of IPLs, which is much simpler model than we used to have, and
the problem you were recently debugging was miss-interpretation of certain
spin-mutex behaviour.

Also, since it tracks IPL and not direct spl() calls, it should have rather
been IPLDEBUG option, not SPLDEBUG. :)

> > - can you explain "cpu_index(ci) > MAXCPUS" and "cpu_index(ci) >=
> > MAXCPUS" ?
> 
> Typo.

These cannot happen.  Should be KASSERTs, at least.  I think same with
if (ipl >= IPL_HIGH) check.  Also, this code adds arrays of MAXCPUS size.
While it is not a problem at the moment, in the long term we should not
depend on MAXCPUS, or perhaps even look for a way to remove it.

-- 
Mindaugas


Home | Main Index | Thread Index | Old Index