[{"id":1765481,"web_url":"http://patchwork.ozlabs.org/comment/1765481/","msgid":"<1504886479.15310.74.camel@edumazet-glaptop3.roam.corp.google.com>","list_archive_url":null,"date":"2017-09-08T16:01:19","subject":"Re: [PATCH 3/3] net: skb_queue_purge(): lock/unlock the list only\n\tonce","submitter":{"id":2404,"url":"http://patchwork.ozlabs.org/api/people/2404/","name":"Eric Dumazet","email":"eric.dumazet@gmail.com"},"content":"On Fri, 2017-09-08 at 05:06 +0000, Michael Witten wrote:\n> Date: Thu, 7 Sep 2017 20:07:40 +0000\n> With this commit, the list's lock is locked/unlocked only once\n> for the duration of `skb_queue_purge()'.\n> \n> Hitherto, the list's lock has been locked/unlocked every time\n> an item is dequeued; this seems not only inefficient, but also\n> incorrect, as the whole point of `skb_queue_purge()' is to clear\n> the list, presumably without giving anything else a chance to\n> manipulate the list in the interim.\n> \n> Signed-off-by: Michael Witten \n> ---\n> net/core/skbuff.c | 6 +++++-\n> 1 file changed, 5 insertions(+), 1 deletion(-)\n> \n> diff --git a/net/core/skbuff.c b/net/core/skbuff.c\n> index 68065d7d383f..66c0731a2a5f 100644\n> --- a/net/core/skbuff.c\n> +++ b/net/core/skbuff.c\n> @@ -2834,9 +2834,13 @@ EXPORT_SYMBOL(skb_dequeue_tail);\n> */\n> void skb_queue_purge(struct sk_buff_head *list)\n> {\n> +\tunsigned long flags;\n> \tstruct sk_buff *skb;\n> -\twhile ((skb = skb_dequeue(list)) != NULL)\n> +\n> +\tspin_lock_irqsave(&list->lock, flags);\n> +\twhile ((skb = __skb_dequeue(list)) != NULL)\n> \t\tkfree_skb(skb);\n> +\tspin_unlock_irqrestore(&list->lock, flags);\n> }\n> EXPORT_SYMBOL(skb_queue_purge);\n> \n\n\nNo, this is very wrong :\n\nHolding hard IRQ for a potential very long time is going to break\nhorribly. Some lists can have 10,000+ skbs in them.\n\nNote that net-next tree is currently closed, please read \nDocumentation/networking/netdev-FAQ.txt","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"KrWQQh6T\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xphtZ2mCTz9t2d\n\tfor ;\n\tSat, 9 Sep 2017 02:03:02 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752229AbdIHQBZ (ORCPT );\n\tFri, 8 Sep 2017 12:01:25 -0400","from mail-pf0-f194.google.com ([209.85.192.194]:34001 \"EHLO\n\tmail-pf0-f194.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1750807AbdIHQBX (ORCPT\n\t); Fri, 8 Sep 2017 12:01:23 -0400","by mail-pf0-f194.google.com with SMTP id g65so1382629pfe.1;\n\tFri, 08 Sep 2017 09:01:23 -0700 (PDT)","from [192.168.86.171] (c-67-180-167-114.hsd1.ca.comcast.net.\n\t[67.180.167.114]) by smtp.googlemail.com with ESMTPSA id\n\tf69sm4577287pfe.70.2017.09.08.09.01.20\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tFri, 08 Sep 2017 09:01:20 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20161025;\n\th=message-id:subject:from:to:cc:date:in-reply-to:references\n\t:mime-version:content-transfer-encoding;\n\tbh=4P/45NIC+JDj+XbSYz3NeKO1tXX85JfO1aSoHJraDuU=;\n\tb=KrWQQh6TTr91TAx1e9qwYFrPE2kGyOhEDRyyk6JmLxYWRycT69yAz5bj0HAxCJSNf8\n\tDv67tuAPo/K5y24Pmq4DGy8FRlrycge410ZrrrKhKKUuxLoRCDshyU1tlNZyXHNV0ku9\n\tKUS0b9NQlDkESXgtSjfmgKMynXr7FFNty3OZdDgvdH39/NFkk5M0OEpcs94JlB91+5as\n\tQePfQUt1vfa2WL3OvYwBLACbSE3eyAw54j7AxJpjPwzXklN5gUsqNAk5ndL5+4s0FdUH\n\tTsDk4qvy57hLn3FpEOmsNU0IryAOso1uZM1prZ0zgBRyUJP7bll0f4wtU0gzrvbzFOpf\n\t8/Pg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to\n\t:references:mime-version:content-transfer-encoding;\n\tbh=4P/45NIC+JDj+XbSYz3NeKO1tXX85JfO1aSoHJraDuU=;\n\tb=RhSfrtYccl1aILsnqaGjqmwSLaXgnAgcrv024QzCA2CgoB/o+wmoaGzPf+bM7AT6Ll\n\tMIGKL/Z+Z2QjrbIQGxjEZD4Uouk1BqzgarM5aY/GexhgV5rdLFoDmeuGhL1nZJJChm+K\n\t6+Y+9TIqFbJtw2JR1y9udc+ucpyRii6r9drqDE2wVBd4nSi6NvtPQoWI44SQNmdXstlX\n\tSIwdxd1/DHHkPAqNPOi+7KeBBGpXgM8XhAfd0d+ZBjNA7UlbofOYGvYWLVr7HEB3rYdq\n\tE1cHGTwjCHDi6GchXnyqGWimMhcSYg4LXFxcoEh/8Jmr/TddXl1iErgGA6BtS590YWO5\n\tbphA==","X-Gm-Message-State":"AHPjjUgwz9IwcpFiqtc+Zc+TGKGdI2fD61wtI3MCrXHbjKPWw6VeyZU2\n\t3QfoLcAqMD81Hw==","X-Google-Smtp-Source":"ADKCNb7OuK/ZYzdDpw3dtsBw3/SnJXnUyug3fqaJC92uRQ3iXYf0a0Ty2nEQsXvu3sFAOYnFho34Zg==","X-Received":"by 10.98.69.136 with SMTP id n8mr3566737pfi.291.1504886482458;\n\tFri, 08 Sep 2017 09:01:22 -0700 (PDT)","Message-ID":"<1504886479.15310.74.camel@edumazet-glaptop3.roam.corp.google.com>","Subject":"Re: [PATCH 3/3] net: skb_queue_purge(): lock/unlock the list only\n\tonce","From":"Eric Dumazet ","To":"Michael Witten ","Cc":"\"David S. Miller\" ,\n\tAlexey Kuznetsov ,\n\tHideaki YOSHIFUJI ,\n\tnetdev@vger.kernel.org, linux-kernel@vger.kernel.org","Date":"Fri, 08 Sep 2017 09:01:19 -0700","In-Reply-To":"<60c8906b751d4915be456009c220516e-mfwitten@gmail.com>","References":"<45aab5effc0c424a992646a97cf2ec14-mfwitten@gmail.com>\n\t<60c8906b751d4915be456009c220516e-mfwitten@gmail.com>","Content-Type":"text/plain; charset=\"UTF-8\"","X-Mailer":"Evolution 3.10.4-0ubuntu2 ","Mime-Version":"1.0","Content-Transfer-Encoding":"7bit","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1765516,"web_url":"http://patchwork.ozlabs.org/comment/1765516/","msgid":"<20170908095123.098ee101@xeon-e3>","list_archive_url":null,"date":"2017-09-08T16:51:23","subject":"Re: [PATCH 3/3] net: skb_queue_purge(): lock/unlock the list only\n\tonce","submitter":{"id":21389,"url":"http://patchwork.ozlabs.org/api/people/21389/","name":"Stephen Hemminger","email":"stephen@networkplumber.org"},"content":"On Fri, 08 Sep 2017 05:06:30 -0000\nMichael Witten wrote:\n\n> Date: Thu, 7 Sep 2017 20:07:40 +0000\n> With this commit, the list's lock is locked/unlocked only once\n> for the duration of `skb_queue_purge()'.\n> \n> Hitherto, the list's lock has been locked/unlocked every time\n> an item is dequeued; this seems not only inefficient, but also\n> incorrect, as the whole point of `skb_queue_purge()' is to clear\n> the list, presumably without giving anything else a chance to\n> manipulate the list in the interim.\n> \n> Signed-off-by: Michael Witten \n> ---\n> net/core/skbuff.c | 6 +++++-\n> 1 file changed, 5 insertions(+), 1 deletion(-)\n> \n> diff --git a/net/core/skbuff.c b/net/core/skbuff.c\n> index 68065d7d383f..66c0731a2a5f 100644\n> --- a/net/core/skbuff.c\n> +++ b/net/core/skbuff.c\n> @@ -2834,9 +2834,13 @@ EXPORT_SYMBOL(skb_dequeue_tail);\n> */\n> void skb_queue_purge(struct sk_buff_head *list)\n> {\n> +\tunsigned long flags;\n> \tstruct sk_buff *skb;\n> -\twhile ((skb = skb_dequeue(list)) != NULL)\n> +\n> +\tspin_lock_irqsave(&list->lock, flags);\n> +\twhile ((skb = __skb_dequeue(list)) != NULL)\n> \t\tkfree_skb(skb);\n> +\tspin_unlock_irqrestore(&list->lock, flags);\n> }\n> EXPORT_SYMBOL(skb_queue_purge);\n> \n\nAs Eric said, this won't work.\n\nInstead why not introduce something list splice which moves next/prev\nof list head to a local list on the stack.\n\ndiff --git a/net/core/skbuff.c b/net/core/skbuff.c\nindex 68065d7d383f..4988b6efdcc8 100644\n--- a/net/core/skbuff.c\n+++ b/net/core/skbuff.c\n@@ -2824,6 +2824,44 @@ struct sk_buff *skb_dequeue_tail(struct sk_buff_head *list)\n }\n EXPORT_SYMBOL(skb_dequeue_tail);\n \n+static void __skb_splice(const struct sk_buff_head *list,\n+\t\t\t struct sk_buff *prev,\n+\t\t\t struct sk_buff *next)\n+{\n+\tstruct sk_buff *first = list->next;\n+\tstruct sk_buff *last = list->prev;\n+\n+\tlist->qlen = 0;\n+\n+\tfirst->prev = prev;\n+\tprev->next = first;\n+\n+\tlist->next = next;\n+\tnext->prev = last;\n+}\n+\n+/**\n+ *\tskb_splice - join two lists, and initialize the emptied list\n+ *\t@list: the new list to add\n+ *\t@head: the pace to add it in the first list\n+ *\n+ *\tTake the first list (@list) and merge it onto the\n+ *\thead of existing list (@head).\n+ */\n+static void skb_splice_init(const struct sk_buff_head *list,\n+\t\t\t struct sk_buff_head *head)\n+{\n+\tunsigned long flags;\n+\n+\tspin_lock_irqsave(&list->lock, flags);\n+\tif (list->qlen > 0) {\n+\t\thead->qlen += list->qlen;\n+\t\t__skb_splice(list, head, head->next);\n+\t\t__skb_queue_head_init(list);\n+\t}\n+\tspin_unlock_irqrestore(&list->lock, flags);\n+}\n+\n /**\n *\tskb_queue_purge - empty a list\n *\t@list: list to empty\n@@ -2835,7 +2873,12 @@ EXPORT_SYMBOL(skb_dequeue_tail);\n void skb_queue_purge(struct sk_buff_head *list)\n {\n \tstruct sk_buff *skb;\n-\twhile ((skb = skb_dequeue(list)) != NULL)\n+\tstruct skb_buff_head tmp;\n+\n+\t__skb_queue_head_init(&tmp);\n+\tskb_splice_init(list, &tmp);\n+\n+\twhile ((skb = __skb_dequeue(list)) != NULL)\n \t\tkfree_skb(skb);\n }\n EXPORT_SYMBOL(skb_queue_purge);","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=networkplumber-org.20150623.gappssmtp.com\n\theader.i=@networkplumber-org.20150623.gappssmtp.com\n\theader.b=\"2OX4Newk\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xpjyn0ykwz9sBd\n\tfor ;\n\tSat, 9 Sep 2017 02:51:45 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1756526AbdIHQve (ORCPT );\n\tFri, 8 Sep 2017 12:51:34 -0400","from mail-pf0-f173.google.com ([209.85.192.173]:35675 \"EHLO\n\tmail-pf0-f173.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1756216AbdIHQvc (ORCPT\n\t); Fri, 8 Sep 2017 12:51:32 -0400","by mail-pf0-f173.google.com with SMTP id g13so5364686pfm.2\n\tfor ; Fri, 08 Sep 2017 09:51:32 -0700 (PDT)","from xeon-e3 (76-14-207-240.or.wavecable.com. [76.14.207.240])\n\tby smtp.gmail.com with ESMTPSA id\n\tf88sm2112615pfh.95.2017.09.08.09.51.31\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tFri, 08 Sep 2017 09:51:32 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=networkplumber-org.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:in-reply-to:references\n\t:mime-version:content-transfer-encoding;\n\tbh=+o1MupT7GhMkkz7B9WneqeoYsGLNVHa5khjpZg5isOc=;\n\tb=2OX4NewkTjiFH/Okj153E7iM4h0FOOQf+HkiUeggCyh3BsQ7L9WfeoTmQTFe3coExE\n\taHVy1ENJNJK2VaFcQvO8lQBcI3byktB3D4gG80ITD8XAxyPQYDqU8CdgKAVhFSJly9q0\n\tjjU7743CSpUcbB9x0qPE1fxbX3yp5LpkBrv9z1OnrcN1FrI/Q7/6xPV7j+YWCDRqpWO6\n\t5Hn8+9O8cGw0+Y8FUxIjOxcD7ou9vX15tVklC/gM7YmY8pTq3Nce3wQheNciISFhiZ4B\n\taNzQg58th3sX1qQ0ZQE85flv/C7x0lQIhr/PVY6sA30HQ3M6+q+PwzRfI9yQqqqm64BI\n\tq8EA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to\n\t:references:mime-version:content-transfer-encoding;\n\tbh=+o1MupT7GhMkkz7B9WneqeoYsGLNVHa5khjpZg5isOc=;\n\tb=BC3LBbMjvHpdwXw4S674mU3YZQB6Zz4qIDymu7Ad9oRECQD4PhYFNFQr558guX8Ej1\n\tMcJbkJqgwqEh7OfT+cGktwgCYnsZBtwptOKc+Nk/PXr9BOXxZP3EJHdk/zQzn0p0dgN/\n\tJvlUTTW56vVGem60TlQn2gO3UHa21CGmzGrMQMtMwDTtgqe/97q74+PBWBW36Ii4mrpl\n\tFrapb67QTu+Hs1wpujX4rkVyPaK3SExKCHFhKfloPtO7waDyoXTGisAZ/uXa02XMn2nb\n\tgpEv0GLWIyo7i6HJXO+vY0O4lwr5D78aGfiClyFkNfZG5FVHThsxZJhhVJLgzhVpFdSw\n\ttudw==","X-Gm-Message-State":"AHPjjUgucrN19gQ7/LCqlfQvglwmZlZnvoe9scN7BaGdByhc/UfvMDzD\n\t7+LYhvyxhFgYNPMMZuIONw==","X-Google-Smtp-Source":"ADKCNb6wE5d7n5qgZqoFCNcS3Bfmed6uzC9p3xFc3nFOG/EVhtlEBQ2Ru1so1BxGlGoCkWjaKpNeEQ==","X-Received":"by 10.84.241.1 with SMTP id a1mr2658043pll.199.1504889492193;\n\tFri, 08 Sep 2017 09:51:32 -0700 (PDT)","Date":"Fri, 8 Sep 2017 09:51:23 -0700","From":"Stephen Hemminger ","To":"Michael Witten ","Cc":"\"David S. Miller\" ,\n\tAlexey Kuznetsov ,\n\tHideaki YOSHIFUJI ,\n\tnetdev@vger.kernel.org, linux-kernel@vger.kernel.org","Subject":"Re: [PATCH 3/3] net: skb_queue_purge(): lock/unlock the list only\n\tonce","Message-ID":"<20170908095123.098ee101@xeon-e3>","In-Reply-To":"<60c8906b751d4915be456009c220516e-mfwitten@gmail.com>","References":"<45aab5effc0c424a992646a97cf2ec14-mfwitten@gmail.com>\n\t<60c8906b751d4915be456009c220516e-mfwitten@gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=US-ASCII","Content-Transfer-Encoding":"7bit","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}}]