[PATCH] Remove if_watchdog use

[John Baldwin]

I have a patchset that converts all the remaining users of if_watchdog to
using a private callout instead.  In some cases the the driver already used a
private timer to drive a stats timer and I merely hooked into that timer.  In
other cases a new callout needed to be added to the driver.  Some drivers
even abused the if_watchdog interface to provide a stats timer that fired
every second. :)  For a few drivers I also fixed other things such as busted
locking, order-of-operations issues in detach, or just completely busted
drivers (fea(4) and fpa(4) which share the pdq backend).  Please test.  
Barring any major screaming and shouting I plan to commit this in a week or
so and after that to work on removing the if_watchdog/if_timer stuff from the
network stack.

The patch is at http://www.FreeBSD.org/~jhb/patches/cleanup.patch

Driver details:
- an(4)
  - Locking fixes to not do silly things like drop the lock only to call a
    function that immediately reacquires the lock.  Also removes recursive
    locking.
  - Hooks into the stat timer to drive the watchdog timer.
- bwi(4), cm(4), ep(4), fatm(4), malo(4), mwl(4), vx(4)
  - Adds a new private timer to drive the watchdog timer.
- ce(4), cp(4), ctau(4), cx(4), lmc(4)
  - These drivers have two modes, a netgraph mode and an ifnet mode.  In the
    netgraph mode they used a private timer to drive the watchdog.  In the
    ifnet mode they used if_watchdog.  Now they just always use the private
    timer.
- de(4)
  - This driver abused the watchdog interface to manage its once-a-second
    stat timer.  It uses a callout to manage this now instead.
- ed(4)
  - This driver used to provide a hook to allow individual attachments to
    provide a 'tick' event to be called from an optional stats timer.
    The stats timer only ran if the tick function pointer was non-NULL
    and the attachment's tick routine had to call callout_reset(), etc.
    Now the driver always schedules a stat timer and manages the
    callout_reset() internally.  This timer is used to drive the watchdog
    and will also call the attachment's 'tick' handler if one is provided.
- ixgb(4)
  - Uses callout_init_mtx() instead of callout_init(..., CALLOUT_MPSAFE).
  - Remove silly callout handling in a few places (cancelling the callout
    only to rescheduled it again immediately afterwards).
  - Hooks into the stat timer to drive the watchdog timer.
- lge(4), nve(4), pcn(4)
  - Hooks into the stat timer to drive the watchdog timer.
- my(4)
  - This driver used the watchdog timer both as a watchdog on transmit and
    auto-negotiation.  To make this simpler and easier to understand I have
    split this out into two separate timers.  One just manages the auto-neg
    side of things and one is a transmit watchdog.
- fea(4), fpa(4)
  - These two drivers share a common backend, pdq, which was plagued with
    several issues.  I'm quite confident no one has used these drivers in
    years since the are guaranteed to panic during attach.
  - Add real locking.  Previously these drivers only acquired their lock
    in their interrupt handler or in the ioctl routine (but too broadly in
    the latter).  No locking was used for the stack calling down into the
    driver via if_init() or if_start(), for device shutdown or detach.  Also,
    the interrupt handler held the driver lock while calling if_input().  All
    this stuff should be fixed in the locking changes.
  - Really fix these drivers to handle if_alloc().  The front-end attachments
    were using if_initname() before the ifnet was allocated.  Fix this by
    moving some of the duplicated logic from each driver into pdq_ifattach().
    While here, make pdq_ifattach() return an error so that the driver just
    fails to attach if if_alloc() fails rather than panic'ing.
  - Adds a new private timer to drive the watchdog timer.
- sn(4)
  - Use bus_*() rather than bus_space_*().
  - Adds a new private timer to drive the watchdog timer.
  - Fixup detach.
- ste(4), ti(4)
  - Adds a new private timer to drive the watchdog timer.
  - Fixup detach.
- tl(4), wb(4)
  - Use bus_*() rather than bus_space_*().
  - Hooks into the stat timer to drive the watchdog timer.
  - Fixup detach.
- vge(4)
  - Overhaul the locking to avoid recursion and add missing locking in a few
    places.
  - Don't schedule a task to call vge_start() from contexts that are safe to
    call vge_start() directly.  Just invoke the routine directly instead
    (this is what all of the other NIC drivers I am familiar with do).  Note
    that vge(4) does not use an interrupt filter handler which is the primary
    reason some other drivers use tasks.
  - Adds a new private timer to drive the watchdog timer.
  - Use bus_*() rather than bus_space_*().
  - Fixup detach.
