cli()/sti() removal

cli()/sti() removal

[M. Asselstine]

Looking at Documentation/cli-sti-removal.txt and grepping for these
macros in the kernel tree reveal that things are close to allowing
this task to be completed. I would like to propose that the remainder
of this work be completed, cleaning up documentation and code related
to this deprecation. I am willing to do the work but just wanted to
put a feeler out there to determine if there is any reason not to
proceed. I also wanted to make sure there wasn't anyone else doing
this already.

Regards,
M. Asselstine

Re: cli()/sti() removal

[Randy Dunlap]

On Mon, 14 Apr 2008 15:38:27 -0400 M. Asselstine wrote:

> Looking at Documentation/cli-sti-removal.txt and grepping for these
> macros in the kernel tree reveal that things are close to allowing
> this task to be completed. I would like to propose that the remainder
> of this work be completed, cleaning up documentation and code related
> to this deprecation. I am willing to do the work but just wanted to
> put a feeler out there to determine if there is any reason not to
> proceed. I also wanted to make sure there wasn't anyone else doing
> this already.

Hi,

The biggest issue is that this isn't a simple grep for and then
substitute new strings for the old ones.  If that's all it was,
it would have been completed long ago.

Some actual thought/design has to be done on several of these in
order to do the locking correctly in all possible contexts, so if
you are up for that, go ahead.  I don't think that anyone else is
doing this just now.

---
~Randy

Re: cli()/sti() removal

[Matthew Wilcox]

On Mon, Apr 14, 2008 at 03:38:27PM -0400, M. Asselstine wrote:
> Looking at Documentation/cli-sti-removal.txt and grepping for these
> macros in the kernel tree reveal that things are close to allowing
> this task to be completed. I would like to propose that the remainder
> of this work be completed, cleaning up documentation and code related
> to this deprecation. I am willing to do the work but just wanted to
> put a feeler out there to determine if there is any reason not to
> proceed. I also wanted to make sure there wasn't anyone else doing
> this already.

People who suggest this usually don't realise just how hard the problem
is.  Let's see how we're doing ...

drivers/atm/nicstar.c:    save_flags(nsdsf); cli();\
drivers/atm/nicstar.c:    save_flags(nsdsf); cli();\
drivers/atm/nicstar.c:    save_flags(nsdsf); cli();\
drivers/isdn/hysdn/boardergo.c:               cli();          /* no further ints */
drivers/net/hamradio/dmascc.c:                cli();
drivers/net/tulip/xircom_tulip_cb.c:          cli();
drivers/net/tulip/xircom_tulip_cb.c:          cli();
drivers/net/tulip/xircom_tulip_cb.c:          save_flags(flags); cli();
drivers/net/tulip/xircom_tulip_cb.c:    cli();
include/asm-frv/system.h:     cli();                                  \

Wow.  That's a big improvement over where we were.  Just some of the
nastiest bits of the kernel still using it ;-)

ok ... the three occurrences in nicstar look like they can be deleted.
Replacing 'restore_flags(nsdsf);' with local_irq_restore(flags) would be
a nice touch, but to be honest the generic lockdep code we have these
days should do even better, so killing the nicstar NS_DEBUG_SPINLOCKS
code would be a good idea.

boardergo looks like it needs its locking reviewed.  It has the
card->hysdn_lock already, so I don't quite understand what the sti()
and cli() are doing in ergo_irq_bh()

dmascc appears to be calling cli() while it's already in interrupt
context.  If so, it already has the register_lock and so this is
unnecessary.  Someone should dig into the driver to verify that this
is true.

xircom_tulip_cb also has a per-device spinlock.  Again, someone needs to
verify why the various places calling cli() aren't grabbing the spinlock.

I actually have a card which I believe to be a xircom_tulip_cb.
Unfortunately, it's twice as tall as any cardbus slot I have in any
functioning computers here, so I can't even plug it in to check it.
I also note that there's a xircom_cb that appears to be replacing
xircom_tulip_cb, so maybe deleting xircom_tulip_cb is possible?

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: cli()/sti() removal

