From patchwork Fri Sep 19 10:32:25 2008 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jarek Poplawski X-Patchwork-Id: 596 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.176.167]) by ozlabs.org (Postfix) with ESMTP id 9A534DDE24 for ; Fri, 19 Sep 2008 20:32:49 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751294AbYISKcp (ORCPT ); Fri, 19 Sep 2008 06:32:45 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751263AbYISKco (ORCPT ); Fri, 19 Sep 2008 06:32:44 -0400 Received: from fg-out-1718.google.com ([72.14.220.152]:38819 "EHLO fg-out-1718.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750836AbYISKcn (ORCPT ); Fri, 19 Sep 2008 06:32:43 -0400 Received: by fg-out-1718.google.com with SMTP id 19so521143fgg.17 for ; Fri, 19 Sep 2008 03:32:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:date:from:to:cc:subject :message-id:references:mime-version:content-type:content-disposition :in-reply-to:user-agent; bh=K4wK/dbjq+AjB8KiDRznyfQNnDavz6cp4by0KAY32hk=; b=EIXikcnLidsUkeV4Xgm30uivMPLUdCIrSPEXq4HVEoy5h97Z4E+y/1aMQIvVh8Vbfm YgG6Sm7E6Gnc85UGFZ71Q+ZF5Qpvk7I8kpuvxp3elezlKTHAtphs+xecfa01N0Stf2mX wRAR+AapsvWKL5Y+1Kcc4Br0/9oty1MMixFPE= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; b=NQoVSOhPEODx7Fycug83hWqf73Z4LWt+dfmvrNoIgf9kroor9CAvLEeapXpof/HDXw Uy93BjEArv+BBaKIEpQa7o70Efyq5EP3jnhbTVwAqajTY+kH3JXnTx66+wiuIPWSXNCZ FIqolR1+zAkRC+jgppSWp6rOkfypcI+tpugh4= Received: by 10.180.220.5 with SMTP id s5mr3267395bkg.31.1221820361821; Fri, 19 Sep 2008 03:32:41 -0700 (PDT) Received: from ff.dom.local ( [80.53.205.170]) by mx.google.com with ESMTPS id 13sm605292fks.6.2008.09.19.03.32.29 (version=SSLv3 cipher=RC4-MD5); Fri, 19 Sep 2008 03:32:40 -0700 (PDT) Date: Fri, 19 Sep 2008 10:32:25 +0000 From: Jarek Poplawski To: Alexander Duyck Cc: Alexander Duyck , netdev@vger.kernel.org, herbert@gondor.apana.org.au, davem@davemloft.net, kaber@trash.net Subject: [take 2] Re: [RFC PATCH] sched: only dequeue if packet can be queued to hardware queue. Message-ID: <20080919103225.GB9135@ff.dom.local> References: <20080918063036.27934.91273.stgit@localhost.localdomain> <20080918194419.GA2982@ami.dom.local> <5f2db9d90809181811q74c3211fp9a099acb8e895fd4@mail.gmail.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <5f2db9d90809181811q74c3211fp9a099acb8e895fd4@mail.gmail.com> User-Agent: Mutt/1.5.18 (2008-05-17) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Take 2: I missed __netif_schedule() in my patch, so the update below. But, because of this a little change of mind: there is no reason to repeat __netif_schedule() if there is a full stop or freeze of all queues, so it seems we need additional check for this anyway. I'll send my proposal of this test soon. Jarek P. On Thu, Sep 18, 2008 at 06:11:45PM -0700, Alexander Duyck wrote: > Once again if you have a suggestion on approach feel free to modify > the patch and see how it works for you. My only concern is that there > are several qdiscs which won't give you the same packet twice and so > you don't know what is going to pop out until you go in and check. ... Actually, here is my suggestion: I think, that your obviously more complex solution should be compared with something simple which IMHO has similar feature, i.e. limited overhead of requeuing. My proposal is to use partly David's idea of killing ->ops->requeue(), for now only the first two patches, so don't requeue back to the qdiscs, but leave qdisc->ops->requeue() for internal usage (like in HFSC's qdisc_peek_len() hack). Additionaly I use your idea of early checking the state of tx queue to make this even lighter after possibly removing the entry check from qdisc_run(). I attach my patch at the end, after original David's two patches. Thanks, Jarek P. ---------------> patch 1/3 Subject: [PATCH 1/9]: pkt_sched: Make qdisc->gso_skb a list. Date: Mon, 18 Aug 2008 01:36:47 -0700 (PDT) From: David Miller To: netdev@vger.kernel.org CC: jarkao2@gmail.com Newsgroups: gmane.linux.network pkt_sched: Make qdisc->gso_skb a list. The idea is that we can use this to get rid of ->requeue(). Signed-off-by: David S. Miller diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h index 84d25f2..140c48b 100644 --- a/include/net/sch_generic.h +++ b/include/net/sch_generic.h @@ -52,7 +52,7 @@ struct Qdisc u32 parent; atomic_t refcnt; unsigned long state; - struct sk_buff *gso_skb; + struct sk_buff_head requeue; struct sk_buff_head q; struct netdev_queue *dev_queue; struct Qdisc *next_sched; diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c index 6f96b7b..39d969e 100644 --- a/net/sched/sch_generic.c +++ b/net/sched/sch_generic.c @@ -45,7 +45,7 @@ static inline int qdisc_qlen(struct Qdisc *q) static inline int dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q) { if (unlikely(skb->next)) - q->gso_skb = skb; + __skb_queue_head(&q->requeue, skb); else q->ops->requeue(skb, q); @@ -57,9 +57,8 @@ static inline struct sk_buff *dequeue_skb(struct Qdisc *q) { struct sk_buff *skb; - if ((skb = q->gso_skb)) - q->gso_skb = NULL; - else + skb = __skb_dequeue(&q->requeue); + if (!skb) skb = q->dequeue(q); return skb; @@ -328,6 +327,7 @@ struct Qdisc noop_qdisc = { .flags = TCQ_F_BUILTIN, .ops = &noop_qdisc_ops, .list = LIST_HEAD_INIT(noop_qdisc.list), + .requeue.lock = __SPIN_LOCK_UNLOCKED(noop_qdisc.q.lock), .q.lock = __SPIN_LOCK_UNLOCKED(noop_qdisc.q.lock), .dev_queue = &noop_netdev_queue, }; @@ -353,6 +353,7 @@ static struct Qdisc noqueue_qdisc = { .flags = TCQ_F_BUILTIN, .ops = &noqueue_qdisc_ops, .list = LIST_HEAD_INIT(noqueue_qdisc.list), + .requeue.lock = __SPIN_LOCK_UNLOCKED(noqueue_qdisc.q.lock), .q.lock = __SPIN_LOCK_UNLOCKED(noqueue_qdisc.q.lock), .dev_queue = &noqueue_netdev_queue, }; @@ -473,6 +474,7 @@ struct Qdisc *qdisc_alloc(struct netdev_queue *dev_queue, sch->padded = (char *) sch - (char *) p; INIT_LIST_HEAD(&sch->list); + skb_queue_head_init(&sch->requeue); skb_queue_head_init(&sch->q); sch->ops = ops; sch->enqueue = ops->enqueue; @@ -543,7 +545,7 @@ void qdisc_destroy(struct Qdisc *qdisc) module_put(ops->owner); dev_put(qdisc_dev(qdisc)); - kfree_skb(qdisc->gso_skb); + __skb_queue_purge(&qdisc->requeue); kfree((char *) qdisc - qdisc->padded); } -------------> patch 2/3 Subject: [PATCH 2/9]: pkt_sched: Always use q->requeue in dev_requeue_skb(). Date: Mon, 18 Aug 2008 01:36:50 -0700 (PDT) From: David Miller To: netdev@vger.kernel.org CC: jarkao2@gmail.com Newsgroups: gmane.linux.network pkt_sched: Always use q->requeue in dev_requeue_skb(). There is no reason to call into the complicated qdiscs just to remember the last SKB where we found the device blocked. The SKB is outside of the qdiscs realm at this point. Signed-off-by: David S. Miller --- net/sched/sch_generic.c | 5 +---- 1 files changed, 1 insertions(+), 4 deletions(-) diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c index 39d969e..96d7d08 100644 --- a/net/sched/sch_generic.c +++ b/net/sched/sch_generic.c @@ -44,10 +44,7 @@ static inline int qdisc_qlen(struct Qdisc *q) static inline int dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q) { - if (unlikely(skb->next)) - __skb_queue_head(&q->requeue, skb); - else - q->ops->requeue(skb, q); + __skb_queue_head(&q->requeue, skb); __netif_schedule(q); return 0; ----------------> patch 3/3 (take 2) pkt_sched: Check the state of tx_queue in dequeue_skb() Check in dequeue_skb() the state of tx_queue for requeued skb to save on locking and re-requeuing, and possibly remove the current check in qdisc_run(). Based on the idea of Alexander Duyck. Signed-off-by: Jarek Poplawski --- diff -Nurp net-next-2.6-/net/sched/sch_generic.c net-next-2.6+/net/sched/sch_generic.c --- net-next-2.6-/net/sched/sch_generic.c 2008-09-19 07:21:44.000000000 +0000 +++ net-next-2.6+/net/sched/sch_generic.c 2008-09-19 10:11:04.000000000 +0000 @@ -52,11 +52,24 @@ static inline int dev_requeue_skb(struct static inline struct sk_buff *dequeue_skb(struct Qdisc *q) { - struct sk_buff *skb; + struct sk_buff *skb = skb_peek(&q->requeue); - skb = __skb_dequeue(&q->requeue); - if (!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)) { + __skb_unlink(skb, &q->requeue); + } else { + skb = NULL; + __netif_schedule(q); + } + } else { skb = q->dequeue(q); + } return skb; }