Patch Detail
get:
Show a patch.
patch:
Update a patch.
put:
Update a patch.
GET /api/patches/1283/?format=api
{ "id": 1283, "url": "http://patchwork.ozlabs.org/api/patches/1283/?format=api", "web_url": "http://patchwork.ozlabs.org/project/netdev/patch/48DA3E2D.4070909@iki.fi/", "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": "<48DA3E2D.4070909@iki.fi>", "list_archive_url": null, "date": "2008-09-24T13:18:37", "name": "xfrm_state locking regression...", "commit_ref": null, "pull_url": null, "state": "changes-requested", "archived": true, "hash": "a7969c030180463fc4185330533522d8d32512f9", "submitter": { "id": 393, "url": "http://patchwork.ozlabs.org/api/people/393/?format=api", "name": "Timo Teras", "email": "timo.teras@iki.fi" }, "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/48DA3E2D.4070909@iki.fi/mbox/", "series": [], "comments": "http://patchwork.ozlabs.org/api/patches/1283/comments/", "check": "pending", "checks": "http://patchwork.ozlabs.org/api/patches/1283/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 B80FADDE03\n\tfor <patchwork-incoming@ozlabs.org>;\n\tWed, 24 Sep 2008 23:19:05 +1000 (EST)", "(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752723AbYIXNSs (ORCPT <rfc822;patchwork-incoming@ozlabs.org>);\n\tWed, 24 Sep 2008 09:18:48 -0400", "(majordomo@vger.kernel.org) by vger.kernel.org id S1752600AbYIXNSs\n\t(ORCPT <rfc822; netdev-outgoing>); Wed, 24 Sep 2008 09:18:48 -0400", "from ey-out-2122.google.com ([74.125.78.26]:59640 \"EHLO\n\tey-out-2122.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1752203AbYIXNSq (ORCPT\n\t<rfc822;netdev@vger.kernel.org>); Wed, 24 Sep 2008 09:18:46 -0400", "by ey-out-2122.google.com with SMTP id 6so760018eyi.37\n\tfor <netdev@vger.kernel.org>; Wed, 24 Sep 2008 06:18:44 -0700 (PDT)", "by 10.210.62.12 with SMTP id k12mr8459562eba.17.1222262324685;\n\tWed, 24 Sep 2008 06:18:44 -0700 (PDT)", "from ?10.254.3.100? (xdsl-83-150-94-239.nebulazone.fi\n\t[83.150.94.239])\n\tby mx.google.com with ESMTPS id k7sm1596448nfh.17.2008.09.24.06.18.42\n\t(version=SSLv3 cipher=RC4-MD5);\n\tWed, 24 Sep 2008 06:18:43 -0700 (PDT)" ], "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma;\n\th=domainkey-signature:received:received:message-id:date:from\n\t:user-agent:mime-version:to:cc:subject:references:in-reply-to\n\t:x-enigmail-version:content-type:content-transfer-encoding:sender;\n\tbh=RXMxjecOEh3TzZddqixolZ8HyWSEtqJL0HsNip9QW+Q=;\n\tb=cogr091sJ+9nEZJ8HKXU2aTbnfchHL/ugQj6HxUxpiVoy7tRFulE3C6OkHdzCR+rlI\n\tn3dyIThLcTKZe87KBU0Rs02nW0C7jEsooeGUkObdphNPXPYj7ch5rnZtRxP8kwv+4Hy8\n\tvVVKXaPHheW885kVyRyt/JfZAoHa21ny+gX6I=", "DomainKey-Signature": "a=rsa-sha1; c=nofws; d=gmail.com; s=gamma;\n\th=message-id:date:from:user-agent:mime-version:to:cc:subject\n\t:references:in-reply-to:x-enigmail-version:content-type\n\t:content-transfer-encoding:sender;\n\tb=iL+R4l/3riTUPNLfSRBPVj3033G07yLdpCCbENZ4ouI5Wzi2HhMSbMguA9uPj3FaAW\n\tccVIwFx+pfVY/iWDRSfrJOBDmstKOxPFm0ipvm4raZSMvNsFb6uHcBRdheddwg6Nqr17\n\tf6qsrBme4O4hjy00fwQdwMkniUSZ50QAqp4L4=", "Message-ID": "<48DA3E2D.4070909@iki.fi>", "Date": "Wed, 24 Sep 2008 16:18:37 +0300", "From": "=?ISO-8859-1?Q?Timo_Ter=E4s?= <timo.teras@iki.fi>", "User-Agent": "Thunderbird 2.0.0.16 (X11/20080724)", "MIME-Version": "1.0", "To": "Herbert Xu <herbert@gondor.apana.org.au>", "CC": "David Miller <davem@davemloft.net>, netdev@vger.kernel.org,\n\tjamal <hadi@cyberus.ca>", "Subject": "Re: xfrm_state locking regression...", "References": "<20080924042349.GA5419@gondor.apana.org.au>\n\t<48D9CCA3.2040500@iki.fi>\n\t<20080924051555.GA6218@gondor.apana.org.au>\n\t<48D9D421.9050500@iki.fi>\n\t<20080924055550.GA6506@gondor.apana.org.au>\n\t<48D9D86E.7020205@iki.fi>\n\t<20080924061349.GA6679@gondor.apana.org.au>\n\t<48D9DC1A.10903@iki.fi>\n\t<20080924062138.GA6764@gondor.apana.org.au>\n\t<48D9EC4B.4050804@iki.fi>\n\t<20080924075441.GA7391@gondor.apana.org.au>", "In-Reply-To": "<20080924075441.GA7391@gondor.apana.org.au>", "X-Enigmail-Version": "0.95.0", "Content-Type": "text/plain; charset=ISO-8859-1", "Content-Transfer-Encoding": "8bit", "Sender": "netdev-owner@vger.kernel.org", "Precedence": "bulk", "List-ID": "<netdev.vger.kernel.org>", "X-Mailing-List": "netdev@vger.kernel.org" }, "content": "Herbert Xu wrote:\n> On Wed, Sep 24, 2008 at 10:29:15AM +0300, Timo Teräs wrote:\n>> Same warning still applies: only compile tested.\n> \n> Looks good to me, I espeically like the count/seq bit. But I'm\n> sure Dave would appreciate it if we actually ran this too :)\n\nGave it a test spin and found one bug. The walking routine will\nget called one extra time when it has successfully dumped\neverything so we need to check for that in the beginning; the\nearlier version would start all over from the beginning resulting\nnever ending dumps.\n\nSo the patch is now as follows. I have an almost ready patch\nfor xfrm_policy dumping too. Will finish it and send tomorrow.\n\n- Timo\n\nipsec: Put xfrm_state dumpers on the dump list\n\nBased on Herbert Xu's patch and idea. Improved the original patch\nto apply cleanly and modified iteration code to be more readable\nby using a common struct for entries in all list.\n\nAs it is we go to extraordinary lengths to ensure that states\ndon't go away while dumpers go to sleep. It's much easier if\nwe just put the dumpers themselves on the list since they can't\ngo away while they're going.\n\nI've also changed the order of addition on new states to prevent\na never-ending dump.\n\nFinally the obsolete last optimisation is now gone.\n\nSigned-off-by: Herbert Xu <herbert@gondor.apana.org.au>\nSigned-off-by: Timo Teras <timo.teras@iki.fi>\n---\n include/linux/netlink.h | 2 +-\n include/net/xfrm.h | 29 ++++++-------\n net/xfrm/xfrm_state.c | 109 +++++++++++++++-------------------------------\n 3 files changed, 50 insertions(+), 90 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", "diff": "diff --git a/include/linux/netlink.h b/include/linux/netlink.h\nindex cbba776..9ff1b54 100644\n--- a/include/linux/netlink.h\n+++ b/include/linux/netlink.h\n@@ -220,7 +220,7 @@ struct netlink_callback\n \tint\t\t(*dump)(struct sk_buff * skb, struct netlink_callback *cb);\n \tint\t\t(*done)(struct netlink_callback *cb);\n \tint\t\tfamily;\n-\tlong\t\targs[7];\n+\tlong\t\targs[6];\n };\n \n struct netlink_notify\ndiff --git a/include/net/xfrm.h b/include/net/xfrm.h\nindex 48630b2..becaa1e 100644\n--- a/include/net/xfrm.h\n+++ b/include/net/xfrm.h\n@@ -117,12 +117,21 @@ extern struct mutex xfrm_cfg_mutex;\n metrics. Plus, it will be made via sk->sk_dst_cache. Solved.\n */\n \n+struct xfrm_state_walk {\n+\tstruct list_head\tall;\n+\tu8\t\t\tstate;\n+\tunion {\n+\t\tu8\t\tdying;\n+\t\tu8\t\tproto;\n+\t};\n+\tu32\t\t\tseq;\n+};\n+\n /* Full description of state of transformer. */\n struct xfrm_state\n {\n-\tstruct list_head\tall;\n \tunion {\n-\t\tstruct list_head\tgclist;\n+\t\tstruct hlist_node\tgclist;\n \t\tstruct hlist_node\tbydst;\n \t};\n \tstruct hlist_node\tbysrc;\n@@ -136,12 +145,8 @@ struct xfrm_state\n \n \tu32\t\t\tgenid;\n \n-\t/* Key manger bits */\n-\tstruct {\n-\t\tu8\t\tstate;\n-\t\tu8\t\tdying;\n-\t\tu32\t\tseq;\n-\t} km;\n+\t/* Key manager bits */\n+\tstruct xfrm_state_walk\tkm;\n \n \t/* Parameters of this state. */\n \tstruct {\n@@ -1245,14 +1250,6 @@ struct xfrm6_tunnel {\n \tint priority;\n };\n \n-struct xfrm_state_walk {\n-\tstruct list_head list;\n-\tunsigned long genid;\n-\tstruct xfrm_state *state;\n-\tint count;\n-\tu8 proto;\n-};\n-\n struct xfrm_policy_walk {\n \tstruct xfrm_policy *policy;\n \tint count;\ndiff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c\nindex 053970e..747fd8c 100644\n--- a/net/xfrm/xfrm_state.c\n+++ b/net/xfrm/xfrm_state.c\n@@ -59,14 +59,6 @@ static unsigned int xfrm_state_hashmax __read_mostly = 1 * 1024 * 1024;\n static unsigned int xfrm_state_num;\n static unsigned int xfrm_state_genid;\n \n-/* Counter indicating ongoing walk, protected by xfrm_state_lock. */\n-static unsigned long xfrm_state_walk_ongoing;\n-/* Counter indicating walk completion, protected by xfrm_cfg_mutex. */\n-static unsigned long xfrm_state_walk_completed;\n-\n-/* List of outstanding state walks used to set the completed counter. */\n-static LIST_HEAD(xfrm_state_walks);\n-\n static struct xfrm_state_afinfo *xfrm_state_get_afinfo(unsigned int family);\n static void xfrm_state_put_afinfo(struct xfrm_state_afinfo *afinfo);\n \n@@ -199,8 +191,7 @@ static DEFINE_RWLOCK(xfrm_state_afinfo_lock);\n static struct xfrm_state_afinfo *xfrm_state_afinfo[NPROTO];\n \n static struct work_struct xfrm_state_gc_work;\n-static LIST_HEAD(xfrm_state_gc_leftovers);\n-static LIST_HEAD(xfrm_state_gc_list);\n+static HLIST_HEAD(xfrm_state_gc_list);\n static DEFINE_SPINLOCK(xfrm_state_gc_lock);\n \n int __xfrm_state_delete(struct xfrm_state *x);\n@@ -412,23 +403,16 @@ static void xfrm_state_gc_destroy(struct xfrm_state *x)\n \n static void xfrm_state_gc_task(struct work_struct *data)\n {\n-\tstruct xfrm_state *x, *tmp;\n-\tunsigned long completed;\n+\tstruct xfrm_state *x;\n+\tstruct hlist_node *entry, *tmp;\n+\tstruct hlist_head gc_list;\n \n-\tmutex_lock(&xfrm_cfg_mutex);\n \tspin_lock_bh(&xfrm_state_gc_lock);\n-\tlist_splice_tail_init(&xfrm_state_gc_list, &xfrm_state_gc_leftovers);\n+\thlist_move_list(&xfrm_state_gc_list, &gc_list);\n \tspin_unlock_bh(&xfrm_state_gc_lock);\n \n-\tcompleted = xfrm_state_walk_completed;\n-\tmutex_unlock(&xfrm_cfg_mutex);\n-\n-\tlist_for_each_entry_safe(x, tmp, &xfrm_state_gc_leftovers, gclist) {\n-\t\tif ((long)(x->lastused - completed) > 0)\n-\t\t\tbreak;\n-\t\tlist_del(&x->gclist);\n+\thlist_for_each_entry_safe(x, entry, tmp, &gc_list, gclist)\n \t\txfrm_state_gc_destroy(x);\n-\t}\n \n \twake_up(&km_waitq);\n }\n@@ -529,7 +513,7 @@ struct xfrm_state *xfrm_state_alloc(void)\n \tif (x) {\n \t\tatomic_set(&x->refcnt, 1);\n \t\tatomic_set(&x->tunnel_users, 0);\n-\t\tINIT_LIST_HEAD(&x->all);\n+\t\tINIT_LIST_HEAD(&x->km.all);\n \t\tINIT_HLIST_NODE(&x->bydst);\n \t\tINIT_HLIST_NODE(&x->bysrc);\n \t\tINIT_HLIST_NODE(&x->byspi);\n@@ -556,7 +540,7 @@ void __xfrm_state_destroy(struct xfrm_state *x)\n \tWARN_ON(x->km.state != XFRM_STATE_DEAD);\n \n \tspin_lock_bh(&xfrm_state_gc_lock);\n-\tlist_add_tail(&x->gclist, &xfrm_state_gc_list);\n+\thlist_add_head(&x->gclist, &xfrm_state_gc_list);\n \tspin_unlock_bh(&xfrm_state_gc_lock);\n \tschedule_work(&xfrm_state_gc_work);\n }\n@@ -569,8 +553,7 @@ int __xfrm_state_delete(struct xfrm_state *x)\n \tif (x->km.state != XFRM_STATE_DEAD) {\n \t\tx->km.state = XFRM_STATE_DEAD;\n \t\tspin_lock(&xfrm_state_lock);\n-\t\tx->lastused = xfrm_state_walk_ongoing;\n-\t\tlist_del_rcu(&x->all);\n+\t\tlist_del(&x->km.all);\n \t\thlist_del(&x->bydst);\n \t\thlist_del(&x->bysrc);\n \t\tif (x->id.spi)\n@@ -871,7 +854,7 @@ xfrm_state_find(xfrm_address_t *daddr, xfrm_address_t *saddr,\n \n \t\tif (km_query(x, tmpl, pol) == 0) {\n \t\t\tx->km.state = XFRM_STATE_ACQ;\n-\t\t\tlist_add_tail(&x->all, &xfrm_state_all);\n+\t\t\tlist_add(&x->km.all, &xfrm_state_all);\n \t\t\thlist_add_head(&x->bydst, xfrm_state_bydst+h);\n \t\t\th = xfrm_src_hash(daddr, saddr, family);\n \t\t\thlist_add_head(&x->bysrc, xfrm_state_bysrc+h);\n@@ -940,7 +923,7 @@ static void __xfrm_state_insert(struct xfrm_state *x)\n \n \tx->genid = ++xfrm_state_genid;\n \n-\tlist_add_tail(&x->all, &xfrm_state_all);\n+\tlist_add(&x->km.all, &xfrm_state_all);\n \n \th = xfrm_dst_hash(&x->id.daddr, &x->props.saddr,\n \t\t\t x->props.reqid, x->props.family);\n@@ -1069,7 +1052,7 @@ static struct xfrm_state *__find_acq_core(unsigned short family, u8 mode, u32 re\n \t\txfrm_state_hold(x);\n \t\tx->timer.expires = jiffies + sysctl_xfrm_acq_expires*HZ;\n \t\tadd_timer(&x->timer);\n-\t\tlist_add_tail(&x->all, &xfrm_state_all);\n+\t\tlist_add(&x->km.all, &xfrm_state_all);\n \t\thlist_add_head(&x->bydst, xfrm_state_bydst+h);\n \t\th = xfrm_src_hash(daddr, saddr, family);\n \t\thlist_add_head(&x->bysrc, xfrm_state_bysrc+h);\n@@ -1566,79 +1549,59 @@ int xfrm_state_walk(struct xfrm_state_walk *walk,\n \t\t int (*func)(struct xfrm_state *, int, void*),\n \t\t void *data)\n {\n-\tstruct xfrm_state *old, *x, *last = NULL;\n+\tstruct xfrm_state *state;\n+\tstruct xfrm_state_walk *x;\n \tint err = 0;\n \n-\tif (walk->state == NULL && walk->count != 0)\n+\tif (walk->seq != 0 && list_empty(&walk->all))\n \t\treturn 0;\n \n-\told = x = walk->state;\n-\twalk->state = NULL;\n \tspin_lock_bh(&xfrm_state_lock);\n-\tif (x == NULL)\n-\t\tx = list_first_entry(&xfrm_state_all, struct xfrm_state, all);\n+\tif (list_empty(&walk->all))\n+\t\tx = list_first_entry(&xfrm_state_all, struct xfrm_state_walk, all);\n+\telse\n+\t\tx = list_entry(&walk->all, struct xfrm_state_walk, all);\n \tlist_for_each_entry_from(x, &xfrm_state_all, all) {\n-\t\tif (x->km.state == XFRM_STATE_DEAD)\n+\t\tif (x->state == XFRM_STATE_DEAD)\n \t\t\tcontinue;\n-\t\tif (!xfrm_id_proto_match(x->id.proto, walk->proto))\n+\t\tstate = container_of(x, struct xfrm_state, km);\n+\t\tif (!xfrm_id_proto_match(state->id.proto, walk->proto))\n \t\t\tcontinue;\n-\t\tif (last) {\n-\t\t\terr = func(last, walk->count, data);\n-\t\t\tif (err) {\n-\t\t\t\txfrm_state_hold(last);\n-\t\t\t\twalk->state = last;\n-\t\t\t\tgoto out;\n-\t\t\t}\n+\t\terr = func(state, walk->seq, data);\n+\t\tif (err) {\n+\t\t\tlist_move_tail(&walk->all, &x->all);\n+\t\t\tgoto out;\n \t\t}\n-\t\tlast = x;\n-\t\twalk->count++;\n+\t\twalk->seq++;\n \t}\n-\tif (walk->count == 0) {\n+\tif (walk->seq == 0) {\n \t\terr = -ENOENT;\n \t\tgoto out;\n \t}\n-\tif (last)\n-\t\terr = func(last, 0, data);\n+\tlist_del_init(&walk->all);\n out:\n \tspin_unlock_bh(&xfrm_state_lock);\n-\tif (old != NULL)\n-\t\txfrm_state_put(old);\n \treturn err;\n }\n EXPORT_SYMBOL(xfrm_state_walk);\n \n void xfrm_state_walk_init(struct xfrm_state_walk *walk, u8 proto)\n {\n+\tINIT_LIST_HEAD(&walk->all);\n \twalk->proto = proto;\n-\twalk->state = NULL;\n-\twalk->count = 0;\n-\tlist_add_tail(&walk->list, &xfrm_state_walks);\n-\twalk->genid = ++xfrm_state_walk_ongoing;\n+\twalk->state = XFRM_STATE_DEAD;\n+\twalk->seq = 0;\n }\n EXPORT_SYMBOL(xfrm_state_walk_init);\n \n void xfrm_state_walk_done(struct xfrm_state_walk *walk)\n {\n-\tstruct list_head *prev;\n-\n-\tif (walk->state != NULL) {\n-\t\txfrm_state_put(walk->state);\n-\t\twalk->state = NULL;\n-\t}\n-\n-\tprev = walk->list.prev;\n-\tlist_del(&walk->list);\n-\n-\tif (prev != &xfrm_state_walks) {\n-\t\tlist_entry(prev, struct xfrm_state_walk, list)->genid =\n-\t\t\twalk->genid;\n+\tif (list_empty(&walk->all))\n \t\treturn;\n-\t}\n-\n-\txfrm_state_walk_completed = walk->genid;\n \n-\tif (!list_empty(&xfrm_state_gc_leftovers))\n-\t\tschedule_work(&xfrm_state_gc_work);\n+\tspin_lock_bh(&xfrm_state_lock);\n+\tlist_del(&walk->all);\n+\tspin_lock_bh(&xfrm_state_lock);\n }\n EXPORT_SYMBOL(xfrm_state_walk_done);\n \n", "prefixes": [] }