Re: [PATCH] Improved network performance by balancing Rx against other work

[Nick Piggin <npiggin@suse.de>]

On Wed, Mar 17, 2010 at 01:55:58PM +1100, Peter Chubb wrote:
> +static int e1000_intr_thread(void *data)
> +{
> +	struct net_device *netdev = data;
> +	struct e1000_adapter *adapter = netdev_priv(netdev);
> +	const int budget = 32; // FIXME should be auto-tuneable
> +	int tx_clean_complete = 0, work_done = 0;
> +
> +	while(!e1000_wait_for_intr(adapter)) {
> +		do {
> +			work_done = 0;
> +
> +			tx_clean_complete = e1000_clean_tx_irq(adapter, &adapter->tx_ring[0]);
> +			adapter->clean_rx(adapter, &adapter->rx_ring[0], &work_done, budget);
> +			if (!tx_clean_complete)
> +				work_done = budget;
> +
> +			if (work_done == budget) {
> +				/*
> +				 * Give up the rest of the timeslice to allow
> +				 * userspace to make forward progress
> +				 */
> +				sys_sched_yield();

sched_yield literally has undefined semantics if you are in SCHED_OTHER
priority (which your thread is). sched_yield use in userspace and even
kernel has caused headaches whenever the scheduler changes
significantly.

Please use anything but sched_yield. cond_resched() looks appropriate
here, it gives a simple resched point on !PREEMPT kernels. Then the
scheduler nice levels and newer resource controls should give sufficient
flexibility to fine tune CPU allocation after that.


> +			}
> +		} while (work_done == budget);
> +
> +		/* If budget not fully consumed, wait for an interrupt */
> +		adapter->last_icr = 0;
> +		if (likely(adapter->itr_setting & 3))
> +			e1000_set_itr(adapter);
> +		if (!test_bit(__E1000_DOWN, &adapter->flags))
> +			e1000_irq_enable(adapter);
> +	}
> +	return 0;
> +}