From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <adrien.mazarguil@6wind.com>
Received: from mail-wm0-f51.google.com (mail-wm0-f51.google.com [74.125.82.51])
 by dpdk.org (Postfix) with ESMTP id F41591CCA9
 for <dev@dpdk.org>; Fri,  6 Apr 2018 22:33:28 +0200 (CEST)
Received: by mail-wm0-f51.google.com with SMTP id p9so5747262wmc.3
 for <dev@dpdk.org>; Fri, 06 Apr 2018 13:33:28 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=6wind-com.20150623.gappssmtp.com; s=20150623;
 h=date:from:to:cc:subject:message-id:references:mime-version
 :content-disposition:in-reply-to;
 bh=WGwAf4STC0J1Ja/OV7hsORxdQVbAD6FaA1mATWQg+Bs=;
 b=PXeiSvjsJQsZtPIhunENJ40reAT40veAXglvdX6UifGRatqNj5yMe7xyy4mvMqLFMF
 cvIdeX4YuHBNLstuhFuYVxcgF4+ZC3KEPLUy/z0Dqfiv8FRqz/mzSTEmSWH+iSBO5fNH
 +56OFpmWvwx8eq8JVSpTnHkaLBeuKpmPN76P4Z0MxteiHtxZI27H4J8R9DNl3mE7DkXS
 kg+d9TTA0SA9VDSBXvOghEXp2iU37yZJbJYiBpBrsiYggG/xNJpDL/830CK6SrrkdwMb
 bLLpMhzB2MnldcWzwU9p1SfiasPdMt6natCF9biLeg6TmLDm87j3e4BBRO2+Xag+dLRr
 ikrw==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=1e100.net; s=20161025;
 h=x-gm-message-state:date:from:to:cc:subject:message-id:references
 :mime-version:content-disposition:in-reply-to;
 bh=WGwAf4STC0J1Ja/OV7hsORxdQVbAD6FaA1mATWQg+Bs=;
 b=mT4JOhxPC41YwB2URE5Mc/hx4ZGE/bg8AWNLsc3vDjHrAeEPb8ucQ+eU6jHz73Wsyo
 ylYLNMYkHkJL60S3KFglrwO9yJ44klzNLdJILglX5T3GC76nQhInqIKcQ9SJhXMW7auE
 4kSg3RehLbCLFRAjJbRkIcI69u9BoJbBj7xcQQnxAn7kusmZgyWxeWAJ1krTO6esPQMq
 0pcbqAINnUNDtGEOaBc8cdbBYWTQkExLRhEdoDwfucJcJHzQDNTbMDPuP5IuoST849EK
 GlTxCA0kVCMmmeKZlRzsN+KtxT4VSg9emfiM9haxTPgXFYdt0s4oAnkolb1ZqmbMPsjB
 K0tQ==
X-Gm-Message-State: ALQs6tBjCSwHlihf//151vtZZbTXWIkdsYKld/xArxpO8VBF0E/TJdj/
 VTTSC2r/xZjUV+e5ovtKUKHCtA==
X-Google-Smtp-Source: AIpwx49nQbLxd7t+/o9d0Mqq380wVOn6hCLb+1S7zU3yOEG6yYwtJYJzbtwMF7+83IS43AwdY0GkcA==
X-Received: by 10.28.88.141 with SMTP id m135mr12988580wmb.156.1523046425275; 
 Fri, 06 Apr 2018 13:27:05 -0700 (PDT)
Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78])
 by smtp.gmail.com with ESMTPSA id 59sm12287544wro.35.2018.04.06.13.27.04
 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);
 Fri, 06 Apr 2018 13:27:04 -0700 (PDT)
Date: Fri, 6 Apr 2018 22:26:51 +0200
From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
To: Declan Doherty <declan.doherty@intel.com>
Cc: dev@dpdk.org
Message-ID: <20180406202651.GT4957@6wind.com>
References: <1523017443-12414-1-git-send-email-declan.doherty@intel.com>
 <1523017443-12414-4-git-send-email-declan.doherty@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1523017443-12414-4-git-send-email-declan.doherty@intel.com>
