[REVIEW/TEST] polling(4) changes

[John Baldwin]

On Friday 30 September 2005 08:40 am, Gleb Smirnoff wrote:
>   [please, follow-up on net@ only]
>
>   Colleagues,
>
>   here are some patches for review.
>
>   Problems addressed:
>
>   1) When Giant was removed from polling a problem was introduced. The idle
> poll feature was broken. The idle poll thread can enter polling handler on
> one interface and put to sleep for a long time, until CPU resources found.
> During this time no traffic is received on interface. Well, this is what
> idle thread is supposed to do. Why didn't this happen with Giant? Because
> idle poll entered poll handler holding Giant, and other threads (in
> particular netisr poll) contested on Giant and propagated their priority to
> idle poll. Well, this is a hack, but idle poll significantly improves
> polling performance on an idle box, that's why I won't axe it but try to
> fix it.
>
>   To address the problem we need to use the same technique as before, but
> use poll_mtx instead of Giant. However, this will resurrect LORs, that were
> fixed with Giant removal. The alternative lock path happens, when driver
> decides to deregister from polling itself. The LOR is fixed by further
> changes. See 3).
>
>   2) Drivers indicate their ability to do polling(4) with IFCAP_POLLING
> flag int if_capabilites field. Setting the flag in if_capenable should
> register interface with polling and disable interrupts. However, the
> if_flags is also abused with IFF_POLLING flag. The aim is to remove
> IFF_POLLING flag.
>
>   3) The polling is switched on and off not functionally. That is, when you
> say 'sysctl kern.polling.enable=1' or 'ifconfig fxp0 -polling', the polling
> is switched on/off not immediately but on next tick or next interrupt. This
> non-functional approach leads to a lot of ambiguouties in code, makes it
> harder to understand and maintain. It also exposes race conditions.
>
> The attached patch removes:
>   - IFF_POLLING flag.
>   - Use of if_flags, if_drv_flags, if_capenable from kern_poll.c.
>     All current accesses to these fields are not locked and polling
> shouldn't look there.
>   - poll in trap feature. Sorry, we can't acquire mutexes in trap(). Anyone
>     used it, anyway?
>   - POLL_DEREGISTER command. No hacks. Everything is done functionally via
>     ioctl().
>
> The new world order for driver is the following:
>
>   1) Declare IFCAP_POLLING in if_capabilities on attach. Do not touch
> if_capenable. 2) in ioctl method, in SIOCSIFCAP case the driver should:
> 	- call ether_poll_[de]register
> 	- if no error, set the IFCAP_POLLING flag in if_capenable
> 	- obtain driver lock
> 	- [dis/en]able interrupts
> 	- drop driver lock
>   3) In poll method, check IFF_DRV_RUNNING flag after obtaining driver lock
>   4) In interrupt handler check IFCAP_POLLING flag in if_capenable. If
> present, then return. This is important to protect from spurious
> interrupts. 5) In device detach method, call ether_poll_deregister() before
> obtaining driver lock.

From my limited experience with locking various NIC drivers, I like this 
change.  I think it is much better to tweak the polling state in the ioctl() 
handler rather than in the poll handler.

-- 
John Baldwin <jhb at FreeBSD.org>  <><  http://www.FreeBSD.org/~jhb/
"Power Users Use the Power to Serve"  =  http://www.FreeBSD.org