From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR01-DB5-obe.outbound.protection.outlook.com (mail-db5eur01on0075.outbound.protection.outlook.com [104.47.2.75]) by dpdk.org (Postfix) with ESMTP id 03C621BB72 for ; Wed, 4 Jul 2018 00:27:36 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Mellanox.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=7qBKf1lar26sWkC+I0sP3dqp8vpgR86NMGP3r1/vuK4=; b=oe5yvjqM67v+ORFjdAsuUo2lWTIYYVsIH16zV+hfZ83kCxzJrsAmLnxre0VW/GvtmN0wXNWxy2XIw39BvKUq0biljbJRR59nEhJlYnE4lEJvoWNngzjIKj0ObKNHgGYzbH/mUG5kg5/v8bXbVMbIeaDf9LHsDaz2SZm8Dd3NI7g= Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=yskoh@mellanox.com; Received: from yongseok-MBP.local (209.116.155.178) by DB6PR0501MB2039.eurprd05.prod.outlook.com (2603:10a6:4:6::21) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.930.18; Tue, 3 Jul 2018 22:27:33 +0000 Date: Tue, 3 Jul 2018 15:27:18 -0700 From: Yongseok Koh To: Nelio Laranjeiro Cc: dev@dpdk.org, Adrien Mazarguil Message-ID: <20180703222717.GB41721@yongseok-MBP.local> References: <957989d98f2363d6e2e377bd54893cf7b29e4985.1530111623.git.nelio.laranjeiro@6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <957989d98f2363d6e2e377bd54893cf7b29e4985.1530111623.git.nelio.laranjeiro@6wind.com> User-Agent: Mutt/1.9.3 (2018-01-21) X-Originating-IP: [209.116.155.178] X-ClientProxiedBy: BYAPR02CA0020.namprd02.prod.outlook.com (2603:10b6:a02:ee::33) To DB6PR0501MB2039.eurprd05.prod.outlook.com (2603:10a6:4:6::21) X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 7e6bd43f-a46a-476f-626d-08d5e1342c32 X-MS-Office365-Filtering-HT: Tenant X-Microsoft-Antispam: UriScan:; BCL:0; PCL:0; RULEID:(7020095)(4652040)(8989117)(5600053)(711020)(48565401081)(4534165)(4627221)(201703031133081)(201702281549075)(8990107)(2017052603328)(7153060)(7193020); SRVR:DB6PR0501MB2039; X-Microsoft-Exchange-Diagnostics: 1; DB6PR0501MB2039; 3:JgIHHt7hIQYn62mQtqxEfKNb9XSiH1rnkYmHfeolClIjOVrKJav1Fs+jOYMtaLNLs/tk/iHf8G0HFBIUEFFcnKZSNhQbdofFO7CDg1n3Q1q+l5HmffFJ9nx7zA44bfvZHUeWq0yhEgPwrHGhoRfv97Bvk0KBqOUYu0uxpBl7I/vyMiJB34OND+UGZIA0VUEsj8dbN16j22XKsmO+D6KrE7rBu54VzQelZeIvn99g/nJkfElKHf315EV97tNEJaOW; 25:5Ilzk/0dMTam+TYUN6AqPYUoQ80XVUgO5SwyjI/Pzx3jjlA2j7Z26SlQfdlZ9wHw717bIQ1Kj8muYBIKxei6Q5ggy1j44ZoRrXFegejVFt9mq8iUWenFFlC3emf5AurllQ8+4UxythpsxLdVRf+wzDKG/gLd8ATocJ+H3eqp7HDUMkaVvyOYsGQHxYJkaLuh5ccth2SId8jqedPJ8XtwS7dmJqE1PcL+y1C8uQX76dDq3gp49aFT2IO1EZCUo4Fnx1WSazNKo9kcGPDO7gnMKbFceU7OVWbIY+smlXo5nebpamefUtn+8zKWXDHMbg5P4260E8Pdf8zFpoh3RVZx8A==; 31:xErrmHVB+qGxZL29ZnxanQIwFmSCprohXzjqrP/s41rzTP/NC9t2iihA6OzofLY4nUatwGcBHgjHL06ZBF++PYqbhSYVYBb/q8Rb4lGyBlRdf+L7GZyVyu8cVISwaEFb6d77466K9ZO4K9Mu/rE7Lwt8EFFLjeeGQaAqAbgLvlwK5k8M73q9P1P12ptY2aqUYpPphAzS9mkltc0AOQBTIvTbz4OhOIXgcmOlOEomvyY= X-MS-TrafficTypeDiagnostic: DB6PR0501MB2039: X-LD-Processed: a652971c-7d2e-4d9b-a6a4-d149256f461b,ExtAddr X-Microsoft-Exchange-Diagnostics: 1; DB6PR0501MB2039; 20:iBMIMWA48Kd2hTUeoiv8sZ1ePJZyn9NDyOV8LIVQ5wYaZ1tDsYlWzVZP/UlgoaNeyJYbOuII4IwSv+MuBkDS0pl0vFF4IQMI3MQUaA3+WBcPid4x/7yJEtoaHj6xuVIqf5TbMotUp1+Vegmog91lyAjDisCJBVJnOMDqPSE4xnpQIFqrLOZLI334EBoba9eGZZ20HnFD8+EceamY7qEXuzVYL1AdZXqvh5zFweFwSKJ89pZr/aJCF9xz/q4a6oPM3gP7qcoxyhJJpL1cJoHnRTCArPk5B+kDJopGdyAUc+cm6gxOhFZ1n7evRIscxALvWtjp6rncfAVVZzeRpusnWQeROQVNCAA9ycmJE6MsFYZEHs0CdY+jbp+DA+VvGtd1vi5295m5Z4JnoGa0dzcTCqekUPp4DXMSDbCZdYKoa7jdlVFJDelO+bcDbdAXVqbA/Pe9dzkO7wgqrWLGQfQ608iIzr9mO1TSMJTNns4dV1JDvJggUyj5o+Q/++vmXDD/; 4:NJnxrr7GPYMicZfXZPnN0od0vrbzdcpxF1cLneM8F/I3cbNdPjoiGaOizZec+y5q5I3f4+MSEw3UCtuMuiE+G2J8SJSvfRqaR9aq25jOYraJp2pAEee9XvnbhTFE3eMdrPsAniNe/zME9X0GCiUgdpINYr699xVAsYQFRP0fVEPJRZco8TPja6gtHuy6vqy5EqS89uBI7DrEBFOtbfNEkpNwLN4r+PCnbFSh0PDbnp25abuso0JjLi1fEWkG5FJ/OVhG0Kfm1/GJZnYswNFm2cyWr0n5uSk2Aui8YdDQd36+SaSFquW1p9Ur4L/HOCVu X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:(211171220733660); X-MS-Exchange-SenderADCheck: 1 X-Exchange-Antispam-Report-CFA-Test: BCL:0; PCL:0; RULEID:(8211001083)(6040522)(2401047)(5005006)(8121501046)(10201501046)(93006095)(93001095)(3231254)(944501410)(52105095)(3002001)(6055026)(149027)(150027)(6041310)(20161123564045)(20161123560045)(20161123562045)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123558120)(6072148)(201708071742011)(7699016); SRVR:DB6PR0501MB2039; BCL:0; PCL:0; RULEID:; SRVR:DB6PR0501MB2039; X-Forefront-PRVS: 0722981D2A X-Forefront-Antispam-Report: SFV:NSPM; SFS:(10009020)(366004)(346002)(376002)(39860400002)(396003)(136003)(199004)(189003)(11346002)(956004)(50466002)(446003)(476003)(105586002)(106356001)(53936002)(1076002)(26005)(68736007)(6116002)(3846002)(33896004)(76176011)(23726003)(6246003)(52116002)(7696005)(478600001)(14444005)(81166006)(81156014)(486006)(58126008)(386003)(6506007)(16586007)(16526019)(8936002)(6666003)(6916009)(25786009)(305945005)(33656002)(229853002)(98436002)(47776003)(5660300001)(66066001)(316002)(86362001)(4326008)(55016002)(9686003)(7736002)(186003)(2906002)(8676002)(97736004)(18370500001); DIR:OUT; SFP:1101; SCL:1; SRVR:DB6PR0501MB2039; H:yongseok-MBP.local; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; A:1; MX:1; Received-SPF: None (protection.outlook.com: mellanox.com does not designate permitted sender hosts) X-Microsoft-Exchange-Diagnostics: =?us-ascii?Q?1; DB6PR0501MB2039; 23:3p3HA34kB9umKZaArQAp8vXuLVcmajO9PBIcH6f?= =?us-ascii?Q?pccL8QRkCsNszbLYoc84o9RFSWWXB4/CFXIz95TsM68PLpRNt4wItGk2Joe8?= =?us-ascii?Q?uzlGgY9HdVQwmTfVqFGWRXvnLzndaQxqe1VQwAtvxgGWyvIRvFqNYRhEzi7p?= =?us-ascii?Q?GXuXtkq4m1LmlVMRXfUV7xskaqA3Pc1QT5aqOkHkGoM/NaKQVXLAAsFE0nFl?= =?us-ascii?Q?Cfgs2L1iTvRi0bGhFGAmdpuCHJBaJlVyQBOrTBX7sApuSh/5BkVlhVlQ7RFe?= =?us-ascii?Q?ZBEEo/NWPOrTlnqmZj+6hfzQhsAH3zmpUem2o9k31fZv9rcq/BVxUvMR3i5p?= =?us-ascii?Q?H5IRHhSSJk9v1BXXDe93x/GESXetjXhsOQd+nXI2Zjjj8U++Ce0FFYvI5RtR?= =?us-ascii?Q?HPVhxw/ipp3lHMMXm+WzNeODBTOekkZB/vNL9XvyaHs4DkoWW8jY9NWd5qXf?= =?us-ascii?Q?Gw31InSsvJrkBAoLIbwye/Q7UAOrd4b/DcGnw5S89ZtI8A03iZ2zqvAnI8FX?= =?us-ascii?Q?Jqmi9gVUuin1LiecedXA8EO7CJ2ed0rzik5z+opWQM3fs7qrSlZI+aRGUnup?= =?us-ascii?Q?eI4+ckkP5ry9WekwCTlINao1kxvzbAqcX3Nu1yEHv3seHVgjSTsA8ABXParW?= =?us-ascii?Q?iHrTvaXwnmdgjHZeb4MEiEagjcLAEqsF0DUO2NplWfvlxIBBSD2F1r92Otxh?= =?us-ascii?Q?L5h+VqWf0ySLn4QeQivwGZ10NebqGpsmHFcytojAvrHN3e7Too2AQ+5rd3MC?= =?us-ascii?Q?0LIXfsmWkxOv8Wvhj8x/paUc/JvriYuqXpm47OWPKKZMzy3huy1piORkTC60?= =?us-ascii?Q?yT0M3nDbHJJblVsEmTrOwmsv7jQ8IAfJxBhiRmIGI2KyeO53duptzYsgF99y?= =?us-ascii?Q?r8KlQCzt9e09M6R0YgC2iZrThzvlDVVzd1b6DncPiVe26/I/pHr+BBpItplW?= =?us-ascii?Q?/M14/lTTUulpSc5SMuyDlCMJkUhim191c7kywJXNxejHqe2G+JedjpCvxE8a?= =?us-ascii?Q?M+LGg/wVkCiKwJcYAHWFjyfW+s2DXdDhCu1saVSMtI67C3fnFMisCzYq60ja?= =?us-ascii?Q?eQXNa+fDQjGqQAKu8ymLdZalPans564mxJyXi6t2trui+Hjt6/tOJmArUHGP?= =?us-ascii?Q?WN+dtlNitaxwktjXIgedUyCPgOQrKTbX10bKoU2gYbbNORbZhn52IkYjEZFZ?= =?us-ascii?Q?fg5T993IVan0xOcHDlbLkW1pzZ58dFQwrVfi7PzwbGamKONsHSMdiqiyzj3c?= =?us-ascii?Q?sR47aDMSoZKeSRVCjn+AEfPBe2L0XUIjJ9x7ZCpxlwHn8/oDZuUVXjixB8Ny?= =?us-ascii?Q?pvg=3D=3D?= X-Microsoft-Antispam-Message-Info: MOVXV/XvaM/59hAU5qNMwNuT2nHdUiIG1dGzca0lit43K8kUlscc/U24jxAFAkaeLD0rfWthdgqRkzqRxUnSBbN/8xFAVfcoQtdtKoMNRy92khdT9sBNI6nwKs9HHQC9cifFRx+l/R8koisTqYeXgdvHc9kQCC9Bvd2uDr6a4uVXFd3vaFSsk3AkAIC1rUZ+x/i9oqMb9L7Ucef61vBA4wbRqeOw6XCFocfQMd6Klpq4yZfPGvXnWJD6l2tlBqHRuBpWJLWlrGSk4Q/Hf58dCQxHXZb88ym0s0336Yezspe96EpE85556gMLs2TS28S9O6OPVgIlSPY+hwI0SZDtMVC+DByXatdRBdugsiopMEE= X-Microsoft-Exchange-Diagnostics: 1; DB6PR0501MB2039; 6:FsUf4FZg/9dXVIlk8d5mrllKKD9PCimD4dKA5pThWLKbs45K8eftE6pZV1udmJKWPSKwst4jZvD+hLDS02MxoHM6WqrKaT8vRycffrfIszly8uVtVtkjdaCgIeXKiRxQYHq+uKPXFT45WspuznWuBFCb14A/+MleQQhVDU61SKfhYKjoEgvyQOUj62GCmlmjA6F9Z0wP48zh2rBGiPa6ozM2QWOlfZjZdxdQZj02+BNYcR8wsCKnY0UCSzMSN/IRcY6yzsyEjO8CATLiPo9/lF0FV7uhinRvc3CtrPr+Pjg8SisR0M+hcNs898GamUFJ7RusQbTMRjcfKBE7PWHmDV5ki/HDpZ2EBgoNtsj6FTueOMU0XpSpOZa9eaoRk96puNG6QHOPc5OtgLJug/hhR1uxGv+Kzht6LI9xh+0ueHr+ocBRW6LHgAJFYWXIjoeJyT4/XNfSvVYoxmNppjrhkQ==; 5:m4lkyFn07OgT/slQESwqBV1h1DJmNkFwr/CKIj5pdbldNBLDHgN8+0NaRtZxVfo86VDWcby+VzvzqtYX1XtNukcsPR7UOPIe8Xy6xpRHwz7bWts7BI8YiAJ7X18/vyiB157dd+ItnlvEbDH8xT6erWO+W5zHrknkoJNIDIaL7Vc=; 24:8o7wpv8o/diC2UbiMpwrzawwXC6WTdyfq83GSxV+t/Ukn5ssJON4u+A7GXBu9nX8NT16YIh9JUOfCasmTlmpaF1N4A5W1NYL8890BmPxi54= SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-Microsoft-Exchange-Diagnostics: 1; DB6PR0501MB2039; 7:tpFo7b+nA/ZimhUJP4RmrZtwoWY9qQAB/LiQ1r+YUem9ej2VW4Nx0Wt6vmXE5Ffemnv0eajd3eXVSR4+ZE9wkZUjmSQjF2zwI/JUyEFOnKo92B9EojHB3Sqg2L1DLMUj2d8LQiMP5SVC/DM1s99UZdNpL2c9MdYGzuZBsLk53msJNqUtG0gTaUX7iHkXFhzul3Jua1HDddrlpMmywiWfiBoXqnayyVYtRLr5eyyKlQn/Cmqf5wKK7ZXprmwcDvyQ X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Jul 2018 22:27:33.1665 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: 7e6bd43f-a46a-476f-626d-08d5e1342c32 X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR0501MB2039 Subject: Re: [dpdk-dev] [PATCH v2 04/20] net/mlx5: support flow Ethernet item among with drop action 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: , X-List-Received-Date: Tue, 03 Jul 2018 22:27:37 -0000 On Wed, Jun 27, 2018 at 05:07:36PM +0200, Nelio Laranjeiro wrote: net/mlx5: support flow Ethernet item among with drop action Typo? "among" -> "along"? > Signed-off-by: Nelio Laranjeiro > --- > drivers/net/mlx5/mlx5.c | 1 + > drivers/net/mlx5/mlx5_flow.c | 671 ++++++++++++++++++++++++++++++++--- > 2 files changed, 630 insertions(+), 42 deletions(-) > > diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c > index c3c8dffae..665a3c31f 100644 > --- a/drivers/net/mlx5/mlx5.c > +++ b/drivers/net/mlx5/mlx5.c > @@ -241,6 +241,7 @@ mlx5_dev_close(struct rte_eth_dev *dev) > /* In case mlx5_dev_stop() has not been called. */ > mlx5_dev_interrupt_handler_uninstall(dev); > mlx5_traffic_disable(dev); > + mlx5_flow_flush(dev, NULL); > /* Prevent crashes when queues are still in use. */ > dev->rx_pkt_burst = removed_rx_burst; > dev->tx_pkt_burst = removed_tx_burst; > diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c > index 5d3bc183d..bc8c5de33 100644 > --- a/drivers/net/mlx5/mlx5_flow.c > +++ b/drivers/net/mlx5/mlx5_flow.c > @@ -35,11 +35,50 @@ > extern const struct eth_dev_ops mlx5_dev_ops; > extern const struct eth_dev_ops mlx5_dev_ops_isolate; > > +/* Pattern Layer bits. */ > +#define MLX5_FLOW_LAYER_OUTER_L2 (1u << 0) > +#define MLX5_FLOW_LAYER_OUTER_L3_IPV4 (1u << 1) > +#define MLX5_FLOW_LAYER_OUTER_L3_IPV6 (1u << 2) > +#define MLX5_FLOW_LAYER_OUTER_L4_UDP (1u << 3) > +#define MLX5_FLOW_LAYER_OUTER_L4_TCP (1u << 4) > +#define MLX5_FLOW_LAYER_OUTER_VLAN (1u << 5) > +/* Masks. */ > +#define MLX5_FLOW_LAYER_OUTER_L3 \ > + (MLX5_FLOW_LAYER_OUTER_L3_IPV4 | MLX5_FLOW_LAYER_OUTER_L3_IPV6) > +#define MLX5_FLOW_LAYER_OUTER_L4 \ > + (MLX5_FLOW_LAYER_OUTER_L4_UDP | MLX5_FLOW_LAYER_OUTER_L4_TCP) > + > +/* Action fate on the packet. */ > +#define MLX5_FLOW_FATE_DROP (1u << 0) I understand what you mean by 'fate' but 'action fate' sounds strange to me. How about 'terminal action' instead? E.g. flow->terminal_actions or flow->term_actions instead of flow->fate. And, I'm not sure why you define a new (redundant) macro. You could use RTE_FLOW_ACTION_TYPE_DROP instead. I don't see any reason for setting different action flag in flow->fate. If you want to specify a group of terminal actions, you could've done: #define MLX5_FLOW_TERMINAL_ACTIONS \ (RTE_FLOW_ACTION_TYPE_DROP | RTE_FLOW_ACTION_TYPE_RSS | \ RTE_FLOW_ACTION_TYPE_QUEUE) > +/** Handles information leading to a drop fate. */ > +struct mlx5_flow_verbs { > + unsigned int size; /**< Size of the attribute. */ > + struct { > + struct ibv_flow_attr *attr; > + /**< Pointer to the Specification buffer. */ > + uint8_t *specs; /**< Pointer to the specifications. */ > + }; > + struct ibv_flow *flow; /**< Verbs flow pointer. */ > + struct mlx5_hrxq *hrxq; /**< Hash Rx queue object. */ > +}; > + > +/* Flow structure. */ > struct rte_flow { > TAILQ_ENTRY(rte_flow) next; /**< Pointer to the next flow structure. */ > + struct rte_flow_attr attributes; /**< User flow attribute. */ > + uint32_t layers; > + /**< Bit-fields of present layers see MLX5_FLOW_ITEMS_*. */ > + uint32_t fate; > + /**< Bit-fields of present fate see MLX5_FLOW_FATE_*. */ > + struct mlx5_flow_verbs verbs; /* Verbs drop flow. */ > }; > > static const struct rte_flow_ops mlx5_flow_ops = { > + .validate = mlx5_flow_validate, > + .create = mlx5_flow_create, > + .destroy = mlx5_flow_destroy, > + .flush = mlx5_flow_flush, > .isolate = mlx5_flow_isolate, > }; > > @@ -128,12 +167,418 @@ mlx5_flow_priorities(struct rte_eth_dev *dev) > } > > /** > - * Convert a flow. > + * Flow debug purpose function only available when > + * CONFIG_RTE_LIBRTE_MLX5_DEBUG=y > + * > + * @param flow > + * Pointer to the flow structure to display. > + */ > +void > +mlx5_flow_print(struct rte_flow *flow __rte_unused) > +{ > +#ifndef NDEBUG > + fprintf(stdout, "---------8<------------\n"); > + fprintf(stdout, "%s: flow information\n", MLX5_DRIVER_NAME); > + fprintf(stdout, " attributes: group %u priority %u ingress %d egress %d" > + " transfer %d\n", flow->attributes.group, > + flow->attributes.priority, > + flow->attributes.ingress, > + flow->attributes.egress, > + flow->attributes.transfer); > + fprintf(stdout, " layers: %s/%s/%s\n", > + flow->layers & MLX5_FLOW_LAYER_OUTER_L2 ? "l2" : "-", > + flow->layers & MLX5_FLOW_LAYER_OUTER_L3 ? "l3" : "-", > + flow->layers & MLX5_FLOW_LAYER_OUTER_L4 ? "l4" : "-"); > + if (flow->fate & MLX5_FLOW_FATE_DROP) > + fprintf(stdout, " fate: drop queue\n"); > + if (flow->verbs.attr) { > + struct ibv_spec_header *hdr = > + (struct ibv_spec_header *)flow->verbs.specs; > + const int n = flow->verbs.attr->num_of_specs; > + int i; > + > + fprintf(stdout, " Verbs attributes: specs_n %u\n", > + flow->verbs.attr->num_of_specs); > + for (i = 0; i != n; ++i) { > + rte_hexdump(stdout, " ", hdr, hdr->size); > + hdr = (struct ibv_spec_header *) > + ((uint8_t *)hdr + hdr->size); > + } > + } > + fprintf(stdout, "--------->8------------\n"); > +#endif > +} > + > +/** > + * Validate Attributes provided by the user. And it also stores attributes. > * > * @param dev > * Pointer to Ethernet device. > - * @param list > - * Pointer to a TAILQ flow list. > + * @param attr > + * Pointer to flow attributes > + * @param flow > + * Pointer to the rte_flow structure. > + * @param error > + * Pointer to error structure. > + * > + * @return > + * 0 on success, a negative errno value otherwise and rte_errno is set. > + */ > +static int > +mlx5_flow_attributes(struct rte_eth_dev *dev, const struct rte_flow_attr *attr, > + struct rte_flow *flow, struct rte_flow_error *error) Better to have mlx5_flow_attributes_validate() like mlx5_flow_item_validate()? > +{ > + uint32_t priority_max = > + ((struct priv *)dev->data->dev_private)->config.flow_prio; > + > + if (attr->group) > + return rte_flow_error_set(error, ENOTSUP, > + RTE_FLOW_ERROR_TYPE_ATTR_GROUP, > + NULL, > + "groups are not supported"); "group is not supported"? > + if (attr->priority >= priority_max) > + return rte_flow_error_set(error, ENOTSUP, > + RTE_FLOW_ERROR_TYPE_ATTR_PRIORITY, > + NULL, > + "priority value is not supported"); "priority is out of range" or something else? > + if (attr->egress) > + return rte_flow_error_set(error, ENOTSUP, > + RTE_FLOW_ERROR_TYPE_ATTR_EGRESS, > + NULL, > + "egress is not supported"); > + if (attr->transfer) > + return rte_flow_error_set(error, ENOTSUP, > + RTE_FLOW_ERROR_TYPE_ATTR_TRANSFER, > + NULL, > + "transfer is not supported"); > + if (!attr->ingress) > + return rte_flow_error_set(error, ENOTSUP, > + RTE_FLOW_ERROR_TYPE_ATTR_INGRESS, > + NULL, > + "only ingress is supported"); Better to have "ingress should be specified" or something else? > + flow->attributes = *attr; > + return 0; > +} > + > +/** > + * Check support for a given item. > + * > + * @param item[in] > + * Item specification. > + * @param default_mask[in] > + * Bit-masks covering supported fields to compare with spec, last and mask in > + * \item. Remove the back-slash and the description isn't clear. From the code, it seems to be a default mask to be used if item doesn't have mask. > + * @param nic_mask[in] > + * Bit-masks covering supported fields by the NIC to compare with user mask. s/Bit-masks/Bit-mask > + * @param size > + * Bit-Mask size in bytes. s/Bit-Mask/Bit-mask > + * @param error[out] > + * Pointer to error structure. > + * > + * @return > + * 0 on success, a negative errno value otherwise and rte_errno is set. > + */ > +static int > +mlx5_flow_item_validate(const struct rte_flow_item *item, > + const uint8_t *default_mask, > + const uint8_t *nic_mask, > + unsigned int size, > + struct rte_flow_error *error) > +{ > + const uint8_t *mask = item->mask ? item->mask : default_mask; In the mlx5_flow_item_eth(), rte_flow_item_eth_mask is passed to default_mask if item->mask is null, then why doing it again here? > + unsigned int i; > + > + assert(nic_mask); > + for (i = 0; i < size; ++i) > + if ((nic_mask[i] | mask[i]) != nic_mask[i]) > + return rte_flow_error_set(error, ENOTSUP, > + RTE_FLOW_ERROR_TYPE_ITEM, > + item, > + "mask enables non supported" > + " bits"); default_mask is determined by caller, which is rte_flow_item_eth_mask or item->mask. And nic_mask is also given by PMD, which is a function local structure in the caller func. Doesn't look like a clear definition. Intead of having both default_mask and nic_mask here, you can take supported_mask, which can be (rte_flow_item_{name}_mask | nic_mask) in the calling func. And check validity of item->mask against supported_mask only if item->mask isn't null. > + if (!item->spec && (item->mask || item->last)) > + return rte_flow_error_set(error, EINVAL, > + RTE_FLOW_ERROR_TYPE_ITEM, > + item, > + "mask/last without a spec is not" > + " supported"); > + if (item->spec && item->last) { > + uint8_t spec[size]; > + uint8_t last[size]; > + unsigned int i; > + int ret; > + > + for (i = 0; i < size; ++i) { > + spec[i] = ((const uint8_t *)item->spec)[i] & mask[i]; > + last[i] = ((const uint8_t *)item->last)[i] & mask[i]; > + } > + ret = memcmp(spec, last, size); > + if (ret != 0) > + return rte_flow_error_set(error, ENOTSUP, > + RTE_FLOW_ERROR_TYPE_ITEM, > + item, > + "spec and last different" > + " is not supported"); Can you rephrase the error message? > + } > + return 0; > +} > + > +/** > + * Add a verbs specification. > + * > + * @param flow > + * Pointer to flow structure. > + * @param src > + * Create specification. > + * @param size > + * Size in bytes of the specification to copy. > + */ > +static void > +mlx5_flow_spec_verbs_add(struct rte_flow *flow, void *src, unsigned int size) > +{ > + if (flow->verbs.specs) { > + void *dst; > + > + dst = (void *)(flow->verbs.specs + flow->verbs.size); > + memcpy(dst, src, size); > + ++flow->verbs.attr->num_of_specs; > + } > + flow->verbs.size += size; > +} > + > +/** > + * Validate Ethernet layer and possibly create the Verbs specification. > + * > + * @param item[in] > + * Item specification. > + * @param flow[in, out] > + * Pointer to flow structure. > + * @param flow_size[in] > + * Size of the buffer to store the specification. > + * @param error > + * Pointer to error structure. > + * > + * @return > + * size in bytes necessary for the conversion, a negative errno value > + * otherwise and rte_errno is set. > + */ > +static int > +mlx5_flow_item_eth(const struct rte_flow_item *item, struct rte_flow *flow, > + const size_t flow_size, struct rte_flow_error *error) > +{ > + const struct rte_flow_item_eth *spec = item->spec; > + const struct rte_flow_item_eth *mask = item->mask; > + const struct rte_flow_item_eth nic_mask = { > + .dst.addr_bytes = "\xff\xff\xff\xff\xff\xff", > + .src.addr_bytes = "\xff\xff\xff\xff\xff\xff", > + .type = RTE_BE16(0xffff), > + }; If it is a device-specific one, shouldn't it be located in a header file? > + const unsigned int size = sizeof(struct ibv_flow_spec_eth); > + struct ibv_flow_spec_eth eth = { > + .type = IBV_FLOW_SPEC_ETH, > + .size = size, > + }; > + int ret; > + > + if (flow->layers & MLX5_FLOW_LAYER_OUTER_L2) > + return rte_flow_error_set(error, ENOTSUP, > + RTE_FLOW_ERROR_TYPE_ITEM, > + item, > + "L2 layers already configured"); > + if (!mask) > + mask = &rte_flow_item_eth_mask; What is the meaning of default_mask for mlx5_flow_item_validate()? Also refer to my comments in mlx5_flow_item_validate(). > + ret = mlx5_flow_item_validate(item, (const uint8_t *)mask, > + (const uint8_t *)&nic_mask, > + sizeof(struct rte_flow_item_eth), > + error); > + if (ret) > + return ret; > + flow->layers |= MLX5_FLOW_LAYER_OUTER_L2; > + if (size > flow_size) > + return size; > + if (spec) { > + unsigned int i; > + > + memcpy(ð.val.dst_mac, spec->dst.addr_bytes, ETHER_ADDR_LEN); > + memcpy(ð.val.src_mac, spec->src.addr_bytes, ETHER_ADDR_LEN); > + eth.val.ether_type = spec->type; > + memcpy(ð.mask.dst_mac, mask->dst.addr_bytes, ETHER_ADDR_LEN); > + memcpy(ð.mask.src_mac, mask->src.addr_bytes, ETHER_ADDR_LEN); > + eth.mask.ether_type = mask->type; > + /* Remove unwanted bits from values. */ > + for (i = 0; i < ETHER_ADDR_LEN; ++i) { > + eth.val.dst_mac[i] &= eth.mask.dst_mac[i]; > + eth.val.src_mac[i] &= eth.mask.src_mac[i]; > + } > + eth.val.ether_type &= eth.mask.ether_type; > + } > + mlx5_flow_spec_verbs_add(flow, ð, size); Do you have a plan to change this redundant memcpy? I mean, writing directly to the allocated memory for verbs spec instead of the local micro-scratchpad. > + return size; > +} > + > +/** > + * Validate items provided by the user. > + * > + * @param items > + * Pointer to flow items array. > + * @param flow > + * Pointer to the rte_flow structure. > + * @param flow_size > + * Size in bytes of the available space for to store the flow information. > + * @param error > + * Pointer to error structure. > + * > + * @return > + * size in bytes necessary for the conversion, a negative errno value > + * otherwise and rte_errno is set. > + */ > +static int > +mlx5_flow_items(const struct rte_flow_item items[], > + struct rte_flow *flow, const size_t flow_size, > + struct rte_flow_error *error) > +{ > + int remain = flow_size; > + size_t size = 0; > + > + for (; items->type != RTE_FLOW_ITEM_TYPE_END; items++) { > + int ret = 0; > + > + switch (items->type) { > + case RTE_FLOW_ITEM_TYPE_VOID: > + break; > + case RTE_FLOW_ITEM_TYPE_ETH: > + ret = mlx5_flow_item_eth(items, flow, remain, error); > + break; > + default: > + return rte_flow_error_set(error, ENOTSUP, > + RTE_FLOW_ERROR_TYPE_ITEM, > + items, > + "item not supported"); > + } > + if (ret < 0) > + return ret; > + if (remain > ret) > + remain -= ret; > + else > + remain = 0; > + size += ret; > + } > + if (!flow->layers) { > + const struct rte_flow_item item = { > + .type = RTE_FLOW_ITEM_TYPE_ETH, > + }; > + > + return mlx5_flow_item_eth(&item, flow, flow_size, error); > + } > + return size; > +} > + > +/** > + * Validate action drop provided by the user. > + * > + * @param actions > + * Pointer to flow actions array. > + * @param flow > + * Pointer to the rte_flow structure. > + * @param flow_size > + * Size in bytes of the available space for to store the flow information. > + * @param error > + * Pointer to error structure. > + * > + * @return > + * size in bytes necessary for the conversion, a negative errno value > + * otherwise and rte_errno is set. > + */ > +static int > +mlx5_flow_action_drop(const struct rte_flow_action *actions, > + struct rte_flow *flow, const size_t flow_size, > + struct rte_flow_error *error) > +{ > + unsigned int size = sizeof(struct ibv_flow_spec_action_drop); > + struct ibv_flow_spec_action_drop drop = { > + .type = IBV_FLOW_SPEC_ACTION_DROP, > + .size = size, > + }; > + > + if (flow->fate) > + return rte_flow_error_set(error, ENOTSUP, > + RTE_FLOW_ERROR_TYPE_ACTION, > + actions, > + "multiple fate actions are not" > + " supported"); > + if (size < flow_size) > + mlx5_flow_spec_verbs_add(flow, &drop, size); > + flow->fate |= MLX5_FLOW_FATE_DROP; > + return size; > +} > + > +/** > + * Validate actions provided by the user. > + * > + * @param dev > + * Pointer to Ethernet device structure. > + * @param actions > + * Pointer to flow actions array. > + * @param flow > + * Pointer to the rte_flow structure. > + * @param flow_size > + * Size in bytes of the available space for to store the flow information. > + * @param error > + * Pointer to error structure. > + * > + * @return > + * size in bytes necessary for the conversion, a negative errno value > + * otherwise and rte_errno is set. > + */ > +static int > +mlx5_flow_actions(struct rte_eth_dev *dev __rte_unused, > + const struct rte_flow_action actions[], > + struct rte_flow *flow, const size_t flow_size, > + struct rte_flow_error *error) > +{ > + size_t size = 0; > + int remain = flow_size; > + int ret = 0; > + > + for (; actions->type != RTE_FLOW_ACTION_TYPE_END; actions++) { > + switch (actions->type) { > + case RTE_FLOW_ACTION_TYPE_VOID: > + break; > + case RTE_FLOW_ACTION_TYPE_DROP: > + ret = mlx5_flow_action_drop(actions, flow, remain, > + error); > + break; > + default: > + return rte_flow_error_set(error, ENOTSUP, > + RTE_FLOW_ERROR_TYPE_ACTION, > + actions, > + "action not supported"); > + } > + if (ret < 0) > + return ret; > + if (remain > ret) > + remain -= ret; > + else > + remain = 0; > + size += ret; > + } > + if (!flow->fate) > + return rte_flow_error_set(error, ENOTSUP, > + RTE_FLOW_ERROR_TYPE_UNSPECIFIED, > + NULL, > + "no fate action found"); > + return size; > +} > + > +/** > + * Validate the rule and return a flow structure filled accordingly. Please add more detailed description. This is the most important function in mlx5_flow.c, it deserves more lines of explanation. Make sure to explain about the multiple functionality it has - validate, get required size and execute translation only if it gets allocated memory. Plus, how to validate a flow, e.g. setting flow_size=0 and flow can be null in that case. > + * > + * @param dev > + * Pointer to Ethernet device. > + * @param flow > + * Pointer an flow structure. s/an/to shouldn't it be [out]? And please mention flow can be null for testing call. > + * @param flow_size > + * Size of the allocated space to store the flow information. Please mention what happens if the size isn't enough or announce it can be intentionally zero in order to validate the flow and to know the required size for translation. > * @param[in] attr > * Flow rule attributes. > * @param[in] pattern > @@ -144,21 +589,39 @@ mlx5_flow_priorities(struct rte_eth_dev *dev) > * Perform verbose error reporting if not NULL. > * > * @return > - * A flow on success, NULL otherwise and rte_errno is set. > + * The amount of bytes necessary to create the flow, a negative errno value > + * otherwise and rte_errno is set. 'amount of bytes' -> 'size of memory' And not clear what is 'otherwise' because there's no definition of success. > */ > -static struct rte_flow * > -mlx5_flow_list_create(struct rte_eth_dev *dev __rte_unused, > - struct mlx5_flows *list __rte_unused, > - const struct rte_flow_attr *attr __rte_unused, > - const struct rte_flow_item items[] __rte_unused, > - const struct rte_flow_action actions[] __rte_unused, > - struct rte_flow_error *error) > +static int > +mlx5_flow_merge(struct rte_eth_dev *dev, struct rte_flow *flow, > + const size_t flow_size, > + const struct rte_flow_attr *attr, > + const struct rte_flow_item items[], > + const struct rte_flow_action actions[], > + struct rte_flow_error *error) > { > - rte_flow_error_set(error, ENOTSUP, > - RTE_FLOW_ERROR_TYPE_UNSPECIFIED, > - NULL, > - "action not supported"); > - return NULL; > + struct rte_flow local_flow = { .layers = 0, }; > + size_t size = sizeof(*flow) + sizeof(struct ibv_flow_attr); > + int remain = (flow_size > size) ? flow_size - size : 0; > + int ret; > + > + if (!remain) > + flow = &local_flow; > + ret = mlx5_flow_attributes(dev, attr, flow, error); > + if (ret < 0) > + return ret; > + ret = mlx5_flow_items(items, flow, remain, error); > + if (ret < 0) > + return ret; > + size += ret; > + remain = (flow_size > size) ? flow_size - size : 0; > + ret = mlx5_flow_actions(dev, actions, flow, remain, error); > + if (ret < 0) > + return ret; > + size += ret; > + if (size <= flow_size) > + flow->verbs.attr->priority = flow->attributes.priority; > + return size; > } > > /** > @@ -168,16 +631,142 @@ mlx5_flow_list_create(struct rte_eth_dev *dev __rte_unused, > * @see rte_flow_ops > */ > int > -mlx5_flow_validate(struct rte_eth_dev *dev __rte_unused, > - const struct rte_flow_attr *attr __rte_unused, > - const struct rte_flow_item items[] __rte_unused, > - const struct rte_flow_action actions[] __rte_unused, > +mlx5_flow_validate(struct rte_eth_dev *dev, > + const struct rte_flow_attr *attr, > + const struct rte_flow_item items[], > + const struct rte_flow_action actions[], > struct rte_flow_error *error) > { > - return rte_flow_error_set(error, ENOTSUP, > - RTE_FLOW_ERROR_TYPE_UNSPECIFIED, > - NULL, > - "action not supported"); > + int ret = mlx5_flow_merge(dev, NULL, 0, attr, items, actions, error); > + > + if (ret < 0) > + return ret; > + return 0; > +} > + > +/** > + * Remove the flow. > + * > + * @param dev > + * Pointer to Ethernet device. > + * @param flow > + * Pointer to flow structure. > + */ > +static void > +mlx5_flow_fate_remove(struct rte_eth_dev *dev, struct rte_flow *flow) Sounds weird. Is it removing a fate of a flow? Description is just 'removing a flow'. > +{ > + if (flow->fate & MLX5_FLOW_FATE_DROP) { > + if (flow->verbs.flow) { > + claim_zero(mlx5_glue->destroy_flow(flow->verbs.flow)); > + flow->verbs.flow = NULL; > + } > + } > + if (flow->verbs.hrxq) { > + mlx5_hrxq_drop_release(dev, flow->verbs.hrxq); > + flow->verbs.hrxq = NULL; > + } > +} > + > +/** > + * Apply the flow. > + * > + * @param dev > + * Pointer to Ethernet device structure. > + * @param flow > + * Pointer to flow structure. > + * @param error > + * Pointer to error structure. > + * > + * @return > + * 0 on success, a negative errno value otherwise and rte_errno is set. > + */ > +static int > +mlx5_flow_fate_apply(struct rte_eth_dev *dev, struct rte_flow *flow, > + struct rte_flow_error *error) Same here. Either change the name or add more description. > +{ > + flow->verbs.hrxq = mlx5_hrxq_drop_new(dev); > + if (!flow->verbs.hrxq) > + return rte_flow_error_set > + (error, errno, > + RTE_FLOW_ERROR_TYPE_UNSPECIFIED, > + NULL, > + "cannot allocate Drop queue"); > + flow->verbs.flow = > + mlx5_glue->create_flow(flow->verbs.hrxq->qp, flow->verbs.attr); > + if (!flow->verbs.flow) { > + mlx5_hrxq_drop_release(dev, flow->verbs.hrxq); > + flow->verbs.hrxq = NULL; > + return rte_flow_error_set(error, errno, > + RTE_FLOW_ERROR_TYPE_UNSPECIFIED, > + NULL, > + "kernel module refuses to create" > + " flow"); > + } > + return 0; > +} > + > +/** > + * Create a flow. And "insert to the provided list". > + * > + * @param dev > + * Pointer to Ethernet device. > + * @param list > + * Pointer to a TAILQ flow list. > + * @param[in] attr > + * Flow rule attributes. > + * @param[in] items > + * Pattern specification (list terminated by the END pattern item). > + * @param[in] actions > + * Associated actions (list terminated by the END action). > + * @param[out] error > + * Perform verbose error reporting if not NULL. > + * > + * @return > + * A flow on success, NULL otherwise and rte_errno is set. > + */ > +static struct rte_flow * > +mlx5_flow_list_create(struct rte_eth_dev *dev, > + struct mlx5_flows *list, > + const struct rte_flow_attr *attr, > + const struct rte_flow_item items[], > + const struct rte_flow_action actions[], > + struct rte_flow_error *error) > +{ > + struct rte_flow *flow; > + size_t size; > + int ret; > + > + ret = mlx5_flow_merge(dev, NULL, 0, attr, items, actions, error); > + if (ret < 0) > + return NULL; > + size = ret; > + flow = rte_zmalloc(__func__, size, 0); > + if (!flow) { > + rte_flow_error_set(error, ENOMEM, > + RTE_FLOW_ERROR_TYPE_UNSPECIFIED, > + NULL, > + "cannot allocate memory"); > + return NULL; > + } > + flow->verbs.attr = (struct ibv_flow_attr *)(flow + 1); > + flow->verbs.specs = (uint8_t *)(flow->verbs.attr + 1); > + ret = mlx5_flow_merge(dev, flow, size, attr, items, actions, error); > + if (ret < 0) > + goto error; > + assert((size_t)ret == size); > + if (dev->data->dev_started) { > + ret = mlx5_flow_fate_apply(dev, flow, error); > + if (ret < 0) > + goto error; > + } > + TAILQ_INSERT_TAIL(list, flow, next); > + return flow; > +error: > + ret = rte_errno; /* Save rte_errno before cleanup. */ > + mlx5_flow_fate_remove(dev, flow); > + rte_free(flow); > + rte_errno = ret; /* Restore rte_errno. */ > + return NULL; > } > > /** > @@ -187,17 +776,15 @@ mlx5_flow_validate(struct rte_eth_dev *dev __rte_unused, > * @see rte_flow_ops > */ > struct rte_flow * > -mlx5_flow_create(struct rte_eth_dev *dev __rte_unused, > - const struct rte_flow_attr *attr __rte_unused, > - const struct rte_flow_item items[] __rte_unused, > - const struct rte_flow_action actions[] __rte_unused, > +mlx5_flow_create(struct rte_eth_dev *dev, > + const struct rte_flow_attr *attr, > + const struct rte_flow_item items[], > + const struct rte_flow_action actions[], > struct rte_flow_error *error) > { > - rte_flow_error_set(error, ENOTSUP, > - RTE_FLOW_ERROR_TYPE_UNSPECIFIED, > - NULL, > - "action not supported"); > - return NULL; > + return mlx5_flow_list_create > + (dev, &((struct priv *)dev->data->dev_private)->flows, > + attr, items, actions, error); > } > > /** > @@ -211,10 +798,12 @@ mlx5_flow_create(struct rte_eth_dev *dev __rte_unused, > * Flow to destroy. > */ > static void > -mlx5_flow_list_destroy(struct rte_eth_dev *dev __rte_unused, > - struct mlx5_flows *list __rte_unused, > - struct rte_flow *flow __rte_unused) > +mlx5_flow_list_destroy(struct rte_eth_dev *dev, struct mlx5_flows *list, > + struct rte_flow *flow) > { > + mlx5_flow_fate_remove(dev, flow); > + TAILQ_REMOVE(list, flow, next); > + rte_free(flow); > } > > /** > @@ -422,9 +1011,8 @@ int > mlx5_flow_flush(struct rte_eth_dev *dev, > struct rte_flow_error *error __rte_unused) > { > - struct priv *priv = dev->data->dev_private; > - > - mlx5_flow_list_flush(dev, &priv->flows); > + mlx5_flow_list_flush(dev, > + &((struct priv *)dev->data->dev_private)->flows); Can you keep priv and make this line simpler? > return 0; > } > > @@ -737,9 +1325,8 @@ mlx5_fdir_filter_update(struct rte_eth_dev *dev, > static void > mlx5_fdir_filter_flush(struct rte_eth_dev *dev) > { > - struct priv *priv = dev->data->dev_private; > - > - mlx5_flow_list_flush(dev, &priv->flows); > + mlx5_flow_list_flush(dev, > + &((struct priv *)dev->data->dev_private)->flows); Same here. > } > > /** > -- > 2.18.0 >