Date: Mon, 28 Jan 2013 16:25:15 -0500 From: John Baldwin <jhb@freebsd.org> To: freebsd-net@freebsd.org Cc: Luigi Rizzo <rizzo@iet.unipi.it>, Jack Vogel <jfvogel@gmail.com>, head@freebsd.org Subject: Re: two problems in dev/e1000/if_lem.c::lem_handle_rxtx() Message-ID: <201301281625.16157.jhb@freebsd.org> In-Reply-To: <CA%2BhQ2%2BjEETBeN%2BHx=5aThr3pzcuK2L-%2BRd3cd1qDnYR67WSyxA@mail.gmail.com> References: <CA%2BhQ2%2BjEETBeN%2BHx=5aThr3pzcuK2L-%2BRd3cd1qDnYR67WSyxA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201301281625.16157.jhb>