Message ID | 20210727073121.895620-1-dkozlyuk@nvidia.com (mailing list archive) |
---|---|
Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@inbox.dpdk.org Delivered-To: patchwork@inbox.dpdk.org Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 720A3A0C4D; Tue, 27 Jul 2021 09:31:41 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id E62E9410ED; Tue, 27 Jul 2021 09:31:40 +0200 (CEST) Received: from NAM11-DM6-obe.outbound.protection.outlook.com (mail-dm6nam11on2054.outbound.protection.outlook.com [40.107.223.54]) by mails.dpdk.org (Postfix) with ESMTP id 1634C410EC for <dev@dpdk.org>; Tue, 27 Jul 2021 09:31:39 +0200 (CEST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=ieJiG9pVBVRV4X4joS4luW7JbzFVGV9YJ5hBWmEr6j0TS2R0Lo28oJE0koY0Ay10HXjJGCpeuZIQHP0FUJwcKKpGKd4WWqRSDqfNPEVbbS2e7vrfFkDkCu63DbF18NAXkpAFm7Q1XPc4w/3JGOOCC1lYzDsK2QXSrk43JZVnQ+uVtguMdmxUijVg2cb6ZjBA5qepnQFwdKO0rFHl3y5P0nRv7FDwl6TJ6Dfqvo1cgcgjtfLpMsES4SAHEr0Gr6SvmkrUXC8pH/WyCX+4MYC42lXK57jT5OSjTZO9TGodPOkzLtWF1pWOIdfiUnv0C0ZzZqU5AQ6RGmEk0SmngBy1xw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=Cp8uH5M1lk9rwxrZ7kVOBvBsNDDHgSYlKB7fg+LLte4=; b=YqRITG7dHwzqu+mwF0qBNHV5ZbaXoVpx+IOz32TanffyPuR2oaJIjuuAU0FrkVhagkssY5CoPaKu9e86EG4DdtsJForc18Qii9CDa4MLfdHQeYoRRZhQmTa1E4qQQTEbRjQ9dnyDQSHhQ3Xy3q6nh8vp+48q9s/dTZJnlMw5FeR11CqKb2VDPnNWbXRpoToyTpBB4CyHr2rtrTpJQcr47WLmZ83Edk4l9U6FxsTKfHvwIdicpCI1U9b1niKo+4YXcU05lTiXArOGDYf121ef6DqNtIIL+pdVSm6tdkRO8mDAkk2xC9lEBxMdFJRxxZZm3oImQC4q8FTSsric6Gn7AA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass (sender ip is 216.228.112.34) smtp.rcpttodomain=redhat.com smtp.mailfrom=nvidia.com; dmarc=pass (p=quarantine sp=none pct=100) action=none header.from=nvidia.com; dkim=none (message not signed); arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Nvidia.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=Cp8uH5M1lk9rwxrZ7kVOBvBsNDDHgSYlKB7fg+LLte4=; b=thYBgbtVO5cdfw+OVznLnf4WbWxgSE4YRzPaTrQduYJCANtmAkRtIia+0j1k48Ik7mUlBM2a60+VpcQWqyGMcAZr9XDGBw86zCgFYLtSLkiFi56ouRgdXdbjuWlqBAc1OTxthgApQbLwkSxeIhd3a/xQrdhwLtzImy6+hL0J2y3uHWu5SnmjCd1+dwI58COXuRMa8WxJnd6RoqdOqxA7XjJvCtt+Cb+B/UDEvSsSjluYZ1QVaf4zUj7mltH6bo/lwnXHsN+SdZl462Zmg/X2CAi+ij62shFdqWOarPVQU5WLkHOm7/0prP4KFLII7kZfiy0/XXAQbm9WWY0qsqD6HQ== Received: from BN1PR14CA0015.namprd14.prod.outlook.com (2603:10b6:408:e3::20) by BYAPR12MB2936.namprd12.prod.outlook.com (2603:10b6:a03:12f::17) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4352.29; Tue, 27 Jul 2021 07:31:37 +0000 Received: from BN8NAM11FT020.eop-nam11.prod.protection.outlook.com (2603:10b6:408:e3:cafe::82) by BN1PR14CA0015.outlook.office365.com (2603:10b6:408:e3::20) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4352.25 via Frontend Transport; Tue, 27 Jul 2021 07:31:37 +0000 X-MS-Exchange-Authentication-Results: spf=pass (sender IP is 216.228.112.34) smtp.mailfrom=nvidia.com; redhat.com; dkim=none (message not signed) header.d=none;redhat.com; dmarc=pass action=none header.from=nvidia.com; Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates 216.228.112.34 as permitted sender) receiver=protection.outlook.com; client-ip=216.228.112.34; helo=mail.nvidia.com; Received: from mail.nvidia.com (216.228.112.34) by BN8NAM11FT020.mail.protection.outlook.com (10.13.176.223) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384) id 15.20.4352.24 via Frontend Transport; Tue, 27 Jul 2021 07:31:37 +0000 Received: from nvidia.com (172.20.187.5) by HQMAIL107.nvidia.com (172.20.187.13) with Microsoft SMTP Server (TLS) id 15.0.1497.2; Tue, 27 Jul 2021 07:31:35 +0000 From: Dmitry Kozlyuk <dkozlyuk@nvidia.com> To: <dev@dpdk.org> CC: David Marchand <david.marchand@redhat.com> Date: Tue, 27 Jul 2021 10:31:16 +0300 Message-ID: <20210727073121.895620-1-dkozlyuk@nvidia.com> X-Mailer: git-send-email 2.25.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-Originating-IP: [172.20.187.5] X-ClientProxiedBy: HQMAIL105.nvidia.com (172.20.187.12) To HQMAIL107.nvidia.com (172.20.187.13) X-EOPAttributedMessage: 0 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 08b2d02b-ce42-4ce5-1d10-08d950d09173 X-MS-TrafficTypeDiagnostic: BYAPR12MB2936: X-Microsoft-Antispam-PRVS: <BYAPR12MB29362D0D5F223B0B6E717401B9E99@BYAPR12MB2936.namprd12.prod.outlook.com> X-MS-Oob-TLC-OOBClassifiers: OLM:5797; X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: cnZNth5dlDwPmvS0IAbY8ywj9ikS135xsksU2MTTVZ31MV21qivxgImt/SkiLC4fS7OKUu27HI8JyM5xwLR1l8ZCdYoqOS/SkcNCLupTMzb9oNGOGIp7IQsm9C828mZJIDRAxwlcjHIdOiGC2ADW6XU8kGzzLsbWeGu5KUnbHkdkCzNbWXC7OoKwHwUc454X+pLKjub2gjpPXCr9x3Rku+RQ2elrW+z0IXGc/jXJ50T5yqJ/QoF1Vri5RKI/gmmbhd01kBN4OJsypUp++Z4kqb5K0jy+ffZRWTrYmn5le/hC5NClMYVDFSdwp4+nMYgmqmpOA1yanWzNnX/jcMZwj3WEjEMI1V7XnovxnSFyORMXZvK7RxXmxZ/QlO+OxF4y3up5fDIV7cVw7m0V7bdAiQtF2zEmHggkUcvJ7e4DAIDnlpI2KTKkbJ4OJcgY5Er3gLCx6NzSIkKYsumGVb0KhXCds7aH2ZaerNplQF/picU8LyNLVNRZviw1EREbBI1ALWElJXZm/rphRDKvZcCUQ375TF8CRQc+9OtkKK37y363DYbZCo8uEzQOnLmQ3aRcM8upUdw0DjLCM3FhQdqvp92MxmcTBxB7E6LjBtaHhIYu2vvJ37D32A8R0b+igSZBy69AsJtYVBBnd6T6f3gTMgEraPj28bgngd7oC6axGKmnZXUhl3Qx8xcAyU31U5yHLP0OTBJgsgletHSyop1nfg== X-Forefront-Antispam-Report: CIP:216.228.112.34; CTRY:US; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:mail.nvidia.com; PTR:schybrid03.nvidia.com; CAT:NONE; SFS:(4636009)(36840700001)(46966006)(8676002)(82310400003)(26005)(316002)(7696005)(47076005)(2906002)(1076003)(36756003)(5660300002)(508600001)(426003)(6666004)(70586007)(8936002)(70206006)(336012)(2616005)(4326008)(83380400001)(36860700001)(6286002)(86362001)(55016002)(16526019)(186003)(356005)(7636003)(36906005)(6916009); DIR:OUT; SFP:1101; X-OriginatorOrg: Nvidia.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Jul 2021 07:31:37.1636 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: 08b2d02b-ce42-4ce5-1d10-08d950d09173 X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a; Ip=[216.228.112.34]; Helo=[mail.nvidia.com] X-MS-Exchange-CrossTenant-AuthSource: BN8NAM11FT020.eop-nam11.prod.protection.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Anonymous X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR12MB2936 Subject: [dpdk-dev] [PATCH 0/4] net/mlx5: keep indirect actions across port restart X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions <dev.dpdk.org> List-Unsubscribe: <https://mails.dpdk.org/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://mails.dpdk.org/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <https://mails.dpdk.org/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Series |
net/mlx5: keep indirect actions across port restart
|
|
Message
Dmitry Kozlyuk
July 27, 2021, 7:31 a.m. UTC
It was unspecified what happens to indirect actions when a port is stopped, possibly reconfigured, and started again. MLX5 PMD, the first one to use indirect actions, intended to keep them across such a sequence, but the implementation was buggy. Patches 1-3 fix the PMD behavior, patch 4 adds common specification with rationale. Dmitry Kozlyuk (4): net/mlx5: discover max flow priority using DevX net/mlx5: create drop queue using DevX net/mlx5: preserve indirect actions across port restart ethdev: document indirect flow action life cycle doc/guides/prog_guide/rte_flow.rst | 10 + drivers/net/mlx5/linux/mlx5_os.c | 5 - drivers/net/mlx5/mlx5_devx.c | 204 +++++++++++++++++--- drivers/net/mlx5/mlx5_flow.c | 292 ++++++++++++++++++++++++++--- drivers/net/mlx5/mlx5_flow.h | 6 + drivers/net/mlx5/mlx5_flow_dv.c | 103 ++++++++++ drivers/net/mlx5/mlx5_flow_verbs.c | 77 +------- drivers/net/mlx5/mlx5_rx.h | 4 + drivers/net/mlx5/mlx5_rxq.c | 99 ++++++++-- drivers/net/mlx5/mlx5_trigger.c | 10 + lib/ethdev/rte_flow.h | 4 + 11 files changed, 680 insertions(+), 134 deletions(-)
Comments
On 7/27/21 10:31 AM, Dmitry Kozlyuk wrote: > It was unspecified what happens to indirect actions when a port > is stopped, possibly reconfigured, and started again. MLX5 PMD, > the first one to use indirect actions, intended to keep them across > such a sequence, but the implementation was buggy. Patches 1-3 fix > the PMD behavior, patch 4 adds common specification with rationale. I'm sorry, but it looks very inconsistent. If flow rules are not preserved across restart, indirect actions should not be preserved as well. We need very strong reasons to introduce the inconsistency. If we finally accept it, I think it would be very useful to care about PMDs which cannot preserve it in HW across restart from the very beginning and save it in ethdev layer and restore on start automatically (i.e. do not force all such PMDs to care about the restore internally and basically duplicate the code).
Hi Andrew, > From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> > On 7/27/21 10:31 AM, Dmitry Kozlyuk wrote: > > It was unspecified what happens to indirect actions when a port is > > stopped, possibly reconfigured, and started again. MLX5 PMD, the first > > one to use indirect actions, intended to keep them across such a > > sequence, but the implementation was buggy. Patches 1-3 fix the PMD > > behavior, patch 4 adds common specification with rationale. > > I'm sorry, but it looks very inconsistent. If flow rules are not preserved across > restart, indirect actions should not be preserved as well. We need very strong > reasons to introduce the inconsistency. Indirect actions really don't need to behave like flow rules. They are just objects owned by the port and they can exist while it exists. Consider a counter: stopping and starting the port doesn't logically affect its state. MLX5 PMD destroys flow rules on port stop for internal reasons and documents this behavior, but ethdev API doesn't require it either. > If we finally accept it, I think it would be very useful to care about PMDs which > cannot preserve it in HW across restart from the very beginning and save it in > ethdev layer and restore on start automatically (i.e. do not force all such PMDs > to care about the restore internally and basically duplicate the code). Or keeping indirect actions can be an advertised PMD capability. Given Ori's comments to patch 4, I think the common spec needs more work. For this patchset that fixes MLX5 we can have the behavior documented for PMD and not require it from all the drivers.
Hi Dmitry and Andrew, > -----Original Message----- > From: dev <dev-bounces@dpdk.org> On Behalf Of Dmitry Kozlyuk > Sent: Wednesday, July 28, 2021 2:19 PM > > Hi Andrew, > > > From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> On 7/27/21 > > 10:31 AM, Dmitry Kozlyuk wrote: > > > It was unspecified what happens to indirect actions when a port is > > > stopped, possibly reconfigured, and started again. MLX5 PMD, the > > > first one to use indirect actions, intended to keep them across such > > > a sequence, but the implementation was buggy. Patches 1-3 fix the > > > PMD behavior, patch 4 adds common specification with rationale. > > > > I'm sorry, but it looks very inconsistent. If flow rules are not > > preserved across restart, indirect actions should not be preserved as > > well. We need very strong reasons to introduce the inconsistency. > > Indirect actions really don't need to behave like flow rules. They are just > objects owned by the port and they can exist while it exists. Consider a > counter: stopping and starting the port doesn't logically affect its state. MLX5 > PMD destroys flow rules on port stop for internal reasons and documents > this behavior, but ethdev API doesn't require it either. > > > If we finally accept it, I think it would be very useful to care about > > PMDs which cannot preserve it in HW across restart from the very > > beginning and save it in ethdev layer and restore on start > > automatically (i.e. do not force all such PMDs to care about the restore > internally and basically duplicate the code). > > Or keeping indirect actions can be an advertised PMD capability. > Given Ori's comments to patch 4, I think the common spec needs more work. > For this patchset that fixes MLX5 we can have the behavior documented for > PMD and not require it from all the drivers. This also effects if flows can be stored or not, (there was other thread about it) I think we should have device cap that says if flows are preserved, if they can be created before start, the same goes to actions, but what if some actions can be preserved and some not? For example RSS can't in some HW (or due to configuration change) while other can? For example counter? I don't want to have cap for each action, I think this info is based explained in each driver documentation. Maybe we can have some general flag one for flows and one for actions, and each PMD will have detail doc. Best, Ori
On 7/28/21 2:18 PM, Dmitry Kozlyuk wrote: > Hi Andrew, > >> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> >> On 7/27/21 10:31 AM, Dmitry Kozlyuk wrote: >>> It was unspecified what happens to indirect actions when a port is >>> stopped, possibly reconfigured, and started again. MLX5 PMD, the first >>> one to use indirect actions, intended to keep them across such a >>> sequence, but the implementation was buggy. Patches 1-3 fix the PMD >>> behavior, patch 4 adds common specification with rationale. >> >> I'm sorry, but it looks very inconsistent. If flow rules are not preserved across >> restart, indirect actions should not be preserved as well. We need very strong >> reasons to introduce the inconsistency. > > Indirect actions really don't need to behave like flow rules. They are just objects owned by the port and they can exist while it exists. Consider a counter: stopping and starting the port doesn't logically affect its state. MLX5 PMD destroys flow rules on port stop for internal reasons and documents this behavior, but ethdev API doesn't require it either. It all sounds bad. All these gray areas just make it hard for DPDK applications to switch from one HW to another. Any rules must not be motivated because of some PMD internal reasons. We should not adjust ethdev rules to fit some PMD behaviour. ethdev rules should be motivated by common sense and convenience from applications point of view. For example, it is strange to preserve indirect RSS action with queues specified across device reconfiguration when queues count may change. I'd say that reconfiguration must drop all indirect actions. However, just stop/start could preserve both indirect actions and flow rues since it could be more convenient from application point of view. If application really wants to remove all flow rules, it can call rte_flow_flush(). The strong reason to flush indirect actions and flow rules across restart is possible actions or rules restore failure on start. However, may be it is sufficient to document that start should really fail, if it can't restore everything and application should retry after rte_flow_flush() taking it into account. >> If we finally accept it, I think it would be very useful to care about PMDs which >> cannot preserve it in HW across restart from the very beginning and save it in >> ethdev layer and restore on start automatically (i.e. do not force all such PMDs >> to care about the restore internally and basically duplicate the code). > > Or keeping indirect actions can be an advertised PMD capability. > Given Ori's comments to patch 4, I think the common spec needs more work. > For this patchset that fixes MLX5 we can have the behavior documented for PMD and not require it from all the drivers. Are you going to drop 4th patch? In general documenting PMD behaviour specifics in its documentation is a wrong direction since it does not help DPDK applications to be portable across different HW.
> -----Original Message----- > From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> > Sent: 28 июля 2021 г. 15:27 > To: Dmitry Kozlyuk <dkozlyuk@nvidia.com>; dev@dpdk.org > Cc: David Marchand <david.marchand@redhat.com> > Subject: Re: [dpdk-dev] [PATCH 0/4] net/mlx5: keep indirect actions across port > restart > > External email: Use caution opening links or attachments > > > On 7/28/21 2:18 PM, Dmitry Kozlyuk wrote: > > Hi Andrew, > > > >> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> On 7/27/21 > >> 10:31 AM, Dmitry Kozlyuk wrote: > >>> It was unspecified what happens to indirect actions when a port is > >>> stopped, possibly reconfigured, and started again. MLX5 PMD, the > >>> first one to use indirect actions, intended to keep them across such > >>> a sequence, but the implementation was buggy. Patches 1-3 fix the > >>> PMD behavior, patch 4 adds common specification with rationale. > >> > >> I'm sorry, but it looks very inconsistent. If flow rules are not > >> preserved across restart, indirect actions should not be preserved as > >> well. We need very strong reasons to introduce the inconsistency. > > > > Indirect actions really don't need to behave like flow rules. They are just > objects owned by the port and they can exist while it exists. Consider a counter: > stopping and starting the port doesn't logically affect its state. MLX5 PMD > destroys flow rules on port stop for internal reasons and documents this > behavior, but ethdev API doesn't require it either. > > It all sounds bad. All these gray areas just make it hard for DPDK applications to > switch from one HW to another. > Any rules must not be motivated because of some PMD internal reasons. > We should not adjust ethdev rules to fit some PMD behaviour. > ethdev rules should be motivated by common sense and convenience from > applications point of view. That is what this patchset is trying to do. Current specification is unclear, application doesn't know if it should destroy and recreate indirect actions or not. MLX5 PMD is only mentioned above because it's the only one implementing indirect action API, but it's not an attempt to tailor API to it, quite the opposite. > For example, it is strange to preserve indirect RSS action with queues specified > across device reconfiguration when queues count may change. > I'd say that reconfiguration must drop all indirect actions. I don't like it because 1) it is implicit, 2) it may be unnecessary even for RSS, and it's only one example of an indirect action. > However, just stop/start could preserve both indirect actions and flow rues since > it could be more convenient from application point of view. For many cases I agree, but not for all. What if an application creates numerous flows from its data path? They are transient by nature, but PMD will have to save them all at the cost of RAM and CPU but without benefit to anyone. OTOH, application always controls indirect actions it creates, because it is going to reuse or query them. Therefore, it is both logical and convenient to preserve them. > If application really wants to remove all flow rules, it can call rte_flow_flush(). > The strong reason to flush indirect actions and flow rules across restart is > possible actions or rules restore failure on start. > However, may be it is sufficient to document that start should really fail, if it > can't restore everything and application should retry after rte_flow_flush() > taking it into account. > > >> If we finally accept it, I think it would be very useful to care > >> about PMDs which cannot preserve it in HW across restart from the > >> very beginning and save it in ethdev layer and restore on start > >> automatically (i.e. do not force all such PMDs to care about the restore > internally and basically duplicate the code). > > > > Or keeping indirect actions can be an advertised PMD capability. > > Given Ori's comments to patch 4, I think the common spec needs more work. > > For this patchset that fixes MLX5 we can have the behavior documented for > PMD and not require it from all the drivers. > > Are you going to drop 4th patch? Yes. > In general documenting PMD behaviour specifics in its documentation is a wrong > direction since it does not help DPDK applications to be portable across different > HW. I agree. But currently there is a clear resource leak in MLX5 PMD, that can be solved either by destroying indirect actions on port stop or by keeping them (this is what PMD maintainers prefer). The leak should be fixed and what happens to indirect actions must be clearly documented. Ideally the fix should be aligned with common ethdev API, but if you and Ori think its design is wrong, then at least behavior can be described in PMD docs and later fixed or promoted to API.
Hi, > -----Original Message----- > From: Dmitry Kozlyuk <dkozlyuk@nvidia.com> > Sent: Wednesday, July 28, 2021 5:08 PM > > > -----Original Message----- > > From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> > > Sent: 28 июля 2021 г. 15:27 > > To: Dmitry Kozlyuk <dkozlyuk@nvidia.com>; dev@dpdk.org > > Cc: David Marchand <david.marchand@redhat.com> > > Subject: Re: [dpdk-dev] [PATCH 0/4] net/mlx5: keep indirect actions > > across port restart > > > > External email: Use caution opening links or attachments > > > > > > On 7/28/21 2:18 PM, Dmitry Kozlyuk wrote: > > > Hi Andrew, > > > > > >> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> On > 7/27/21 > > >> 10:31 AM, Dmitry Kozlyuk wrote: > > >>> It was unspecified what happens to indirect actions when a port is > > >>> stopped, possibly reconfigured, and started again. MLX5 PMD, the > > >>> first one to use indirect actions, intended to keep them across > > >>> such a sequence, but the implementation was buggy. Patches 1-3 fix > > >>> the PMD behavior, patch 4 adds common specification with rationale. > > >> > > >> I'm sorry, but it looks very inconsistent. If flow rules are not > > >> preserved across restart, indirect actions should not be preserved > > >> as well. We need very strong reasons to introduce the inconsistency. > > > > > > Indirect actions really don't need to behave like flow rules. They > > > are just > > objects owned by the port and they can exist while it exists. Consider a > counter: > > stopping and starting the port doesn't logically affect its state. > > MLX5 PMD destroys flow rules on port stop for internal reasons and > > documents this behavior, but ethdev API doesn't require it either. > > > > It all sounds bad. All these gray areas just make it hard for DPDK > > applications to switch from one HW to another. > > Any rules must not be motivated because of some PMD internal reasons. > > We should not adjust ethdev rules to fit some PMD behaviour. > > ethdev rules should be motivated by common sense and convenience > from > > applications point of view. > > That is what this patchset is trying to do. > Current specification is unclear, application doesn't know if it should destroy > and recreate indirect actions or not. > MLX5 PMD is only mentioned above because it's the only one implementing > indirect action API, but it's not an attempt to tailor API to it, quite the > opposite. > I agree gray areas are bad, but as more and more application are using more and more flows, insertion of flows become part of the datapath, optimization of actions and rules become even more critical. (To address this we are going to introduce additional API, which will enable async insertion, allocation of resources before flow insertion) So since each HW implements the actions and flows differently forcing the exact same behavior will result in performance degradation. > > For example, it is strange to preserve indirect RSS action with queues > > specified across device reconfiguration when queues count may change. > > I'd say that reconfiguration must drop all indirect actions. > > I don't like it because 1) it is implicit, 2) it may be unnecessary even for RSS, > and it's only one example of an indirect action. > > > However, just stop/start could preserve both indirect actions and flow > > rues since it could be more convenient from application point of view. > > For many cases I agree, but not for all. > What if an application creates numerous flows from its data path? > They are transient by nature, but PMD will have to save them all at the cost > of RAM and CPU but without benefit to anyone. > OTOH, application always controls indirect actions it creates, because it is > going to reuse or query them. > Therefore, it is both logical and convenient to preserve them. > > > If application really wants to remove all flow rules, it can call > rte_flow_flush(). > > The strong reason to flush indirect actions and flow rules across > > restart is possible actions or rules restore failure on start. > > However, may be it is sufficient to document that start should really > > fail, if it can't restore everything and application should retry > > after rte_flow_flush() taking it into account. > > > > >> If we finally accept it, I think it would be very useful to care > > >> about PMDs which cannot preserve it in HW across restart from the > > >> very beginning and save it in ethdev layer and restore on start > > >> automatically (i.e. do not force all such PMDs to care about the > > >> restore > > internally and basically duplicate the code). > > > > > > Or keeping indirect actions can be an advertised PMD capability. > > > Given Ori's comments to patch 4, I think the common spec needs more > work. > > > For this patchset that fixes MLX5 we can have the behavior > > > documented for > > PMD and not require it from all the drivers. > > > > Are you going to drop 4th patch? > > Yes. > > > In general documenting PMD behaviour specifics in its documentation is > > a wrong direction since it does not help DPDK applications to be > > portable across different HW. > > I agree. But currently there is a clear resource leak in MLX5 PMD, that can be > solved either by destroying indirect actions on port stop or by keeping them > (this is what PMD maintainers prefer). The leak should be fixed and what > happens to indirect actions must be clearly documented. Ideally the fix > should be aligned with common ethdev API, but if you and Ori think its > design is wrong, then at least behavior can be described in PMD docs and > later fixed or promoted to API. I think application should be aware to different possibilities between the PMD. If possible, it is best that all PMD will act the same but if there is HW issue I think different behavior is better then not supporting at all. In any case the doc should state the min requirement if HW can support better than it can do so. Best, Ori