Patch Detail
get:
Show a patch.
patch:
Update a patch.
put:
Update a patch.
GET /api/patches/777/?format=api
{ "id": 777, "url": "http://patchwork.ozlabs.org/api/patches/777/?format=api", "web_url": "http://patchwork.ozlabs.org/project/netdev/patch/20080921111551.GB2551@ami.dom.local/", "project": { "id": 7, "url": "http://patchwork.ozlabs.org/api/projects/7/?format=api", "name": "Linux network development", "link_name": "netdev", "list_id": "netdev.vger.kernel.org", "list_email": "netdev@vger.kernel.org", "web_url": null, "scm_url": null, "webscm_url": null, "list_archive_url": "", "list_archive_url_format": "", "commit_url_format": "" }, "msgid": "<20080921111551.GB2551@ami.dom.local>", "list_archive_url": null, "date": "2008-09-21T11:15:51", "name": "[take,2] pkt_sched: Fix qdisc_watchdog() vs. dev_deactivate() race", "commit_ref": null, "pull_url": null, "state": "accepted", "archived": true, "hash": "7edc673f0c6d44b2e687188505657bcb5503f430", "submitter": { "id": 277, "url": "http://patchwork.ozlabs.org/api/people/277/?format=api", "name": "Jarek Poplawski", "email": "jarkao2@gmail.com" }, "delegate": { "id": 34, "url": "http://patchwork.ozlabs.org/api/users/34/?format=api", "username": "davem", "first_name": "David", "last_name": "Miller", "email": "davem@davemloft.net" }, "mbox": "http://patchwork.ozlabs.org/project/netdev/patch/20080921111551.GB2551@ami.dom.local/mbox/", "series": [], "comments": "http://patchwork.ozlabs.org/api/patches/777/comments/", "check": "pending", "checks": "http://patchwork.ozlabs.org/api/patches/777/checks/", "tags": {}, "related": [], "headers": { "Return-Path": "<netdev-owner@vger.kernel.org>", "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])\n\tby ozlabs.org (Postfix) with ESMTP id A68D5DDED8\n\tfor <patchwork-incoming@ozlabs.org>;\n\tSun, 21 Sep 2008 21:14:19 +1000 (EST)", "(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751065AbYIULOO (ORCPT <rfc822;patchwork-incoming@ozlabs.org>);\n\tSun, 21 Sep 2008 07:14:14 -0400", "(majordomo@vger.kernel.org) by vger.kernel.org id S1751350AbYIULOO\n\t(ORCPT <rfc822; netdev-outgoing>); Sun, 21 Sep 2008 07:14:14 -0400", "from fg-out-1718.google.com ([72.14.220.153]:1641 \"EHLO\n\tfg-out-1718.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751052AbYIULON (ORCPT\n\t<rfc822;netdev@vger.kernel.org>); Sun, 21 Sep 2008 07:14:13 -0400", "by fg-out-1718.google.com with SMTP id 19so1044808fgg.17\n\tfor <netdev@vger.kernel.org>; Sun, 21 Sep 2008 04:14:11 -0700 (PDT)", "by 10.86.28.2 with SMTP id b2mr3766640fgb.31.1221995651279;\n\tSun, 21 Sep 2008 04:14:11 -0700 (PDT)", "from ami.dom.local (avb217.neoplus.adsl.tpnet.pl [83.27.35.217])\n\tby mx.google.com with ESMTPS id d4sm4301305fga.5.2008.09.21.04.14.08\n\t(version=SSLv3 cipher=RC4-MD5);\n\tSun, 21 Sep 2008 04:14:10 -0700 (PDT)" ], "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma;\n\th=domainkey-signature:received:received:date:from:to:cc:subject\n\t:message-id:references:mime-version:content-type:content-disposition\n\t:in-reply-to:user-agent;\n\tbh=hzjE0YAGt0vqRxbBrC00XchZz/t3SpJvWHPMJVzFzpY=;\n\tb=VyByIpYwRAfFFYkKYJDXLke34vM+JHfiRhgduV3607NvMROyx3CyXaFEL90rOF7zCc\n\tePKL31IK4JC/8GiZV9uVDju7Ng6DqIVEYiAVYRNGrYuQsTF6Hbwr5Ze+5w3OCNaWx1eN\n\ttP3hw5sYroEFWOuoweZSk2iZdgFjRud71T4rE=", "DomainKey-Signature": "a=rsa-sha1; c=nofws; d=gmail.com; s=gamma;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-type:content-disposition:in-reply-to:user-agent;\n\tb=ccHaP95Sus4/cxPCdMEyrTTafFDelIHHSsl44wRUDFaZRrSalUq9FMCsXwX3vhGWSO\n\tQF7lvJ2WzG2B1LZrHxd/h600ZoleO0/nRcNwGywCygXRkwqzUjD1/LM/cKI3VeqESRsE\n\tSaCbXjq7i1v1yzmUivFiu7ShM/m5gqUMFPQoA=", "Date": "Sun, 21 Sep 2008 13:15:51 +0200", "From": "Jarek Poplawski <jarkao2@gmail.com>", "To": "David Miller <davem@davemloft.net>", "Cc": "herbert@gondor.apana.org.au, netdev@vger.kernel.org,\n\tkaber@trash.net", "Subject": "Re: [PATCH take 2] pkt_sched: Fix qdisc_watchdog() vs.\n\tdev_deactivate() race", "Message-ID": "<20080921111551.GB2551@ami.dom.local>", "References": "<20080920234843.GA2531@ami.dom.local>\n\t<20080920.223538.130375517.davem@davemloft.net>\n\t<20080921095705.GA2551@ami.dom.local>\n\t<20080921.031829.86500526.davem@davemloft.net>", "MIME-Version": "1.0", "Content-Type": "text/plain; charset=us-ascii", "Content-Disposition": "inline", "In-Reply-To": "<20080921.031829.86500526.davem@davemloft.net>", "User-Agent": "Mutt/1.5.18 (2008-05-17)", "Sender": "netdev-owner@vger.kernel.org", "Precedence": "bulk", "List-ID": "<netdev.vger.kernel.org>", "X-Mailing-List": "netdev@vger.kernel.org" }, "content": "On Sun, Sep 21, 2008 at 03:18:29AM -0700, David Miller wrote:\n> From: Jarek Poplawski <jarkao2@gmail.com>\n> Date: Sun, 21 Sep 2008 11:57:06 +0200\n...\n> > BTW, since this problem is strongly conected with the requeuing\n> > policy, I wonder why you seemingly lost interest in this. I tried to\n> > advocate for your simple, one level requeuing, but also Herbert's\n> > peek, and Alexander's early detection, after some polish(!), should\n> > make this initial test meaningless.\n> \n> Yes, thanks for reminding me about the the multiq qdisc head of line\n> blocking thing.\n> \n> I really don't like the requeue/peek patches, because they resulted in\n> so much code duplication in the CBQ and other classful qdiscs.\n> \n> Alexander's patch has similar code duplication issues.\n\nThat's why I think you should reconsider this simple solution for now,\nuntil somebody proves this is wrong or something else is better.\n\nJarek P.\n\n[RESEND]\nFrom: Jarek Poplawski <jarkao2@gmail.com>\nNewsgroups: gmane.linux.network\nSubject: Re: [RFC PATCH] sched: only dequeue if packet can be queued to\n\thardware queue.\nDate: Fri, 19 Sep 2008 09:17:53 +0000\nArchived-At: <http://permalink.gmane.org/gmane.linux.network/106324>\n\nOn Thu, Sep 18, 2008 at 06:11:45PM -0700, Alexander Duyck wrote:\n> Once again if you have a suggestion on approach feel free to modify\n> the patch and see how it works for you. My only concern is that there\n> are several qdiscs which won't give you the same packet twice and so\n> you don't know what is going to pop out until you go in and check.\n...\n\nActually, here is my suggestion: I think, that your obviously more\ncomplex solution should be compared with something simple which IMHO\nhas similar feature, i.e. limited overhead of requeuing.\n\nMy proposal is to use partly David's idea of killing ->ops->requeue(),\nfor now only the first two patches, so don't requeue back to the\nqdiscs, but leave qdisc->ops->requeue() for internal usage (like in\nHFSC's qdisc_peek_len() hack). Additionaly I use your idea of early\nchecking the state of tx queue to make this even lighter after\npossibly removing the entry check from qdisc_run().\n\nI attach my patch at the end, after original David's two patches.\n\nThanks,\nJarek P.\n\n---------------> patch 1/3\nSubject: [PATCH 1/9]: pkt_sched: Make qdisc->gso_skb a list.\nDate: Mon, 18 Aug 2008 01:36:47 -0700 (PDT)\nFrom: David Miller <davem@davemloft.net>\nTo: netdev@vger.kernel.org\nCC: jarkao2@gmail.com\nNewsgroups: gmane.linux.network\n\n\npkt_sched: Make qdisc->gso_skb a list.\n\nThe idea is that we can use this to get rid of\n->requeue().\n\nSigned-off-by: David S. Miller <davem@davemloft.net>\n---\n include/net/sch_generic.h | 2 +-\n net/sched/sch_generic.c | 12 +++++++-----\n 2 files changed, 8 insertions(+), 6 deletions(-)\n\n--\nTo unsubscribe from this list: send the line \"unsubscribe netdev\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html\n\n--\nTo unsubscribe from this list: send the line \"unsubscribe netdev\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html", "diff": "diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h\nindex 84d25f2..140c48b 100644\n--- a/include/net/sch_generic.h\n+++ b/include/net/sch_generic.h\n@@ -52,7 +52,7 @@ struct Qdisc\n \tu32\t\t\tparent;\n \tatomic_t\t\trefcnt;\n \tunsigned long\t\tstate;\n-\tstruct sk_buff\t\t*gso_skb;\n+\tstruct sk_buff_head\trequeue;\n \tstruct sk_buff_head\tq;\n \tstruct netdev_queue\t*dev_queue;\n \tstruct Qdisc\t\t*next_sched;\ndiff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c\nindex 6f96b7b..39d969e 100644\n--- a/net/sched/sch_generic.c\n+++ b/net/sched/sch_generic.c\n@@ -45,7 +45,7 @@ static inline int qdisc_qlen(struct Qdisc *q)\n static inline int dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)\n {\n \tif (unlikely(skb->next))\n-\t\tq->gso_skb = skb;\n+\t\t__skb_queue_head(&q->requeue, skb);\n \telse\n \t\tq->ops->requeue(skb, q);\n \n@@ -57,9 +57,8 @@ static inline struct sk_buff *dequeue_skb(struct Qdisc *q)\n {\n \tstruct sk_buff *skb;\n \n-\tif ((skb = q->gso_skb))\n-\t\tq->gso_skb = NULL;\n-\telse\n+\tskb = __skb_dequeue(&q->requeue);\n+\tif (!skb)\n \t\tskb = q->dequeue(q);\n \n \treturn skb;\n@@ -328,6 +327,7 @@ struct Qdisc noop_qdisc = {\n \t.flags\t\t=\tTCQ_F_BUILTIN,\n \t.ops\t\t=\t&noop_qdisc_ops,\n \t.list\t\t=\tLIST_HEAD_INIT(noop_qdisc.list),\n+\t.requeue.lock\t=\t__SPIN_LOCK_UNLOCKED(noop_qdisc.q.lock),\n \t.q.lock\t\t=\t__SPIN_LOCK_UNLOCKED(noop_qdisc.q.lock),\n \t.dev_queue\t=\t&noop_netdev_queue,\n };\n@@ -353,6 +353,7 @@ static struct Qdisc noqueue_qdisc = {\n \t.flags\t\t=\tTCQ_F_BUILTIN,\n \t.ops\t\t=\t&noqueue_qdisc_ops,\n \t.list\t\t=\tLIST_HEAD_INIT(noqueue_qdisc.list),\n+\t.requeue.lock\t=\t__SPIN_LOCK_UNLOCKED(noqueue_qdisc.q.lock),\n \t.q.lock\t\t=\t__SPIN_LOCK_UNLOCKED(noqueue_qdisc.q.lock),\n \t.dev_queue\t=\t&noqueue_netdev_queue,\n };\n@@ -473,6 +474,7 @@ struct Qdisc *qdisc_alloc(struct netdev_queue *dev_queue,\n \tsch->padded = (char *) sch - (char *) p;\n \n \tINIT_LIST_HEAD(&sch->list);\n+\tskb_queue_head_init(&sch->requeue);\n \tskb_queue_head_init(&sch->q);\n \tsch->ops = ops;\n \tsch->enqueue = ops->enqueue;\n@@ -543,7 +545,7 @@ void qdisc_destroy(struct Qdisc *qdisc)\n \tmodule_put(ops->owner);\n \tdev_put(qdisc_dev(qdisc));\n \n-\tkfree_skb(qdisc->gso_skb);\n+\t__skb_queue_purge(&qdisc->requeue);\n \n \tkfree((char *) qdisc - qdisc->padded);\n }\n\n\n-------------> patch 2/3\nSubject: [PATCH 2/9]: pkt_sched: Always use q->requeue in dev_requeue_skb().\nDate: Mon, 18 Aug 2008 01:36:50 -0700 (PDT)\nFrom: David Miller <davem@davemloft.net>\nTo: netdev@vger.kernel.org\nCC: jarkao2@gmail.com\nNewsgroups: gmane.linux.network\n\n\npkt_sched: Always use q->requeue in dev_requeue_skb().\n\nThere is no reason to call into the complicated qdiscs\njust to remember the last SKB where we found the device\nblocked.\n\nThe SKB is outside of the qdiscs realm at this point.\n\nSigned-off-by: David S. Miller <davem@davemloft.net>\n---\n net/sched/sch_generic.c | 5 +----\n 1 files changed, 1 insertions(+), 4 deletions(-)\n\ndiff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c\nindex 39d969e..96d7d08 100644\n--- a/net/sched/sch_generic.c\n+++ b/net/sched/sch_generic.c\n@@ -44,10 +44,7 @@ static inline int qdisc_qlen(struct Qdisc *q)\n \n static inline int dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)\n {\n-\tif (unlikely(skb->next))\n-\t\t__skb_queue_head(&q->requeue, skb);\n-\telse\n-\t\tq->ops->requeue(skb, q);\n+\t__skb_queue_head(&q->requeue, skb);\n \n \t__netif_schedule(q);\n \treturn 0;\n\n\n----------------> patch 3/3\n\npkt_sched: Check the state of tx_queue in dequeue_skb()\n\nCheck in dequeue_skb() the state of tx_queue for requeued skb to save\non locking and re-requeuing, and possibly remove the current check in\nqdisc_run(). Based on the idea of Alexander Duyck.\n\nSigned-off-by: Jarek Poplawski <jarkao2@gmail.com>\n\n---\n\ndiff -Nurp net-next-2.6-/net/sched/sch_generic.c net-next-2.6+/net/sched/sch_generic.c\n--- net-next-2.6-/net/sched/sch_generic.c\t2008-09-19 07:21:44.000000000 +0000\n+++ net-next-2.6+/net/sched/sch_generic.c\t2008-09-19 07:49:15.000000000 +0000\n@@ -52,11 +52,21 @@ static inline int dev_requeue_skb(struct\n \n static inline struct sk_buff *dequeue_skb(struct Qdisc *q)\n {\n-\tstruct sk_buff *skb;\n+\tstruct sk_buff *skb = skb_peek(&q->requeue);\n \n-\tskb = __skb_dequeue(&q->requeue);\n-\tif (!skb)\n+\tif (unlikely(skb)) {\n+\t\tstruct net_device *dev = qdisc_dev(q);\n+\t\tstruct netdev_queue *txq;\n+\n+\t\t/* check the reason of requeuing without tx lock first */\n+\t\ttxq = netdev_get_tx_queue(dev, skb_get_queue_mapping(skb));\n+\t\tif (!netif_tx_queue_stopped(txq) && !netif_tx_queue_frozen(txq))\n+\t\t\t__skb_unlink(skb, &q->requeue);\n+\t\telse\n+\t\t\tskb = NULL;\n+\t} else {\n \t\tskb = q->dequeue(q);\n+\t}\n \n \treturn skb;\n }\n", "prefixes": [ "take", "2" ] }