Re: [RFC] [PATCH] Don't run __qdisc_run() on a stopped TX queue

[Jarek Poplawski <jarkao2@gmail.com>]

On 20-07-2009 09:46, Krishna Kumar wrote:
> From: Krishna Kumar <krkumar2@in.ibm.com>
> 
> Driver sends an skb and stops the queue if no more space is
> available on the device, and returns OK. When dev_queue_xmit
> runs for the next skb, it enqueue's the skb & calls qdisc_run.
> qdisc_run dequeue's the skb and finds the queue is stopped
> after getting the HARD_LOCK_TX, and puts the skb back on
> gso_skb (the next iteration fails faster at dequeue_skb).
> 
> This patch avoids calling __qdisc_run if the queue is stopped.
> This also lets us remove the queue_stopped check in dequeue_skb
> and in qdisc_restart.

It was designed wrt. multiqueue handling mainly, were such a check
isn't enough at this place.

Jarek P.

> When the queue is re-enabled, it runs with
> one less queue_stopped() check for every skb on the queue (we
> still need to own the __QDISC_STATE_RUNNING bit to catch the
> stopped condition correctly since stopped cannot be set without
> holding this bit).
> 
> Results for 3 iteration testing for 1, 4, 8, 12 netperf sessions
> running for 1 minute each:
> 
> -----------------------------------------------------------------
> 	       System-X			         P6
> -----------------------------------------------------------------
> Size	ORG BW		NEW BW		ORG BW		NEW BW
> -----------------------------------------------------------------
> 16K	72183.05	74417.79	60775.76	63413.17
> 128K	69097.87	72447.12	61692.16	62251.07
> 256K	60456.21	61415.81	59694.03	61641.81
> -----------------------------------------------------------------
> 
> Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>
> ---
> 
>  include/net/pkt_sched.h |    9 +++++++--
>  net/sched/sch_generic.c |    9 ++-------
>  2 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff -ruNp org/include/net/pkt_sched.h new/include/net/pkt_sched.h
> --- org/include/net/pkt_sched.h	2009-06-22 11:40:31.000000000 +0530
> +++ new/include/net/pkt_sched.h	2009-07-20 13:09:18.000000000 +0530
> @@ -92,8 +92,13 @@ extern void __qdisc_run(struct Qdisc *q)
>  
>  static inline void qdisc_run(struct Qdisc *q)
>  {
> -	if (!test_and_set_bit(__QDISC_STATE_RUNNING, &q->state))
> -		__qdisc_run(q);
> +	if (!test_and_set_bit(__QDISC_STATE_RUNNING, &q->state)) {
> +		if (!netif_tx_queue_stopped(q->dev_queue))
> +			__qdisc_run(q);
> +		else
> +			/* driver will wake us up anyway, just clear */
> +			clear_bit(__QDISC_STATE_RUNNING, &q->state);
> +	}
>  }
>  
>  extern int tc_classify_compat(struct sk_buff *skb, struct tcf_proto *tp,
> diff -ruNp org/net/sched/sch_generic.c new/net/sched/sch_generic.c
> --- org/net/sched/sch_generic.c	2009-05-25 07:48:07.000000000 +0530
> +++ new/net/sched/sch_generic.c	2009-07-20 13:09:18.000000000 +0530
> @@ -56,12 +56,8 @@ static inline struct sk_buff *dequeue_sk
>  	struct sk_buff *skb = q->gso_skb;
>  
>  	if (unlikely(skb)) {
> -		struct net_device *dev = qdisc_dev(q);
> -		struct netdev_queue *txq;
> -
>  		/* check the reason of requeuing without tx lock first */
> -		txq = netdev_get_tx_queue(dev, skb_get_queue_mapping(skb));
> -		if (!netif_tx_queue_stopped(txq) && !netif_tx_queue_frozen(txq))
> +		if (!netif_tx_queue_frozen(q->dev_queue))
>  			q->gso_skb = NULL;
>  		else
>  			skb = NULL;
> @@ -142,8 +138,7 @@ static inline int qdisc_restart(struct Q
>  	txq = netdev_get_tx_queue(dev, skb_get_queue_mapping(skb));
>  
>  	HARD_TX_LOCK(dev, txq, smp_processor_id());
> -	if (!netif_tx_queue_stopped(txq) &&
> -	    !netif_tx_queue_frozen(txq))
> +	if (!netif_tx_queue_frozen(txq))
>  		ret = dev_hard_start_xmit(skb, dev, txq);
>  	HARD_TX_UNLOCK(dev, txq);
>  
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>