two problems in dev/e1000/if_lem.c::lem_handle_rxtx()

[John Baldwin]

On Tuesday, January 15, 2013 8:23:24 pm Luigi Rizzo wrote:
> Hi,
> i found a couple of problems in
>         dev/e1000/if_lem.c::lem_handle_rxtx() ,
> (compare with dev/e1000/if_em.c::em_handle_que() for better understanding):
> 
> 1. in if_em.c::em_handle_que(), when em_rxeof() exceeds the
>   rx_process_limit, the task is rescheduled so it can complete the work.
>   Conversely, in if_lem.c::lem_handle_rxtx() the lem_rxeof() is
>   only run once, and if there are more pending packets the only
>   chance to drain them is to receive (many) more interrupts.
> 
>   This is a relatively serious problem, because the receiver has
>   a hard time recovering.
> 
>   I'd like to commit a fix to this same as it is done in e1000.

This seems sensible.

> 2. in if_em.c::em_handle_que(), interrupts are reenabled unconditionally,
>    whereas lem_handle_rxtx() only enables them if IFF_DRV_RUNNING is set.
> 
>    I cannot really tell what is the correct way here, so I'd like
>    to put a comment there unless there is a good suggestion on
>    what to do.
> 
>    Accesses to the intr register are race-prone anyways
>    (disabled in fastintr, enabled in the rxtx task without
>    holding any lock, and generally accessed under EM_CORE_LOCK
>    in other places), and presumably enabling/disabling the
>    interrupts around activations of the taks is just an
>    optimization (and on a VM, it is actually a pessimization
>    due to the huge cost of VM exits).

Actually, this is quite important.  The reason being that you don't want the 
interrupt handler and the task both running at the same time (ever).  If you 
do they will both process RX packets and deliver them in different order to 
the stack wreaking havoc on TCP.  In the case of what lem is doing, it is
not racy (except with ifconfig up/down which is handled by checking for
IFF_DRV_RUNNING changing after reacquiring the RX lock) as the algorith, is 
that once the fast interrupt handler masks the interrupt, that code path 
cannot run again until either the threaded handler or the task re-enables 
interrupts.

-- 
John Baldwin