Possible eHEA performance issue

Possible eHEA performance issue

[Michael Neuling]

>From ehea_start_xmit in ehea_main.c we have:

    if (unlikely(atomic_read(&pr->swqe_avail) <= 1)) {
	    spin_lock_irqsave(&pr->netif_queue, flags);
	    if (unlikely(atomic_read(&pr->swqe_avail) <= 1)) {
		    pr->p_stats.queue_stopped++;
		    netif_stop_queue(dev);
		    pr->queue_stopped = 1;
	    }
	    spin_unlock_irqrestore(&pr->netif_queue, flags);
    }

Since the conditions are the same, isn't it likely that the second 'if'
is going to be taken.  Hence, shouldn't the second 'unlikely' hint be
removed or even changed to likely?

Either way, some documentation here as to why it's done this way would
be useful.  I assume the atomic_read is cheap compared to the
spin_unlock_irqsave, so we quickly check swqe_avail before we check it
again properly with the lock on so we can change some stuff.

Mikey

Re: Possible eHEA performance issue

[Thomas Klein]

Michael Neuling wrote:
> From ehea_start_xmit in ehea_main.c we have:
> 
>     if (unlikely(atomic_read(&pr->swqe_avail) <= 1)) {
> 	    spin_lock_irqsave(&pr->netif_queue, flags);
> 	    if (unlikely(atomic_read(&pr->swqe_avail) <= 1)) {
> 		    pr->p_stats.queue_stopped++;
> 		    netif_stop_queue(dev);
> 		    pr->queue_stopped = 1;
> 	    }
> 	    spin_unlock_irqrestore(&pr->netif_queue, flags);
>     }
> 
> Since the conditions are the same, isn't it likely that the second 'if'
> is going to be taken.  Hence, shouldn't the second 'unlikely' hint be
> removed or even changed to likely?
> 
> Either way, some documentation here as to why it's done this way would
> be useful.  I assume the atomic_read is cheap compared to the
> spin_unlock_irqsave, so we quickly check swqe_avail before we check it
> again properly with the lock on so we can change some stuff.
> 
> Mikey

Hi Mike,

good point the second if could be a likely(). The impact isn't that big
because the if statement is true in the unlikely() case that the send queue
is full - which doesn't happen often. Anyway we will modify this in one of
the next driver versions. Thanks for the hint!

Thomas