[{"id":1761644,"web_url":"http://patchwork.ozlabs.org/comment/1761644/","msgid":"<20170901122647.GB4938@vergenet.net>","list_archive_url":null,"date":"2017-09-01T12:26:49","subject":"Re: [PATCH net-next 1/2] flow_dissector: Cleanup control flow","submitter":{"id":64714,"url":"http://patchwork.ozlabs.org/api/people/64714/","name":"Simon Horman","email":"simon.horman@netronome.com"},"content":"Hi Tom,\n\nOn Thu, Aug 31, 2017 at 03:22:38PM -0700, Tom Herbert wrote:\n> __skb_flow_dissect is riddled with gotos that make discerning the flow,\n> debugging, and extending the capability difficult. This patch\n> reorganizes things so that we only perform goto's after the two main\n> switch statements (no gotos within the cases now). It also eliminates\n> several goto labels so that there are only two labels that can be target\n> for goto.\n\nI agree that the flow of __skb_flow_dissect() is difficult to follow\nbut its not obvious that this significant change in terms of loc\ntakes us to a better place.\n\nMaybe it makes follow-up work easier. If so perhaps it should be motivated\nalong those lines.\n\nIn any case I won't stand in the way of this change but I did want to throw\nmy 2c worth in.\n\n> \n> Reported-by: Alexander Popov \n> Signed-off-by: Tom Herbert \n> ---\n> include/net/flow_dissector.h | 9 ++\n> net/core/flow_dissector.c | 225 ++++++++++++++++++++++++++++---------------\n> 2 files changed, 156 insertions(+), 78 deletions(-)\n> \n> diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h\n> index e2663e900b0a..c358c3ff6acc 100644\n> --- a/include/net/flow_dissector.h\n> +++ b/include/net/flow_dissector.h\n> @@ -19,6 +19,15 @@ struct flow_dissector_key_control {\n> #define FLOW_DIS_FIRST_FRAG\tBIT(1)\n> #define FLOW_DIS_ENCAPSULATION\tBIT(2)\n> \n> +enum flow_dissect_ret {\n> +\tFLOW_DISSECT_RET_OUT_GOOD,\n> +\tFLOW_DISSECT_RET_OUT_BAD,\n> +\tFLOW_DISSECT_RET_PROTO_AGAIN,\n> +\tFLOW_DISSECT_RET_IPPROTO_AGAIN,\n> +\tFLOW_DISSECT_RET_IPPROTO_AGAIN_EH,\n> +\tFLOW_DISSECT_RET_CONTINUE,\n> +};\n\nMinor nit:\n\nMy reading is that this patch does not seem to differentiate between the\nhandling of FLOW_DISSECT_RET_IPPROTO_AGAIN and\nFLOW_DISSECT_RET_IPPROTO_AGAIN_EH. Perhaps it would be better to add\nFLOW_DISSECT_RET_IPPROTO_AGAIN_EH in the following patch where it is used.\n\n> +\n> /**\n> * struct flow_dissector_key_basic:\n> * @thoff: Transport header offset\n\n...","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=netronome-com.20150623.gappssmtp.com\n\theader.i=@netronome-com.20150623.gappssmtp.com\n\theader.b=\"mU9jB0Ft\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xkJQW6Kxmz9sRV\n\tfor ;\n\tFri, 1 Sep 2017 22:26:59 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751901AbdIAM04 (ORCPT );\n\tFri, 1 Sep 2017 08:26:56 -0400","from mail-wm0-f52.google.com ([74.125.82.52]:36825 \"EHLO\n\tmail-wm0-f52.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751715AbdIAM0z (ORCPT\n\t); Fri, 1 Sep 2017 08:26:55 -0400","by mail-wm0-f52.google.com with SMTP id f127so506819wmf.1\n\tfor ; Fri, 01 Sep 2017 05:26:55 -0700 (PDT)","from vergenet.net ([217.111.208.18])\n\tby smtp.gmail.com with ESMTPSA id v2sm27750wrd.68.2017.09.01.05.26.52\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tFri, 01 Sep 2017 05:26:53 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=netronome-com.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:in-reply-to:user-agent;\n\tbh=ObPG6qShjLFG/5GyqlwgvKD/uXG4C9tc8eNx3FZAXZ0=;\n\tb=mU9jB0FtTZpMwyjgTg8jxgoq7IKb2B3hCQ+xsKq81fO7Jy97zoNYSwu6eOZ1qBEf0e\n\t70e0+TpSojWRzKSw/7V3sOVGrey8o1zZBqd0AwVRBuYVk0bEcRwR9oQIxZQ4mstUDtLP\n\teBphqDhKvCqzQWyCEazxIgcpqAzWSl3Ez9s+gufjtaoSPpWcnDoLjd0FJCL9BOo7AAOz\n\tSVtsm2y6s5R/LweTIaTNp+3lVVIp+fer6UGzi8zSDc6CpqyOsTwXdNWsS1w5WiTEho1Y\n\t0aTmofXvhBq5tvlBwTFXA30O4/ubYyF1+W4Og2HNG/Ac8+tGgUVYm9Bkv6GFjxv8+po1\n\tCa1A==","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:references\n\t:mime-version:content-disposition:in-reply-to:user-agent;\n\tbh=ObPG6qShjLFG/5GyqlwgvKD/uXG4C9tc8eNx3FZAXZ0=;\n\tb=EzUE06Oj2DUSz/nt3lwURQgdZusKBHGvWxhHt1KJsgqP+I/zugCHNeHY0QNtsvbUx3\n\tAIa5K6ceTqkVKcOxgFolPv9usQmAI0FrGES/DH2kovlUl1xBXuNuMd0EPTqbDRTdV1h2\n\t45JkrfGX3pIunBhd9E426pf1dOvKlT4Hm3C0xrhhFW3s1pMfiBgaxczJ4vDFwB7IvCj3\n\tTQlqpq9Yf+XXiZftAG0GvGNE5LqxbFeTJ/Z/9j75niK1CPQXNj14xiYi/v5Bi1DFyu06\n\tW7Bid15VbUwYdSHRbtZTV87tP4mzh1QdKO/6EOwaGTOvaMVQBxbzxt8cv3R/0XoGoXiw\n\tfaRw==","X-Gm-Message-State":"AHPjjUh9xfmadrKlQFFcKceX8Z1OZWzR6VIzWTYDIOkyVAe9GhJK6fzN\n\tSo6dJsPyPXo3gGiN","X-Google-Smtp-Source":"ADKCNb658HdRxTpvH4FQX6zXCRecO/pI0f7AhKhI5tCFSR6lbNNPIozTQOIEHV6Iw4LGggmlaAP3bg==","X-Received":"by 10.28.48.23 with SMTP id w23mr407096wmw.176.1504268814370;\n\tFri, 01 Sep 2017 05:26:54 -0700 (PDT)","Date":"Fri, 1 Sep 2017 14:26:49 +0200","From":"Simon Horman ","To":"Tom Herbert ","Cc":"davem@davemloft.net, netdev@vger.kernel.org, alex.popov@linux.com,\n\thannes@stressinduktion.org","Subject":"Re: [PATCH net-next 1/2] flow_dissector: Cleanup control flow","Message-ID":"<20170901122647.GB4938@vergenet.net>","References":"<20170831222239.21509-1-tom@quantonium.net>\n\t<20170831222239.21509-2-tom@quantonium.net>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170831222239.21509-2-tom@quantonium.net>","User-Agent":"Mutt/1.5.23 (2014-03-12)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1761651,"web_url":"http://patchwork.ozlabs.org/comment/1761651/","msgid":"<87k21izjdc.fsf@stressinduktion.org>","list_archive_url":null,"date":"2017-09-01T12:35:59","subject":"Re: [PATCH net-next 1/2] flow_dissector: Cleanup control flow","submitter":{"id":18284,"url":"http://patchwork.ozlabs.org/api/people/18284/","name":"Hannes Frederic Sowa","email":"hannes@stressinduktion.org"},"content":"Tom Herbert writes:\n\n> __skb_flow_dissect is riddled with gotos that make discerning the flow,\n> debugging, and extending the capability difficult. This patch\n> reorganizes things so that we only perform goto's after the two main\n> switch statements (no gotos within the cases now). It also eliminates\n> several goto labels so that there are only two labels that can be target\n> for goto.\n\nThe problem with the\n\nfdret = ... ;\nbreak;\n\nis that we now have to count curly braces to look what break does\nactually break and where fdret is being processed. With the number of\ncontext lines you send for the patch this is hard to review.\n\nI tend to like the gotos a bit more for now.\n\n[...]\n\nBye,\nHannes","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=stressinduktion.org\n\theader.i=@stressinduktion.org header.b=\"srNjS9Hw\"; \n\tdkim=pass (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"WEzDOd46\"; \n\tdkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xkJd114tMz9s7c\n\tfor ;\n\tFri, 1 Sep 2017 22:36:05 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751984AbdIAMgD (ORCPT );\n\tFri, 1 Sep 2017 08:36:03 -0400","from out1-smtp.messagingengine.com ([66.111.4.25]:59101 \"EHLO\n\tout1-smtp.messagingengine.com\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1751778AbdIAMgC (ORCPT\n\t); Fri, 1 Sep 2017 08:36:02 -0400","from compute7.internal (compute7.nyi.internal [10.202.2.47])\n\tby mailout.nyi.internal (Postfix) with ESMTP id 9E9D620D06;\n\tFri, 1 Sep 2017 08:36:01 -0400 (EDT)","from frontend1 ([10.202.2.160])\n\tby compute7.internal (MEProxy); Fri, 01 Sep 2017 08:36:01 -0400","from z.localhost.stressinduktion.org (unknown [217.192.177.51])\n\tby mail.messagingengine.com (Postfix) with ESMTPA id 780527F3FA;\n\tFri, 1 Sep 2017 08:36:00 -0400 (EDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/relaxed; d=\n\tstressinduktion.org; h=cc:content-type:date:from:in-reply-to\n\t:message-id:mime-version:references:subject:to:x-me-sender\n\t:x-me-sender:x-sasl-enc:x-sasl-enc; s=fm1; bh=ccctSOpjcL3T1B6F74\n\t2erRP9B3WYoJd1zO3PEzU35Lc=; b=srNjS9HwPuu9ipgFJ6R87kOb4kvcQfiSmw\n\tuPG428svcFL2rFfz29uijufKxe8XHM5srDlDECRzUZTakRhqpjTBS2wQeBGPSam9\n\ta8pF1isrkYtuv95PBwMcwfi5fcZkK5iMqOL8YIVgEbdhP8DynWDcBROEWzSxr4LQ\n\t1r0eaBNd3Sln1fQ0a4tvaCCZtWNFqnwdOXlU/MyQaFWfrzITnDINB+rwVR2cdhSl\n\ta0xQT/fJcUDhQf0CPYnquxQjnuWwFa3avUFCAtuyqQuQnRvnQNT2DQjmVJtdg9YU\n\tLwYsRFmzyAvcBkh7FR7bKSGl/7rvZ/XFRMMFsh52bt7KOuDJjiuA==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=\n\tmessagingengine.com; h=cc:content-type:date:from:in-reply-to\n\t:message-id:mime-version:references:subject:to:x-me-sender\n\t:x-me-sender:x-sasl-enc:x-sasl-enc; s=fm1; bh=ccctSOpjcL3T1B6F74\n\t2erRP9B3WYoJd1zO3PEzU35Lc=; b=WEzDOd46tlon7v1kz1LOVHMBYIBmgvlucm\n\tHWLJKNRfoRkJxIf0VuboJrgoORfH7iN9T3ra+kIYhjk5y+Z9kMVOdgk1Xnn4uH6k\n\tVooB0dy7Dueh/vuTZH3GACN1xZCLbyZxLzJ/erdU2uUn8drhU5tkLHtXZe8yYwyT\n\tkUEOhkY6Mov8nfLFcEVggXkKKe5uS6M3Ji5adftk+00ad4ZpkeptVoxSSubHT2MT\n\tV6/E8Byy/zhXRRt5pfp5IpPoFt1WFYdbW38ZbVR8vNNEh0VUqAigMFjKC2xd385Y\n\tRFa5sZmheIefPWrdArdL2TOFTk7me3qG/cLEp5S9EyMyUZmB/Q3g=="],"X-ME-Sender":"","X-Sasl-enc":"vQFWKaud3K7jDkXMk3nbIhOmt2MQ6wPi9+Q9OzQ28DDK 1504269361","From":"Hannes Frederic Sowa ","To":"Tom Herbert ","Cc":"davem@davemloft.net, netdev@vger.kernel.org, alex.popov@linux.com","Subject":"Re: [PATCH net-next 1/2] flow_dissector: Cleanup control flow","References":"<20170831222239.21509-1-tom@quantonium.net>\n\t<20170831222239.21509-2-tom@quantonium.net>","Date":"Fri, 01 Sep 2017 14:35:59 +0200","In-Reply-To":"<20170831222239.21509-2-tom@quantonium.net> (Tom Herbert's\n\tmessage of \"Thu, 31 Aug 2017 15:22:38 -0700\")","Message-ID":"<87k21izjdc.fsf@stressinduktion.org>","User-Agent":"Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux)","MIME-Version":"1.0","Content-Type":"text/plain","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1761788,"web_url":"http://patchwork.ozlabs.org/comment/1761788/","msgid":"","list_archive_url":null,"date":"2017-09-01T16:12:48","subject":"Re: [PATCH net-next 1/2] flow_dissector: Cleanup control flow","submitter":{"id":72064,"url":"http://patchwork.ozlabs.org/api/people/72064/","name":"Tom Herbert","email":"tom@quantonium.net"},"content":"On Fri, Sep 1, 2017 at 5:35 AM, Hannes Frederic Sowa\n wrote:\n> Tom Herbert writes:\n>\n>> __skb_flow_dissect is riddled with gotos that make discerning the flow,\n>> debugging, and extending the capability difficult. This patch\n>> reorganizes things so that we only perform goto's after the two main\n>> switch statements (no gotos within the cases now). It also eliminates\n>> several goto labels so that there are only two labels that can be target\n>> for goto.\n>\n> The problem with the\n>\n> fdret = ... ;\n> break;\n>\n> is that we now have to count curly braces to look what break does\n> actually break and where fdret is being processed. With the number of\n> context lines you send for the patch this is hard to review.\n>\n> I tend to like the gotos a bit more for now.\n\nThis is a step towards a more modular design for flow dissector. The\ngoto's force a monolithic design and make it hard to implement new\nfunctionality like trying to enforce limits on encapsulation which\nrequires a single point for logic. The ip, ipv6, and mpls labels were\nreally unnecessary to begin with, the proto again logic works fine for\nthose. This is also a segway to breaking up the very large\n__skb_flow_dissect function into more manageable components (IP\nprotocol dissection should be its own function for instance). Follow\non patches will allow protocol specific implementations of flow\ndissection located with the rest of the protocol implementation, so\nhopefully we can end the practice of adding support for every\nnetworking protocol in one single core function (analogous to how we\nparse protocols in GRO).\n\nTom","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=quantonium-net.20150623.gappssmtp.com\n\theader.i=@quantonium-net.20150623.gappssmtp.com\n\theader.b=\"McGrKRvD\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xkPRB50fpz9sRV\n\tfor ;\n\tSat, 2 Sep 2017 02:12:54 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752169AbdIAQMx (ORCPT );\n\tFri, 1 Sep 2017 12:12:53 -0400","from mail-wm0-f53.google.com ([74.125.82.53]:36503 \"EHLO\n\tmail-wm0-f53.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751945AbdIAQMv (ORCPT\n\t); Fri, 1 Sep 2017 12:12:51 -0400","by mail-wm0-f53.google.com with SMTP id f127so4361035wmf.1\n\tfor ; Fri, 01 Sep 2017 09:12:50 -0700 (PDT)","by 10.223.198.2 with HTTP; Fri, 1 Sep 2017 09:12:48 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=quantonium-net.20150623.gappssmtp.com; s=20150623;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc; bh=ZOfsKiZFXrY15bBcsrEMhCZOsajMCVwqk5Ck7GK/Auw=;\n\tb=McGrKRvDIRjEXzftk7LYyUSftkWbxvjlGkmkVkeegoYN+emcfOO7xu5BuKYfzxYtPJ\n\tn2RLCP8y5lNCu9wOBy7ZVr8pT1rpR1v9Bo9qxZScJDpxrd+zvwRxu3AwnpNt/t5JDcTg\n\tgdh0Eye4sMFuBjQoePZfiwZEVw8ku99dQfdoIheNHgC6/Ymav6vkOx5OmVw3CFgSPsDT\n\tTqou1fDhiRs9JqWWj/hhLZX8fAiFRYZwrxmw9Q2LYlWjK4IXAnHN/H8tyg7IgH/9z0EI\n\tCDewQQ+RwMdKK20Mqla0Fv+b+XB+tz8Iy/F91MItmjgEPcx/Grcn0NDnt8+VSnEMwiTq\n\tlDig==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc;\n\tbh=ZOfsKiZFXrY15bBcsrEMhCZOsajMCVwqk5Ck7GK/Auw=;\n\tb=ukRuWOIrOOxSj/aoOMvwFYtU5EA0U9nplmeQLtq9THqmCOpFET2eo92Q6oERZLCoYX\n\txnVeTDKWe7ilQrMXp1l+mlTquJmbrsjEc29r/8jPXHXeqtylpOzLP47Xekbo2jmPwMEs\n\tqN9sAs4tgjdGeh1U66zslJ7SuybL4HxxmQb84BrLRKrYFoVyenkQXrmTfrGgS6/1+HYn\n\tzRned9FdwF9fXTdMOxsqpON8u5GrfwCYl6riYmMRQMb2y1hL6HyZ6FjNppRsaGZcdOSx\n\t2Hmks/6RpkVGHFz9eRS+LZVmCHOZiVJJqioVKfK3G0K5rz93GfEbAva89PImKx2gVpmZ\n\tWktQ==","X-Gm-Message-State":"AHPjjUh6ltvciPuPbOwSTlo3RbTDyja1yEBVMR99MEMPDy31E+wLqn6+\n\tIRpSnNdTQ/oDsW7gZb2nan3DxwLMRfak","X-Google-Smtp-Source":"ADKCNb6dh0E3jCz90UfN0Bb1P/4+pCi+7TVYPFnYGtXyOPze3aREet4fM/ZR486fcHlOdQtI0vuSPVBmUbFGy9vzG3s=","X-Received":"by 10.28.138.66 with SMTP id m63mr723689wmd.29.1504282369696;\n\tFri, 01 Sep 2017 09:12:49 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<87k21izjdc.fsf@stressinduktion.org>","References":"<20170831222239.21509-1-tom@quantonium.net>\n\t<20170831222239.21509-2-tom@quantonium.net>\n\t<87k21izjdc.fsf@stressinduktion.org>","From":"Tom Herbert ","Date":"Fri, 1 Sep 2017 09:12:48 -0700","Message-ID":"","Subject":"Re: [PATCH net-next 1/2] flow_dissector: Cleanup control flow","To":"Hannes Frederic Sowa ","Cc":"\"David S . Miller\" ,\n\tLinux Kernel Network Developers ,\n\talex.popov@linux.com","Content-Type":"text/plain; charset=\"UTF-8\"","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}}]