- netfront(4)
  - This doesn't actually implement a watchdog, it just sets if_watchdog to a
    non-existent function under '#ifdef notyet'.
- admsw(4)
  - This driver is a bit special in that it has no locking at all, not even
    a poor attempt. :)  It also appears to be for a specific MIPS board of
    some sort.
  - It has multiple ifnet's for multiple ports, but it only used if_timer and
    if_watchdog from the first ifnet.  For this driver I added a single
    private timer to replace the if_timer use on the first ifnet.  I marked
    the callout MPSAFE, but the driver really needs to have locking added at
    which point it could use callout_init_mtx().

--
John Baldwin
_______________________________________________
freebsd-current@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@..."

[Gavin Atkinson-4]

On Fri, 6 Nov 2009, John Baldwin wrote:

> I have a patchset that converts all the remaining users of if_watchdog to
> using a private callout instead.  In some cases the the driver already used a
> private timer to drive a stats timer and I merely hooked into that timer.  In
> other cases a new callout needed to be added to the driver.  Some drivers
> even abused the if_watchdog interface to provide a stats timer that fired
> every second. :)  For a few drivers I also fixed other things such as busted
> locking, order-of-operations issues in detach, or just completely busted
> drivers (fea(4) and fpa(4) which share the pdq backend).  Please test.
> Barring any major screaming and shouting I plan to commit this in a week or
> so and after that to work on removing the if_watchdog/if_timer stuff from the
> network stack.
>
> The patch is at http://www.FreeBSD.org/~jhb/patches/cleanup.patch
>
> Driver details:
> - an(4)
>  - Locking fixes to not do silly things like drop the lock only to call a
>    function that immediately reacquires the lock.  Also removes recursive
>    locking.
>  - Hooks into the stat timer to drive the watchdog timer.

I managed to get a panic when running wpa_supplicant:

System call ioctl returning with the following locks held:
exclusive sleep mutex an0 (network driver) r=0 (0xc58fc180) locked @
/usr/src/sys/dev/an/if_an.c:2341
panic: witness_warn

This seems to fix that:

--- /usr/src/sys/dev/an/if_an.c.orig 2009-11-10 19:26:21.000000000
+0000
+++ /usr/src/sys/dev/an/if_an.c 2009-11-10 19:27:24.000000000 +0000
@@ -2570,6 +2570,9 @@
  an_setdef(sc, &sc->areq);
  AN_UNLOCK(sc);
  break;