[M. Asselstine]

On Mon, Apr 14, 2008 at 3:59 PM, Randy Dunlap <rdunlap@xenotime.net> wrote:
>
> On Mon, 14 Apr 2008 15:38:27 -0400 M. Asselstine wrote:
>
>  > Looking at Documentation/cli-sti-removal.txt and grepping for these
>  > macros in the kernel tree reveal that things are close to allowing
>  > this task to be completed. I would like to propose that the remainder
>  > of this work be completed, cleaning up documentation and code related
>  > to this deprecation. I am willing to do the work but just wanted to
>  > put a feeler out there to determine if there is any reason not to
>  > proceed. I also wanted to make sure there wasn't anyone else doing
>  > this already.
>
>  Hi,
>
>  The biggest issue is that this isn't a simple grep for and then
>  substitute new strings for the old ones.  If that's all it was,
>  it would have been completed long ago.
>
>  Some actual thought/design has to be done on several of these in
>  order to do the locking correctly in all possible contexts, so if
>  you are up for that, go ahead.  I don't think that anyone else is
>  doing this just now.
>

Yes I realize that this is more then a simple replacement job and that
it will involve some thought. I think with the small numbers of these
remaining I can devote the necessary time and thought. If not I can at
minimum get a few steps closer to getting rid of these, even if I
manage to get rid of only one or two.

Mark

>  ---
>  ~Randy
>

Re: cli()/sti() removal

[M. Asselstine]

On Mon, Apr 14, 2008 at 4:17 PM, Matthew Wilcox <matthew@wil.cx> wrote:
>
> On Mon, Apr 14, 2008 at 03:38:27PM -0400, M. Asselstine wrote:
>  > Looking at Documentation/cli-sti-removal.txt and grepping for these
>  > macros in the kernel tree reveal that things are close to allowing
>  > this task to be completed. I would like to propose that the remainder
>  > of this work be completed, cleaning up documentation and code related
>  > to this deprecation. I am willing to do the work but just wanted to
>  > put a feeler out there to determine if there is any reason not to
>  > proceed. I also wanted to make sure there wasn't anyone else doing
>  > this already.
>
>  People who suggest this usually don't realise just how hard the problem
>  is.  Let's see how we're doing ...
>
I definitely see the complexity and respect it. With some careful
thought and guidance however do you not feel we can finish this up?

>  drivers/atm/nicstar.c:    save_flags(nsdsf); cli();\
>  drivers/atm/nicstar.c:    save_flags(nsdsf); cli();\
>  drivers/atm/nicstar.c:    save_flags(nsdsf); cli();\
>  drivers/isdn/hysdn/boardergo.c:               cli();          /* no further ints */
>  drivers/net/hamradio/dmascc.c:                cli();
>  drivers/net/tulip/xircom_tulip_cb.c:          cli();
>  drivers/net/tulip/xircom_tulip_cb.c:          cli();
>  drivers/net/tulip/xircom_tulip_cb.c:          save_flags(flags); cli();
>  drivers/net/tulip/xircom_tulip_cb.c:    cli();
>  include/asm-frv/system.h:     cli();                                  \
>
>  Wow.  That's a big improvement over where we were.  Just some of the
>  nastiest bits of the kernel still using it ;-)
>
>  ok ... the three occurrences in nicstar look like they can be deleted.
>  Replacing 'restore_flags(nsdsf);' with local_irq_restore(flags) would be
>  a nice touch, but to be honest the generic lockdep code we have these
>  days should do even better, so killing the nicstar NS_DEBUG_SPINLOCKS
>  code would be a good idea.
>
I am thinking this is a good strategy. There is really no need for the
NS_DEBUG_SPINLOCKS code as far as I can tell even any documentation
for NS_DEBUG_SPINLOCKS use seems to of been pulled from the header.

