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

[Luigi Rizzo]

On Wed, Jan 16, 2013 at 06:19:01AM -0800, Barney Cordoba wrote:
> 
> 
> --- On Tue, 1/15/13, Luigi Rizzo <rizzo at iet.unipi.it> wrote:
> 
> > From: Luigi Rizzo <rizzo at iet.unipi.it>
> > Subject: two problems in dev/e1000/if_lem.c::lem_handle_rxtx()
> > To: head at freebsd.org, "freebsd-net at freebsd.org" <freebsd-net at freebsd.org>
> > Cc: "Jack Vogel" <jfvogel at gmail.com>
> > Date: Tuesday, January 15, 2013, 8:23 PM
> > 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.
> > 
> > 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).
> > 
> > cheers
> > luigi
> 
> This is not really a big deal; this is how things works for a million 
> years before we had task queues.

i agree that the second issue is not a big deal.

The first one, on the contrary, is a real problem no matter
how you set the 'work' parameter (unless you make it large
enough to drain the entire queue in one call).

cheers
luigi