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 64CE2A00C5; Thu, 4 Jun 2020 13:12:35 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 8FA341D5D1; Thu, 4 Jun 2020 13:12:34 +0200 (CEST) Received: from mail-lj1-f181.google.com (mail-lj1-f181.google.com [209.85.208.181]) by dpdk.org (Postfix) with ESMTP id 71C3A2862 for ; Thu, 4 Jun 2020 13:12:33 +0200 (CEST) Received: by mail-lj1-f181.google.com with SMTP id z18so6698419lji.12 for ; Thu, 04 Jun 2020 04:12:33 -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; bh=x/lMHzij0sFB7NewC3FPlK01fObfFfu5IL1sZjMKIgk=; b=VJjXbeRXyA36ZCNG1dPCMQJ9OQ6SQ7wQEuB93i0OLmyZmds6WyGiYsrWFXZywrOZfP g8agjOh75m+jXCXpc78Wwh3hR6IUGjd365/m2cnQFACBmQyqDUzkdtY3hp/cJPJ6g8JX NFA4FdKJA32sMKu3aP8K7QDpLe6IBP3mrcJNpWYgh+g6at34fKa2vKKw1OBAJOylQF0d ckGZzpTf3fIE+LNPy2VX0siD7s7ebIyyp6cVzjFM1g9+5SaZkM8iU6VoX9EPsaQzBZb8 o+UAlZzLPupHSxrPzlfZWy7AJM4YtKsEcNU6QWV7oLNoqxp2VzjXCL7y9amV7qdFFDP7 53vA== 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; bh=x/lMHzij0sFB7NewC3FPlK01fObfFfu5IL1sZjMKIgk=; b=QDzWFGvxU4xm/sIzlHhUZ9v6s7PN+ria33okToU9Ik5tkqjR9qeYfDOlflgR+8WWpa rxCL9jae97ijhMY2EIPr1mhmPAmSLDavVpFcUWiuDrhkxaccg+g8aqmqoRmDEGwUwtWU WRvGj91pr6T43D28k/M67q+1MfqqSJXMR9WWP49/zgxQqDNhgr6zusXDmLiJHIHrS78c 50mN2U8aiEaBdkXAoQBPL3+01341lVZkQ3zY3cgVDG5uz3Man9ZAtyq0aqJ5cLOyDR0a J8h8Hlgi8bCKcf/NgRBu3KKo1WW6XGAqyrUG8drovr4ml7VQl8jNGnB6J8d4NjwzFRuM iFOA== X-Gm-Message-State: AOAM531FHYlGbNI2C2EN4O8dz9ZyB6yFkRVTIJFnCfK98zGHb23hKKOg F4uzR/P/b2kkVnsE2RyalOS/c08VswNtR0LzBOw= X-Google-Smtp-Source: ABdhPJxpSNSm1Mwo0pza76vPBqF8vgKhRNf5N7xovRrnD0I+PLEZHwsUnmbjDrqnI9TKrEO9GnpylBZk1ZxqnAKnnqc= X-Received: by 2002:a2e:910c:: with SMTP id m12mr2003570ljg.332.1591269152964; Thu, 04 Jun 2020 04:12:32 -0700 (PDT) MIME-Version: 1.0 References: <20200520091801.30163-1-andrey.vesnovaty@gmail.com> <318236911.yWikKUQR1j@thomas> In-Reply-To: <318236911.yWikKUQR1j@thomas> From: Andrey Vesnovaty Date: Thu, 4 Jun 2020 14:12:21 +0300 Message-ID: To: Thomas Monjalon Cc: Ferruh Yigit , Andrew Rybchenko , Ori Kam , dev@dpdk.org Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [RFC] add flow action context 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 Wed, Jun 3, 2020 at 1:02 PM Thomas Monjalon wrote: > 20/05/2020 11:18, Andrey Vesnovaty: > > This commit introduces extension of DPDK flow action API enabling > > modification of single rte_flow_action. > > > > Motivation and example > > === > > Adding or removing one or more queues to RSS actions cloned in multiple > > flow rules imposes per rule toll for current DPDK flow API; the scenario > > requires for each flow sharing cloned RSS action: > > - call `rte_flow_destroy()` > > - call `rte_flow_create()` with modified RSS action > > > > In order to prevent the overhead of multiple RSS flow rules > reconfiguration > > API for in-place flow action modification introduced in this commit. > > It seems there is an usability improvement with this new API. > If I understand well, the main motivation is to improve the performance? > The PMD implementation should try to keep a shared object > to benefit of the performance improvement, right? > > Right, the goal is performance improvement. Single API call modifies behaviour of multiple flows. > > The existing rte_flow API functions are: > rte_flow_validate() > rte_flow_create() > rte_flow_destroy() > rte_flow_flush() > rte_flow_query() > rte_flow_isolate() > rte_flow_get_aged_flows() > > > + # added in 20.08 > > + rte_flow_action_ctx_create; > > + rte_flow_action_ctx_destoy; > > + rte_flow_action_ctx_modify; > > + rte_flow_action_ctx_query; > > We had "create", "destroy", "query", but no "modify" capability. > The new API is adding 2 things in my opinion: > - shared action object > - "modify" capability (is "update" a better wording?) > Naming is one of the most challenging parts of this RFC. Some similarity I have found in existing code is rte_mtr_policer_actions_update() Is there any existing code having update/modify semantics? > > About the wording, do we need "ctx"? > I feel rte_flow_action is a good enough prefix for this API, > and should be documented as a shared action object. > I think the word "object" is more meaningful than "context". > Am I missing something? > > CTX comes for the fact that each flow_rule doesn't have an ownership for the given action but operates inside some context (shared action context actually). As mentioned above, naming is one of the most challenging parts of this RFC. > > > + /** > > + * Describes action context. > > + * > > + * Enables multiple rules reference the same action by id/ctx. > > + * > > + * No action specific struct here (void*) since it can be any > > + * action type. > > + */ > > + RTE_FLOW_ACTION_TYPE_CTX, > > Why do we need a new action type? > > Because it's not an action itself but a reference/handle to it. > > > @@ -101,6 +101,28 @@ struct rte_flow_ops { > > + /** See rte_flow_action_ctx_destoy() */ > > + void *(*action_ctx_create) > > + (struct rte_eth_dev *dev, > > + const struct rte_flow_action *action, > > + struct rte_flow_error *error); > > + /** See rte_flow_action_ctx_create() */ > > + int (*action_ctx_destroy) > > + (struct rte_eth_dev *dev, > > + void *ctx, > > + struct rte_flow_error *error); > > + /** See rte_flow_action_ctx_modify() */ > > + int (*action_ctx_modify) > > + (struct rte_eth_dev *dev, > > + void *ctx, > > + const void *action_conf, > > + struct rte_flow_error *error); > > + /** See rte_flow_action_ctx_query() */ > > + int (*action_ctx_query) > > + (struct rte_eth_dev *dev, > > + const void *ctx, > > + void *data, > > + struct rte_flow_error *error); > > API functions are directly linked to PMD ops, it looks simple and good. > > >