+ default:
+ AN_UNLOCK(sc);
+ break;
  }

  /*

I also get the following LOR on unplug (but see it before your patch too):

lock order reversal:
1st 0xc50f5208 if_afdata (if_afdata) @ /usr/src/sys/net/if.c:912
2nd 0xc0f9db68 mld_mtx (mld_mtx) @ /usr/src/sys/netinet6/mld6.c:569
KDB: stack backtrace:
db_trace_self_wrapper(c0cb835f,e54b1b20,c08e7b55,c08d891b,c0cbb215,...) at db_trace_self_wrapper+0x26
kdb_backtrace(c08d891b,c0cbb215,c4d30e18,c4d2a9c0,e54b1b7c,...) at kdb_backtrace+0x29
_witness_debugger(c0cbb215,c0f9db68,c0cbb365,c4d2a9c0,c0cd457d,...) at _witness_debugger+0x25
witness_checkorder(c0f9db68,9,c0cd457d,239,0,...) at witness_checkorder+0x839
_mtx_lock_flags(c0f9db68,0,c0cd457d,239,c5340ba0,...) at _mtx_lock_flags+0xc9
mld_domifdetach(c50f5000,c0db5e54,c0dc0880,e54b1c24,c09532a6,...) at mld_domifdetach+0x2c
in6_domifdetach(c50f5000,c5340ba0,390,4be,c50f522c,...) at in6_domifdetach+0x15
if_detach(c50f5000) at if_detach+0x916
ether_ifdetach(c50f5000,0,c0c6cbbc,34f,c555d380,...) at ether_ifdetach+0x3d
an_detach(c555d380,c4ead060,c0da0758,a76,e54b1c94,...) at an_detach+0xb5
device_detach(c555d380,0,c0d5a9b8,0,c4ff6500,...) at device_detach+0x8c
pccard_detach_card(c4ff6500,c4e3e8bc,c0d5a9b8) at pccard_detach_card+0x44
exca_removal(c4ed2004,0,c0c93731,1da,c8,...) at exca_removal+0x59
cbb_event_thread(c4ed2000,e54b1d38,c0cb02eb,343,c4d81aa0,...) at cbb_event_thread+0x107
fork_exit(c0720cb0,c4ed2000,e54b1d38) at fork_exit+0xb8
fork_trampoline() at fork_trampoline+0x8
  --- trap 0, eip = 0, esp = 0xe54b1d70, ebp = 0 ---
an0: detached

Other than that, the patches to an(4) seem to work as well before your
patch as after, with light TCP traffic and heavy ping traffic.  However,
an(4) appears to require a lot of work (unrelated to your patch) to bring
it up to the state of other wireless drivers.

> - ixgb(4)
>  - Uses callout_init_mtx() instead of callout_init(..., CALLOUT_MPSAFE).
>  - Remove silly callout handling in a few places (cancelling the callout
>    only to rescheduled it again immediately afterwards).
>  - Hooks into the stat timer to drive the watchdog timer.

These changes to ixgb_detach() don't compile as ifp is no longer used in
the !DEVICE_POLLING case.

/usr/src/sys/dev/ixgb/if_ixgb.c: In function 'ixgb_detach':
/usr/src/sys/dev/ixgb/if_ixgb.c:369: warning: unused variable 'ifp'

Hope that helps,

Gavin
_______________________________________________
freebsd-current@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@..."

[John Baldwin]

On Tuesday 10 November 2009 4:45:03 pm Gavin Atkinson wrote:

> On Fri, 6 Nov 2009, John Baldwin wrote:
>
> > I have a patchset that converts all the remaining users of if_watchdog to
> > using a private callout instead.  In some cases the the driver already used a
> > private timer to drive a stats timer and I merely hooked into that timer.  In
> > other cases a new callout needed to be added to the driver.  Some drivers
> > even abused the if_watchdog interface to provide a stats timer that fired
> > every second. :)  For a few drivers I also fixed other things such as busted
> > locking, order-of-operations issues in detach, or just completely busted
> > drivers (fea(4) and fpa(4) which share the pdq backend).  Please test.
> > Barring any major screaming and shouting I plan to commit this in a week or
> > so and after that to work on removing the if_watchdog/if_timer stuff from the
> > network stack.
> >
> > The patch is at http://www.FreeBSD.org/~jhb/patches/cleanup.patch
> >
> > Driver details:
> > - an(4)
> >  - Locking fixes to not do silly things like drop the lock only to call a
> >    function that immediately reacquires the lock.  Also removes recursive
> >    locking.
> >  - Hooks into the stat timer to drive the watchdog timer.
>
> I managed to get a panic when running wpa_supplicant:
>
> System call ioctl returning with the following locks held:
> exclusive sleep mutex an0 (network driver) r=0 (0xc58fc180) locked @
> /usr/src/sys/dev/an/if_an.c:2341
> panic: witness_warn
>
> This seems to fix that:
>
> --- /usr/src/sys/dev/an/if_an.c.orig 2009-11-10 19:26:21.000000000
> +0000
> +++ /usr/src/sys/dev/an/if_an.c 2009-11-10 19:27:24.000000000 +0000
> @@ -2570,6 +2570,9 @@
>   an_setdef(sc, &sc->areq);
>   AN_UNLOCK(sc);
>   break;
> + default:
> + AN_UNLOCK(sc);
> + break;
>   }
>
>   /*

Ok, thanks.  Sadly the ioctl handling probably needs a bit more work since it
calls copyin() while holding the an(4) mutex, but I will leave that for
another day.

> I also get the following LOR on unplug (but see it before your patch too):
>
> lock order reversal:
> 1st 0xc50f5208 if_afdata (if_afdata) @ /usr/src/sys/net/if.c:912
> 2nd 0xc0f9db68 mld_mtx (mld_mtx) @ /usr/src/sys/netinet6/mld6.c:569

I think this has to do with the stack in general and is not specific to an(4).

> Other than that, the patches to an(4) seem to work as well before your
> patch as after, with light TCP traffic and heavy ping traffic.  However,
> an(4) appears to require a lot of work (unrelated to your patch) to bring
> it up to the state of other wireless drivers.

Thanks, I will commit the an(4) bits in a bit.

> > - ixgb(4)
> >  - Uses callout_init_mtx() instead of callout_init(..., CALLOUT_MPSAFE).
> >  - Remove silly callout handling in a few places (cancelling the callout
> >    only to rescheduled it again immediately afterwards).
> >  - Hooks into the stat timer to drive the watchdog timer.
>
> These changes to ixgb_detach() don't compile as ifp is no longer used in
> the !DEVICE_POLLING case.
>
> /usr/src/sys/dev/ixgb/if_ixgb.c: In function 'ixgb_detach':
> /usr/src/sys/dev/ixgb/if_ixgb.c:369: warning: unused variable 'ifp'

Oops, ixgb isn't in LINT so I keep missing it.  I need to just add it to
NOTES.  Thanks.

--
John Baldwin
_______________________________________________
freebsd-current@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@..."

[Gavin Atkinson-4]

On Tue, 10 Nov 2009, John Baldwin wrote:

> On Tuesday 10 November 2009 4:45:03 pm Gavin Atkinson wrote:
>> I managed to get a panic when running wpa_supplicant:
>>
>> System call ioctl returning with the following locks held:
>> exclusive sleep mutex an0 (network driver) r=0 (0xc58fc180) locked @
>> /usr/src/sys/dev/an/if_an.c:2341
>> panic: witness_warn
>>
>> This seems to fix that:
>>
>> --- /usr/src/sys/dev/an/if_an.c.orig 2009-11-10 19:26:21.000000000
>> +0000
>> +++ /usr/src/sys/dev/an/if_an.c 2009-11-10 19:27:24.000000000 +0000
>> @@ -2570,6 +2570,9 @@
>>   an_setdef(sc, &sc->areq);
>>   AN_UNLOCK(sc);
>>   break;
>> + default:
>> + AN_UNLOCK(sc);
>> + break;
>>   }
>>
>>   /*
>
> Ok, thanks.  Sadly the ioctl handling probably needs a bit more work since it
> calls copyin() while holding the an(4) mutex, but I will leave that for
> another day.

It actually appears that the above panic is not something that has been
introduced with your patch - I can reproduce it on a vanilla system.
The above patch fixes it in the original code too.

Gavin
_______________________________________________
freebsd-current@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@..."

[Watanabe Kazuhiro]

Hi,

I've tested the following NICs with your patch on CURRENT, and they
works fine.  Thanks!

* Corega FastEther PCI-TX (DEC 21140-AF)

de0: <Digital 21140A Fast Ethernet> port 0xe000-0xe07f mem 0xd9001000-0xd900107f irq 12 at device 15.0 on pci0
de0: 21140A [10-100Mb/s] pass 2.2
de0: Ethernet address: 00:00:f4:xx:xx:xx
de0: [ITHREAD]

* Acer ALN-201C (Realtek RTL8029AS)

ed0: <RealTek 8029> port 0xe000-0xe01f irq 12 at device 15.0 on pci0
ed0: Ethernet address: 00:60:67:xx:xx:xx
ed0: [ITHREAD]

At Fri, 6 Nov 2009 15:08:22 -0500,
John Baldwin wrote:

> I have a patchset that converts all the remaining users of if_watchdog to
> using a private callout instead.  In some cases the the driver already used a
> private timer to drive a stats timer and I merely hooked into that timer.  In
> other cases a new callout needed to be added to the driver.  Some drivers
> even abused the if_watchdog interface to provide a stats timer that fired
> every second. :)  For a few drivers I also fixed other things such as busted
> locking, order-of-operations issues in detach, or just completely busted
> drivers (fea(4) and fpa(4) which share the pdq backend).  Please test.  
> Barring any major screaming and shouting I plan to commit this in a week or
> so and after that to work on removing the if_watchdog/if_timer stuff from the
> network stack.
>
> The patch is at http://www.FreeBSD.org/~jhb/patches/cleanup.patch

(snip)
---
WATANABE Kazuhiro (CQG00620@...)
_______________________________________________
freebsd-current@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@..."

[John Baldwin]

On Sunday 15 November 2009 12:17:57 am WATANABE Kazuhiro wrote:
> Hi,
>
> I've tested the following NICs with your patch on CURRENT, and they
> works fine.  Thanks!
>
> * Corega FastEther PCI-TX (DEC 21140-AF)
>
> de0: <Digital 21140A Fast Ethernet> port 0xe000-0xe07f mem
0xd9001000-0xd900107f irq 12 at device 15.0 on pci0
> de0: 21140A [10-100Mb/s] pass 2.2
> de0: Ethernet address: 00:00:f4:xx:xx:xx
> de0: [ITHREAD]
>
> * Acer ALN-201C (Realtek RTL8029AS)
>
> ed0: <RealTek 8029> port 0xe000-0xe01f irq 12 at device 15.0 on pci0
> ed0: Ethernet address: 00:60:67:xx:xx:xx
> ed0: [ITHREAD]

Great, thanks for testing!

--
John Baldwin
_______________________________________________
freebsd-current@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscribe@..."