Patch Detail

HTTP 200 OK
Allow: GET, PUT, PATCH, HEAD, OPTIONS
Content-Type: application/json
Vary: Accept

{
    "id": 1019,
    "url": "http://patchwork.ozlabs.org/api/patches/1019/?format=api",
    "web_url": "http://patchwork.ozlabs.org/project/netdev/patch/48D88BCC.5030806@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": "<48D88BCC.5030806@iki.fi>",
    "list_archive_url": null,
    "date": "2008-09-23T06:25:16",
    "name": "xfrm_state locking regression...",
    "commit_ref": null,
    "pull_url": null,
    "state": "changes-requested",
    "archived": true,
    "hash": "36afa6624983f8907f58f23311bb0544eb850583",
    "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/48D88BCC.5030806@iki.fi/mbox/",
    "series": [],
    "comments": "http://patchwork.ozlabs.org/api/patches/1019/comments/",
    "check": "pending",
    "checks": "http://patchwork.ozlabs.org/api/patches/1019/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 6A991DDDE7\n\tfor <patchwork-incoming@ozlabs.org>;\n\tTue, 23 Sep 2008 16:25:32 +1000 (EST)",
            "(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752267AbYIWGZW (ORCPT <rfc822;patchwork-incoming@ozlabs.org>);\n\tTue, 23 Sep 2008 02:25:22 -0400",
            "(majordomo@vger.kernel.org) by vger.kernel.org id S1752211AbYIWGZW\n\t(ORCPT <rfc822; netdev-outgoing>); Tue, 23 Sep 2008 02:25:22 -0400",
            "from nf-out-0910.google.com ([64.233.182.191]:2226 \"EHLO\n\tnf-out-0910.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1752202AbYIWGZV (ORCPT\n\t<rfc822;netdev@vger.kernel.org>); Tue, 23 Sep 2008 02:25:21 -0400",
            "by nf-out-0910.google.com with SMTP id d3so650024nfc.21\n\tfor <netdev@vger.kernel.org>; Mon, 22 Sep 2008 23:25:19 -0700 (PDT)",
            "by 10.210.18.8 with SMTP id 8mr2981154ebr.189.1222151119263;\n\tMon, 22 Sep 2008 23:25:19 -0700 (PDT)",
            "from ?10.254.3.100? ( [83.150.94.239])\n\tby mx.google.com with ESMTPS id d2sm5587876nfc.20.2008.09.22.23.25.17\n\t(version=SSLv3 cipher=RC4-MD5);\n\tMon, 22 Sep 2008 23:25:18 -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=DCNv9xcPvZeLvLLkWS4Qfv6xM8UERp35ByoZPz4+nhw=;\n\tb=b4xKQkW83/yCcDKo/XWqq5Z0oCx99WN8hfQVfXzR1pYJib+rtygAh8bM6bevoCVEhJ\n\tpgyrBbJlxktf3Qj7f9WJmKkXUasdHFpb02nIcYqLRzf3ngCbdYqg+5wmelHwTZZFvmun\n\tsoZVWv3jehOz5Iq/ravJBXe0GPxG5wd6HFfAA=",
        "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=bCuJEijujvEphyAy6IdcPCVLd0aSs9EnrqiKMiHBAcLjHMVS3PLn7gWYJgW/PuTwNm\n\ttZAN1Nkn0kyn0krA20kGeNKojxJQjdiZ4Kgw8jlMULoxtr+0NZDBXXBOmcT6a+9WlWZC\n\tKeQT/8PSMZ4e7pV2P7jhHAzuQIwU8FtXK55Qc=",
        "Message-ID": "<48D88BCC.5030806@iki.fi>",
        "Date": "Tue, 23 Sep 2008 09:25:16 +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",
        "Subject": "Re: xfrm_state locking regression...",
        "References": "<20080908.172513.162820960.davem@davemloft.net>\n\t<20080909143312.GA29952@gondor.apana.org.au>\n\t<48D63E3A.90301@iki.fi> <48D66677.2040309@iki.fi>\n\t<20080922114256.GA27055@gondor.apana.org.au>\n\t<48D7971A.5050107@iki.fi>\n\t<20080922235012.GA23658@gondor.apana.org.au>\n\t<48D8763E.4030607@iki.fi>\n\t<20080923045951.GA26048@gondor.apana.org.au>\n\t<48D87BDA.8040804@iki.fi>\n\t<20080923052239.GA26233@gondor.apana.org.au>",
        "In-Reply-To": "<20080923052239.GA26233@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 Tue, Sep 23, 2008 at 08:17:14AM +0300, Timo Teräs wrote:\n>> The extra step there wold be a hold call. The recursive _put on a\n>> _put of some entry can happen only on dump path. As otherwise the\n>> ->next entry is first held in state delete, but would be immediately\n>> _put on the _put as the final step of _delete().\n>>\n>> So basically one additional atomic_inc() and one atomic_dec() on the\n>> normal _delete() path.\n> \n> Can you post a patch? If this can be done locklessly then yes\n> it would probably be a good way to go.  However, I'm not sure\n> whether I understand your lockless proposal yet.\n\nSomething like this. I just compile tested, so I'm not sure if it\nactually works.\n\nThis basically reverts the GC changes. The interesting bits are in\nxfrm_state_delete(). It will _hold() the ->next entry unless we\nwere at last entry. This will make sure that when an entry is\nin XFRM_STATE_DEAD the all.next is valid all the way until the\nentry is destroyed.\n\nThe corresponding put is in _destroy(). Added it as a final thing\nto do so hopefully compiler optimizes tail recursion to iteration.\n(Seemed to do that in my case.)\n\nAnd finally, _walk() was right all along. It returns to dumping\nand first skips all dead entries. And just before return, when\nall dead entries are already skipped the originally held entry\nis _put(). That _put call (or the one in _walk_done()) will result\nall dead entries that were held, to be iteratively put.\n\n- Timo\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..d5cba7b 100644\n--- a/include/net/xfrm.h\n+++ b/include/net/xfrm.h\n@@ -122,7 +122,7 @@ 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@@ -1246,8 +1246,6 @@ struct xfrm6_tunnel {\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;\ndiff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c\nindex 053970e..f45f006 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,17 @@ 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+\tgc_list.first = xfrm_state_gc_list.first;\n+\tINIT_HLIST_HEAD(&xfrm_state_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@@ -553,12 +538,19 @@ EXPORT_SYMBOL(xfrm_state_alloc);\n \n void __xfrm_state_destroy(struct xfrm_state *x)\n {\n+\tstruct xfrm_state *next = NULL;\n+\n \tWARN_ON(x->km.state != XFRM_STATE_DEAD);\n \n+\tif (x->all.next != &xfrm_state_all)\n+\t\tnext = container_of(x->all.next, struct xfrm_state, all);\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+\tif (next != NULL)\n+\t\txfrm_state_put(next);\n }\n EXPORT_SYMBOL(__xfrm_state_destroy);\n \n@@ -569,8 +561,10 @@ 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\tif (x->all.next != &xfrm_state_all)\n+\t\t\txfrm_state_hold(container_of(x->all.next,\n+\t\t\t\t\t\t     struct xfrm_state, all));\n \t\thlist_del(&x->bydst);\n \t\thlist_del(&x->bysrc);\n \t\tif (x->id.spi)\n@@ -1612,33 +1606,15 @@ void xfrm_state_walk_init(struct xfrm_state_walk *walk, u8 proto)\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 }\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-\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 }\n EXPORT_SYMBOL(xfrm_state_walk_done);\n \n",
    "prefixes": []
}