Subject: Re: [dpdk-dev] [PATCH v3 3/4] ethdev: Add group action type to
 rte_flow
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://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Fri, 06 Apr 2018 20:33:29 -0000

On Fri, Apr 06, 2018 at 01:24:02PM +0100, Declan Doherty wrote:
> Add group action type which defines a terminating action which
> allows a matched flow to be redirect to a group. This allows logical
> flow table hierarchies to be managed through rte_flow.
> 
> Signed-off-by: Declan Doherty <declan.doherty@intel.com>

OK, I'm wondering if perhaps with the addition of this action, we should
redefine groups as unlinked by default?

Currently traffic enters through the flow rule with the lowest priority of
the group with the lowest ID and iterates through subsequent flow rules and
groups until matched by a flow rule without PASSTHRU (according to latest
definition [1]).

This would make jumps between groups always explicit, not necessarily a bad
idea given no PMD implements groups as of yet. Thoughts?

Also as a rather fundamental API addition, I suggest to add it after
RTE_FLOW_ACTION_TYPE_PASSTHRU. It's OK because ABI is already broken. You
just need to mention it in the commit log [1].

Another suggestion would be to rename it "JUMP" (reasons below).

[1] "ethdev: alter behavior of flow API actions"
    http://dpdk.org/ml/archives/dev/2018-April/095779.html

> ---
>  doc/guides/prog_guide/rte_flow.rst | 23 +++++++++++++++++++++++
>  lib/librte_ether/rte_flow.h        | 15 +++++++++++++++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index 106fb93..2f0a47a 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -1557,6 +1557,29 @@ set of overlay header type.
>     | ``item type`` | Item type of tunnel end-point to decapsulate |
>     +---------------+----------------------------------------------+
>  
> +

Unnecessary empty line.

> +Action: ``GROUP``
> +^^^^^^^^^^^^^^^^^
> +
> +Redirects packets to a group on the current device.
> +
> +In a hierarchy of groups, which can be used to represent physical or logical
> +flow tables on the device, this action allows the terminating action to be a
> +group on that device.
> +
> +- Terminating by default.

Keep in mind there's no such thing as a terminating action anymore [1].

> +
> +.. _table_rte_flow_action_group:
> +
> +.. table:: GROUP
> +
> +   +--------------+---------------------------------+
> +   | Field        | Value                           |
> +   +==============+=================================+
> +   | ``id``       | Group ID to redirect packets to |
> +   +--------------+---------------------------------+

"Field" column can be shrunk somewhat.

> +
> +
>  Negative types
>  ~~~~~~~~~~~~~~
>  
> diff --git a/lib/librte_ether/rte_flow.h b/lib/librte_ether/rte_flow.h
> index 6d94423..968a23b 100644
> --- a/lib/librte_ether/rte_flow.h
> +++ b/lib/librte_ether/rte_flow.h
> @@ -1251,6 +1251,21 @@ struct rte_flow_action_tunnel_decap {
>  };
>  
>  /**
> + * RTE_FLOW_ACTION_TYPE_GROUP

Its addition to enum rte_flow_action_type should be part of this commit.

> + *
> + * Redirects packets to a group on the current device.
> + *
> + * In a hierarchy of groups, which can be used to represent physical or logical
> + * flow tables on the device, this action allows the terminating action to be a
> + * group on that device.
> + *
> + * Terminating by default.

See [1].

> + */
> +struct rte_flow_action_group {
> +	uint32_t id;

Assuming this structure is named rte_flow_action_jump, naming this field
"group" would match the attribute of the same name.

> +};
> +
> +/**
>   * Definition of a single action.
>   *
>   * A list of actions is terminated by a END action.
> -- 
> 2.7.4
> 

Don't forget testpmd code and documentation update.

-- 
Adrien Mazarguil
6WIND