>  boardergo looks like it needs its locking reviewed.  It has the
>  card->hysdn_lock already, so I don't quite understand what the sti()
>  and cli() are doing in ergo_irq_bh()
>
It looks like Thomas Gleixner and Amol Lad did the initial legwork
(2bd7e20e0d24325b0799544bc8105cc57cc8e2aa and
0d9ba869e103d91d471146378ad85bf1fb8e74b4) but a bunch of sti() and one
cli() survived. This is going to be a tough one to finish as I do not
have any way to test it.

>  dmascc appears to be calling cli() while it's already in interrupt
>  context.  If so, it already has the register_lock and so this is
>  unnecessary.  Someone should dig into the driver to verify that this
>  is true.
>
Upon examination the only existing cli()/sti() is found in
start_timer() which as you say is called only when we are already in
an interrupt (scc_isr()) which already has the register_lock with the
exception of one call to start_timer() in scc_send_packet() which
again already has the register_lock. So I think these instances will
be safe to remove.

>  xircom_tulip_cb also has a per-device spinlock.  Again, someone needs to
>  verify why the various places calling cli() aren't grabbing the spinlock.
>
This one looks like it was just neglected and the save_flags();cli()
were just never moved over to
spin_lock_irqsave()/spin_unlock_irqrestore(). Do you not think this
would be a straight forward translation? as it looks like many other
places in the kernel these were done with only compile testing.

>  I actually have a card which I believe to be a xircom_tulip_cb.
>  Unfortunately, it's twice as tall as any cardbus slot I have in any
>  functioning computers here, so I can't even plug it in to check it.
>  I also note that there's a xircom_cb that appears to be replacing
>  xircom_tulip_cb, so maybe deleting xircom_tulip_cb is possible?
>
I wouldn't even know where to start to propose this removal.

There are a few in sound/oss, would these be of interest or is this
code maintained outside of the kernel?

Regards,
Mark

>  --
>  Intel are signing my paycheques ... these opinions are still mine
>  "Bill, look, we understand that you're interested in selling us this
>  operating system, but compare it to ours.  We can't possibly take such
>  a retrograde step."
>

Re: cli()/sti() removal

[Matthew Wilcox]

On Wed, Apr 16, 2008 at 12:41:44PM -0400, M. Asselstine wrote:
> I definitely see the complexity and respect it. With some careful
> thought and guidance however do you not feel we can finish this up?

I do, now the task is so small ;-)

> >  ok ... the three occurrences in nicstar look like they can be deleted.
> >  Replacing 'restore_flags(nsdsf);' with local_irq_restore(flags) would be
> >  a nice touch, but to be honest the generic lockdep code we have these
> >  days should do even better, so killing the nicstar NS_DEBUG_SPINLOCKS
> >  code would be a good idea.
> >
> I am thinking this is a good strategy. There is really no need for the
> NS_DEBUG_SPINLOCKS code as far as I can tell even any documentation
> for NS_DEBUG_SPINLOCKS use seems to of been pulled from the header.

We're in agreement then.  Please propose a patch.

> >  boardergo looks like it needs its locking reviewed.  It has the
> >  card->hysdn_lock already, so I don't quite understand what the sti()
> >  and cli() are doing in ergo_irq_bh()
> >
> It looks like Thomas Gleixner and Amol Lad did the initial legwork
> (2bd7e20e0d24325b0799544bc8105cc57cc8e2aa and
> 0d9ba869e103d91d471146378ad85bf1fb8e74b4) but a bunch of sti() and one
> cli() survived. This is going to be a tough one to finish as I do not
> have any way to test it.

I don't think anyone else does either.  It's going to require careful
analysis of the driver to check they're simply obsolete.  It might be
worth asking Thomas and Amol why they were left (it may be a simple
oversight).

