Re: [PATCH] Celleb: improve htab lock

[Christoph Hellwig <hch@lst.de>]

On Thu, Feb 08, 2007 at 07:51:44AM +0100, Arnd Bergmann wrote:
> On Thursday 08 February 2007 07:18, Ishizaki Kou wrote:
> > s);
> > +???????local_irq_save(flags);
> > +???????spin_lock(&beat_htab_lock);
> > ????????dummy1 = beat_lpar_hpte_getword0(slot);
> > ?
> > ????????if ((dummy1 & ~0x7FUL) != (want_v & ~0x7FUL)) {
> > ????????????????DBG_LOW("not found !\n");
> > -???????????????spin_unlock_irqrestore(&beat_htab_lock, flags);
> > +???????????????spin_unlock(&beat_htab_lock);
> > +???????????????local_irq_restore(flags);
> > ????????????????return;
> > ????????}
> > ?
> > ????????lpar_rc = beat_write_htab_entry(0, slot, 0, 0, HPTE_V_VALID, 0,
> > ????????????????&dummy1, &dummy2);
> > -???????spin_unlock_irqrestore(&beat_htab_lock, flags);
> > +???????spin_unlock(&beat_htab_lock);
> > +???????local_irq_restore(flags);
> 
> The function normally used here would be spin_lock_irq()/spin_unlock_irq(),
> which does spin_{,un}lock along with local_irq_{save,restore}.

But I think the issue here is that we don't want to disable irqs because
we want to protect against IRQ context, but rather because the
hypervisor expects it.  Using the separate routines makes this a little
cleaner.  Then again a comment explaining why we do it in addition to
the separate routines would be even better.  And if this is really the
case doing the disabling inside the spinlock might be more explanative
and reduces the irq disabling time.  Also if we're sure that this is
always called from process context and with irqs enabled we should
be using local_irq_disable/local_irq_enable and avoid saving the
flags word.