From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id E74E4A04A2; Tue, 12 May 2020 13:08:14 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id AB62A1BFA1; Tue, 12 May 2020 13:08:13 +0200 (CEST) Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [67.231.154.164]) by dpdk.org (Postfix) with ESMTP id B32281BF8A for ; Tue, 12 May 2020 13:08:11 +0200 (CEST) Received: from mx1-us1.ppe-hosted.com (unknown [10.110.50.143]) by dispatch1-us1.ppe-hosted.com (PPE Hosted ESMTP Server) with ESMTP id 67C0C2008D; Tue, 12 May 2020 11:08:10 +0000 (UTC) Received: from us4-mdac16-70.at1.mdlocal (unknown [10.110.50.188]) by mx1-us1.ppe-hosted.com (PPE Hosted ESMTP Server) with ESMTP id 666408009B; Tue, 12 May 2020 11:08:10 +0000 (UTC) X-Virus-Scanned: Proofpoint Essentials engine Received: from mx1-us1.ppe-hosted.com (unknown [10.110.49.104]) by mx1-us1.ppe-hosted.com (PPE Hosted ESMTP Server) with ESMTPS id D050E4006C; Tue, 12 May 2020 11:08:09 +0000 (UTC) Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by mx1-us1.ppe-hosted.com (PPE Hosted ESMTP Server) with ESMTPS id 43390B8005D; Tue, 12 May 2020 11:08:09 +0000 (UTC) Received: from [192.168.38.17] (10.17.10.39) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Tue, 12 May 2020 12:08:02 +0100 To: Wisam Monther , "dev@dpdk.org" , "Jack Min" , Thomas Monjalon , "jerinjacobk@gmail.com" , "ajit.khaparde@broadcom.com" References: <20200506123627.22340-2-wisamm@mellanox.com> <20200511110912.11535-1-wisamm@mellanox.com> <20200511110912.11535-3-wisamm@mellanox.com> From: Andrew Rybchenko Autocrypt: addr=arybchenko@solarflare.com; keydata= mQINBF2681gBEACbdTxu8eLL3UX2oAelsnK9GkeaJeUYSOHPJQpV7RL/iaIskqTwBRnhjXt7 j9UEwGA+omnOmqQMpeQTb/F9Ma2dYE+Hw4/t/1KVjxr3ehFaASvwR4fWJfO4e2l/Rk4rG6Yi 5r6CWU2y8su2654Fr8KFc+cMGOAgKoZTZHZsRy5lHpMlemeF+VZkv8L5sYJWPnsypgqlCG3h v6lbtfZs+QqYbFH6bqoZwBAl5irmxywGR7ZJr1GLUZZ1lfdazSY8r6Vz0/Ip/KVxGu2uxo81 QCsAj0ZsQtwji9Sds/prTiPrIjx8Fc/tfbnAuVuPcnPbczwCJACzQr4q26XATL3kVuZhSBWh 4XfO/EAUuEq5AemUG5DDTM87g7Lp4eT9gMZB6P+rJwWPNWTiV3L7Cn+fO+l9mTPnOqdzBgDe OaulKiNSft1o0DY4bGzOmM2ad2cZt0jfnbMPMTE9zsr6+RFa+M8Ct20o6U1MUE4vP6veErMK of4kZ8PdoMM+Sq1hxMPNtlcVBSP9xMmdSZPlfDYI5VWosOceEcz7XZdjBJKdwKuz70V7eac4 ITSxgNFCTbeJ03zL2MR5s0IvD9ghISAwZ6ieCjU5UATn5+63qpD0nVNLsAdb/UpfvQcKAmvj 0fKlxu/PMVkjBa7/4cfNogYOhWDKUO+1pMaFwvb6/XTo6uMpfQARAQABtCxBbmRyZXcgUnli Y2hlbmtvIDxhcnliY2hlbmtvQHNvbGFyZmxhcmUuY29tPokCVAQTAQoAPhYhBP6NPgcKRj/Y X0yXQahue0sAy4m+BQJduvNYAhsDBQkB4TOABQsJCAcDBRUKCQgLBRYCAwEAAh4BAheAAAoJ EKhue0sAy4m+t3gP/j1MNc63CEozZo1IZ2UpVPAVWTYbLdPjIRdFqhlwvZYIgGIgIBk3ezKL K0/oc4ZeIwL6wQ5+V24ahuXvvcxLlKxfbJ6lo2iQGC7GLGhsDG9Y2k6sW13/sTJB/XuR2yov k5FtIgJ+aHa1PDZnepnGGOt9ka9n/Jzrc9WKYapOIIyLRe9U26ikoVgyqsD37PVeq5tLWHHA NGTUKupe9G6DFWidxx0KzyMoWDTbW2AWYcEmV2eQsgRT094AZwLFN5ErfefYzsGdO8TAUU9X YTiQN2MvP1pBxY/r0/5UfwV4UKBcR0S3ZvzyvrPoYER2Kxdf/qurx0Mn7StiCQ/JlNZb/GWQ TQ7huduuZHNQKWm7ufbqvKSfbPYvfl3akj7Wl8/zXhYdLqb5mmK45HXrgYGEqPN53OnK2Ngx IgYKEWr05KNv09097jLT5ONgYvszflqlLIzC4dV245g7ucuf9fYmsvmM1p/gFnOJBJL18YE5 P1fuGYNfLP+qp4WMiDqXlzaJfB4JcinyU49BXUj3Utd6f6sNBsO8YWcLbKBV9WmA324S3+wj f4NPRp3A5E+6OmTVMLWire2ZvnYp3YvifUj1r8lhoZ2B2vKuWwiTlHOKYBEjnOQJQnqYZEF0 JQQ1xzVDBQKE01BPlA3vy6BGWe6I4psBVqMOB9lAev/H+xa4u6Z3uQINBF269JsBEAC2KB3W 8JES/fh74avN7LOSdK4QA7gFIUQ4egVL81KnxquLzzilABuOhmZf3Rq6rMHSM8xmUAWa7Dkt YtzXStjEBI/uF0mAR3mMz1RcL2Wp+WD/15HjVpA7hPjXSEsWY0K2ymPerK4yrLcfFTHdMonY JfuACCC9NtOZxrWHOJoUS+RT7AWk80q/6D2iwQ47/2dBTznVG+gSeHSes9l91TB09w6f9JX/ sT+Ud0NQfm7HJ7t2pmGI9O6Po/NLZsDogmnIpJp/WwYOZN9JK7u2FyX2UyRzR8jK42aJkRsh DXs16Cc2/eYGakjrdO3x9a+RoxN7EuFtYhGR1PzMXdUiB5i+FyddYXkYUyO43QE/3VPA5l1v TUOagzZq6aONsdNonGJkV3TIG3JmUNtM+D/+r6QKzmgoJ8w576JxEZI09I/ZFN+g7BnUmlMx 6Z3IUOXVX/SWfGFga0YajwajHz03IBhChEbYbbqndVhmshu2GFURxrfUPYWdDXEqkh+08a5U Didia9jm2Opv4oE1e1TXAePyYJl/Zyps4Cv00GObAxibvMBQCUZQ+IBnNldRBOwXXRQV2xpx P+9iO1VYA/QXn0KqRK+SH1JGRXbJYi42YFaW1gE0EU0fiR2Wb9pK+doNEjjOhlzUGuvOEAUS +4m0m3dlfEvpCV9GMr7ERRpZzh9QkQARAQABiQI8BBgBCgAmFiEE/o0+BwpGP9hfTJdBqG57 SwDLib4FAl269JsCGwwFCQlmAYAACgkQqG57SwDLib7x6g//e+eCtNnJz7qFGbjWRJYNLCe5 gQwkhdyEGk4omr3VmjGj3z9kNFy/muh4pmHUngSAnnpwZggx14N4hhKf9y8G4Dwvsqa6b1zB Jq/c4t/SBDtGW4M/E331N04PaQZpcrbTfp1KqHNknk2N7yOk4CcoLVuIZmA5tPguASV8aAfz ZwhWAwn6vUEw9552eXEAnGFGDTCbyryNwzB5jtVQOEEDjTxcCkpcXMB45Tb1QUslRTu/sBAe HhPCQSUcJHR+KOq+P6yKICGAr291PZd6Qc7C3UyE+A3pY/UfdEVWj0STBWx1qvYLaHLrI4O9 KXDgh7luLjZZafcueCaPYmNo4V2lmNb3+7S4TvqhoZS+wN+9ldRQ4gH3wmRZybN6Y/ZCqxol RaZpE3AqdWsGvIgAkD0FpmtZNii9s2pnrhw0K6S4t4tYgXGTossxNSJUltfFQZdXM1xkZhtv dBZuUEectbZWuviGvQXahOMuH2pM64mx2hpdZzPcI2beeJNHkAsGT2KcaMETgvtHUBFRlLVB YxsUYz3UZmi2JSua4tbcGd6iWVN90eb8CxszYtivfpz6o2nPSjNwg0NaVGSHXjAK0tdByZ9t SkwjC3tEPljVycRSDpbauogOiAkvjENfaPd/H26V5hY822kaclaKDAW6ZG9UKiMijcAgb9u5 CJoOyqE8aGS5Ag0EXbr1RwEQAMXZHbafqmZiu6Kudp+Filgdkj2/XJva5Elv3fLfpXvhVt0Y if5Rzds3RpffoLQZk9nPwK8TbZFqNXPu7HSgg9AY7UdCM94WRFTkUCGKzbgiqGdXZ7Vyc8cy teGW+BcdfQycDvjfy50T3fO4kJNVp2LDNdknPaZVe8HJ80Od63+9ksB6Ni+EijMkh6Uk3ulB CSLnT4iFV57KgU2IsxOQVLnm+0bcsWMcCnGfphkY0yKP+aJ6MfmZkEeaDa7kf24N14ktg50m vOGDitcxA/+XXQXOsOIDJx1VeidxYsQ2FfsKu1G8+G6ejuaLf4rV5MI/+B/tfLbbOdikM5PF pxZVgTir9q13qHumMxdme7w5c7hybW412yWAe9TsrlXktFmFjRSFzAAxQhQSQxArS6db4oBk yeYJ59mW52i4occkimPWSm/raSgdSM+0P6zdWUlxxj+r1qiLgCYvruzLNtp5Nts5tR/HRQjE /ohQYaWDSVJEsc/4eGmgwzHzmvHtXeKkasn01381A1Lv3xwtpnfwERMAhxBZ8EGKEkc5gNdk vIPhknnGgPXqKmE1aWu8LcHiY+RHAF8gYPCDMuwyzBYnbiosKcicuIUp0Fj8XIaPao6F+WTi In4UOrqrYhsaCUvhVjsTBbNphGih9xbFJ8E+lkTLL8P3umtTcMPnpsB4xqcDABEBAAGJBHIE GAEKACYWIQT+jT4HCkY/2F9Ml0GobntLAMuJvgUCXbr1RwIbAgUJCWYBgAJACRCobntLAMuJ vsF0IAQZAQoAHRYhBNTYjdjWgdaEN5MrAN+9UR5r/4d3BQJduvVHAAoJEN+9UR5r/4d3EiQP /3lyby6v49HTU94Q2Fn2Xat6uifR7kWE5SO/1pUwYzx6v+z5K2jqPgqUYmuNoejcGl0CTNhg LbsxzUmAuf1OTAdE+ZYvOAjjKQhY4haxHc4enby/ltnHfWJYWJZ9UN5SsIQLvITvYu6rqthO CYjpXJhwkj3ODmC9H1TrvjrBGc6i7CTnR8RCjMEwCs2LI2frHa4R6imViEr9ScMfUnzdABMQ B0T5MOg8NX92/FRjTldU2KovG0ML9mSveSvVHAoEBLy4UIs5nEDdNiO1opJgKb5CXvWQugub 7AR52phNdKVdEB0S4tigJT4NalyTaPiUhFEm+CzZpMQDJ5E+/OowaPRfN4HeJX+c8sB+vUAZ mkAaG75N+IEk5JKFK9Z+bBYgPgaBDFZYdWDB/TMH0ANt+KI5uYg0i12TB4M8pwKG1DEPUmWc F2YpvB3jnbwzsOpSFiJOOlSs6nOB0Sb5GRtPOO3h6XGj+6mzQd6tcL63c9TrrUkjq7LDkxCz SJ2hTYRC8WNX8Uw9skWo5728JNrXdazEYCenUWmYiKLNKLslXCFodUCRDh/sUiyqRwS7PHEA LYC/UIWLMomI0Yvju3KA5v3RQVXhL+Gx2CzSj3GDz9xxGhJB2LfRfjzPbTR/Z27UpjCkd8z0 Ro3Ypmi1FLQwnRgoOKDbetTAIhugEShaLTITzJAP/iRDJCQsrZah5tE8oIl81qKEmBJEGcdt HYikbpQe7ydcXhqTj7+IECa3O7azI5OhCxUH2jNyonJ/phUslHH2G1TTBZK8y4Hrx5RpuRNS esn3P9uKu9DHqBAL7DMsCPwb2p1VNnapD72DBmRhzS/e6zS2R4+r9yNv03Hv7VCxKkmtE63H qpS//qpjfrtsIcHAjnKDaDtL1LYCtHoweI+DOpKKULSAYp/JE6F8LNibPQ0/P3S5ZIJNC4QZ uESjFOalJwFIqGQdkQB7ltRNJENLrHc+2jKGOuyFHm/Sbvp5EMGdaeQ0+u8CY0P+y6oXenwx 7WrJz/GvbNoFhJoJ6RzxCMQrFgxrssVZ7w5HcUj94lbnJ6osdYE/WpSd50B6jet6LKh5revg u9XI9CoqsPQ1V4wKYYdllPuogCye7KNYNKuiiuSNpaF4gHq1ZWGArwZtWHjgc2v3LegOpRQF SwOskMKmWsUyHIRMG1p8RpkBQTqY2rGSeUqPSvaqjT0nq+SUEM6qxEXD/2Wqri/X6bamuPDb S0PkBvFD2+0zr5Bc2YkMGPBYPNGZiTp3UjmZlLfn3TiBKIC92jherY563CULjSsiBEJCOSvv 4VPLn5aAcfbCXJnE3IGCp/hPl50iQqu7BPOYBbWXeb9ptDjGCAThNxSz0WAXkmcjAFE8gdE6 Znk9 Message-ID: <7c0236f2-bf69-7877-3116-020f3d51f54a@solarflare.com> Date: Tue, 12 May 2020 14:07:52 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [10.17.10.39] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ukex01.SolarFlarecom.com (10.17.10.4) X-TM-AS-Product-Ver: SMEX-12.5.0.1300-8.5.1020-25414.003 X-TM-AS-Result: No-27.222000-8.000000-10 X-TMASE-MatchedRID: IeZYkn8zfFrmLzc6AOD8DfHkpkyUphL9Ud7Bjfo+5jQZFDQxUvPcmH2p 64gN8o+Dkv1FtX9S2ykDs2TPzGsg/eF/B1jEjV+6NVRz+HwqL4LS4uB0RhSwWcuSXx71bvSLhL3 rCQ6U0+Rs05VeVN5NtEwEOB12w9KNcxn0f85VZlqm7ZVSyheM6Y/O/yyvtrKj4nhkC0SGkyQxIj a5FZvWvEaTCbc2jnqFwoHE1u2W4vGa4z180qrIkJxVZzZr7+O7y733NwuklsIHZBaLwEXlKGlF7 OhYLlcts7g2CX19w5Ry0UqVZxbdK1CmyOJJq7C06oMqSPTYRnWn1zdO3ivmTcnJhTYnTng9s+Gb dNi+FiJUQMQEHgICqvSrrq2eCJ04Kh5xfxw1U6HMbQu1fPiCDwgqPpbA7sp1VWQnHKxp38jjxOs Q+OAAXfglxh1uSkrlp0J7CJ90iKbu2hpt/pkbiDdfT4zyWoZSOhJ9m53n4aB2tmRAtFk5V+EMl/ vI3P9nL9pXR1dEHvvP7uGwFapky87Ay8EurnRqjoyKzEmtrEd99ekRHlOQkc8DuAI4aSPIsSmkx AWYGb9yqN99Q4IT82RMBocOrKXujFye9ONvgXP0hv/rD7WVZCIk3dpe5X+hqd0MUWQ8k8McHx0G 208EBt1dwzZ2yBXW4e3VSAHh52a6RaO0k2Dyi0sh+mzT1UnbEPlgwlUI7au2rF0FMhg2hEJx9kV j3qcjo77diPFVjSyqKuQTxuBXaODocHyqS9HwOUdMzbeJ0Vib6i6a65DFtlrXKFPCbXO5TXOj1X BAu3AeGE6NlODf9eCznAZixV3A0khhRsMGQweeAiCmPx4NwLTrdaH1ZWqC1B0Hk1Q1KyJHtBsf5 /UXJbVQu1GNZ+sikGUtrowrXLg= X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--27.222000-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1020-25414.003 X-MDID: 1589281690-bMebiLINtey6 Subject: Re: [dpdk-dev] [PATCH v6 2/5] app/flow-perf: add insertion rate calculation X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 5/12/20 1:34 PM, Wisam Monther wrote: >> -----Original Message----- >> From: Andrew Rybchenko >> Sent: Monday, May 11, 2020 3:05 PM >> To: Wisam Monther ; dev@dpdk.org; Jack Min >> ; Thomas Monjalon ; >> jerinjacobk@gmail.com; ajit.khaparde@broadcom.com >> Subject: Re: [PATCH v6 2/5] app/flow-perf: add insertion rate calculation >> >> On 5/11/20 2:09 PM, Wisam Jaddo wrote: >>> Add insertion rate calculation feature into flow >>> performance application. >>> >>> The application now provide the ability to test >>> insertion rate of specific rte_flow rule, by >>> stressing it to the NIC, and calculate the >>> insertion rate. >>> >>> The application offers some options in the command >>> line, to configure which rule to apply. >>> >>> After that the application will start producing >>> rules with same pattern but increasing the outer IP >>> source address by 1 each time, thus it will give >>> different flow each time, and all other items will >>> have open masks. >>> >>> The current design have single core insertion rate. >>> In the future we may have a multi core insertion >>> rate measurement support in the app. >>> >>> Signed-off-by: Wisam Jaddo >>> --- >>> app/test-flow-perf/Makefile | 3 + >>> app/test-flow-perf/actions_gen.c | 164 +++++++++ >>> app/test-flow-perf/actions_gen.h | 29 ++ >>> app/test-flow-perf/config.h | 16 + >>> app/test-flow-perf/flow_gen.c | 145 ++++++++ >>> app/test-flow-perf/flow_gen.h | 37 ++ >>> app/test-flow-perf/items_gen.c | 277 +++++++++++++++ >>> app/test-flow-perf/items_gen.h | 31 ++ >>> app/test-flow-perf/main.c | 472 ++++++++++++++++++++++++- >>> app/test-flow-perf/meson.build | 3 + >>> doc/guides/rel_notes/release_20_05.rst | 3 + >>> doc/guides/tools/flow-perf.rst | 195 +++++++++- >>> 12 files changed, 1368 insertions(+), 7 deletions(-) >>> create mode 100644 app/test-flow-perf/actions_gen.c >>> create mode 100644 app/test-flow-perf/actions_gen.h >>> create mode 100644 app/test-flow-perf/flow_gen.c >>> create mode 100644 app/test-flow-perf/flow_gen.h >>> create mode 100644 app/test-flow-perf/items_gen.c >>> create mode 100644 app/test-flow-perf/items_gen.h >>> >>> diff --git a/app/test-flow-perf/Makefile b/app/test-flow-perf/Makefile >>> index db043c17a..4f2db7591 100644 >>> --- a/app/test-flow-perf/Makefile >>> +++ b/app/test-flow-perf/Makefile >>> @@ -16,6 +16,9 @@ CFLAGS += $(WERROR_FLAGS) >>> # >>> # all source are stored in SRCS-y >>> # >>> +SRCS-y += actions_gen.c >>> +SRCS-y += flow_gen.c >>> +SRCS-y += items_gen.c >>> SRCS-y += main.c >>> >>> include $(RTE_SDK)/mk/rte.app.mk >>> diff --git a/app/test-flow-perf/actions_gen.c b/app/test-flow- >> perf/actions_gen.c >>> new file mode 100644 >>> index 000000000..16bb3cf20 >>> --- /dev/null >>> +++ b/app/test-flow-perf/actions_gen.c >>> @@ -0,0 +1,164 @@ >>> +/* SPDX-License-Identifier: BSD-3-Clause >>> + * Copyright 2020 Mellanox Technologies, Ltd >>> + * >>> + * The file contains the implementations of actions generators. >>> + * Each generator is responsible for preparing it's action instance >>> + * and initializing it with needed data. >>> + */ >>> + >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +#include "actions_gen.h" >>> +#include "config.h" >>> + >>> +/* Storage for struct rte_flow_action_rss including external data. */ >>> +struct action_rss_data { >>> + struct rte_flow_action_rss conf; >>> + uint8_t key[40]; >>> + uint16_t queue[128]; >>> +}; >>> + >>> +void >>> +add_mark(struct rte_flow_action *actions, >>> + uint8_t actions_counter) >>> +{ >>> + static struct rte_flow_action_mark mark_action; >> >> Function-local static variables a bit better than file-local >> or global variable, but just a bit. See below. >> At bare minimum it requires a check that the action is not >> in use already. Same in many cases below. > > Yes it's better, > What you mean by " At bare minimum it requires a check that the action is not in use already" > Can you please elaborate? In theory, nothing prevents to call the function twice in attempt to add MARK action twice. Right now design design guaranees that it will be used only once since bitmask is used to mark required actions. However the design using bitmask is bad since it does not allow to control order of actions in flow rule. Same concern is applicable to items bitmask. Application user should control order, not internal logic. So, if the design drawback is fixed, something should ensure that the action is not added twice. It is not that important for MASK, since it is constant now, but important for QUEUE. >> >>> + >>> + do { >>> + mark_action.id = MARK_ID; >>> + } while (0); >> >> Why do you use dummy do-while loop here? Many similar cases >> below. > > Sometimes, it create the flow before setting the correct id, I think it's compiler stuff > So the dummy loop to make sure the compiler finish it's execution and make sure > When the flow is created the action have correct value. As far as I know, C does not work like this. It will not help to cope with race conditions if your design has race conditions. Right now it is hard to judge for me, but do/while loop is definitely unnecessary/useless here. >> >>> + >>> + actions[actions_counter].type = RTE_FLOW_ACTION_TYPE_MARK; >>> + actions[actions_counter].conf = &mark_action; >>> +} >>> + >>> +void >>> +add_queue(struct rte_flow_action *actions, >>> + uint8_t actions_counter, uint16_t queue) >>> +{ >>> + static struct rte_flow_action_queue queue_action; >> >> It does not allow to use the action twice to deliver to >> to queues. > > Yes, it's needed only once What if I would like to test a rule which delivers to two QUEUEs using DUP? I'm just highlighting hard limitation in the the design. Not a blocker itself, but should be taken into account. > >> >>> + >>> + do { >>> + queue_action.index = queue; >>> + } while (0); >>> + >>> + actions[actions_counter].type = RTE_FLOW_ACTION_TYPE_QUEUE; >>> + actions[actions_counter].conf = &queue_action; >>> +} >>> + >>> +void >>> +add_jump(struct rte_flow_action *actions, >>> + uint8_t actions_counter, uint16_t next_table) >>> +{ >>> + static struct rte_flow_action_jump jump_action; >>> + >>> + do { >>> + jump_action.group = next_table; >>> + } while (0); >>> + >>> + actions[actions_counter].type = RTE_FLOW_ACTION_TYPE_JUMP; >>> + actions[actions_counter].conf = &jump_action; >>> +} >>> + >>> +void >>> +add_rss(struct rte_flow_action *actions, >>> + uint8_t actions_counter, uint16_t *queues, >>> + uint16_t queues_number) >>> +{ >>> + static struct rte_flow_action_rss *rss_action; >>> + static struct action_rss_data *rss_data; >> >> It is better to add an empty line here to split static and >> non-static variable and make it easy to catch the difference. >> >>> + uint16_t queue; >>> + >>> + rss_data = rte_malloc("rss_data", >>> + sizeof(struct action_rss_data), 0); >> >> Does it mean that the second invocation will make >> a memory leak? > > Not exactly, because the second invocation may have different queues and need > To be used with different flow, so I think it's ok to re malloc it. Sorry, but it does not answer my question. Is it a memory leak? If yes, why it is not a problem here? >> >>> + >>> + if (rss_data == NULL) >>> + rte_exit(EXIT_FAILURE, "No Memory available!"); >>> + >>> + *rss_data = (struct action_rss_data){ >>> + .conf = (struct rte_flow_action_rss){ >>> + .func = RTE_ETH_HASH_FUNCTION_DEFAULT, >>> + .level = 0, >>> + .types = GET_RSS_HF(), >>> + .key_len = sizeof(rss_data->key), >>> + .queue_num = queues_number, >>> + .key = rss_data->key, >>> + .queue = rss_data->queue, >>> + }, >>> + .key = { 1 }, >>> + .queue = { 0 }, >>> + }; >>> + >>> + for (queue = 0; queue < queues_number; queue++) >>> + rss_data->queue[queue] = queues[queue]; >>> + >>> + rss_action = &rss_data->conf; >>> + >>> + actions[actions_counter].type = RTE_FLOW_ACTION_TYPE_RSS; >>> + actions[actions_counter++].conf = rss_action; >>> +} >>> + >>> +void >>> +add_count(struct rte_flow_action *actions, >>> + uint8_t actions_counter) >>> +{ >>> + static struct rte_flow_action_count count_action; >> >> Again it means it is impossible to use the action twice in one >> rule. > > Yes, > If I removed the static from the inner scope here, > The action will be freed when we reach the end of the file, Not sure that I understand. > And since the design here to add the action into actions array to be used later in the creation, > It will not have correct action in case of none-static in the inner scope. > > If any action needs to have privilege to be called twice in single rule > New support needs to be done. > > This is the design for it, I'll add into the known limitation. > >>> +static void >>> +fill_items(struct rte_flow_item *items, >>> + uint32_t flow_items, uint32_t outer_ip_src) >> >> It looks like it is better to have the function inside >> items_gen.c. It would allow to make all add_ functions >> local to items_gen.c. >> >>> +{ >>> + uint8_t items_counter = 0; >>> + >>> + /* Support outer items up to tunnel layer only. */ >>> + >>> + if (flow_items & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_META)) >>> + add_meta_data(items, items_counter++); >>> + if (flow_items & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_TAG)) >>> + add_meta_tag(items, items_counter++); >>> + if (flow_items & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_ETH)) >>> + add_ether(items, items_counter++); >>> + if (flow_items & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_VLAN)) >>> + add_vlan(items, items_counter++); >>> + if (flow_items & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_IPV4)) >>> + add_ipv4(items, items_counter++, outer_ip_src); >>> + if (flow_items & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_IPV6)) >>> + add_ipv6(items, items_counter++, outer_ip_src); >>> + if (flow_items & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_TCP)) >>> + add_tcp(items, items_counter++); >>> + if (flow_items & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_UDP)) >>> + add_udp(items, items_counter++); >>> + if (flow_items & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_VXLAN)) >>> + add_vxlan(items, items_counter++); >>> + if (flow_items & >> FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_VXLAN_GPE)) >>> + add_vxlan_gpe(items, items_counter++); >>> + if (flow_items & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_GRE)) >>> + add_gre(items, items_counter++); >>> + if (flow_items & >> FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_GENEVE)) >>> + add_geneve(items, items_counter++); >>> + if (flow_items & FLOW_ITEM_MASK(RTE_FLOW_ITEM_TYPE_GTP)) >>> + add_gtp(items, items_counter++); >> >> It could be done in a loop: define an array of structures >> FLOW_ITEM_MASK(proto) values and add function which should be >> called. The only exception is IPv4/IPv6 which requires extra argument - >> so all add callbacks should have add_data argument >> which is a structure with possible tunings. > > Ok, let me re-phrase to make sure I got it: > 1- Move it to items_gen.c > 2- Create array of structures with all items. > The structure should be something like this: > static const struct items_dict { > const uint64_t mask; const is not required here > void (*funct)(struct rte_flow_item *items, > uint8_t items_counter, rte_be32_t src_ip); The last argument should be a pointer to a structure with src_ip field to make it generic and be able to add more argument values there. > bool add_args; > } > 3- I need to change the signature "parameters" to all other than ipv4,ipv6 to have another parameter to > Have matched singture. > 4- in none-ipv4-ipv6 item I need to call the RTE_SET_USED(src_ip); > 5- loop over the array to add items > > Is this right? Yes, with above corrections. >> >>> + >>> + items[items_counter].type = RTE_FLOW_ITEM_TYPE_END; >>> +} >>> + >>> +static void >>> +fill_actions(struct rte_flow_action *actions, >>> + uint32_t flow_actions, uint32_t counter, uint16_t next_table, >>> + uint16_t hairpinq) >> >> >> It looks like it is better to have the function inside >> actions_gen.c. It would allow to make all add_ >> functions local to actions_gen.c. > > Sure, but this one will remain as is, since the actions have different signature and other actions when be added > Will have more different parameters. Sorry, still don't understand why. >> >>> +{ >>> + uint8_t actions_counter = 0; >>> + uint16_t hairpin_queues[hairpinq]; >>> + uint16_t queues[RXQ_NUM]; >>> + uint16_t i; >>> + >>> + /* None-fate actions */ >>> + if (flow_actions & >> FLOW_ITEM_MASK(RTE_FLOW_ACTION_TYPE_MARK)) >>> + add_mark(actions, actions_counter++); >>> + if (flow_actions & >> FLOW_ITEM_MASK(RTE_FLOW_ACTION_TYPE_COUNT)) >>> + add_count(actions, actions_counter++); >>> + if (flow_actions & >> FLOW_ITEM_MASK(RTE_FLOW_ACTION_TYPE_SET_META)) >>> + add_set_meta(actions, actions_counter++); >>> + if (flow_actions & >> FLOW_ITEM_MASK(RTE_FLOW_ACTION_TYPE_SET_TAG)) >>> + add_set_tag(actions, actions_counter++); >>> + >>> + /* Fate actions */ >>> + if (flow_actions & >> FLOW_ITEM_MASK(RTE_FLOW_ACTION_TYPE_QUEUE)) >>> + add_queue(actions, actions_counter++, counter % >> RXQ_NUM); >>> + if (flow_actions & >> FLOW_ITEM_MASK(RTE_FLOW_ACTION_TYPE_RSS)) { >>> + for (i = 0; i < RXQ_NUM; i++) >>> + queues[i] = i; >>> + add_rss(actions, actions_counter++, queues, RXQ_NUM); >>> + } >>> + if (flow_actions & >> FLOW_ITEM_MASK(RTE_FLOW_ACTION_TYPE_JUMP)) >>> + add_jump(actions, actions_counter++, next_table); >>> + if (flow_actions & >> FLOW_ITEM_MASK(RTE_FLOW_ACTION_TYPE_PORT_ID)) >>> + add_port_id(actions, actions_counter++); >>> + if (flow_actions & >> FLOW_ITEM_MASK(RTE_FLOW_ACTION_TYPE_DROP)) >>> + add_drop(actions, actions_counter++); >>> + if (flow_actions & HAIRPIN_QUEUE_ACTION) >>> + add_queue(actions, actions_counter++, >>> + (counter % hairpinq) + RXQ_NUM); >>> + if (flow_actions & HAIRPIN_RSS_ACTION) { >>> + for (i = 0; i < hairpinq; i++) >>> + hairpin_queues[i] = i + RXQ_NUM; >>> + add_rss(actions, actions_counter++, hairpin_queues, >> hairpinq); >>> + } >>> + >>> + actions[actions_counter].type = RTE_FLOW_ACTION_TYPE_END; >>> +} >>> + >>> + >>> +void >>> +add_ether(struct rte_flow_item *items, uint8_t items_counter) >>> +{ >>> + static struct rte_flow_item_eth eth_spec; >>> + static struct rte_flow_item_eth eth_mask; >> >> Same as actions, it does not allow to have two Eth items >> in one rule. However, it looks like current design does not >> cover it already on mask level. > > Yes, indeed. > Already add this in flow_perf.rst as known limitation. > >> >>> /* Control */ >>> { "help", 0, 0, 0 }, >>> + { "flows-count", 1, 0, 0 }, >>> + { "dump-iterations", 0, 0, 0 }, >> >> It looks like above it the path which should be defined >> here. > > I'm not following, what do you mean? See below. I was just trying to separately static and dynamically added parts. >> >>> + /* Attributes */ >>> + { "ingress", 0, 0, 0 }, >>> + { "egress", 0, 0, 0 }, >>> + { "transfer", 0, 0, 0 }, >>> + { "group", 1, 0, 0 }, >>> + /* Items */ >>> + { "ether", 0, 0, 0 }, >>> + { "vlan", 0, 0, 0 }, >>> + { "ipv4", 0, 0, 0 }, >>> + { "ipv6", 0, 0, 0 }, >>> + { "tcp", 0, 0, 0 }, >>> + { "udp", 0, 0, 0 }, >>> + { "vxlan", 0, 0, 0 }, >>> + { "vxlan-gpe", 0, 0, 0 }, >>> + { "gre", 0, 0, 0 }, >>> + { "geneve", 0, 0, 0 }, >>> + { "gtp", 0, 0, 0 }, >>> + { "meta", 0, 0, 0 }, >>> + { "tag", 0, 0, 0 }, >>> + /* Actions */ >>> + { "port-id", 0, 0, 0 }, >>> + { "rss", 0, 0, 0 }, >>> + { "queue", 0, 0, 0 }, >>> + { "jump", 0, 0, 0 }, >>> + { "mark", 0, 0, 0 }, >>> + { "count", 0, 0, 0 }, >>> + { "set-meta", 0, 0, 0 }, >>> + { "set-tag", 0, 0, 0 }, >>> + { "drop", 0, 0, 0 }, >>> + { "hairpin-queue", 1, 0, 0 }, >>> + { "hairpin-rss", 1, 0, 0 }, >> >> This part should be added by code which iterates by >> flow_options. I.e. allocate lgopts dynamically, copy >> static options there by memcpy() and add dynamic as >> described above. May be flow_options require extra >> field 'has_arg'. > > Regard this one, > Some option have special code, like hairpin, group and control, so even with has_arg > It cannot be looped in pretty way, I tried it and it became ugly, I prefer to leave this as it. > I don't think it's critical to get rid of it? What do you think? Yes, it is not critical, but looking at above array, I don't understand how it could become ugly.