> >  dmascc appears to be calling cli() while it's already in interrupt
> >  context.  If so, it already has the register_lock and so this is
> >  unnecessary.  Someone should dig into the driver to verify that this
> >  is true.
>
> Upon examination the only existing cli()/sti() is found in
> start_timer() which as you say is called only when we are already in
> an interrupt (scc_isr()) which already has the register_lock with the
> exception of one call to start_timer() in scc_send_packet() which
> again already has the register_lock. So I think these instances will
> be safe to remove.

Great!  Please send a patch.

> >  xircom_tulip_cb also has a per-device spinlock.  Again, someone needs to
> >  verify why the various places calling cli() aren't grabbing the spinlock.
>
> This one looks like it was just neglected and the save_flags();cli()
> were just never moved over to
> spin_lock_irqsave()/spin_unlock_irqrestore(). Do you not think this
> would be a straight forward translation? as it looks like many other
> places in the kernel these were done with only compile testing.

Compile testing and a great deal of thinking.  For example, outl_CSR6() is
called from xircom_init_one() a few lines before we call spin_lock_init(),
so grabbing the spinlock in outl_CSR6() would cause a lockup.  Also, the
device isn't open at this point, so the interrupt routine can't be
called.

But simply deleting 'cli()' might cause trouble when it's called from,
for example xircom_tx_timeout().

> There are a few in sound/oss, would these be of interest or is this
> code maintained outside of the kernel?

I think you're wrong:

$ grep -wr cli sound
sound/oss/dmabuf.c:/* No kernel lock - DMAbuf_activate_recording protected by global cli/sti */
sound/oss/midibuf.c:                    /* yes, think the same, so I removed the cli() brackets 
sound/oss/midibuf.c:                    /* yes removed the cli() brackets again
sound/oss/midi_synth.c: cli(); 

Initially, that looks like only one user, but examining the context:

        /* save_flags(flags);
        cli(); 
        don't know against what irqhandler to protect*/

it's actually commented out.  So I don't see any users in sound/oss.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: cli()/sti() removal

[M. Asselstine]

On Wed, Apr 16, 2008 at 12:55 PM, Matthew Wilcox <matthew@wil.cx> wrote:
> On Wed, Apr 16, 2008 at 12:41:44PM -0400, M. Asselstine wrote:
>  > I definitely see the complexity and respect it. With some careful
>  > thought and guidance however do you not feel we can finish this up?
>
>  I do, now the task is so small ;-)
>
>
>  > >  ok ... the three occurrences in nicstar look like they can be deleted.
>  > >  Replacing 'restore_flags(nsdsf);' with local_irq_restore(flags) would be
>  > >  a nice touch, but to be honest the generic lockdep code we have these
>  > >  days should do even better, so killing the nicstar NS_DEBUG_SPINLOCKS
>  > >  code would be a good idea.
>  > >
>  > I am thinking this is a good strategy. There is really no need for the
>  > NS_DEBUG_SPINLOCKS code as far as I can tell even any documentation
>  > for NS_DEBUG_SPINLOCKS use seems to of been pulled from the header.
>
>  We're in agreement then.  Please propose a patch.
>
Will do, tonight hopefully, depends on how the hockey game goes.

>
>  > >  boardergo looks like it needs its locking reviewed.  It has the
>  > >  card->hysdn_lock already, so I don't quite understand what the sti()
>  > >  and cli() are doing in ergo_irq_bh()
>  > >
>  > It looks like Thomas Gleixner and Amol Lad did the initial legwork
>  > (2bd7e20e0d24325b0799544bc8105cc57cc8e2aa and
>  > 0d9ba869e103d91d471146378ad85bf1fb8e74b4) but a bunch of sti() and one
>  > cli() survived. This is going to be a tough one to finish as I do not
>  > have any way to test it.
>
>  I don't think anyone else does either.  It's going to require careful
>  analysis of the driver to check they're simply obsolete.  It might be
>  worth asking Thomas and Amol why they were left (it may be a simple
>  oversight).
Yep a simple email to them might provide an answer or at minimum some insight.

