Re: [PATCH]: Tigon3 new NAPI locking v2

[Jeff Garzik <jgarzik@xxxxxxxxx>]

David S. Miller wrote:
> [TG3]: Eliminate all hw IRQ handler spinlocks.
>
> Move all driver spinlocks to be taken at sw IRQ
> context only.
>
> This fixes the skb_copy() we were doing with hw
> IRQs disabled (which is illegal and triggers a
> BUG() with HIGHMEM enabled).  It also simplifies
> the locking all over the driver tremendously.
>
> We accomplish this feat by creating a special
> sequence to synchronize with the hw IRQ handler
> using a 2-bit atomic state.
>
> Signed-off-by: David S. Miller <davem@xxxxxxxxxxxxx>

overall, pretty spiffy :)

As further work, I would like to see how much (alot? all?) of the timer 
code could be moved into a workqueue, where we could kill the last of 
the horrible-udelay loops in the driver.  Particularly awful is

        while (++tick < 195000) {
                status = tg3_fiber_aneg_smachine(tp, &aninfo);
                if (status == ANEG_DONE || status == ANEG_FAILED)
                        break;

                udelay(1);
        }

where you could freeze a uniprocess box (lock out everything but 
interrupts) for over 1 second.  IOW, the slower the phy, the more these 
slow-path delays can affect the overall system.

This is a MINOR, low priority issue; but long delays are uglies that 
should be fixed, if its relatively painless.


> +static void tg3_irq_quiesce(struct tg3 *tp)
> +{
> +       BUG_ON(test_bit(TG3_IRQSTATE_SYNC, &tp->irq_state));
> +
> +       set_bit(TG3_IRQSTATE_SYNC, &tp->irq_state);
> +       smp_mb();
> +       tw32(GRC_LOCAL_CTRL,
> +            tp->grc_local_ctrl | GRC_LCLCTRL_SETINT);
> +
> +       while (!test_bit(TG3_IRQSTATE_COMPLETE, &tp->irq_state)) {
> +               u32 val = tr32(MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW);
> +
> +               if (val == 0x00000001)
> +                       break;
> +
> +               cpu_relax();
> +       }
> +}

* This loop makes me nervous...  If there's a fault on the PCI bus or 
the hardware is unplugged, val will equal 0xffffffff.

* A few comments for normal humans like "force an interrupt" and "wait 
for interrupt handler to complete" might be nice.

* a BUG_ON(if-interrupts-are-disabled) line might be nice


> +static inline int tg3_irq_sync(struct tg3 *tp)
> +{
> +       if (test_bit(TG3_IRQSTATE_SYNC, &tp->irq_state)) {
> +               set_bit(TG3_IRQSTATE_COMPLETE, &tp->irq_state);
> +               return 1;
> +       }
> +       return 0;
> +}
> +
> +/* Fully shutdown all tg3 driver activity elsewhere in the system.
> + * If irq_sync is non-zero, then the IRQ handler must be synchronized
> + * with as well.  Most of the time, this is not necessary except when
> + * shutting down the device.
> + */
> +static inline void tg3_full_lock(struct tg3 *tp, int irq_sync)
> +{
> +       if (irq_sync)
> +               tg3_irq_quiesce(tp);
> +       spin_lock_bh(&tp->lock);
> +       spin_lock(&tp->tx_lock);
> +}

Rather than an 'irq_sync' arg, my instinct would have been to create 
tg3_full_lock() and tg3_full_lock_sync().  This makes the action -much- 
more obvious to the reader, and since its inline doesn't cost anything 
(compiler's optimizer even does a tiny bit less work my way).

        Jeff