Patch Detail
get:
Show a patch.
patch:
Update a patch.
put:
Update a patch.
GET /api/patches/1243/?format=api
{ "id": 1243, "url": "http://patchwork.ozlabs.org/api/patches/1243/?format=api", "web_url": "http://patchwork.ozlabs.org/project/netdev/patch/48D9EC4B.4050804@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": "<48D9EC4B.4050804@iki.fi>", "list_archive_url": null, "date": "2008-09-24T07:29:15", "name": "xfrm_state locking regression...", "commit_ref": null, "pull_url": null, "state": "changes-requested", "archived": true, "hash": "7e6ff648d2a4080a4f3525345dfd1ae3401192ae", "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/48D9EC4B.4050804@iki.fi/mbox/", "series": [], "comments": "http://patchwork.ozlabs.org/api/patches/1243/comments/", "check": "pending", "checks": "http://patchwork.ozlabs.org/api/patches/1243/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 AB0FADDEED\n\tfor <patchwork-incoming@ozlabs.org>;\n\tWed, 24 Sep 2008 17:29:44 +1000 (EST)", "(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751234AbYIXH30 (ORCPT <rfc822;patchwork-incoming@ozlabs.org>);\n\tWed, 24 Sep 2008 03:29:26 -0400", "(majordomo@vger.kernel.org) by vger.kernel.org id S1751280AbYIXH3Z\n\t(ORCPT <rfc822; netdev-outgoing>); Wed, 24 Sep 2008 03:29:25 -0400", "from ey-out-2122.google.com ([74.125.78.25]:36748 \"EHLO\n\tey-out-2122.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1750875AbYIXH3Y (ORCPT\n\t<rfc822;netdev@vger.kernel.org>); Wed, 24 Sep 2008 03:29:24 -0400", "by ey-out-2122.google.com with SMTP id 6so725099eyi.37\n\tfor <netdev@vger.kernel.org>; Wed, 24 Sep 2008 00:29:22 -0700 (PDT)", "by 10.210.125.7 with SMTP id x7mr7990061ebc.96.1222241362068;\n\tWed, 24 Sep 2008 00:29:22 -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 f6sm6876551nfh.12.2008.09.24.00.29.18\n\t(version=SSLv3 cipher=RC4-MD5);\n\tWed, 24 Sep 2008 00:29:20 -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=x31VbE5JMyvSRKGsjuECC2bhJygVTbKAlIFtBVdpelo=;\n\tb=RCIvszHjfR89R3p5OFKzc/2s2PVk6zuZW7Nw3ixSm8lMH2iWewMZYRvJ1xCaudvT2G\n\tU5bgkPE/7/0zPytYW4HLMMIcqtTzt+jVUXWKoEsA7tozi5ssygh3nCgyEUeCrWnKWX0p\n\twFIw9xDh4w19gCodU0CDaPKAroAB6sjCS0Kc8=", "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=b0O7Y5Zq42GWSNBUqq9JcRQ644nmMrIhEn7bI8btAXu794bWeJkKQWC2I5aOefOOJu\n\tYgkmwbZUQLGnHA0loBD5eEaSvzYKlhU3VVK3VG1ioUZlUoAVAqBRxS8ltTqZ0lqCDjJ2\n\tS5dxpHkLB6VvWAIp7wX6ZVGTUXmMr2h1rxccU=", "Message-ID": "<48D9EC4B.4050804@iki.fi>", "Date": "Wed, 24 Sep 2008 10:29:15 +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": "<20080923133234.GA30370@gondor.apana.org.au>\n\t<48D8F337.2050103@iki.fi>\n\t<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>", "In-Reply-To": "<20080924062138.GA6764@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 09:20:10AM +0300, Timo Teräs wrote:\n>> Just saying that the code easily misleads the reader. And in\n>> this kind of non-obvious places we should have some comments.\n>> Or make the code more readable by adding the intermediate struct.\n> \n> Fair enough. Feel free to reformat the patch and add the struct\n> to make it more bullet-proof. I'm sorry I've got some paid work\n> to do now :)\n\nOk, here's one try. I also updated the list_add() calls introduced\nin commit \"ipsec: Restore larval states and socket policies in\ndump\".\n\nSame warning still applies: only compile tested.\n\nWe should probably do similar fix for the SPD dumping. I can make\na patch for that later today.\n\n- Timo\n\nipsec: Put 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--\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..f2deae0 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,56 @@ 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-\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": [] }