>
>
>  > >  dmascc appears to be calling cli() while it's already in interrupt
>  > >  context.  If so, it already has the register_lock and so this is
>  > >  unnecessary.  Someone should dig into the driver to verify that this
>  > >  is true.
>  >
>  > Upon examination the only existing cli()/sti() is found in
>  > start_timer() which as you say is called only when we are already in
>  > an interrupt (scc_isr()) which already has the register_lock with the
>  > exception of one call to start_timer() in scc_send_packet() which
>  > again already has the register_lock. So I think these instances will
>  > be safe to remove.
>
>  Great!  Please send a patch.
>
Will do.

>
>  > >  xircom_tulip_cb also has a per-device spinlock.  Again, someone needs to
>  > >  verify why the various places calling cli() aren't grabbing the spinlock.
>  >
>  > This one looks like it was just neglected and the save_flags();cli()
>  > were just never moved over to
>  > spin_lock_irqsave()/spin_unlock_irqrestore(). Do you not think this
>  > would be a straight forward translation? as it looks like many other
>  > places in the kernel these were done with only compile testing.
>
>  Compile testing and a great deal of thinking.  For example, outl_CSR6() is
>  called from xircom_init_one() a few lines before we call spin_lock_init(),
>  so grabbing the spinlock in outl_CSR6() would cause a lockup.  Also, the
>  device isn't open at this point, so the interrupt routine can't be
>  called.
>
>  But simply deleting 'cli()' might cause trouble when it's called from,
>  for example xircom_tx_timeout().
>
Good thoughts. I will have to dig deeper into this.

>
>  > There are a few in sound/oss, would these be of interest or is this
>  > code maintained outside of the kernel?
>
>  I think you're wrong:
>
You think correctly. Silly me I should of dove deeper.

>  $ grep -wr cli sound
>  sound/oss/dmabuf.c:/* No kernel lock - DMAbuf_activate_recording protected by global cli/sti */
>  sound/oss/midibuf.c:                    /* yes, think the same, so I removed the cli() brackets
>  sound/oss/midibuf.c:                    /* yes removed the cli() brackets again
>  sound/oss/midi_synth.c: cli();
>
>  Initially, that looks like only one user, but examining the context:
>
>         /* save_flags(flags);
>         cli();
>         don't know against what irqhandler to protect*/
>
>  it's actually commented out.  So I don't see any users in sound/oss.
>
>  --
>
>
> Intel are signing my paycheques ... these opinions are still mine

BTW WindRiver pays mine :).

Mark

>  "Bill, look, we understand that you're interested in selling us this
>  operating system, but compare it to ours.  We can't possibly take such
>  a retrograde step."
>

Re: cli()/sti() removal

[Adrian Bunk]

On Mon, Apr 14, 2008 at 02:17:50PM -0600, Matthew Wilcox wrote:
>...
> include/asm-frv/system.h:     cli();                                  \

I'll send a patch for this one (it's in an unused macro).

>...
> I also note that there's a xircom_cb that appears to be replacing
> xircom_tulip_cb, so maybe deleting xircom_tulip_cb is possible?
>...

I beat you by 3 months.  ;)

Patch is in the net tree and should go into Linus' tree within the next
two weeks.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: cli()/sti() removal

[Matthew Wilcox]

On Fri, Apr 18, 2008 at 07:18:14PM +0300, Adrian Bunk wrote:
> On Mon, Apr 14, 2008 at 02:17:50PM -0600, Matthew Wilcox wrote:
> >...
> > include/asm-frv/system.h:     cli();                                  \
> 
> I'll send a patch for this one (it's in an unused macro).

Thanks.

