From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id ABABFA00BE; Tue, 7 Jul 2020 21:39:02 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id F35331DE51; Tue, 7 Jul 2020 21:39:01 +0200 (CEST) Received: from mail-io1-f65.google.com (mail-io1-f65.google.com [209.85.166.65]) by dpdk.org (Postfix) with ESMTP id 3F0B11BFB4 for ; Tue, 7 Jul 2020 21:39:01 +0200 (CEST) Received: by mail-io1-f65.google.com with SMTP id l1so2519083ioh.5 for ; Tue, 07 Jul 2020 12:39:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=Z+2nP0W2gnHCu3itQzNZsjgDHu7ayvSqXo/+uD9VxhU=; b=VppLSIF9l0gpYH0RGPxjUa0nO6pra9AcJgsMHNqMgsZTIQV9A63tuDEqWf+4bl4JQY PYy2aFPq7QKnytWSLK0nMl1xsWb4L740XacfQ6NVKcJm6tgOvxdpKIL/DXeH/lAUpvwX dIUgKUHaaXDuAZLw1IzIM7KHloW0gs/xtnTr14jg3XCkUhJKprMxM0T4mNSO2wKMedYa Q+Tgz3rpUEI7u6csEsK7zWYJAWYWxHQpxVYhzA3IzcwfAcuRT7BHvLtjsSgXrmy4A1H3 KunBk05DAfYLK1dqbmZBgxxZ14lXT9cXDSKwIbTflxhOFLHrdVYVmd9TVBny0jLbtn9B 4TJQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=Z+2nP0W2gnHCu3itQzNZsjgDHu7ayvSqXo/+uD9VxhU=; b=dNcEuFINevoNF6afK07d7EUX5ae5Poq/XdZXk2zhmJWW17rDqae0Afo9FyBCh5IUin x82w9S+qX6grcNUxL0QnnBr7RHkr71AIcG2VkEYlyx6bJKxBgWmbwrBL2hfrI9ginr1y zKoY0LLPanzk4u7wisGlubkxLgi4g6iO/ZE6c2+XUXI12+mTMUg1ew+vzq8hyi2q3Olx IY6f2MychGdeukITfC13bUeYDIFbi2DPOUlsDiQR3hD4MX9ULbRDjCIamNvFMhmf6cLD 12Fz8BRxYGX+mMudvCuZlE8JG+bomLi/+zKegWTCH8TC6W8kusq8MNMiXo3THWDnG1J3 bPhQ== X-Gm-Message-State: AOAM533azmUJU0ViwD+gQqwumrzaUeaEX7TCtJLhAK0nfBvij/45Agon d5DZMKWrTCjHLGhzPHxPPab+6E3pCIgocXNfFU0= X-Google-Smtp-Source: ABdhPJytVkDJPWNdrv0MTEJpo9NpS1bqOWuZT+c0EBAR20Xl/TSfAg0lOSH09bw46p62HOrI3WzycO7Up5wY3D+ngUE= X-Received: by 2002:a5e:880f:: with SMTP id l15mr31148272ioj.94.1594150740495; Tue, 07 Jul 2020 12:39:00 -0700 (PDT) MIME-Version: 1.0 References: <20200702120511.16315-1-andreyv@mellanox.com> In-Reply-To: From: Jerin Jacob Date: Wed, 8 Jul 2020 01:08:44 +0530 Message-ID: To: Ori Kam Cc: Andrey Vesnovaty , Andrey Vesnovaty , Thomas Monjalon , Ferruh Yigit , Andrew Rybchenko , dpdk-dev Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Subject: Re: [dpdk-dev] [PATCH] add flow shared action API X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Tue, Jul 7, 2020 at 11:51 AM Ori Kam wrote: > > Hi Jerin, > Thanks you for your quick reply. > > > -----Original Message----- > > From: Jerin Jacob > > Subject: Re: [dpdk-dev] [PATCH] add flow shared action API > > > > On Mon, Jul 6, 2020 at 7:02 PM Andrey Vesnovaty > > wrote: > > > > > > Hi, Jerin. > > > > Hi Ori and Andrey, > > > > > > > > > > Please see below Ori's suggestion below to implement your > > rte_flow_action_update() idea > > > with some API changes of rte_flow_shared_action_xxx API changes. > > > > > > On Mon, Jul 6, 2020 at 3:28 PM Ori Kam wrote: > > >> > > >> Hi Jerin, > > >> > > >> > -----Original Message----- > > >> > From: Jerin Jacob > > >> > Sent: Monday, July 6, 2020 12:00 PM > > >> > Subject: Re: [dpdk-dev] [PATCH] add flow shared action API > > >> > > > >> > On Sun, Jul 5, 2020 at 3:56 PM Ori Kam wrote: > > >> > > > > >> > > Hi Jerin, > > >> > > PSB, > > >> > > > > >> > > Thanks, > > >> > > Ori > > >> > > > > >> > > > -----Original Message----- > > >> > > > From: Jerin Jacob > > >> > > > Sent: Saturday, July 4, 2020 3:33 PM > > >> > > > dpdk-dev > > >> > > > Subject: Re: [dpdk-dev] [PATCH] add flow shared action API > > >> > > > > > >> > > > On Sat, Jul 4, 2020 at 3:40 PM Andrey Vesnovaty > > >> > > > wrote: > > >> > > > > > > >> > > > > > > >> > > > > Thanks, > > >> > > > > > > >> > > > > Andrey Vesnovaty > > >> > > > > (+972)526775512 | Skype: andrey775512 > > >> > > > > > > >> > > > > >> > > > > >> > > [..Nip ..] > > >> > > > > >> > > > > I need to mention the locking issue once again. > > >> > > > > If there is a need to maintain "shared session" in the gener= ic > > rte_flow > > >> > layer > > >> > > > all > > >> > > > > calls to flow_create() with shared action & all delete need = to take > > >> > > > sharedsession > > >> > > > > management locks at least for verification. Lock partitionin= g is also > > bit > > >> > > > problematic > > >> > > > > since one flow may have more than one shared action. > > >> > > > > > >> > > > Then, I think better 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. This would allow HW'= s > > >> > > > without have the shared context ID > > >> > > > to use the action update. > > >> > > > > >> > > Can you please explain your idea? > > >> > > > >> > I see two types of HW schemes supporting action updates without go= ing > > >> > through call `rte_flow_destroy()` and call `rte_flow_create()` > > >> > - The shared HW action context feature > > >> > - The HW has "pattern" and "action" mapped to different HW objects= and > > >> > action can be updated any time. > > >> > Other than above-mentioned RSS use case, another use case would be= to > > >> > a) create rte_flow and set the action as DROP (Kind of reserving t= he HW > > object) > > >> > b) Update the action only when the rest of the requirements ready. > > >> > > > >> > Any API schematic that supports both notions of HW is fine with me= . > > >> > > > >> I have an idea if the API will be changed to something like this, > > >> Rte_flow_shared_action_update(uint16_port port, rte_shared_ctx *ctx, > > rte_flow_action *action, error) > > >> This will enable the application to send a different action than the= original > > one to be switched. > > >> Assuming the PMD supports this. > > >> Does it answer your concerns? > > > > > > > > > This allows both: > > > 1. Update action configuration > > > 2. Replace action by some other action > > > For 2 pure software implementation may carate shred action (that can = be > > shared > > > with one flow only, depends on PMD) and later on > > rte_flow_shared_action_update may replace this > > > action with some other action by handle returned from > > rte_flow_shared_action_create > > > Doesign between 1 and 2 is per PMD. > > > > struct rte_flow * object holds the driver representation of the > > pattern + action. > > So in order to update the action, we would need struct rte_flow * in AP= I. > > > Why is that? The idea is to change the action, the action itself is conne= cted to flows. > The PMD can save in the shared_ctx all flows that are connected to this a= ction. > > > I think, simple API change would be to accommodate "rte_shared_ctx > > *ctx, rte_flow_action *action" modes > > without introducing the emulation for one or other mode, will be. > > > > enum rte_flow_action_update_type { > > RTE_FLOW_ACTION_UPDATE_TYPE_SHARED_ACTION, > > RTE_FLOW_ACTION_UPDATE_TYPE_ACTION, > > }; > > > > struct rte_flow_action_update_type_param { > > enum rte_flow_action_update_type type; > > union { > > struct rte_flow_action_update_type_shared_action_p= aram { > > rte_shared_ctx *ctx; > > } shared_action; > > struct rte_flow_action_update_type_shared_action_= param { > > rte_flow *flow, > > rte_flow_action *action; > > } action; > > } > > } > > > Thank you for the idea but I fall to see how your suggested API is simple= r than the one suggested by me? My thought process with the below-proposed API[1] is that It is dictates "_shared_action_" in API name as well as arguments. I just thought of expressing it as either-or case hence I thought [2] is better. i.e The PMD does not support shared_action, not even need to create one to use rte_flow_action_update() to avoid the confusion. Thoughts? [1] rte_flow_shared_action_update(uint16_port port, rte_shared_ctx *ctx, rte_flow_action *action, error) [2] rte_flow_action_update(uint16_port port, struct rte_flow_action_update_type_param *param, error) > In my suggestion the PMD simply needs to check if the new action and chan= ge the > context and to that action, or just change parameters in the action, if i= t is the same action. > > Let's go with the original patch API modified to support like you request= ed also changing the action, > based on my comments. > > > rte_flow_action_update(uint16_port port, struct > > rte_flow_action_update_type_param *param, error) > > > > > > > >> > > >> > > > >> > > As I can see if we use the flow_action array it may result in bu= gs. > > >> > > For example, the application created two flows with the same RSS= (not > > using > > >> > the context) > > >> > > Then he wants to change one flow to use different RSS, but the r= esult will > > that > > >> > both flows > > >> > > will be changed. > > >> > > > >> > Sorry. I don't quite follow this. > > >> > > > >> I was trying to show that there must be some context. But I don=E2= =80=99t think this is > > relevant to > > >> your current ideas. > > >> > > >> > > Also this will enforce the PMD to keep track on all flows which = will have > > >> > memory penalty for > > >> > > some PMDs. > > >> > > >> Best, > > >> Ori > > > > > > > > > Thanks, > > > Andrey > Best, > Ori