Message ID | 20200620133231.12355-1-andrey.vesnovaty@gmail.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 dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 2EDB1A051A; Sat, 20 Jun 2020 15:32:45 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id F29181BFA1; Sat, 20 Jun 2020 15:32:43 +0200 (CEST) Received: from mail-wm1-f44.google.com (mail-wm1-f44.google.com [209.85.128.44]) by dpdk.org (Postfix) with ESMTP id 94DD11BF9D for <dev@dpdk.org>; Sat, 20 Jun 2020 15:32:43 +0200 (CEST) Received: by mail-wm1-f44.google.com with SMTP id l26so10812231wme.3 for <dev@dpdk.org>; Sat, 20 Jun 2020 06:32:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=+2QgVkYX/88KoH487FF0MAvwruIlmgvR1iqkveM7m6M=; b=NRTUyRxzNPoDEotXtFSRRfKm3hE8YP+idFLq/LqMSjVLvlO+RIp4u/dNwODU0z4uuY bQ49u8xipOQqV0fkKLC6UDoEhLGp7EwQgg5+qvcXIWMa0X/yQ/+NiKEI/CAjIrJSecSN TpAuX2NecbVRBuNp4Gw0t97aBxl53Uq9FRiNHj42e0eUnrdhBXHQEcaBVBJJ1bt+ybBI TNc1XxF4dxO2IdNL3ljs+y2Y4i73KueecSvXTOabv4jj4+03YQX1iFBq9Ra3NSplhLFW 9bTOi5BQbLlZ++uyj0SOCjhcy3z9qN0eqEZBh3ppn/Haz7RrhoCgBQK/C2OKcqCokxZM YT8Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=+2QgVkYX/88KoH487FF0MAvwruIlmgvR1iqkveM7m6M=; b=CVOSX9nKPn4GKIEtiQ5fIqqiXd/9QKuTkIEdhgIHULwluPf4BxoIH3nokT2kXsClv5 A/BkR1RTz8lnlHvXUEGVkAxvl19kZR0JdZ/GQqaXCXmB9RyLYdPDd8o8dUGrZFZJ+XUB 0VpjOJdBAzLJroZlkg80agNwN65Y5nI9SPQNV908nLf060NIbslOSjWhv+RcUhDPxGV0 I9xgpN9F8O7HtGR/ktpE3lhYUsxrevE+XkTejYfgmgWl3Z5kXJUT/dzpw+tOXFmLEdNH ELDHyQmgBahXrFEnENDJSN6RXpIMJgAuyAsAhNS/2yg7qUb1oRRLyit6SV1x8E3j0o8z dnFw== X-Gm-Message-State: AOAM531d5Hl79EdOZGTEllXigNwzoL7ltISSeyPDFm8AjOjPJKRUM1vg A7WAgpMcREHZ3kdMCxy5q3U= X-Google-Smtp-Source: ABdhPJwXoVDCnn+xoV+lF2GluH/kqMkC7FjdIPgF+CMVfIVwuHiNp4VEnPMNvZ9n2uOZX5WJXh0pfA== X-Received: by 2002:a7b:ce1a:: with SMTP id m26mr6549678wmc.166.1592659963221; Sat, 20 Jun 2020 06:32:43 -0700 (PDT) Received: from r-arch-host11.mtr.labs.mlnx. ([37.142.13.130]) by smtp.gmail.com with ESMTPSA id c20sm2750287wrb.65.2020.06.20.06.32.41 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Sat, 20 Jun 2020 06:32:42 -0700 (PDT) From: Andrey Vesnovaty <andrey.vesnovaty@gmail.com> To: jerinjacobk@gmail.com, thomas@monjalon.net Cc: dev@dpdk.org, Andrey Vesnovaty <andrey.vesnovaty@gmail.com> Date: Sat, 20 Jun 2020 16:32:31 +0300 Message-Id: <20200620133231.12355-1-andrey.vesnovaty@gmail.com> X-Mailer: git-send-email 2.26.2 In-Reply-To: <CALBAE1M+0CN2k=+bzXN7n4S=XYwFa2am3rWn84onteV6e=3y2Q@mail.gmail.com> References: <CALBAE1M+0CN2k=+bzXN7n4S=XYwFa2am3rWn84onteV6e=3y2Q@mail.gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [dpdk-dev] [RFC v2 0/1] add flow action context API X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 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 |
add flow action context API
|
|
Message
Andrey Vesnovaty
June 20, 2020, 1:32 p.m. UTC
Hi, and thanks a lot for your RFC v1 comments. RFC v2 emphasize the intent for sharing the flow action: * The term 'action context' was unclear and replaced with 'shared action'. * RFC v2 subject became 'add flow shared action API'. * all proposed APIs renamed according the above. The new shared action is an independent entity decoupled from any flow while any flow can reuse such an action. Please go over the RFC description, it was almost entirely rewritten. @Jerin Jacob: Thanks again for your comments, it made me admit that v1 description was incomplete & unclear. I hope v2 will be better at least in terms of clarity. @Thomas Monjalon: rte_flow_action_ctx_modify() -> rte_flow_action_ctx_modify() Looking forward to your responses on v2, thanks in advance. Andrey Vesnovaty (1): add flow shared action API lib/librte_ethdev/rte_ethdev_version.map | 6 + lib/librte_ethdev/rte_flow.c | 81 +++++++++++++ lib/librte_ethdev/rte_flow.h | 143 ++++++++++++++++++++++- lib/librte_ethdev/rte_flow_driver.h | 22 ++++ 4 files changed, 251 insertions(+), 1 deletion(-)
Comments
20/06/2020 15:32, Andrey Vesnovaty: > Hi, and thanks a lot for your RFC v1 comments. > > RFC v2 emphasize the intent for sharing the flow action: > * The term 'action context' was unclear and replaced with > 'shared action'. > * RFC v2 subject became 'add flow shared action API'. > * all proposed APIs renamed according the above. > > The new shared action is an independent entity decoupled from any flow > while any flow can reuse such an action. Please go over the RFC > description, it was almost entirely rewritten. > > @Jerin Jacob: > Thanks again for your comments, it made me admit that v1 description was > incomplete & unclear. I hope v2 will be better at least in terms of > clarity. > @Thomas Monjalon: > rte_flow_action_ctx_modify() -> rte_flow_action_ctx_modify() I guess it is a typo. I see the name "rte_flow_shared_action_update" in the patch
On Mon, Jun 22, 2020 at 6:22 PM Thomas Monjalon <thomas@monjalon.net> wrote: > 20/06/2020 15:32, Andrey Vesnovaty: > > Hi, and thanks a lot for your RFC v1 comments. > > > > RFC v2 emphasize the intent for sharing the flow action: > > * The term 'action context' was unclear and replaced with > > 'shared action'. > > * RFC v2 subject became 'add flow shared action API'. > > * all proposed APIs renamed according the above. > > > > The new shared action is an independent entity decoupled from any flow > > while any flow can reuse such an action. Please go over the RFC > > description, it was almost entirely rewritten. > > > > @Jerin Jacob: > > Thanks again for your comments, it made me admit that v1 description was > > incomplete & unclear. I hope v2 will be better at least in terms of > > clarity. > > @Thomas Monjalon: > > rte_flow_action_ctx_modify() -> rte_flow_action_ctx_modify() > > I guess it is a typo. > I see the name "rte_flow_shared_action_update" in the patch > > Right, a typo. Should be: rte_flow_action_ctx_modify() -> rte_flow_shared_action_update ()
On Sat, Jun 20, 2020 at 7:02 PM Andrey Vesnovaty <andrey.vesnovaty@gmail.com> wrote: > > Hi, and thanks a lot for your RFC v1 comments. > > RFC v2 emphasize the intent for sharing the flow action: > * The term 'action context' was unclear and replaced with > 'shared action'. > * RFC v2 subject became 'add flow shared action API'. > * all proposed APIs renamed according the above. > > The new shared action is an independent entity decoupled from any flow > while any flow can reuse such an action. Please go over the RFC > description, it was almost entirely rewritten. > > @Jerin Jacob: > Thanks again for your comments, it made me admit that v1 description was > incomplete & unclear. I hope v2 will be better at least in terms of > clarity. The public API and its usage is very clear. Thanks for this RFC. I think, RFC v2 still not addressing the concern raised in the http://mails.dpdk.org/archives/dev/2020-June/169296.html. Since MLX hardware has an HW based shared object it is fine to have public API based on that level of abstraction. But at the PMD driver level we need to choose the correct abstraction to support all PMD and support shared object scheme if possible. I purpose to introduce something below or similar int (*action_update) (struct rte_eth_dev *, struct rte_flow *flow, const struct rte_flow_action [], struct rte_flow_error *); in addition to: shared_action_create, shared_action_destroy, shared_action_update, shared_action_query Have generic implementation of above, if action_update callback is not NULL. So that, it can work all PMDs and to avoid the duplication of "complex" shared session management code.
Hi On Fri, Jun 26, 2020 at 2:44 PM Jerin Jacob <jerinjacobk@gmail.com> wrote: > On Sat, Jun 20, 2020 at 7:02 PM Andrey Vesnovaty > <andrey.vesnovaty@gmail.com> wrote: > > > > Hi, and thanks a lot for your RFC v1 comments. > > > > RFC v2 emphasize the intent for sharing the flow action: > > * The term 'action context' was unclear and replaced with > > 'shared action'. > > * RFC v2 subject became 'add flow shared action API'. > > * all proposed APIs renamed according the above. > > > > The new shared action is an independent entity decoupled from any flow > > while any flow can reuse such an action. Please go over the RFC > > description, it was almost entirely rewritten. > > > > @Jerin Jacob: > > Thanks again for your comments, it made me admit that v1 description was > > incomplete & unclear. I hope v2 will be better at least in terms of > > clarity. > > The public API and its usage is very clear. Thanks for this RFC. My pleasure. > > I think, RFC v2 still not addressing the concern raised in the > http://mails.dpdk.org/archives/dev/2020-June/169296.html. > > Since MLX hardware has an HW based shared object it is fine to have > public API based on that level of abstraction. > But at the PMD driver level we need to choose the correct abstraction > to support all PMD and support shared object scheme if possible. > > I purpose to introduce something below or similar > int (*action_update) > (struct rte_eth_dev *, > struct rte_flow *flow, > const struct rte_flow_action [], > struct rte_flow_error *); > Where this callback suppose to belong (struct rte_flow_ops)? How should it be implemented by PMD? Is it about shared action and if "yes" why there is 'flow' argument? > > in addition to: shared_action_create, shared_action_destroy, > shared_action_update, shared_action_query > > Have generic implementation of above, if action_update callback is not > NULL. "is not NULL" -> "is NULL"? > So that, it can work all PMDs and to > avoid the duplication of "complex" shared session management code. > Do you mean shared action in use by multiple flows by "shared session"?
On Sun, Jun 28, 2020 at 2:14 PM Andrey Vesnovaty <andrey.vesnovaty@gmail.com> wrote: > > Hi > > On Fri, Jun 26, 2020 at 2:44 PM Jerin Jacob <jerinjacobk@gmail.com> wrote: >> >> On Sat, Jun 20, 2020 at 7:02 PM Andrey Vesnovaty >> <andrey.vesnovaty@gmail.com> wrote: >> > >> > Hi, and thanks a lot for your RFC v1 comments. >> > >> > RFC v2 emphasize the intent for sharing the flow action: >> > * The term 'action context' was unclear and replaced with >> > 'shared action'. >> > * RFC v2 subject became 'add flow shared action API'. >> > * all proposed APIs renamed according the above. >> > >> > The new shared action is an independent entity decoupled from any flow >> > while any flow can reuse such an action. Please go over the RFC >> > description, it was almost entirely rewritten. >> > >> > @Jerin Jacob: >> > Thanks again for your comments, it made me admit that v1 description was >> > incomplete & unclear. I hope v2 will be better at least in terms of >> > clarity. >> >> The public API and its usage is very clear. Thanks for this RFC. > > > My pleasure. >> >> >> I think, RFC v2 still not addressing the concern raised in the >> http://mails.dpdk.org/archives/dev/2020-June/169296.html. >> >> Since MLX hardware has an HW based shared object it is fine to have >> public API based on that level of abstraction. >> But at the PMD driver level we need to choose the correct abstraction >> to support all PMD and support shared object scheme if possible. >> >> I purpose to introduce something below or similar >> int (*action_update) >> (struct rte_eth_dev *, >> struct rte_flow *flow, >> const struct rte_flow_action [], >> struct rte_flow_error *); > > Where this callback suppose to belong (struct rte_flow_ops)? Yes. > How should it be implemented by PMD? See below, > Is it about shared action and if "yes" why there is 'flow' argument? flow holds the "pattern" and "action" data as PMD specific handle. So PMD, implementation can just change that action if it gets the PMD specific handle. >> >> >> in addition to: shared_action_create, shared_action_destroy, >> shared_action_update, shared_action_query >> >> Have generic implementation of above, if action_update callback is not >> NULL. > > "is not NULL" -> "is NULL"? Yes. When it is NULL. > >> >> So that, it can work all PMDs and to >> avoid the duplication of "complex" shared session management code. > > Do you mean shared action in use by multiple flows by "shared session"? Yes.
On Sun, Jun 28, 2020 at 4:42 PM Jerin Jacob <jerinjacobk@gmail.com> wrote: > On Sun, Jun 28, 2020 at 2:14 PM Andrey Vesnovaty > <andrey.vesnovaty@gmail.com> wrote: > > > > Hi > > > > On Fri, Jun 26, 2020 at 2:44 PM Jerin Jacob <jerinjacobk@gmail.com> > wrote: > >> > >> On Sat, Jun 20, 2020 at 7:02 PM Andrey Vesnovaty > >> <andrey.vesnovaty@gmail.com> wrote: > >> > > >> > Hi, and thanks a lot for your RFC v1 comments. > >> > > >> > RFC v2 emphasize the intent for sharing the flow action: > >> > * The term 'action context' was unclear and replaced with > >> > 'shared action'. > >> > * RFC v2 subject became 'add flow shared action API'. > >> > * all proposed APIs renamed according the above. > >> > > >> > The new shared action is an independent entity decoupled from any flow > >> > while any flow can reuse such an action. Please go over the RFC > >> > description, it was almost entirely rewritten. > >> > > >> > @Jerin Jacob: > >> > Thanks again for your comments, it made me admit that v1 description > was > >> > incomplete & unclear. I hope v2 will be better at least in terms of > >> > clarity. > >> > >> The public API and its usage is very clear. Thanks for this RFC. > > > > > > My pleasure. > >> > >> > >> I think, RFC v2 still not addressing the concern raised in the > >> http://mails.dpdk.org/archives/dev/2020-June/169296.html. > >> > >> Since MLX hardware has an HW based shared object it is fine to have > >> public API based on that level of abstraction. > >> But at the PMD driver level we need to choose the correct abstraction > >> to support all PMD and support shared object scheme if possible. > >> > >> I purpose to introduce something below or similar > >> int (*action_update) > >> (struct rte_eth_dev *, > >> struct rte_flow *flow, > >> const struct rte_flow_action [], > >> struct rte_flow_error *); > > > > Where this callback suppose to belong (struct rte_flow_ops)? > > Yes. > > > How should it be implemented by PMD? > > See below, > > > Is it about shared action and if "yes" why there is 'flow' argument? > > flow holds the "pattern" and "action" data as PMD specific handle. > So PMD, implementation can just change that action if it gets the PMD > specific handle. > > > >> > >> > >> in addition to: shared_action_create, shared_action_destroy, > >> shared_action_update, shared_action_query > >> > >> Have generic implementation of above, if action_update callback is not > >> NULL. > > > > "is not NULL" -> "is NULL"? > > Yes. When it is NULL. Jerin, few clarifications regarding generic implementation of shared action: Based on this conversation I'm assuming that generic implementation supposed to be something like: For each flow using some shared action: call ops-> action_update() If the assumption above correct: 1. taking into account that shared_action_update() is atomic, how can this deal with partial success: some flows may fail validation - should it: 1.1.lock all flows 1.2.validate all flows 1.3.update all flows 1.4. unlock 2. action_update callback is PMD specific & if it's unsupported there is no support for shared action any way Please address the issues above > > >> > >> So that, it can work all PMDs and to > >> avoid the duplication of "complex" shared session management code. > > > > Do you mean shared action in use by multiple flows by "shared session"? > > Yes. > Common 'shared session' management code: - can be reduced to atomic usage counter - maintaining list of flow using shared action expected to impact performance & not necessary for all PMD specific implementations Access to other shared resources hard to generalize because: - for some PMDs mutual exclusion is HW feature & no need to protect it in SW - for others there may be multiple resources & access to each one protected by different mechanism An observation related to action_update callback: If replaced (updated) action was shared then the flow won't be influenced any more by updates or removed shared action.
On Mon, Jun 29, 2020 at 3:52 PM Andrey Vesnovaty <andrey.vesnovaty@gmail.com> wrote: > > > > On Sun, Jun 28, 2020 at 4:42 PM Jerin Jacob <jerinjacobk@gmail.com> wrote: >> >> On Sun, Jun 28, 2020 at 2:14 PM Andrey Vesnovaty >> <andrey.vesnovaty@gmail.com> wrote: >> > >> > Hi >> > >> > On Fri, Jun 26, 2020 at 2:44 PM Jerin Jacob <jerinjacobk@gmail.com> wrote: >> >> >> >> On Sat, Jun 20, 2020 at 7:02 PM Andrey Vesnovaty >> >> <andrey.vesnovaty@gmail.com> wrote: >> >> > >> >> > Hi, and thanks a lot for your RFC v1 comments. >> >> > >> >> > RFC v2 emphasize the intent for sharing the flow action: >> >> > * The term 'action context' was unclear and replaced with >> >> > 'shared action'. >> >> > * RFC v2 subject became 'add flow shared action API'. >> >> > * all proposed APIs renamed according the above. >> >> > >> >> > The new shared action is an independent entity decoupled from any flow >> >> > while any flow can reuse such an action. Please go over the RFC >> >> > description, it was almost entirely rewritten. >> >> > >> >> > @Jerin Jacob: >> >> > Thanks again for your comments, it made me admit that v1 description was >> >> > incomplete & unclear. I hope v2 will be better at least in terms of >> >> > clarity. >> >> >> >> The public API and its usage is very clear. Thanks for this RFC. >> > >> > >> > My pleasure. >> >> >> >> >> >> I think, RFC v2 still not addressing the concern raised in the >> >> http://mails.dpdk.org/archives/dev/2020-June/169296.html. >> >> >> >> Since MLX hardware has an HW based shared object it is fine to have >> >> public API based on that level of abstraction. >> >> But at the PMD driver level we need to choose the correct abstraction >> >> to support all PMD and support shared object scheme if possible. >> >> >> >> I purpose to introduce something below or similar >> >> int (*action_update) >> >> (struct rte_eth_dev *, >> >> struct rte_flow *flow, >> >> const struct rte_flow_action [], >> >> struct rte_flow_error *); >> > >> > Where this callback suppose to belong (struct rte_flow_ops)? >> >> Yes. >> >> > How should it be implemented by PMD? >> >> See below, >> >> > Is it about shared action and if "yes" why there is 'flow' argument? >> >> flow holds the "pattern" and "action" data as PMD specific handle. >> So PMD, implementation can just change that action if it gets the PMD >> specific handle. >> >> >> >> >> >> >> >> in addition to: shared_action_create, shared_action_destroy, >> >> shared_action_update, shared_action_query >> >> >> >> Have generic implementation of above, if action_update callback is not >> >> NULL. >> > >> > "is not NULL" -> "is NULL"? >> >> Yes. When it is NULL. > > > Jerin, few clarifications regarding generic implementation of shared action: > Based on this conversation I'm assuming that generic implementation supposed to be something like: > For each flow using some shared action: > call ops-> action_update() > If the assumption above correct: > 1. taking into account that shared_action_update() is atomic, how can this deal with partial success: some flows may fail validation - should it: > 1.1.lock all flows > 1.2.validate all flows > 1.3.update all flows > 1.4. unlock Yes. > 2. action_update callback is PMD specific & if it's unsupported there is no support for shared action any way Yes. > Please address the issues above > >> > >> >> >> >> So that, it can work all PMDs and to >> >> avoid the duplication of "complex" shared session management code. >> > >> > Do you mean shared action in use by multiple flows by "shared session"? >> >> Yes. > > Common 'shared session' management code: > - can be reduced to atomic usage counter > - maintaining list of flow using shared action expected to impact performance & not necessary for all PMD specific implementations > Access to other shared resources hard to generalize because: > - for some PMDs mutual exclusion is HW feature & no need to protect it in SW > - for others there may be multiple resources & access to each one protected by different mechanism The general callback you can assume, it supports only action_update based callback. If PMD has mutual exclusion HW feature then it can override the function pointers. > An observation related to action_update callback: > If replaced (updated) action was shared then the flow won't be influenced any more by updates or removed shared action.
On Tue, Jun 30, 2020 at 12:52 PM Jerin Jacob <jerinjacobk@gmail.com> wrote: > On Mon, Jun 29, 2020 at 3:52 PM Andrey Vesnovaty > <andrey.vesnovaty@gmail.com> wrote: > > > > > > > > On Sun, Jun 28, 2020 at 4:42 PM Jerin Jacob <jerinjacobk@gmail.com> > wrote: > >> > >> On Sun, Jun 28, 2020 at 2:14 PM Andrey Vesnovaty > >> <andrey.vesnovaty@gmail.com> wrote: > >> > > >> > Hi > >> > > >> > On Fri, Jun 26, 2020 at 2:44 PM Jerin Jacob <jerinjacobk@gmail.com> > wrote: > >> >> > >> >> On Sat, Jun 20, 2020 at 7:02 PM Andrey Vesnovaty > >> >> <andrey.vesnovaty@gmail.com> wrote: > >> >> > > >> >> > Hi, and thanks a lot for your RFC v1 comments. > >> >> > > >> >> > RFC v2 emphasize the intent for sharing the flow action: > >> >> > * The term 'action context' was unclear and replaced with > >> >> > 'shared action'. > >> >> > * RFC v2 subject became 'add flow shared action API'. > >> >> > * all proposed APIs renamed according the above. > >> >> > > >> >> > The new shared action is an independent entity decoupled from any > flow > >> >> > while any flow can reuse such an action. Please go over the RFC > >> >> > description, it was almost entirely rewritten. > >> >> > > >> >> > @Jerin Jacob: > >> >> > Thanks again for your comments, it made me admit that v1 > description was > >> >> > incomplete & unclear. I hope v2 will be better at least in terms > of > >> >> > clarity. > >> >> > >> >> The public API and its usage is very clear. Thanks for this RFC. > >> > > >> > > >> > My pleasure. > >> >> > >> >> > >> >> I think, RFC v2 still not addressing the concern raised in the > >> >> http://mails.dpdk.org/archives/dev/2020-June/169296.html. > >> >> > >> >> Since MLX hardware has an HW based shared object it is fine to have > >> >> public API based on that level of abstraction. > >> >> But at the PMD driver level we need to choose the correct abstraction > >> >> to support all PMD and support shared object scheme if possible. > >> >> > >> >> I purpose to introduce something below or similar > >> >> int (*action_update) > >> >> (struct rte_eth_dev *, > >> >> struct rte_flow *flow, > >> >> const struct rte_flow_action [], > >> >> struct rte_flow_error *); > >> > > >> > Where this callback suppose to belong (struct rte_flow_ops)? > >> > >> Yes. > >> > >> > How should it be implemented by PMD? > >> > >> See below, > >> > >> > Is it about shared action and if "yes" why there is 'flow' argument? > >> > >> flow holds the "pattern" and "action" data as PMD specific handle. > >> So PMD, implementation can just change that action if it gets the PMD > >> specific handle. > >> > >> > >> >> > >> >> > >> >> in addition to: shared_action_create, shared_action_destroy, > >> >> shared_action_update, shared_action_query > >> >> > >> >> Have generic implementation of above, if action_update callback is > not > >> >> NULL. > >> > > >> > "is not NULL" -> "is NULL"? > >> > >> Yes. When it is NULL. > > > > > > Jerin, few clarifications regarding generic implementation of shared > action: > > Based on this conversation I'm assuming that generic implementation > supposed to be something like: > > For each flow using some shared action: > > call ops-> action_update() > > If the assumption above correct: > > 1. taking into account that shared_action_update() is atomic, how can > this deal with partial success: some flows may fail validation - should it: > > 1.1.lock all flows > > 1.2.validate all flows > > 1.3.update all flows > > 1.4. unlock > > Yes. > This kind of locking in addition to shared session management requires locking of each flow_create/flow_destroy in addition to action_uodate callback implementation even if there are no shared actions at all. In other words it imposes an overhead on all PMDs that don't support shared action natively. > > > 2. action_update callback is PMD specific & if it's unsupported there is > no support for shared action any way > > Yes. > > > Please address the issues above > > > > >> > > >> >> > >> >> So that, it can work all PMDs and to > >> >> avoid the duplication of "complex" shared session management code. > >> > > >> > Do you mean shared action in use by multiple flows by "shared > session"? > >> > >> Yes. > > > > Common 'shared session' management code: > > - can be reduced to atomic usage counter > > - maintaining list of flow using shared action expected to impact > performance & not necessary for all PMD specific implementations > > Access to other shared resources hard to generalize because: > > - for some PMDs mutual exclusion is HW feature & no need to protect it > in SW > > - for others there may be multiple resources & access to each one > protected by different mechanism > > The general callback you can assume, it supports only action_update > based callback. > If PMD has mutual exclusion HW feature then it can override the > function pointers. > > > > > An observation related to action_update callback: > > If replaced (updated) action was shared then the flow won't be > influenced any more by updates or removed shared action. >
On Wed, Jul 1, 2020 at 2:54 PM Andrey Vesnovaty <andrey.vesnovaty@gmail.com> wrote: > > > On Tue, Jun 30, 2020 at 12:52 PM Jerin Jacob <jerinjacobk@gmail.com> wrote: >> >> On Mon, Jun 29, 2020 at 3:52 PM Andrey Vesnovaty >> <andrey.vesnovaty@gmail.com> wrote: >> > >> > >> > >> > On Sun, Jun 28, 2020 at 4:42 PM Jerin Jacob <jerinjacobk@gmail.com> wrote: >> >> >> >> On Sun, Jun 28, 2020 at 2:14 PM Andrey Vesnovaty >> >> <andrey.vesnovaty@gmail.com> wrote: >> >> > >> >> > Hi >> >> > >> >> > On Fri, Jun 26, 2020 at 2:44 PM Jerin Jacob <jerinjacobk@gmail.com> wrote: >> >> >> >> >> >> On Sat, Jun 20, 2020 at 7:02 PM Andrey Vesnovaty >> >> >> <andrey.vesnovaty@gmail.com> wrote: >> >> >> > >> >> >> > Hi, and thanks a lot for your RFC v1 comments. >> >> >> > >> >> >> > RFC v2 emphasize the intent for sharing the flow action: >> >> >> > * The term 'action context' was unclear and replaced with >> >> >> > 'shared action'. >> >> >> > * RFC v2 subject became 'add flow shared action API'. >> >> >> > * all proposed APIs renamed according the above. >> >> >> > >> >> >> > The new shared action is an independent entity decoupled from any flow >> >> >> > while any flow can reuse such an action. Please go over the RFC >> >> >> > description, it was almost entirely rewritten. >> >> >> > >> >> >> > @Jerin Jacob: >> >> >> > Thanks again for your comments, it made me admit that v1 description was >> >> >> > incomplete & unclear. I hope v2 will be better at least in terms of >> >> >> > clarity. >> >> >> >> >> >> The public API and its usage is very clear. Thanks for this RFC. >> >> > >> >> > >> >> > My pleasure. >> >> >> >> >> >> >> >> >> I think, RFC v2 still not addressing the concern raised in the >> >> >> http://mails.dpdk.org/archives/dev/2020-June/169296.html. >> >> >> >> >> >> Since MLX hardware has an HW based shared object it is fine to have >> >> >> public API based on that level of abstraction. >> >> >> But at the PMD driver level we need to choose the correct abstraction >> >> >> to support all PMD and support shared object scheme if possible. >> >> >> >> >> >> I purpose to introduce something below or similar >> >> >> int (*action_update) >> >> >> (struct rte_eth_dev *, >> >> >> struct rte_flow *flow, >> >> >> const struct rte_flow_action [], >> >> >> struct rte_flow_error *); >> >> > >> >> > Where this callback suppose to belong (struct rte_flow_ops)? >> >> >> >> Yes. >> >> >> >> > How should it be implemented by PMD? >> >> >> >> See below, >> >> >> >> > Is it about shared action and if "yes" why there is 'flow' argument? >> >> >> >> flow holds the "pattern" and "action" data as PMD specific handle. >> >> So PMD, implementation can just change that action if it gets the PMD >> >> specific handle. >> >> >> >> >> >> >> >> >> >> >> >> >> in addition to: shared_action_create, shared_action_destroy, >> >> >> shared_action_update, shared_action_query >> >> >> >> >> >> Have generic implementation of above, if action_update callback is not >> >> >> NULL. >> >> > >> >> > "is not NULL" -> "is NULL"? >> >> >> >> Yes. When it is NULL. >> > >> > >> > Jerin, few clarifications regarding generic implementation of shared action: >> > Based on this conversation I'm assuming that generic implementation supposed to be something like: >> > For each flow using some shared action: >> > call ops-> action_update() >> > If the assumption above correct: >> > 1. taking into account that shared_action_update() is atomic, how can this deal with partial success: some flows may fail validation - should it: >> > 1.1.lock all flows >> > 1.2.validate all flows >> > 1.3.update all flows >> > 1.4. unlock >> >> Yes. > > This kind of locking in addition to shared session management requires locking of each flow_create/flow_destroy in addition to action_uodate callback implementation even if there are no shared actions at all. In other words it imposes an overhead on all PMDs that don't support shared action natively. Yes. That's what my concern with implementing shared session if the PMD only supports only action update for the given rte_flow *. Another approach would be to introduce rte_flow_action_update() public API which can either take "const struct rte_flow_action []" OR shared context ID, to cater to both cases or something on similar lines. >> >> >> > 2. action_update callback is PMD specific & if it's unsupported there is no support for shared action any way >> >> Yes. >> >> > Please address the issues above >> >> > >> >> > >> >> >> >> >> >> So that, it can work all PMDs and to >> >> >> avoid the duplication of "complex" shared session management code. >> >> > >> >> > Do you mean shared action in use by multiple flows by "shared session"? >> >> >> >> Yes. >> > >> > Common 'shared session' management code: >> > - can be reduced to atomic usage counter >> > - maintaining list of flow using shared action expected to impact performance & not necessary for all PMD specific implementations >> > Access to other shared resources hard to generalize because: >> > - for some PMDs mutual exclusion is HW feature & no need to protect it in SW >> > - for others there may be multiple resources & access to each one protected by different mechanism >> >> The general callback you can assume, it supports only action_update >> based callback. >> If PMD has mutual exclusion HW feature then it can override the >> function pointers. >> >> >> >> > An observation related to action_update callback: >> > If replaced (updated) action was shared then the flow won't be influenced any more by updates or removed shared action.