> >...
> > I also note that there's a xircom_cb that appears to be replacing
> > xircom_tulip_cb, so maybe deleting xircom_tulip_cb is possible?
> >...
> 
> I beat you by 3 months.  ;)
> 
> Patch is in the net tree and should go into Linus' tree within the next
> two weeks.

Why do the tulip maintainers not know about this?

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: cli()/sti() removal

[Adrian Bunk]

On Tue, Apr 22, 2008 at 09:47:22AM -0600, Matthew Wilcox wrote:
> On Fri, Apr 18, 2008 at 07:18:14PM +0300, Adrian Bunk wrote:
> > On Mon, Apr 14, 2008 at 02:17:50PM -0600, Matthew Wilcox wrote:
>...
> > > I also note that there's a xircom_cb that appears to be replacing
> > > xircom_tulip_cb, so maybe deleting xircom_tulip_cb is possible?
> > >...
> > 
> > I beat you by 3 months.  ;)
> > 
> > Patch is in the net tree and should go into Linus' tree within the next
> > two weeks.
> 
> Why do the tulip maintainers not know about this?

tulip has a separate maintainer?

/me checks MAINTAINERS

Sorry, there seem to still be parts of MAINTAINERS I am not aware of.

The patch went to Jeff and netdev, and Jeff applied it (perhaps 
wrongly assuming I was aware that there are tulip maintainers?).

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: cli()/sti() removal

[Grant Grundler]

On Tue, Apr 22, 2008 at 06:58:39PM +0300, Adrian Bunk wrote:
> > Why do the tulip maintainers not know about this?
> 
> tulip has a separate maintainer?

Yes :)
Kyle and I adopted tulip becuase parisc port entirely depends on them.

But it's fine. I'm not doing much with either of the xircom
drivers yet. I've only got two of those cards and have been
promised a few more (different ones).

> /me checks MAINTAINERS
> 
> Sorry, there seem to still be parts of MAINTAINERS I am not aware of.
> 
> The patch went to Jeff and netdev, and Jeff applied it (perhaps 
> wrongly assuming I was aware that there are tulip maintainers?).

Sending it to netdev+jeff is in general sufficient.
But I'm not on netdev since I just don't have the BW.
(which also partially answers willy's question).

thanks,
grant

Re: cli()/sti() removal

[M. Asselstine]

On Wed, Apr 16, 2008 at 1:41 PM, M. Asselstine <asselsm@mcmaster.ca> wrote:
> On Wed, Apr 16, 2008 at 12:55 PM, Matthew Wilcox <matthew@wil.cx> wrote:
[...]
>  >  > >  boardergo looks like it needs its locking reviewed.  It has the
>  >  > >  card->hysdn_lock already, so I don't quite understand what the sti()
>  >  > >  and cli() are doing in ergo_irq_bh()
>  >  > >
>  >  > It looks like Thomas Gleixner and Amol Lad did the initial legwork
>  >  > (2bd7e20e0d24325b0799544bc8105cc57cc8e2aa and
>  >  > 0d9ba869e103d91d471146378ad85bf1fb8e74b4) but a bunch of sti() and one
>  >  > cli() survived. This is going to be a tough one to finish as I do not
>  >  > have any way to test it.
>  >
>  >  I don't think anyone else does either.  It's going to require careful
>  >  analysis of the driver to check they're simply obsolete.  It might be
>  >  worth asking Thomas and Amol why they were left (it may be a simple
>  >  oversight).
>  Yep a simple email to them might provide an answer or at minimum some insight.
>
I have analyzed this driver for more time that I am willing to admit
(thankfully my EeePC allows me to spend this time while in transit on
the bus). I do not see any use for the cli()/sit() calls in the bottom
half handler as the register IO is protected by the spinlock and
somewhat by card->hw_lock. And as for the other three sti() calls
found in the various board bringup functions I also see no need as
there is no matching disabling of interrupts anywhere in the driver. I
really wish I had access to this hardware so I could cook up a patch
and try things out.

Mark

[...]