[v2,1/7] ethdev: introduce sample action for rte flow
Checks
Commit Message
When using full offload, all traffic will be handled by the HW, and
directed to the requested vf or wire, the control application loses
visibility on the traffic.
So there's a need for an action that will enable the control application
some visibility.
The solution is introduced a new action that will sample the incoming
traffic and send a duplicated traffic in some predefined ratio to the
application, while the original packet will continue to the target
destination.
The packets sampled equals is '1/ratio', if the ratio value be set to 1
, means that the packets would be completely mirrored. The sample packet
can be assigned with different set of actions from the original packet.
In order to support the sample packet in rte_flow, new rte_flow action
definition RTE_FLOW_ACTION_TYPE_SAMPLE and structure rte_flow_action_sample
will be introduced.
Signed-off-by: Jiawei Wang <jiaweiw@mellanox.com>
Acked-by: Ori Kam <orika@mellanox.com>
---
doc/guides/prog_guide/rte_flow.rst | 25 +++++++++++++++++++++++++
doc/guides/rel_notes/release_20_08.rst | 6 ++++++
lib/librte_ethdev/rte_flow.c | 1 +
lib/librte_ethdev/rte_flow.h | 28 ++++++++++++++++++++++++++++
4 files changed, 60 insertions(+)
Comments
On Fri, Jul 3, 2020 at 12:13 AM Jiawei Wang <jiaweiw@mellanox.com> wrote:
>
> When using full offload, all traffic will be handled by the HW, and
> directed to the requested vf or wire, the control application loses
> visibility on the traffic.
> So there's a need for an action that will enable the control application
> some visibility.
>
> The solution is introduced a new action that will sample the incoming
> traffic and send a duplicated traffic in some predefined ratio to the
> application, while the original packet will continue to the target
> destination.
>
> The packets sampled equals is '1/ratio', if the ratio value be set to 1
> , means that the packets would be completely mirrored. The sample packet
> can be assigned with different set of actions from the original packet.
>
> In order to support the sample packet in rte_flow, new rte_flow action
> definition RTE_FLOW_ACTION_TYPE_SAMPLE and structure rte_flow_action_sample
> will be introduced.
>
> Signed-off-by: Jiawei Wang <jiaweiw@mellanox.com>
> Acked-by: Ori Kam <orika@mellanox.com>
When adding overlapping API(rte_eth_mirror_rule_set()) in the same
library(ethdev).
Please depreciate the old API.
We should not have two separate paths for the same function in the
same ethdev library. It is pain for app and driver developers.
With the above deprecation notice,
Acked-by: Jerin Jacob <jerinj@marvell.com>
> ---
> doc/guides/prog_guide/rte_flow.rst | 25 +++++++++++++++++++++++++
> doc/guides/rel_notes/release_20_08.rst | 6 ++++++
> lib/librte_ethdev/rte_flow.c | 1 +
> lib/librte_ethdev/rte_flow.h | 28 ++++++++++++++++++++++++++++
> 4 files changed, 60 insertions(+)
>
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index d5dd18c..50dfe1f 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -2645,6 +2645,31 @@ timeout passed without any matching on the flow.
> | ``context`` | user input flow context |
> +--------------+---------------------------------+
>
> +Action: ``SAMPLE``
> +^^^^^^^^^^^^^^^^^^
> +
> +Adds a sample action to a matched flow.
> +
> +The matching packets will be duplicated to a special queue or vport
> +with the predefined ``ratio``, the packets sampled equals is '1/ratio'.
> +All the packets continues to the target destination.
> +
> +When the ``ratio`` is set to 1 then the packets will be 100% mirrored.
> +``actions`` represent the different set of actions for the sampled or mirrored
> +packets.
> +
> +.. _table_rte_flow_action_sample:
> +
> +.. table:: SAMPLE
> +
> + +--------------+---------------------------------+
> + | Field | Value |
> + +==============+=================================+
> + | ``ratio`` | 32 bits sample ratio value |
> + +--------------+---------------------------------+
> + | ``actions`` | sub-action list for sampling |
> + +--------------+---------------------------------+
> +
> Negative types
> ~~~~~~~~~~~~~~
>
> diff --git a/doc/guides/rel_notes/release_20_08.rst b/doc/guides/rel_notes/release_20_08.rst
> index 5cbc4ce..313e8d3 100644
> --- a/doc/guides/rel_notes/release_20_08.rst
> +++ b/doc/guides/rel_notes/release_20_08.rst
> @@ -81,6 +81,12 @@ New Features
> * Added support for virtio queue statistics.
> * Added support for MTU update.
>
> +* **Added flow-based traffic sampling support.**
> +
> + Added new action: ``RTE_FLOW_ACTION_TYPE_SAMPLE`` to duplicate the matching
> + packets with given ratio and redirects to vport or queue. The sampled packets
> + also can be assigned with an additional optional actions.
> +
> * **Updated Marvell octeontx2 ethdev PMD.**
>
> Updated Marvell octeontx2 driver with cn98xx support.
> diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> index 1685be5..733871d 100644
> --- a/lib/librte_ethdev/rte_flow.c
> +++ b/lib/librte_ethdev/rte_flow.c
> @@ -173,6 +173,7 @@ struct rte_flow_desc_data {
> MK_FLOW_ACTION(SET_IPV4_DSCP, sizeof(struct rte_flow_action_set_dscp)),
> MK_FLOW_ACTION(SET_IPV6_DSCP, sizeof(struct rte_flow_action_set_dscp)),
> MK_FLOW_ACTION(AGE, sizeof(struct rte_flow_action_age)),
> + MK_FLOW_ACTION(SAMPLE, sizeof(struct rte_flow_action_sample)),
> };
>
> int
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index b0e4199..c9cd80d 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -2099,6 +2099,13 @@ enum rte_flow_action_type {
> * see enum RTE_ETH_EVENT_FLOW_AGED
> */
> RTE_FLOW_ACTION_TYPE_AGE,
> +
> + /**
> + * Redirects specific ratio of packets to vport or queue.
> + *
> + * See struct rte_flow_action_sample.
> + */
> + RTE_FLOW_ACTION_TYPE_SAMPLE,
> };
>
> /**
> @@ -2709,6 +2716,27 @@ struct rte_flow_action {
> struct rte_flow;
>
> /**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice
> + *
> + * RTE_FLOW_ACTION_TYPE_SAMPLE
> + *
> + * Adds a sample action to a matched flow.
> + *
> + * The matching packets will be duplicated to a special queue or vport
> + * in the predefined probabiilty, All the packets continues processing
> + * on the default flow path.
> + *
> + * When the sample ratio is set to 1 then the packets will be 100% mirrored.
> + * Additional action list be supported to add for sampled or mirrored packets.
> + */
> +struct rte_flow_action_sample {
> + const uint32_t ratio; /**< packets sampled equals to '1/ratio'. */
> + const struct rte_flow_action *actions;
> + /**< sub-action list specific for the sampling hit cases. */
> +};
> +
> +/**
> * Verbose error types.
> *
> * Most of them provide the type of the object referenced by struct
> --
> 1.8.3.1
>
Hi Jerin
From: Jerin Jacob:
> On Fri, Jul 3, 2020 at 12:13 AM Jiawei Wang <jiaweiw@mellanox.com> wrote:
> >
> > When using full offload, all traffic will be handled by the HW, and
> > directed to the requested vf or wire, the control application loses
> > visibility on the traffic.
> > So there's a need for an action that will enable the control
> > application some visibility.
> >
> > The solution is introduced a new action that will sample the incoming
> > traffic and send a duplicated traffic in some predefined ratio to the
> > application, while the original packet will continue to the target
> > destination.
> >
> > The packets sampled equals is '1/ratio', if the ratio value be set to
> > 1 , means that the packets would be completely mirrored. The sample
> > packet can be assigned with different set of actions from the original
> packet.
> >
> > In order to support the sample packet in rte_flow, new rte_flow action
> > definition RTE_FLOW_ACTION_TYPE_SAMPLE and structure
> > rte_flow_action_sample will be introduced.
> >
> > Signed-off-by: Jiawei Wang <jiaweiw@mellanox.com>
> > Acked-by: Ori Kam <orika@mellanox.com>
>
> When adding overlapping API(rte_eth_mirror_rule_set()) in the same
> library(ethdev).
> Please depreciate the old API.
> We should not have two separate paths for the same function in the same
> ethdev library. It is pain for app and driver developers.
What are about all the other rte_flow parallel configuration APIs in ethdev:
promiscuous_enable;
promiscuous_disable;
allmulticast_enable;
allmulticast_disable;
mac_addr_remove;
mac_addr_add;
mac_addr_set;
set_mc_addr_list;
vlan_filter_set;
vlan_tpid_set;
vlan_strip_queue_set;
vlan_offload_set;
vlan_pvid_set;
udp_tunnel_port_add;
udp_tunnel_port_del;
...
These APIs can be replaced easily by rte_flow API.
Do you think we need to deprecate all?
> With the above deprecation notice,
> Acked-by: Jerin Jacob <jerinj@marvell.com>
>
>
> > ---
> > doc/guides/prog_guide/rte_flow.rst | 25
> +++++++++++++++++++++++++
> > doc/guides/rel_notes/release_20_08.rst | 6 ++++++
> > lib/librte_ethdev/rte_flow.c | 1 +
> > lib/librte_ethdev/rte_flow.h | 28
> ++++++++++++++++++++++++++++
> > 4 files changed, 60 insertions(+)
> >
> > diff --git a/doc/guides/prog_guide/rte_flow.rst
> > b/doc/guides/prog_guide/rte_flow.rst
> > index d5dd18c..50dfe1f 100644
> > --- a/doc/guides/prog_guide/rte_flow.rst
> > +++ b/doc/guides/prog_guide/rte_flow.rst
> > @@ -2645,6 +2645,31 @@ timeout passed without any matching on the
> flow.
> > | ``context`` | user input flow context |
> > +--------------+---------------------------------+
> >
> > +Action: ``SAMPLE``
> > +^^^^^^^^^^^^^^^^^^
> > +
> > +Adds a sample action to a matched flow.
> > +
> > +The matching packets will be duplicated to a special queue or vport
> > +with the predefined ``ratio``, the packets sampled equals is '1/ratio'.
> > +All the packets continues to the target destination.
> > +
> > +When the ``ratio`` is set to 1 then the packets will be 100% mirrored.
> > +``actions`` represent the different set of actions for the sampled or
> > +mirrored packets.
> > +
> > +.. _table_rte_flow_action_sample:
> > +
> > +.. table:: SAMPLE
> > +
> > + +--------------+---------------------------------+
> > + | Field | Value |
> > + +==============+=================================+
> > + | ``ratio`` | 32 bits sample ratio value |
> > + +--------------+---------------------------------+
> > + | ``actions`` | sub-action list for sampling |
> > + +--------------+---------------------------------+
> > +
> > Negative types
> > ~~~~~~~~~~~~~~
> >
> > diff --git a/doc/guides/rel_notes/release_20_08.rst
> > b/doc/guides/rel_notes/release_20_08.rst
> > index 5cbc4ce..313e8d3 100644
> > --- a/doc/guides/rel_notes/release_20_08.rst
> > +++ b/doc/guides/rel_notes/release_20_08.rst
> > @@ -81,6 +81,12 @@ New Features
> > * Added support for virtio queue statistics.
> > * Added support for MTU update.
> >
> > +* **Added flow-based traffic sampling support.**
> > +
> > + Added new action: ``RTE_FLOW_ACTION_TYPE_SAMPLE`` to duplicate
> the
> > + matching packets with given ratio and redirects to vport or queue.
> > + The sampled packets also can be assigned with an additional optional
> actions.
> > +
> > * **Updated Marvell octeontx2 ethdev PMD.**
> >
> > Updated Marvell octeontx2 driver with cn98xx support.
> > diff --git a/lib/librte_ethdev/rte_flow.c
> > b/lib/librte_ethdev/rte_flow.c index 1685be5..733871d 100644
> > --- a/lib/librte_ethdev/rte_flow.c
> > +++ b/lib/librte_ethdev/rte_flow.c
> > @@ -173,6 +173,7 @@ struct rte_flow_desc_data {
> > MK_FLOW_ACTION(SET_IPV4_DSCP, sizeof(struct
> rte_flow_action_set_dscp)),
> > MK_FLOW_ACTION(SET_IPV6_DSCP, sizeof(struct
> rte_flow_action_set_dscp)),
> > MK_FLOW_ACTION(AGE, sizeof(struct rte_flow_action_age)),
> > + MK_FLOW_ACTION(SAMPLE, sizeof(struct rte_flow_action_sample)),
> > };
> >
> > int
> > diff --git a/lib/librte_ethdev/rte_flow.h
> > b/lib/librte_ethdev/rte_flow.h index b0e4199..c9cd80d 100644
> > --- a/lib/librte_ethdev/rte_flow.h
> > +++ b/lib/librte_ethdev/rte_flow.h
> > @@ -2099,6 +2099,13 @@ enum rte_flow_action_type {
> > * see enum RTE_ETH_EVENT_FLOW_AGED
> > */
> > RTE_FLOW_ACTION_TYPE_AGE,
> > +
> > + /**
> > + * Redirects specific ratio of packets to vport or queue.
> > + *
> > + * See struct rte_flow_action_sample.
> > + */
> > + RTE_FLOW_ACTION_TYPE_SAMPLE,
> > };
> >
> > /**
> > @@ -2709,6 +2716,27 @@ struct rte_flow_action { struct rte_flow;
> >
> > /**
> > + * @warning
> > + * @b EXPERIMENTAL: this structure may change without prior notice
> > + *
> > + * RTE_FLOW_ACTION_TYPE_SAMPLE
> > + *
> > + * Adds a sample action to a matched flow.
> > + *
> > + * The matching packets will be duplicated to a special queue or
> > +vport
> > + * in the predefined probabiilty, All the packets continues
> > +processing
> > + * on the default flow path.
> > + *
> > + * When the sample ratio is set to 1 then the packets will be 100%
> mirrored.
> > + * Additional action list be supported to add for sampled or mirrored
> packets.
> > + */
> > +struct rte_flow_action_sample {
> > + const uint32_t ratio; /**< packets sampled equals to '1/ratio'. */
> > + const struct rte_flow_action *actions;
> > + /**< sub-action list specific for the sampling hit
> > +cases. */ };
> > +
> > +/**
> > * Verbose error types.
> > *
> > * Most of them provide the type of the object referenced by struct
> > --
> > 1.8.3.1
> >
On Fri, Jul 3, 2020 at 8:25 PM Matan Azrad <matan@mellanox.com> wrote:
>
>
> Hi Jerin
Hi Matan,
>
> From: Jerin Jacob:
> > On Fri, Jul 3, 2020 at 12:13 AM Jiawei Wang <jiaweiw@mellanox.com> wrote:
> > >
> > > When using full offload, all traffic will be handled by the HW, and
> > > directed to the requested vf or wire, the control application loses
> > > visibility on the traffic.
> > > So there's a need for an action that will enable the control
> > > application some visibility.
> > >
> > > The solution is introduced a new action that will sample the incoming
> > > traffic and send a duplicated traffic in some predefined ratio to the
> > > application, while the original packet will continue to the target
> > > destination.
> > >
> > > The packets sampled equals is '1/ratio', if the ratio value be set to
> > > 1 , means that the packets would be completely mirrored. The sample
> > > packet can be assigned with different set of actions from the original
> > packet.
> > >
> > > In order to support the sample packet in rte_flow, new rte_flow action
> > > definition RTE_FLOW_ACTION_TYPE_SAMPLE and structure
> > > rte_flow_action_sample will be introduced.
> > >
> > > Signed-off-by: Jiawei Wang <jiaweiw@mellanox.com>
> > > Acked-by: Ori Kam <orika@mellanox.com>
> >
> > When adding overlapping API(rte_eth_mirror_rule_set()) in the same
> > library(ethdev).
> > Please depreciate the old API.
> > We should not have two separate paths for the same function in the same
> > ethdev library. It is pain for app and driver developers.
>
> What are about all the other rte_flow parallel configuration APIs in ethdev:
> promiscuous_enable;
> promiscuous_disable;
> allmulticast_enable;
> allmulticast_disable;
> mac_addr_remove;
> mac_addr_add;
> mac_addr_set;
> set_mc_addr_list;
> vlan_filter_set;
> vlan_tpid_set;
> vlan_strip_queue_set;
> vlan_offload_set;
> vlan_pvid_set;
> udp_tunnel_port_add;
> udp_tunnel_port_del;
> ...
>
> These APIs can be replaced easily by rte_flow API.
> Do you think we need to deprecate all?
I think, basic stuff like below can have separate API.
1) promiscuous_enable;
2) promiscuous_disable;
3) allmulticast_enable;
4) allmulticast_disable;
5) mac_addr_remove;
6) mac_addr_add;
7) mac_addr_set;
8) set_mc_addr_list;
But The VLAN and UDP related should be rte_flow candidates.(IMO)
>
> > With the above deprecation notice,
> > Acked-by: Jerin Jacob <jerinj@marvell.com>
From: Jerin Jacob
> On Fri, Jul 3, 2020 at 8:25 PM Matan Azrad <matan@mellanox.com> wrote:
> >
> >
> > Hi Jerin
>
> Hi Matan,
>
> >
> > From: Jerin Jacob:
> > > On Fri, Jul 3, 2020 at 12:13 AM Jiawei Wang <jiaweiw@mellanox.com>
> wrote:
> > > >
> > > > When using full offload, all traffic will be handled by the HW,
> > > > and directed to the requested vf or wire, the control application
> > > > loses visibility on the traffic.
> > > > So there's a need for an action that will enable the control
> > > > application some visibility.
> > > >
> > > > The solution is introduced a new action that will sample the
> > > > incoming traffic and send a duplicated traffic in some predefined
> > > > ratio to the application, while the original packet will continue
> > > > to the target destination.
> > > >
> > > > The packets sampled equals is '1/ratio', if the ratio value be set
> > > > to
> > > > 1 , means that the packets would be completely mirrored. The
> > > > sample packet can be assigned with different set of actions from
> > > > the original
> > > packet.
> > > >
> > > > In order to support the sample packet in rte_flow, new rte_flow
> > > > action definition RTE_FLOW_ACTION_TYPE_SAMPLE and structure
> > > > rte_flow_action_sample will be introduced.
> > > >
> > > > Signed-off-by: Jiawei Wang <jiaweiw@mellanox.com>
> > > > Acked-by: Ori Kam <orika@mellanox.com>
> > >
> > > When adding overlapping API(rte_eth_mirror_rule_set()) in the same
> > > library(ethdev).
> > > Please depreciate the old API.
> > > We should not have two separate paths for the same function in the
> > > same ethdev library. It is pain for app and driver developers.
> >
> > What are about all the other rte_flow parallel configuration APIs in ethdev:
> > promiscuous_enable;
> > promiscuous_disable;
> > allmulticast_enable;
> > allmulticast_disable;
> > mac_addr_remove;
> > mac_addr_add;
> > mac_addr_set;
> > set_mc_addr_list;
> > vlan_filter_set;
> > vlan_tpid_set;
> > vlan_strip_queue_set;
> > vlan_offload_set;
> > vlan_pvid_set;
> > udp_tunnel_port_add;
> > udp_tunnel_port_del;
> > ...
> >
> > These APIs can be replaced easily by rte_flow API.
> > Do you think we need to deprecate all?
>
> I think, basic stuff like below can have separate API.
> 1) promiscuous_enable;
> 2) promiscuous_disable;
> 3) allmulticast_enable;
> 4) allmulticast_disable;
> 5) mac_addr_remove;
> 6) mac_addr_add;
> 7) mac_addr_set;
> 8) set_mc_addr_list;
>
> But The VLAN and UDP related should be rte_flow candidates.(IMO)
Can you explain why?
Each one of them can be configured by rte_flow, so why do we need duplicate ways to configure the same thing.
What is the expected behavior if there are conflicts? Is it documented?
It is probably different discussion, but I think that rte_flow becomes more popular and it will be makes sense to concentrate all the traffic filtering/actions to the rte_flow.
> >
> > > With the above deprecation notice,
> > > Acked-by: Jerin Jacob <jerinj@marvell.com>
03/07/2020 17:08, Jerin Jacob:
> On Fri, Jul 3, 2020 at 8:25 PM Matan Azrad <matan@mellanox.com> wrote:
> > From: Jerin Jacob:
> > > When adding overlapping API(rte_eth_mirror_rule_set()) in the same
> > > library(ethdev).
> > > Please depreciate the old API.
> > > We should not have two separate paths for the same function in the same
> > > ethdev library. It is pain for app and driver developers.
> >
> > What are about all the other rte_flow parallel configuration APIs in ethdev:
> > promiscuous_enable;
> > promiscuous_disable;
> > allmulticast_enable;
> > allmulticast_disable;
> > mac_addr_remove;
> > mac_addr_add;
> > mac_addr_set;
> > set_mc_addr_list;
> > vlan_filter_set;
> > vlan_tpid_set;
> > vlan_strip_queue_set;
> > vlan_offload_set;
> > vlan_pvid_set;
> > udp_tunnel_port_add;
> > udp_tunnel_port_del;
> > ...
> >
> > These APIs can be replaced easily by rte_flow API.
> > Do you think we need to deprecate all?
>
> I think, basic stuff like below can have separate API.
> 1) promiscuous_enable;
> 2) promiscuous_disable;
> 3) allmulticast_enable;
> 4) allmulticast_disable;
> 5) mac_addr_remove;
> 6) mac_addr_add;
> 7) mac_addr_set;
> 8) set_mc_addr_list;
"Basic" is not a precise definition :)
I would say port-level configuration should remain
out of rte_flow API.
> But The VLAN and UDP related should be rte_flow candidates.(IMO)
Yes definitely, tunneling is better managed with rte_flow.
This is an important discussion, I Cc other ethdev maintainers.
Note: this is an ethdev patch, all ethdev maintainers should be Cc'ed.
Aren't you using --cc-cmd devtools/get-maintainer.sh ?
On Fri, Jul 3, 2020 at 8:57 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 03/07/2020 17:08, Jerin Jacob:
> > On Fri, Jul 3, 2020 at 8:25 PM Matan Azrad <matan@mellanox.com> wrote:
> > > From: Jerin Jacob:
> > > > When adding overlapping API(rte_eth_mirror_rule_set()) in the same
> > > > library(ethdev).
> > > > Please depreciate the old API.
> > > > We should not have two separate paths for the same function in the same
> > > > ethdev library. It is pain for app and driver developers.
> > >
> > > What are about all the other rte_flow parallel configuration APIs in ethdev:
> > > promiscuous_enable;
> > > promiscuous_disable;
> > > allmulticast_enable;
> > > allmulticast_disable;
> > > mac_addr_remove;
> > > mac_addr_add;
> > > mac_addr_set;
> > > set_mc_addr_list;
> > > vlan_filter_set;
> > > vlan_tpid_set;
> > > vlan_strip_queue_set;
> > > vlan_offload_set;
> > > vlan_pvid_set;
> > > udp_tunnel_port_add;
> > > udp_tunnel_port_del;
> > > ...
> > >
> > > These APIs can be replaced easily by rte_flow API.
> > > Do you think we need to deprecate all?
> >
> > I think, basic stuff like below can have separate API.
> > 1) promiscuous_enable;
> > 2) promiscuous_disable;
> > 3) allmulticast_enable;
> > 4) allmulticast_disable;
> > 5) mac_addr_remove;
> > 6) mac_addr_add;
> > 7) mac_addr_set;
> > 8) set_mc_addr_list;
>
> "Basic" is not a precise definition :)
Yep.
> I would say port-level configuration should remain
> out of rte_flow API.
+1.
In addition that, I would say anything needs to configured at
"per-flow" granularity use rte_flow.
>
> > But The VLAN and UDP related should be rte_flow candidates.(IMO)
>
> Yes definitely, tunneling is better managed with rte_flow.
>
> This is an important discussion, I Cc other ethdev maintainers.
> Note: this is an ethdev patch, all ethdev maintainers should be Cc'ed.
> Aren't you using --cc-cmd devtools/get-maintainer.sh ?
>
>
On 7/2/20 9:43 PM, Jiawei Wang wrote:
> When using full offload, all traffic will be handled by the HW, and
> directed to the requested vf or wire, the control application loses
vf->VF
> visibility on the traffic.
> So there's a need for an action that will enable the control application
> some visibility.
>
> The solution is introduced a new action that will sample the incoming
> traffic and send a duplicated traffic in some predefined ratio to the
> application, while the original packet will continue to the target
> destination.
>
May be 1 packet per second is a better sampling approach?
Or just different.
> The packets sampled equals is '1/ratio', if the ratio value be set to 1
> , means that the packets would be completely mirrored. The sample packet
Comma on the next line looks bad.
> can be assigned with different set of actions from the original packet.
>
> In order to support the sample packet in rte_flow, new rte_flow action
> definition RTE_FLOW_ACTION_TYPE_SAMPLE and structure rte_flow_action_sample
> will be introduced.
>
> Signed-off-by: Jiawei Wang <jiaweiw@mellanox.com>
> Acked-by: Ori Kam <orika@mellanox.com>
> ---
> doc/guides/prog_guide/rte_flow.rst | 25 +++++++++++++++++++++++++
> doc/guides/rel_notes/release_20_08.rst | 6 ++++++
> lib/librte_ethdev/rte_flow.c | 1 +
> lib/librte_ethdev/rte_flow.h | 28 ++++++++++++++++++++++++++++
> 4 files changed, 60 insertions(+)
>
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index d5dd18c..50dfe1f 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -2645,6 +2645,31 @@ timeout passed without any matching on the flow.
> | ``context`` | user input flow context |
> +--------------+---------------------------------+
>
> +Action: ``SAMPLE``
> +^^^^^^^^^^^^^^^^^^
> +
> +Adds a sample action to a matched flow.
> +
> +The matching packets will be duplicated to a special queue or vport
what is vport above?
> +with the predefined ``ratio``, the packets sampled equals is '1/ratio'.
> +All the packets continues to the target destination.
continues -> continue (if I'm not mistaken)
> +
> +When the ``ratio`` is set to 1 then the packets will be 100% mirrored.
> +``actions`` represent the different set of actions for the sampled or mirrored
> +packets.
> +
> +.. _table_rte_flow_action_sample:
> +
> +.. table:: SAMPLE
> +
> + +--------------+---------------------------------+
> + | Field | Value |
> + +==============+=================================+
> + | ``ratio`` | 32 bits sample ratio value |
> + +--------------+---------------------------------+
> + | ``actions`` | sub-action list for sampling |
> + +--------------+---------------------------------+
> +
> Negative types
> ~~~~~~~~~~~~~~
>
> diff --git a/doc/guides/rel_notes/release_20_08.rst b/doc/guides/rel_notes/release_20_08.rst
> index 5cbc4ce..313e8d3 100644
> --- a/doc/guides/rel_notes/release_20_08.rst
> +++ b/doc/guides/rel_notes/release_20_08.rst
> @@ -81,6 +81,12 @@ New Features
> * Added support for virtio queue statistics.
> * Added support for MTU update.
>
> +* **Added flow-based traffic sampling support.**
> +
> + Added new action: ``RTE_FLOW_ACTION_TYPE_SAMPLE`` to duplicate the matching
> + packets with given ratio and redirects to vport or queue. The sampled packets
What is vport above?
> + also can be assigned with an additional optional actions.
May actions list be empty or NULL? If no, it does not look
optional.
> +
> * **Updated Marvell octeontx2 ethdev PMD.**
>
> Updated Marvell octeontx2 driver with cn98xx support.
> diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> index 1685be5..733871d 100644
> --- a/lib/librte_ethdev/rte_flow.c
> +++ b/lib/librte_ethdev/rte_flow.c
> @@ -173,6 +173,7 @@ struct rte_flow_desc_data {
> MK_FLOW_ACTION(SET_IPV4_DSCP, sizeof(struct rte_flow_action_set_dscp)),
> MK_FLOW_ACTION(SET_IPV6_DSCP, sizeof(struct rte_flow_action_set_dscp)),
> MK_FLOW_ACTION(AGE, sizeof(struct rte_flow_action_age)),
> + MK_FLOW_ACTION(SAMPLE, sizeof(struct rte_flow_action_sample)),
> };
>
> int
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index b0e4199..c9cd80d 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -2099,6 +2099,13 @@ enum rte_flow_action_type {
> * see enum RTE_ETH_EVENT_FLOW_AGED
> */
> RTE_FLOW_ACTION_TYPE_AGE,
> +
> + /**
> + * Redirects specific ratio of packets to vport or queue.
> + *
> + * See struct rte_flow_action_sample.
> + */
> + RTE_FLOW_ACTION_TYPE_SAMPLE,
> };
>
> /**
> @@ -2709,6 +2716,27 @@ struct rte_flow_action {
> struct rte_flow;
>
> /**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice
> + *
> + * RTE_FLOW_ACTION_TYPE_SAMPLE
> + *
> + * Adds a sample action to a matched flow.
> + *
> + * The matching packets will be duplicated to a special queue or vport
again 'vport' here
It sounds misleading and too restrictive to say "be duplicated
to a special queue or vport". There is no specification of the
queue or vport in the control structure.
You should either describe it in a generic way like "be
duplicated and own set of actions with a fate action applied"
or put a restriction about QUEUE, RSS or "vport"-related action
to be present in the sub-actions list.
> + * in the predefined probabiilty, All the packets continues processing
probabiilty -> probability
I think 'predefined' is misleading here, 'specified' is better.
Also strictly speaking it is not a predefined probability (as
Stephen suggested), it is defined ratio.
> + * on the default flow path.
> + *
> + * When the sample ratio is set to 1 then the packets will be 100% mirrored.
> + * Additional action list be supported to add for sampled or mirrored packets.
> + */
> +struct rte_flow_action_sample {
> + const uint32_t ratio; /**< packets sampled equals to '1/ratio'. */
const is still above and it is meaningless (other actions do
not have 'const' for plain fields).
> + const struct rte_flow_action *actions;
> + /**< sub-action list specific for the sampling hit cases. */
Is it required to have fate action?
May I use it to MARK some packets and do not duplicate?
I guess no. Or COUNT and DROP? Just COUNT?
What I'm trying to say that you're adding a generic packet
selection mechanism with very restricted usage by design.
Anyway, if you go with it, please, process other notes above.
> +};
> +
> +/**
> * Verbose error types.
> *
> * Most of them provide the type of the object referenced by struct
>
On Fri, Jul 3, 2020 at 8:27 AM Thomas Monjalon <thomas@monjalon.net> wrote:
> 03/07/2020 17:08, Jerin Jacob:
> > On Fri, Jul 3, 2020 at 8:25 PM Matan Azrad <matan@mellanox.com> wrote:
> > > From: Jerin Jacob:
> > > > When adding overlapping API(rte_eth_mirror_rule_set()) in the same
> > > > library(ethdev).
> > > > Please depreciate the old API.
> > > > We should not have two separate paths for the same function in the
> same
> > > > ethdev library. It is pain for app and driver developers.
> > >
> > > What are about all the other rte_flow parallel configuration APIs in
> ethdev:
> > > promiscuous_enable;
> > > promiscuous_disable;
> > > allmulticast_enable;
> > > allmulticast_disable;
> > > mac_addr_remove;
> > > mac_addr_add;
> > > mac_addr_set;
> > > set_mc_addr_list;
> > > vlan_filter_set;
> > > vlan_tpid_set;
> > > vlan_strip_queue_set;
> > > vlan_offload_set;
> > > vlan_pvid_set;
> > > udp_tunnel_port_add;
> > > udp_tunnel_port_del;
> > > ...
> > >
> > > These APIs can be replaced easily by rte_flow API.
> > > Do you think we need to deprecate all?
> >
> > I think, basic stuff like below can have separate API.
> > 1) promiscuous_enable;
> > 2) promiscuous_disable;
> > 3) allmulticast_enable;
> > 4) allmulticast_disable;
> > 5) mac_addr_remove;
> > 6) mac_addr_add;
> > 7) mac_addr_set;
> > 8) set_mc_addr_list;
>
> "Basic" is not a precise definition :)
> I would say port-level configuration should remain
> out of rte_flow API.
>
+1
>
> > But The VLAN and UDP related should be rte_flow candidates.(IMO)
>
I do not have a strong opinion on VLAN in port-level or rte_flow list.
But isn't the UDP port number for tunnels a port-level setting for HW?
>
> Yes definitely, tunneling is better managed with rte_flow.
>
> This is an important discussion, I Cc other ethdev maintainers.
> Note: this is an ethdev patch, all ethdev maintainers should be Cc'ed.
> Aren't you using --cc-cmd devtools/get-maintainer.sh ?
>
>
>
On Thu, Jul 2, 2020 at 11:40 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> On Fri, Jul 3, 2020 at 12:13 AM Jiawei Wang <jiaweiw@mellanox.com> wrote:
> >
> > When using full offload, all traffic will be handled by the HW, and
> > directed to the requested vf or wire, the control application loses
> > visibility on the traffic.
> > So there's a need for an action that will enable the control application
> > some visibility.
> >
> > The solution is introduced a new action that will sample the incoming
> > traffic and send a duplicated traffic in some predefined ratio to the
> > application, while the original packet will continue to the target
> > destination.
> >
> > The packets sampled equals is '1/ratio', if the ratio value be set to 1
> > , means that the packets would be completely mirrored. The sample packet
> > can be assigned with different set of actions from the original packet.
> >
> > In order to support the sample packet in rte_flow, new rte_flow action
> > definition RTE_FLOW_ACTION_TYPE_SAMPLE and structure
> rte_flow_action_sample
> > will be introduced.
> >
> > Signed-off-by: Jiawei Wang <jiaweiw@mellanox.com>
> > Acked-by: Ori Kam <orika@mellanox.com>
>
> When adding overlapping API(rte_eth_mirror_rule_set()) in the same
> library(ethdev).
> Please depreciate the old API.
> We should not have two separate paths for the same function in the
> same ethdev library. It is pain for app and driver developers.
>
> With the above deprecation notice,
> Acked-by: Jerin Jacob <jerinj@marvell.com>
>
I am fine with the proposed RTE_FLOW_ACTION_TYPE_SAMPLE. But..
When rte_eth_mirror_rule_set() is deprecated, are we going to add
RTE_FLOW_ACTION_TYPE_MIRROR for full fledged mirror action?
Or we are proposing to use RTE_FLOW_ACTION_TYPE_SAMPLE with
ratio of 1 to mirror all packets, thereby doing away with the need for
a separate RTE_FLOW_ACTION_TYPE_MIRROR?
>
>
> > ---
> > doc/guides/prog_guide/rte_flow.rst | 25 +++++++++++++++++++++++++
> > doc/guides/rel_notes/release_20_08.rst | 6 ++++++
> > lib/librte_ethdev/rte_flow.c | 1 +
> > lib/librte_ethdev/rte_flow.h | 28 ++++++++++++++++++++++++++++
> > 4 files changed, 60 insertions(+)
> >
> > diff --git a/doc/guides/prog_guide/rte_flow.rst
> b/doc/guides/prog_guide/rte_flow.rst
> > index d5dd18c..50dfe1f 100644
> > --- a/doc/guides/prog_guide/rte_flow.rst
> > +++ b/doc/guides/prog_guide/rte_flow.rst
> > @@ -2645,6 +2645,31 @@ timeout passed without any matching on the flow.
> > | ``context`` | user input flow context |
> > +--------------+---------------------------------+
> >
> > +Action: ``SAMPLE``
> > +^^^^^^^^^^^^^^^^^^
> > +
> > +Adds a sample action to a matched flow.
> > +
> > +The matching packets will be duplicated to a special queue or vport
> > +with the predefined ``ratio``, the packets sampled equals is '1/ratio'.
> > +All the packets continues to the target destination.
> > +
> > +When the ``ratio`` is set to 1 then the packets will be 100% mirrored.
> > +``actions`` represent the different set of actions for the sampled or
> mirrored
> > +packets.
> > +
> > +.. _table_rte_flow_action_sample:
> > +
> > +.. table:: SAMPLE
> > +
> > + +--------------+---------------------------------+
> > + | Field | Value |
> > + +==============+=================================+
> > + | ``ratio`` | 32 bits sample ratio value |
> > + +--------------+---------------------------------+
> > + | ``actions`` | sub-action list for sampling |
> > + +--------------+---------------------------------+
> > +
> > Negative types
> > ~~~~~~~~~~~~~~
> >
> > diff --git a/doc/guides/rel_notes/release_20_08.rst
> b/doc/guides/rel_notes/release_20_08.rst
> > index 5cbc4ce..313e8d3 100644
> > --- a/doc/guides/rel_notes/release_20_08.rst
> > +++ b/doc/guides/rel_notes/release_20_08.rst
> > @@ -81,6 +81,12 @@ New Features
> > * Added support for virtio queue statistics.
> > * Added support for MTU update.
> >
> > +* **Added flow-based traffic sampling support.**
> > +
> > + Added new action: ``RTE_FLOW_ACTION_TYPE_SAMPLE`` to duplicate the
> matching
> > + packets with given ratio and redirects to vport or queue. The sampled
> packets
> > + also can be assigned with an additional optional actions.
> > +
> > * **Updated Marvell octeontx2 ethdev PMD.**
> >
> > Updated Marvell octeontx2 driver with cn98xx support.
> > diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> > index 1685be5..733871d 100644
> > --- a/lib/librte_ethdev/rte_flow.c
> > +++ b/lib/librte_ethdev/rte_flow.c
> > @@ -173,6 +173,7 @@ struct rte_flow_desc_data {
> > MK_FLOW_ACTION(SET_IPV4_DSCP, sizeof(struct
> rte_flow_action_set_dscp)),
> > MK_FLOW_ACTION(SET_IPV6_DSCP, sizeof(struct
> rte_flow_action_set_dscp)),
> > MK_FLOW_ACTION(AGE, sizeof(struct rte_flow_action_age)),
> > + MK_FLOW_ACTION(SAMPLE, sizeof(struct rte_flow_action_sample)),
> > };
> >
> > int
> > diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> > index b0e4199..c9cd80d 100644
> > --- a/lib/librte_ethdev/rte_flow.h
> > +++ b/lib/librte_ethdev/rte_flow.h
> > @@ -2099,6 +2099,13 @@ enum rte_flow_action_type {
> > * see enum RTE_ETH_EVENT_FLOW_AGED
> > */
> > RTE_FLOW_ACTION_TYPE_AGE,
> > +
> > + /**
> > + * Redirects specific ratio of packets to vport or queue.
> > + *
> > + * See struct rte_flow_action_sample.
> > + */
> > + RTE_FLOW_ACTION_TYPE_SAMPLE,
> > };
> >
> > /**
> > @@ -2709,6 +2716,27 @@ struct rte_flow_action {
> > struct rte_flow;
> >
> > /**
> > + * @warning
> > + * @b EXPERIMENTAL: this structure may change without prior notice
> > + *
> > + * RTE_FLOW_ACTION_TYPE_SAMPLE
> > + *
> > + * Adds a sample action to a matched flow.
> > + *
> > + * The matching packets will be duplicated to a special queue or vport
> > + * in the predefined probabiilty, All the packets continues processing
> > + * on the default flow path.
> > + *
> > + * When the sample ratio is set to 1 then the packets will be 100%
> mirrored.
> > + * Additional action list be supported to add for sampled or mirrored
> packets.
> > + */
> > +struct rte_flow_action_sample {
> > + const uint32_t ratio; /**< packets sampled equals to '1/ratio'.
> */
> > + const struct rte_flow_action *actions;
> > + /**< sub-action list specific for the sampling hit
> cases. */
> > +};
> > +
> > +/**
> > * Verbose error types.
> > *
> > * Most of them provide the type of the object referenced by struct
> > --
> > 1.8.3.1
> >
>
Hi all
From: Jerin Jacob:
> On Fri, Jul 3, 2020 at 8:57 PM Thomas Monjalon <thomas@monjalon.net>
> wrote:
> >
> > 03/07/2020 17:08, Jerin Jacob:
> > > On Fri, Jul 3, 2020 at 8:25 PM Matan Azrad <matan@mellanox.com>
> wrote:
> > > > From: Jerin Jacob:
> > > > > When adding overlapping API(rte_eth_mirror_rule_set()) in the
> > > > > same library(ethdev).
> > > > > Please depreciate the old API.
> > > > > We should not have two separate paths for the same function in
> > > > > the same ethdev library. It is pain for app and driver developers.
> > > >
> > > > What are about all the other rte_flow parallel configuration APIs in
> ethdev:
> > > > promiscuous_enable;
> > > > promiscuous_disable;
> > > > allmulticast_enable;
> > > > allmulticast_disable;
> > > > mac_addr_remove;
> > > > mac_addr_add;
> > > > mac_addr_set;
> > > > set_mc_addr_list;
> > > > vlan_filter_set;
> > > > vlan_tpid_set;
> > > > vlan_strip_queue_set;
> > > > vlan_offload_set;
> > > > vlan_pvid_set;
> > > > udp_tunnel_port_add;
> > > > udp_tunnel_port_del;
> > > > ...
> > > >
> > > > These APIs can be replaced easily by rte_flow API.
> > > > Do you think we need to deprecate all?
> > >
> > > I think, basic stuff like below can have separate API.
> > > 1) promiscuous_enable;
> > > 2) promiscuous_disable;
> > > 3) allmulticast_enable;
> > > 4) allmulticast_disable;
> > > 5) mac_addr_remove;
> > > 6) mac_addr_add;
> > > 7) mac_addr_set;
> > > 8) set_mc_addr_list;
> >
> > "Basic" is not a precise definition :)
>
> Yep.
>
> > I would say port-level configuration should remain out of rte_flow
> > API.
Thomas, Can you explain what is port-level?
Everything in rte_flow is per port...
Also, can you give reasons for your claim?
> +1.
> In addition that, I would say anything needs to configured at "per-flow"
> granularity use rte_flow.
Jerin, What do you mean "per-flow" ?
Everything in traffic filtering\actions is per flow, for example:
Promiscuous: flow create 0 ingress pattern eth / end actions queue index 0 / end
Multicast\mac related: flow create 0 ingress pattern eth dst is X /end actions queue 0/ end
(in case of legacy RSS queue action will be replaced by rss).
....
Everything here are flows.
> >
> > > But The VLAN and UDP related should be rte_flow candidates.(IMO)
> >
> > Yes definitely, tunneling is better managed with rte_flow.
Can you give reasons for your claim?
Why should Vlan\Tunnel be in rte_flow and promiscuous\Multicast\mac not?
> > This is an important discussion, I Cc other ethdev maintainers.
Agree, this is a good trigger to open this important discussion.
> > Note: this is an ethdev patch, all ethdev maintainers should be Cc'ed.
> > Aren't you using --cc-cmd devtools/get-maintainer.sh ?
On Sun, Jul 5, 2020 at 12:56 AM Matan Azrad <matan@mellanox.com> wrote:
>
> Hi all
>
> From: Jerin Jacob:
> > On Fri, Jul 3, 2020 at 8:57 PM Thomas Monjalon <thomas@monjalon.net>
> > wrote:
> > >
> > > 03/07/2020 17:08, Jerin Jacob:
> > > > On Fri, Jul 3, 2020 at 8:25 PM Matan Azrad <matan@mellanox.com>
> > wrote:
> > > > > From: Jerin Jacob:
> > > > > > When adding overlapping API(rte_eth_mirror_rule_set()) in the
> > > > > > same library(ethdev).
> > > > > > Please depreciate the old API.
> > > > > > We should not have two separate paths for the same function in
> > > > > > the same ethdev library. It is pain for app and driver developers.
> > > > >
> > > > > What are about all the other rte_flow parallel configuration APIs in
> > ethdev:
> > > > > promiscuous_enable;
> > > > > promiscuous_disable;
> > > > > allmulticast_enable;
> > > > > allmulticast_disable;
> > > > > mac_addr_remove;
> > > > > mac_addr_add;
> > > > > mac_addr_set;
> > > > > set_mc_addr_list;
> > > > > vlan_filter_set;
> > > > > vlan_tpid_set;
> > > > > vlan_strip_queue_set;
> > > > > vlan_offload_set;
> > > > > vlan_pvid_set;
> > > > > udp_tunnel_port_add;
> > > > > udp_tunnel_port_del;
> > > > > ...
> > > > >
> > > > > These APIs can be replaced easily by rte_flow API.
> > > > > Do you think we need to deprecate all?
> > > >
> > > > I think, basic stuff like below can have separate API.
> > > > 1) promiscuous_enable;
> > > > 2) promiscuous_disable;
> > > > 3) allmulticast_enable;
> > > > 4) allmulticast_disable;
> > > > 5) mac_addr_remove;
> > > > 6) mac_addr_add;
> > > > 7) mac_addr_set;
> > > > 8) set_mc_addr_list;
> > >
> > > "Basic" is not a precise definition :)
> >
> > Yep.
> >
> > > I would say port-level configuration should remain out of rte_flow
> > > API.
>
> Thomas, Can you explain what is port-level?
> Everything in rte_flow is per port...
>
> Also, can you give reasons for your claim?
>
> > +1.
> > In addition that, I would say anything needs to configured at "per-flow"
> > granularity use rte_flow.
>
> Jerin, What do you mean "per-flow" ?
In Terms of NIC HW features, Typical HW will have
a) Basic "port" level configuration like
- enable/disable promiscuous
b) Advance HW's will have CAM based flow filtering. IMO, CAM related
stuff should go to rte_flow.
This is to enable, The very basic PMD(without advanced features) should work
with port level basic APIs(i.e without rte_flow)
I have seen promiscuous, mac address handling is part of basic NIC
HW(i.e NICs without advanced CAM filters).
That's my reasoning for the split.
> Everything in traffic filtering\actions is per flow, for example:
> Promiscuous: flow create 0 ingress pattern eth / end actions queue index 0 / end
IMO, it is not an accurate representation of promiscuous enable. It
needs to send the traffic
to all queues and patterns is not just eth.
> Multicast\mac related: flow create 0 ingress pattern eth dst is X /end actions queue 0/ end
> (in case of legacy RSS queue action will be replaced by rss).
> ....
>
> Everything here are flows.
>
> > >
> > > > But The VLAN and UDP related should be rte_flow candidates.(IMO)
> > >
> > > Yes definitely, tunneling is better managed with rte_flow.
>
> Can you give reasons for your claim?
> Why should Vlan\Tunnel be in rte_flow and promiscuous\Multicast\mac not?
>
> > > This is an important discussion, I Cc other ethdev maintainers.
>
> Agree, this is a good trigger to open this important discussion.
>
> > > Note: this is an ethdev patch, all ethdev maintainers should be Cc'ed.
> > > Aren't you using --cc-cmd devtools/get-maintainer.sh ?
>
From: Jerin Jacob:
> On Sun, Jul 5, 2020 at 12:56 AM Matan Azrad <matan@mellanox.com> wrote:
> >
> > Hi all
> >
> > From: Jerin Jacob:
> > > On Fri, Jul 3, 2020 at 8:57 PM Thomas Monjalon <thomas@monjalon.net>
> > > wrote:
> > > >
> > > > 03/07/2020 17:08, Jerin Jacob:
> > > > > On Fri, Jul 3, 2020 at 8:25 PM Matan Azrad <matan@mellanox.com>
> > > wrote:
> > > > > > From: Jerin Jacob:
> > > > > > > When adding overlapping API(rte_eth_mirror_rule_set()) in
> > > > > > > the same library(ethdev).
> > > > > > > Please depreciate the old API.
> > > > > > > We should not have two separate paths for the same function
> > > > > > > in the same ethdev library. It is pain for app and driver developers.
> > > > > >
> > > > > > What are about all the other rte_flow parallel configuration
> > > > > > APIs in
> > > ethdev:
> > > > > > promiscuous_enable;
> > > > > > promiscuous_disable;
> > > > > > allmulticast_enable;
> > > > > > allmulticast_disable;
> > > > > > mac_addr_remove;
> > > > > > mac_addr_add;
> > > > > > mac_addr_set;
> > > > > > set_mc_addr_list;
> > > > > > vlan_filter_set;
> > > > > > vlan_tpid_set;
> > > > > > vlan_strip_queue_set;
> > > > > > vlan_offload_set;
> > > > > > vlan_pvid_set;
> > > > > > udp_tunnel_port_add;
> > > > > > udp_tunnel_port_del;
> > > > > > ...
> > > > > >
> > > > > > These APIs can be replaced easily by rte_flow API.
> > > > > > Do you think we need to deprecate all?
> > > > >
> > > > > I think, basic stuff like below can have separate API.
> > > > > 1) promiscuous_enable;
> > > > > 2) promiscuous_disable;
> > > > > 3) allmulticast_enable;
> > > > > 4) allmulticast_disable;
> > > > > 5) mac_addr_remove;
> > > > > 6) mac_addr_add;
> > > > > 7) mac_addr_set;
> > > > > 8) set_mc_addr_list;
> > > >
> > > > "Basic" is not a precise definition :)
> > >
> > > Yep.
> > >
> > > > I would say port-level configuration should remain out of rte_flow
> > > > API.
> >
> > Thomas, Can you explain what is port-level?
> > Everything in rte_flow is per port...
> >
> > Also, can you give reasons for your claim?
> >
> > > +1.
> > > In addition that, I would say anything needs to configured at "per-flow"
> > > granularity use rte_flow.
> >
> > Jerin, What do you mean "per-flow" ?
>
> In Terms of NIC HW features, Typical HW will have
> a) Basic "port" level configuration like
> - enable/disable promiscuous
What is "port level", everything in rte_flow is also per port...
> b) Advance HW's will have CAM based flow filtering. IMO, CAM related stuff
> should go to rte_flow.
It is HW internal, I don't think all HWs use the same logic here.
Since rte_flow is generic for all filtering methods, It is good candidate API for all HWs.
> This is to enable, The very basic PMD(without advanced features) should
> work with port level basic APIs(i.e without rte_flow)
What is "basic"? Do you mean simple match and simple action?
As I said, Also rte_flow is port level API - so "port level" is not good term here.
As you said " When adding overlapping API(rte_eth_mirror_rule_set()) in the same library(ethdev). Please depreciate the old API."
Different APIs to do the same thing is not good, especially in packet filtering:
What should we do if we have conflicts?
For example: legacy filtering APIs say to receive packet A and rte_flow says to drop it.
Don't you think it complicates more the user API understanding, also the PMD implementations?
> I have seen promiscuous, mac address handling is part of basic NIC HW(i.e
> NICs without advanced CAM filters).
> That's my reasoning for the split.
As I said, the nic HW specific implementation should not affect the API.
I don't think we need to split API and to complicate the user because of it.
IMO, It is better to have one generic API for packet filtering:
It is clearer, simpler, generic and classic.
The user and PMD need to understand only one filtering API and that’s it (no need to combine between multiple filtering APIs).
I know this is big change but we can do it in modular way.
It reminds me the big change that was done in Rx\Tx offload configurations:
So, when offload became more popular we modularly forced users to replace the offload API.
Also here, flow filtering becomes popular so maybe this is the time(20.08-20.11) to force changes in the old APIs.
> > Everything in traffic filtering\actions is per flow, for example:
> > Promiscuous: flow create 0 ingress pattern eth / end actions queue
> > index 0 / end
>
> IMO, it is not an accurate representation of promiscuous enable. It needs to
> send the traffic to all queues and patterns is not just eth.
Yes, if legacy RSS is configured we need to replace the above queue action by rss action as I wrote before.(did you read it just below?)
So, we can add legacy RSS APIs to the list above...
> > Multicast\mac related: flow create 0 ingress pattern eth dst is X /end
> > actions queue 0/ end (in case of legacy RSS queue action will be replaced by
> rss).
> > ....
> >
> > Everything here are flows.
> >
> > > >
> > > > > But The VLAN and UDP related should be rte_flow candidates.(IMO)
> > > >
> > > > Yes definitely, tunneling is better managed with rte_flow.
> >
> > Can you give reasons for your claim?
> > Why should Vlan\Tunnel be in rte_flow and promiscuous\Multicast\mac
> not?
> >
> > > > This is an important discussion, I Cc other ethdev maintainers.
> >
> > Agree, this is a good trigger to open this important discussion.
> >
> > > > Note: this is an ethdev patch, all ethdev maintainers should be Cc'ed.
> > > > Aren't you using --cc-cmd devtools/get-maintainer.sh ?
> >
04/07/2020 16:44, Ajit Khaparde:
> On Thu, Jul 2, 2020 at 11:40 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > When adding overlapping API(rte_eth_mirror_rule_set()) in the same
> > library(ethdev).
> > Please depreciate the old API.
> > We should not have two separate paths for the same function in the
> > same ethdev library. It is pain for app and driver developers.
> >
> > With the above deprecation notice,
> > Acked-by: Jerin Jacob <jerinj@marvell.com>
> >
> I am fine with the proposed RTE_FLOW_ACTION_TYPE_SAMPLE. But..
>
> When rte_eth_mirror_rule_set() is deprecated, are we going to add
> RTE_FLOW_ACTION_TYPE_MIRROR for full fledged mirror action?
> Or we are proposing to use RTE_FLOW_ACTION_TYPE_SAMPLE with
> ratio of 1 to mirror all packets, thereby doing away with the need for
> a separate RTE_FLOW_ACTION_TYPE_MIRROR?
The idea is to use RTE_FLOW_ACTION_TYPE_SAMPLE with ratio=1 for mirroring.
Hi Andrew,
I replied to some of your comments.
Best,
Ori
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Andrew Rybchenko
> Sent: Saturday, July 4, 2020 4:05 PM
> Subject: Re: [dpdk-dev] [PATCH v2 1/7] ethdev: introduce sample action for rte
> flow
>
> On 7/2/20 9:43 PM, Jiawei Wang wrote:
> > When using full offload, all traffic will be handled by the HW, and
> > directed to the requested vf or wire, the control application loses
>
> vf->VF
>
> > visibility on the traffic.
> > So there's a need for an action that will enable the control application
> > some visibility.
> >
> > The solution is introduced a new action that will sample the incoming
> > traffic and send a duplicated traffic in some predefined ratio to the
> > application, while the original packet will continue to the target
> > destination.
> >
>
> May be 1 packet per second is a better sampling approach?
> Or just different.
>
Those are two different things, lets take a packet that arrives once every two seconds
and we ask to sample once every second, this means that we will always get that packet.
Also as far as I understand the use case is to have some visibility about the traffic.
so you can assume that if a packet is sent once per second the application will get the packet
with very high delay and very low visibility. Lets take a use case that the hyprivor
wants to check if one of the VM is abusing the system (sends DDOS packets, or just
trying to understand the network) in this case we can assume that the VM will send large
amount of traffic. and if we only check once per second the application will not be able to
understand the traffic meaning, but if we sample 1% of the traffic then the application will
see very fast the type of the traffic the VM is sending and if it is trying to abuse the system.
So I vote in favor of keeping as is.
> > The packets sampled equals is '1/ratio', if the ratio value be set to 1
> > , means that the packets would be completely mirrored. The sample packet
>
> Comma on the next line looks bad.
>
> > can be assigned with different set of actions from the original packet.
> >
> > In order to support the sample packet in rte_flow, new rte_flow action
> > definition RTE_FLOW_ACTION_TYPE_SAMPLE and structure
> rte_flow_action_sample
> > will be introduced.
> >
> > Signed-off-by: Jiawei Wang <jiaweiw@mellanox.com>
> > Acked-by: Ori Kam <orika@mellanox.com>
> > ---
> > doc/guides/prog_guide/rte_flow.rst | 25 +++++++++++++++++++++++++
> > doc/guides/rel_notes/release_20_08.rst | 6 ++++++
> > lib/librte_ethdev/rte_flow.c | 1 +
> > lib/librte_ethdev/rte_flow.h | 28 ++++++++++++++++++++++++++++
> > 4 files changed, 60 insertions(+)
> >
> > diff --git a/doc/guides/prog_guide/rte_flow.rst
> b/doc/guides/prog_guide/rte_flow.rst
> > index d5dd18c..50dfe1f 100644
> > --- a/doc/guides/prog_guide/rte_flow.rst
> > +++ b/doc/guides/prog_guide/rte_flow.rst
> > @@ -2645,6 +2645,31 @@ timeout passed without any matching on the flow.
> > | ``context`` | user input flow context |
> > +--------------+---------------------------------+
> >
> > +Action: ``SAMPLE``
> > +^^^^^^^^^^^^^^^^^^
> > +
> > +Adds a sample action to a matched flow.
> > +
> > +The matching packets will be duplicated to a special queue or vport
>
> what is vport above?
>
I think it should be port (when using E-Switch)
> > +with the predefined ``ratio``, the packets sampled equals is '1/ratio'.
> > +All the packets continues to the target destination.
>
> continues -> continue (if I'm not mistaken)
>
> > +
> > +When the ``ratio`` is set to 1 then the packets will be 100% mirrored.
> > +``actions`` represent the different set of actions for the sampled or mirrored
> > +packets.
> > +
> > +.. _table_rte_flow_action_sample:
> > +
> > +.. table:: SAMPLE
> > +
> > + +--------------+---------------------------------+
> > + | Field | Value |
> > + +==============+=================================+
> > + | ``ratio`` | 32 bits sample ratio value |
> > + +--------------+---------------------------------+
> > + | ``actions`` | sub-action list for sampling |
> > + +--------------+---------------------------------+
> > +
> > Negative types
> > ~~~~~~~~~~~~~~
> >
> > diff --git a/doc/guides/rel_notes/release_20_08.rst
> b/doc/guides/rel_notes/release_20_08.rst
> > index 5cbc4ce..313e8d3 100644
> > --- a/doc/guides/rel_notes/release_20_08.rst
> > +++ b/doc/guides/rel_notes/release_20_08.rst
> > @@ -81,6 +81,12 @@ New Features
> > * Added support for virtio queue statistics.
> > * Added support for MTU update.
> >
> > +* **Added flow-based traffic sampling support.**
> > +
> > + Added new action: ``RTE_FLOW_ACTION_TYPE_SAMPLE`` to duplicate the
> matching
> > + packets with given ratio and redirects to vport or queue. The sampled
> packets
>
> What is vport above?
>
See comment above.
> > + also can be assigned with an additional optional actions.
>
> May actions list be empty or NULL? If no, it does not look
> optional.
>
I think that the action list can't be NULL or empty. There is no meaning to empty list.
I agree it should be stated.
> > +
> > * **Updated Marvell octeontx2 ethdev PMD.**
> >
> > Updated Marvell octeontx2 driver with cn98xx support.
> > diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> > index 1685be5..733871d 100644
> > --- a/lib/librte_ethdev/rte_flow.c
> > +++ b/lib/librte_ethdev/rte_flow.c
> > @@ -173,6 +173,7 @@ struct rte_flow_desc_data {
> > MK_FLOW_ACTION(SET_IPV4_DSCP, sizeof(struct
> rte_flow_action_set_dscp)),
> > MK_FLOW_ACTION(SET_IPV6_DSCP, sizeof(struct
> rte_flow_action_set_dscp)),
> > MK_FLOW_ACTION(AGE, sizeof(struct rte_flow_action_age)),
> > + MK_FLOW_ACTION(SAMPLE, sizeof(struct rte_flow_action_sample)),
> > };
> >
> > int
> > diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> > index b0e4199..c9cd80d 100644
> > --- a/lib/librte_ethdev/rte_flow.h
> > +++ b/lib/librte_ethdev/rte_flow.h
> > @@ -2099,6 +2099,13 @@ enum rte_flow_action_type {
> > * see enum RTE_ETH_EVENT_FLOW_AGED
> > */
> > RTE_FLOW_ACTION_TYPE_AGE,
> > +
> > + /**
> > + * Redirects specific ratio of packets to vport or queue.
> > + *
> > + * See struct rte_flow_action_sample.
> > + */
> > + RTE_FLOW_ACTION_TYPE_SAMPLE,
> > };
> >
> > /**
> > @@ -2709,6 +2716,27 @@ struct rte_flow_action {
> > struct rte_flow;
> >
> > /**
> > + * @warning
> > + * @b EXPERIMENTAL: this structure may change without prior notice
> > + *
> > + * RTE_FLOW_ACTION_TYPE_SAMPLE
> > + *
> > + * Adds a sample action to a matched flow.
> > + *
> > + * The matching packets will be duplicated to a special queue or vport
>
> again 'vport' here
> It sounds misleading and too restrictive to say "be duplicated
> to a special queue or vport". There is no specification of the
> queue or vport in the control structure.
> You should either describe it in a generic way like "be
> duplicated and own set of actions with a fate action applied"
> or put a restriction about QUEUE, RSS or "vport"-related action
> to be present in the sub-actions list.
>
> > + * in the predefined probabiilty, All the packets continues processing
>
> probabiilty -> probability
> I think 'predefined' is misleading here, 'specified' is better.
> Also strictly speaking it is not a predefined probability (as
> Stephen suggested), it is defined ratio.
>
> > + * on the default flow path.
> > + *
> > + * When the sample ratio is set to 1 then the packets will be 100% mirrored.
> > + * Additional action list be supported to add for sampled or mirrored
> packets.
> > + */
> > +struct rte_flow_action_sample {
> > + const uint32_t ratio; /**< packets sampled equals to '1/ratio'. */
>
> const is still above and it is meaningless (other actions do
> not have 'const' for plain fields).
>
+1
> > + const struct rte_flow_action *actions;
> > + /**< sub-action list specific for the sampling hit cases. */
>
> Is it required to have fate action?
> May I use it to MARK some packets and do not duplicate?
> I guess no. Or COUNT and DROP? Just COUNT?
>
I from my understanding you may use mark and count but it also must
have a fate action.
> What I'm trying to say that you're adding a generic packet
> selection mechanism with very restricted usage by design.
>
> Anyway, if you go with it, please, process other notes above.
>
> > +};
> > +
> > +/**
> > * Verbose error types.
> > *
> > * Most of them provide the type of the object referenced by struct
> >
On Sun, Jul 5, 2020 at 3:19 AM Ori Kam <orika@mellanox.com> wrote:
> Hi Andrew,
>
> I replied to some of your comments.
> Best,
> Ori
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Andrew Rybchenko
> > Sent: Saturday, July 4, 2020 4:05 PM
> > Subject: Re: [dpdk-dev] [PATCH v2 1/7] ethdev: introduce sample action
> for rte
> > flow
> >
> > On 7/2/20 9:43 PM, Jiawei Wang wrote:
> > > When using full offload, all traffic will be handled by the HW, and
> > > directed to the requested vf or wire, the control application loses
> >
> > vf->VF
> >
> > > visibility on the traffic.
> > > So there's a need for an action that will enable the control
> application
> > > some visibility.
> > >
> > > The solution is introduced a new action that will sample the incoming
> > > traffic and send a duplicated traffic in some predefined ratio to the
> > > application, while the original packet will continue to the target
> > > destination.
> > >
> >
> > May be 1 packet per second is a better sampling approach?
> > Or just different.
> >
> Those are two different things, lets take a packet that arrives once every
> two seconds
> and we ask to sample once every second, this means that we will always get
> that packet.
> Also as far as I understand the use case is to have some visibility about
> the traffic.
> so you can assume that if a packet is sent once per second the application
> will get the packet
> with very high delay and very low visibility. Lets take a use case that
> the hyprivor
> wants to check if one of the VM is abusing the system (sends DDOS packets,
> or just
> trying to understand the network) in this case we can assume that the VM
> will send large
> amount of traffic. and if we only check once per second the application
> will not be able to
> understand the traffic meaning, but if we sample 1% of the traffic then
> the application will
> see very fast the type of the traffic the VM is sending and if it is
> trying to abuse the system.
> So I vote in favor of keeping as is.
>
Thanks for bringing this up.
So an application may want to sample either "finite" packets per second or
"a percentage" of packets per second or "all" packets.
So a "uint32_t ratio" may not just be enough by itself.
Maybe we need to also couple it with a unit.
uint32_t sampling_unit; /* Specifies one of the units to use while
sampling. */
RTE_FLOW_NUM_PACKETS_PER_SEC /* Samples specific number of packets per
second. */
RTE_FLOW_PERCENT_PACKETS_PER_SEC /* Samples a percentage of packets per
second. */
RTE_FLOW_ALL_PACKETS /* SAMPLES all packets - equivalent to mirror */
This may be redundant if percentage is specified and ratio is 100.
In that case instead of "uint32_t ratio", just use "uint32_t sample"?
>
> > > The packets sampled equals is '1/ratio', if the ratio value be set to 1
> > > , means that the packets would be completely mirrored. The sample
> packet
> >
> > Comma on the next line looks bad.
> >
> > > can be assigned with different set of actions from the original packet.
> > >
> > > In order to support the sample packet in rte_flow, new rte_flow action
> > > definition RTE_FLOW_ACTION_TYPE_SAMPLE and structure
> > rte_flow_action_sample
> > > will be introduced.
> > >
> > > Signed-off-by: Jiawei Wang <jiaweiw@mellanox.com>
> > > Acked-by: Ori Kam <orika@mellanox.com>
> > > ---
> > > doc/guides/prog_guide/rte_flow.rst | 25 +++++++++++++++++++++++++
> > > doc/guides/rel_notes/release_20_08.rst | 6 ++++++
> > > lib/librte_ethdev/rte_flow.c | 1 +
> > > lib/librte_ethdev/rte_flow.h | 28
> ++++++++++++++++++++++++++++
> > > 4 files changed, 60 insertions(+)
> > >
> > > diff --git a/doc/guides/prog_guide/rte_flow.rst
> > b/doc/guides/prog_guide/rte_flow.rst
> > > index d5dd18c..50dfe1f 100644
> > > --- a/doc/guides/prog_guide/rte_flow.rst
> > > +++ b/doc/guides/prog_guide/rte_flow.rst
> > > @@ -2645,6 +2645,31 @@ timeout passed without any matching on the flow.
> > > | ``context`` | user input flow context |
> > > +--------------+---------------------------------+
> > >
> > > +Action: ``SAMPLE``
> > > +^^^^^^^^^^^^^^^^^^
> > > +
> > > +Adds a sample action to a matched flow.
> > > +
> > > +The matching packets will be duplicated to a special queue or vport
> >
> > what is vport above?
> >
> I think it should be port (when using E-Switch)
>
> > > +with the predefined ``ratio``, the packets sampled equals is
> '1/ratio'.
> > > +All the packets continues to the target destination.
> >
> > continues -> continue (if I'm not mistaken)
> >
> > > +
> > > +When the ``ratio`` is set to 1 then the packets will be 100% mirrored.
> > > +``actions`` represent the different set of actions for the sampled or
> mirrored
> > > +packets.
> > > +
> > > +.. _table_rte_flow_action_sample:
> > > +
> > > +.. table:: SAMPLE
> > > +
> > > + +--------------+---------------------------------+
> > > + | Field | Value |
> > > + +==============+=================================+
> > > + | ``ratio`` | 32 bits sample ratio value |
> > > + +--------------+---------------------------------+
> > > + | ``actions`` | sub-action list for sampling |
> > > + +--------------+---------------------------------+
> > > +
> > > Negative types
> > > ~~~~~~~~~~~~~~
> > >
> > > diff --git a/doc/guides/rel_notes/release_20_08.rst
> > b/doc/guides/rel_notes/release_20_08.rst
> > > index 5cbc4ce..313e8d3 100644
> > > --- a/doc/guides/rel_notes/release_20_08.rst
> > > +++ b/doc/guides/rel_notes/release_20_08.rst
> > > @@ -81,6 +81,12 @@ New Features
> > > * Added support for virtio queue statistics.
> > > * Added support for MTU update.
> > >
> > > +* **Added flow-based traffic sampling support.**
> > > +
> > > + Added new action: ``RTE_FLOW_ACTION_TYPE_SAMPLE`` to duplicate the
> > matching
> > > + packets with given ratio and redirects to vport or queue. The
> sampled
> > packets
> >
> > What is vport above?
> >
> See comment above.
>
> > > + also can be assigned with an additional optional actions.
> >
> > May actions list be empty or NULL? If no, it does not look
> > optional.
> >
> I think that the action list can't be NULL or empty. There is no meaning
> to empty list.
> I agree it should be stated.
>
> > > +
> > > * **Updated Marvell octeontx2 ethdev PMD.**
> > >
> > > Updated Marvell octeontx2 driver with cn98xx support.
> > > diff --git a/lib/librte_ethdev/rte_flow.c
> b/lib/librte_ethdev/rte_flow.c
> > > index 1685be5..733871d 100644
> > > --- a/lib/librte_ethdev/rte_flow.c
> > > +++ b/lib/librte_ethdev/rte_flow.c
> > > @@ -173,6 +173,7 @@ struct rte_flow_desc_data {
> > > MK_FLOW_ACTION(SET_IPV4_DSCP, sizeof(struct
> > rte_flow_action_set_dscp)),
> > > MK_FLOW_ACTION(SET_IPV6_DSCP, sizeof(struct
> > rte_flow_action_set_dscp)),
> > > MK_FLOW_ACTION(AGE, sizeof(struct rte_flow_action_age)),
> > > + MK_FLOW_ACTION(SAMPLE, sizeof(struct rte_flow_action_sample)),
> > > };
> > >
> > > int
> > > diff --git a/lib/librte_ethdev/rte_flow.h
> b/lib/librte_ethdev/rte_flow.h
> > > index b0e4199..c9cd80d 100644
> > > --- a/lib/librte_ethdev/rte_flow.h
> > > +++ b/lib/librte_ethdev/rte_flow.h
> > > @@ -2099,6 +2099,13 @@ enum rte_flow_action_type {
> > > * see enum RTE_ETH_EVENT_FLOW_AGED
> > > */
> > > RTE_FLOW_ACTION_TYPE_AGE,
> > > +
> > > + /**
> > > + * Redirects specific ratio of packets to vport or queue.
> > > + *
> > > + * See struct rte_flow_action_sample.
> > > + */
> > > + RTE_FLOW_ACTION_TYPE_SAMPLE,
> > > };
> > >
> > > /**
> > > @@ -2709,6 +2716,27 @@ struct rte_flow_action {
> > > struct rte_flow;
> > >
> > > /**
> > > + * @warning
> > > + * @b EXPERIMENTAL: this structure may change without prior notice
> > > + *
> > > + * RTE_FLOW_ACTION_TYPE_SAMPLE
> > > + *
> > > + * Adds a sample action to a matched flow.
> > > + *
> > > + * The matching packets will be duplicated to a special queue or vport
> >
> > again 'vport' here
> > It sounds misleading and too restrictive to say "be duplicated
> > to a special queue or vport". There is no specification of the
> > queue or vport in the control structure.
> > You should either describe it in a generic way like "be
> > duplicated and own set of actions with a fate action applied"
> > or put a restriction about QUEUE, RSS or "vport"-related action
> > to be present in the sub-actions list.
> >
> > > + * in the predefined probabiilty, All the packets continues processing
> >
> > probabiilty -> probability
> > I think 'predefined' is misleading here, 'specified' is better.
> > Also strictly speaking it is not a predefined probability (as
> > Stephen suggested), it is defined ratio.
> >
> > > + * on the default flow path.
> > > + *
> > > + * When the sample ratio is set to 1 then the packets will be 100%
> mirrored.
> > > + * Additional action list be supported to add for sampled or mirrored
> > packets.
> > > + */
> > > +struct rte_flow_action_sample {
> > > + const uint32_t ratio; /**< packets sampled equals to '1/ratio'. */
> >
> > const is still above and it is meaningless (other actions do
> > not have 'const' for plain fields).
> >
> +1
>
> > > + const struct rte_flow_action *actions;
> > > + /**< sub-action list specific for the sampling hit cases.
> */
> >
> > Is it required to have fate action?
> > May I use it to MARK some packets and do not duplicate?
> > I guess no. Or COUNT and DROP? Just COUNT?
> >
> I from my understanding you may use mark and count but it also must
> have a fate action.
>
> > What I'm trying to say that you're adding a generic packet
> > selection mechanism with very restricted usage by design.
> >
> > Anyway, if you go with it, please, process other notes above.
> >
>
> > > +};
> > > +
> > > +/**
> > > * Verbose error types.
> > > *
> > > * Most of them provide the type of the object referenced by struct
> > >
>
>
On Sun, Jul 5, 2020 at 1:55 AM Thomas Monjalon <thomas@monjalon.net> wrote:
> 04/07/2020 16:44, Ajit Khaparde:
> > On Thu, Jul 2, 2020 at 11:40 PM Jerin Jacob <jerinjacobk@gmail.com>
> wrote:
> > > When adding overlapping API(rte_eth_mirror_rule_set()) in the same
> > > library(ethdev).
> > > Please depreciate the old API.
> > > We should not have two separate paths for the same function in the
> > > same ethdev library. It is pain for app and driver developers.
> > >
> > > With the above deprecation notice,
> > > Acked-by: Jerin Jacob <jerinj@marvell.com>
> > >
> > I am fine with the proposed RTE_FLOW_ACTION_TYPE_SAMPLE. But..
> >
> > When rte_eth_mirror_rule_set() is deprecated, are we going to add
> > RTE_FLOW_ACTION_TYPE_MIRROR for full fledged mirror action?
> > Or we are proposing to use RTE_FLOW_ACTION_TYPE_SAMPLE with
> > ratio of 1 to mirror all packets, thereby doing away with the need for
> > a separate RTE_FLOW_ACTION_TYPE_MIRROR?
>
> The idea is to use RTE_FLOW_ACTION_TYPE_SAMPLE with ratio=1 for mirroring.
>
Thanks for clarifying.
Thanks Andrew and Ori,
> -----Original Message-----
> From: Ori Kam <orika@mellanox.com>
> Sent: Sunday, July 5, 2020 6:19 PM
> To: Andrew Rybchenko <arybchenko@solarflare.com>; Jiawei(Jonny) Wang
> <jiaweiw@mellanox.com>; Slava Ovsiienko <viacheslavo@mellanox.com>;
> Matan Azrad <matan@mellanox.com>
> Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>; Raslan
> Darawsheh <rasland@mellanox.com>; ian.stokes@intel.com;
> fbl@redhat.com
> Subject: RE: [dpdk-dev] [PATCH v2 1/7] ethdev: introduce sample action for
> rte flow
>
> Hi Andrew,
>
> I replied to some of your comments.
> Best,
> Ori
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Andrew Rybchenko
> > Sent: Saturday, July 4, 2020 4:05 PM
> > Subject: Re: [dpdk-dev] [PATCH v2 1/7] ethdev: introduce sample action
> > for rte flow
> >
> > On 7/2/20 9:43 PM, Jiawei Wang wrote:
> > > When using full offload, all traffic will be handled by the HW, and
> > > directed to the requested vf or wire, the control application loses
> >
> > vf->VF
will change in v3 patch.
> >
> > > visibility on the traffic.
> > > So there's a need for an action that will enable the control
> > > application some visibility.
> > >
> > > The solution is introduced a new action that will sample the
> > > incoming traffic and send a duplicated traffic in some predefined
> > > ratio to the application, while the original packet will continue to
> > > the target destination.
> > >
> >
> > May be 1 packet per second is a better sampling approach?
> > Or just different.
> >
> Those are two different things, lets take a packet that arrives once every two
> seconds and we ask to sample once every second, this means that we will
> always get that packet.
> Also as far as I understand the use case is to have some visibility about the
> traffic.
> so you can assume that if a packet is sent once per second the application
> will get the packet with very high delay and very low visibility. Lets take a use
> case that the hyprivor wants to check if one of the VM is abusing the system
> (sends DDOS packets, or just trying to understand the network) in this case
> we can assume that the VM will send large amount of traffic. and if we only
> check once per second the application will not be able to understand the
> traffic meaning, but if we sample 1% of the traffic then the application will
> see very fast the type of the traffic the VM is sending and if it is trying to
> abuse the system.
> So I vote in favor of keeping as is.
>
>
> > > The packets sampled equals is '1/ratio', if the ratio value be set
> > > to 1 , means that the packets would be completely mirrored. The
> > > sample packet
> >
> > Comma on the next line looks bad.
ok, will move the Comma to the same line.
> >
> > > can be assigned with different set of actions from the original packet.
> > >
> > > In order to support the sample packet in rte_flow, new rte_flow
> > > action definition RTE_FLOW_ACTION_TYPE_SAMPLE and structure
> > rte_flow_action_sample
> > > will be introduced.
> > >
> > > Signed-off-by: Jiawei Wang <jiaweiw@mellanox.com>
> > > Acked-by: Ori Kam <orika@mellanox.com>
> > > ---
> > > doc/guides/prog_guide/rte_flow.rst | 25
> +++++++++++++++++++++++++
> > > doc/guides/rel_notes/release_20_08.rst | 6 ++++++
> > > lib/librte_ethdev/rte_flow.c | 1 +
> > > lib/librte_ethdev/rte_flow.h | 28 ++++++++++++++++++++++++++++
> > > 4 files changed, 60 insertions(+)
> > >
> > > diff --git a/doc/guides/prog_guide/rte_flow.rst
> > b/doc/guides/prog_guide/rte_flow.rst
> > > index d5dd18c..50dfe1f 100644
> > > --- a/doc/guides/prog_guide/rte_flow.rst
> > > +++ b/doc/guides/prog_guide/rte_flow.rst
> > > @@ -2645,6 +2645,31 @@ timeout passed without any matching on the
> flow.
> > > | ``context`` | user input flow context |
> > > +--------------+---------------------------------+
> > >
> > > +Action: ``SAMPLE``
> > > +^^^^^^^^^^^^^^^^^^
> > > +
> > > +Adds a sample action to a matched flow.
> > > +
> > > +The matching packets will be duplicated to a special queue or vport
> >
> > what is vport above?
> >
> I think it should be port (when using E-Switch)
Yes, the destination port is working on e-switch mode, change vport->port.
>
> > > +with the predefined ``ratio``, the packets sampled equals is '1/ratio'.
> > > +All the packets continues to the target destination.
> >
> > continues -> continue (if I'm not mistaken)
ok, will change.
> >
> > > +
> > > +When the ``ratio`` is set to 1 then the packets will be 100% mirrored.
> > > +``actions`` represent the different set of actions for the sampled
> > > +or mirrored packets.
> > > +
> > > +.. _table_rte_flow_action_sample:
> > > +
> > > +.. table:: SAMPLE
> > > +
> > > + +--------------+---------------------------------+
> > > + | Field | Value |
> > > + +==============+=================================+
> > > + | ``ratio`` | 32 bits sample ratio value |
> > > + +--------------+---------------------------------+
> > > + | ``actions`` | sub-action list for sampling |
> > > + +--------------+---------------------------------+
> > > +
> > > Negative types
> > > ~~~~~~~~~~~~~~
> > >
> > > diff --git a/doc/guides/rel_notes/release_20_08.rst
> > b/doc/guides/rel_notes/release_20_08.rst
> > > index 5cbc4ce..313e8d3 100644
> > > --- a/doc/guides/rel_notes/release_20_08.rst
> > > +++ b/doc/guides/rel_notes/release_20_08.rst
> > > @@ -81,6 +81,12 @@ New Features
> > > * Added support for virtio queue statistics.
> > > * Added support for MTU update.
> > >
> > > +* **Added flow-based traffic sampling support.**
> > > +
> > > + Added new action: ``RTE_FLOW_ACTION_TYPE_SAMPLE`` to duplicate
> > > + the
> > matching
> > > + packets with given ratio and redirects to vport or queue. The
> > > + sampled
> > packets
> >
> > What is vport above?
> >
> See comment above.
>
> > > + also can be assigned with an additional optional actions.
> >
> > May actions list be empty or NULL? If no, it does not look optional.
> >
> I think that the action list can't be NULL or empty. There is no meaning to
> empty list.
> I agree it should be stated.
>
'optional' means that besides an fate action, also can combine with addition action if needed,
like mark and queue for sampled packet.
> > > +
> > > * **Updated Marvell octeontx2 ethdev PMD.**
> > >
> > > Updated Marvell octeontx2 driver with cn98xx support.
> > > diff --git a/lib/librte_ethdev/rte_flow.c
> > > b/lib/librte_ethdev/rte_flow.c index 1685be5..733871d 100644
> > > --- a/lib/librte_ethdev/rte_flow.c
> > > +++ b/lib/librte_ethdev/rte_flow.c
> > > @@ -173,6 +173,7 @@ struct rte_flow_desc_data {
> > > MK_FLOW_ACTION(SET_IPV4_DSCP, sizeof(struct
> > rte_flow_action_set_dscp)),
> > > MK_FLOW_ACTION(SET_IPV6_DSCP, sizeof(struct
> > rte_flow_action_set_dscp)),
> > > MK_FLOW_ACTION(AGE, sizeof(struct rte_flow_action_age)),
> > > + MK_FLOW_ACTION(SAMPLE, sizeof(struct rte_flow_action_sample)),
> > > };
> > >
> > > int
> > > diff --git a/lib/librte_ethdev/rte_flow.h
> > > b/lib/librte_ethdev/rte_flow.h index b0e4199..c9cd80d 100644
> > > --- a/lib/librte_ethdev/rte_flow.h
> > > +++ b/lib/librte_ethdev/rte_flow.h
> > > @@ -2099,6 +2099,13 @@ enum rte_flow_action_type {
> > > * see enum RTE_ETH_EVENT_FLOW_AGED
> > > */
> > > RTE_FLOW_ACTION_TYPE_AGE,
> > > +
> > > + /**
> > > + * Redirects specific ratio of packets to vport or queue.
> > > + *
> > > + * See struct rte_flow_action_sample.
> > > + */
> > > + RTE_FLOW_ACTION_TYPE_SAMPLE,
> > > };
> > >
> > > /**
> > > @@ -2709,6 +2716,27 @@ struct rte_flow_action { struct rte_flow;
> > >
> > > /**
> > > + * @warning
> > > + * @b EXPERIMENTAL: this structure may change without prior notice
> > > + *
> > > + * RTE_FLOW_ACTION_TYPE_SAMPLE
> > > + *
> > > + * Adds a sample action to a matched flow.
> > > + *
> > > + * The matching packets will be duplicated to a special queue or
> > > + vport
> >
> > again 'vport' here
> > It sounds misleading and too restrictive to say "be duplicated to a
> > special queue or vport". There is no specification of the queue or
> > vport in the control structure.
> > You should either describe it in a generic way like "be duplicated and
> > own set of actions with a fate action applied"
> > or put a restriction about QUEUE, RSS or "vport"-related action to be
> > present in the sub-actions list.
> >
I'll change the description to "The matching packets will be duplicated and applied with own set of actions with a fate action"
> > > + * in the predefined probabiilty, All the packets continues
> > > + processing
> >
> > probabiilty -> probability
> > I think 'predefined' is misleading here, 'specified' is better.
> > Also strictly speaking it is not a predefined probability (as Stephen
> > suggested), it is defined ratio.
> >
Thanks for pointing out typo, I will use 'specified ratio' instead of.
> > > + * on the default flow path.
> > > + *
> > > + * When the sample ratio is set to 1 then the packets will be 100%
> mirrored.
> > > + * Additional action list be supported to add for sampled or
> > > + mirrored
> > packets.
> > > + */
> > > +struct rte_flow_action_sample {
> > > + const uint32_t ratio; /**< packets sampled equals to '1/ratio'. */
> >
> > const is still above and it is meaningless (other actions do not have
> > 'const' for plain fields).
> >
> +1
agree, remove 'const'.
>
> > > + const struct rte_flow_action *actions;
> > > + /**< sub-action list specific for the sampling hit cases. */
> >
> > Is it required to have fate action?
> > May I use it to MARK some packets and do not duplicate?
> > I guess no. Or COUNT and DROP? Just COUNT?
> >
> I from my understanding you may use mark and count but it also must have
> a fate action.
>
Yes, need fate action for sampling, mark or count is optional action.
> > What I'm trying to say that you're adding a generic packet selection
> > mechanism with very restricted usage by design.
> >
> > Anyway, if you go with it, please, process other notes above.
> >
>
> > > +};
> > > +
> > > +/**
> > > * Verbose error types.
> > > *
> > > * Most of them provide the type of the object referenced by struct
> > >
On Sun, Jul 5, 2020 at 10:22 AM Matan Azrad <matan@mellanox.com> wrote:
>
>
>
> From: Jerin Jacob:
> > On Sun, Jul 5, 2020 at 12:56 AM Matan Azrad <matan@mellanox.com> wrote:
> > >
> > > Hi all
> > >
> > > From: Jerin Jacob:
> > > > On Fri, Jul 3, 2020 at 8:57 PM Thomas Monjalon <thomas@monjalon.net>
> > > > wrote:
> > > > >
> > > > > 03/07/2020 17:08, Jerin Jacob:
> > > > > > On Fri, Jul 3, 2020 at 8:25 PM Matan Azrad <matan@mellanox.com>
> > > > wrote:
> > > > > > > From: Jerin Jacob:
> > > > > > > > When adding overlapping API(rte_eth_mirror_rule_set()) in
> > > > > > > > the same library(ethdev).
> > > > > > > > Please depreciate the old API.
> > > > > > > > We should not have two separate paths for the same function
> > > > > > > > in the same ethdev library. It is pain for app and driver developers.
> > > > > > >
> > > > > > > What are about all the other rte_flow parallel configuration
> > > > > > > APIs in
> > > > ethdev:
> > > > > > > promiscuous_enable;
> > > > > > > promiscuous_disable;
> > > > > > > allmulticast_enable;
> > > > > > > allmulticast_disable;
> > > > > > > mac_addr_remove;
> > > > > > > mac_addr_add;
> > > > > > > mac_addr_set;
> > > > > > > set_mc_addr_list;
> > > > > > > vlan_filter_set;
> > > > > > > vlan_tpid_set;
> > > > > > > vlan_strip_queue_set;
> > > > > > > vlan_offload_set;
> > > > > > > vlan_pvid_set;
> > > > > > > udp_tunnel_port_add;
> > > > > > > udp_tunnel_port_del;
> > > > > > > ...
> > > > > > >
> > > > > > > These APIs can be replaced easily by rte_flow API.
> > > > > > > Do you think we need to deprecate all?
> > > > > >
> > > > > > I think, basic stuff like below can have separate API.
> > > > > > 1) promiscuous_enable;
> > > > > > 2) promiscuous_disable;
> > > > > > 3) allmulticast_enable;
> > > > > > 4) allmulticast_disable;
> > > > > > 5) mac_addr_remove;
> > > > > > 6) mac_addr_add;
> > > > > > 7) mac_addr_set;
> > > > > > 8) set_mc_addr_list;
> > > > >
> > > > > "Basic" is not a precise definition :)
> > > >
> > > > Yep.
> > > >
> > > > > I would say port-level configuration should remain out of rte_flow
> > > > > API.
> > >
> > > Thomas, Can you explain what is port-level?
> > > Everything in rte_flow is per port...
> > >
> > > Also, can you give reasons for your claim?
> > >
> > > > +1.
> > > > In addition that, I would say anything needs to configured at "per-flow"
> > > > granularity use rte_flow.
> > >
> > > Jerin, What do you mean "per-flow" ?
> >
> > In Terms of NIC HW features, Typical HW will have
> > a) Basic "port" level configuration like
> > - enable/disable promiscuous
>
> What is "port level", everything in rte_flow is also per port...
>
> > b) Advance HW's will have CAM based flow filtering. IMO, CAM related stuff
> > should go to rte_flow.
>
> It is HW internal, I don't think all HWs use the same logic here.
> Since rte_flow is generic for all filtering methods, It is good candidate API for all HWs.
>
> > This is to enable, The very basic PMD(without advanced features) should
> > work with port level basic APIs(i.e without rte_flow)
>
> What is "basic"? Do you mean simple match and simple action?
> As I said, Also rte_flow is port level API - so "port level" is not good term here.
>
> As you said " When adding overlapping API(rte_eth_mirror_rule_set()) in the same library(ethdev). Please depreciate the old API."
>
> Different APIs to do the same thing is not good, especially in packet filtering:
> What should we do if we have conflicts?
> For example: legacy filtering APIs say to receive packet A and rte_flow says to drop it.
>
> Don't you think it complicates more the user API understanding, also the PMD implementations?
>
>
> > I have seen promiscuous, mac address handling is part of basic NIC HW(i.e
> > NICs without advanced CAM filters).
> > That's my reasoning for the split.
>
> As I said, the nic HW specific implementation should not affect the API.
> I don't think we need to split API and to complicate the user because of it.
>
> IMO, It is better to have one generic API for packet filtering:
> It is clearer, simpler, generic and classic.
> The user and PMD need to understand only one filtering API and that’s it (no need to combine between multiple filtering APIs).
>
> I know this is big change but we can do it in modular way.
> It reminds me the big change that was done in Rx\Tx offload configurations:
> So, when offload became more popular we modularly forced users to replace the offload API.
> Also here, flow filtering becomes popular so maybe this is the time(20.08-20.11) to force changes in the old APIs.
>
> > > Everything in traffic filtering\actions is per flow, for example:
> > > Promiscuous: flow create 0 ingress pattern eth / end actions queue
> > > index 0 / end
> >
> > IMO, it is not an accurate representation of promiscuous enable. It needs to
> > send the traffic to all queues and patterns is not just eth.
>
> Yes, if legacy RSS is configured we need to replace the above queue action by rss action as I wrote before.(did you read it just below?)
>
> So, we can add legacy RSS APIs to the list above...
I meant, If promiscuous enable, then what would be the pattern, Should
be limit just to "eth".
I leave up to ethdev maintainer to decide. Is promiscuous part of
rte_flow API or not? I dnt have a very strong objection.
For me, VLAN(rte_vlan_*) and MIRROR(rte_eth_mirror_rule_set()) are
most worrisome as each PMD need to duplicate that work
as both are CAM based API.
@@ -2645,6 +2645,31 @@ timeout passed without any matching on the flow.
| ``context`` | user input flow context |
+--------------+---------------------------------+
+Action: ``SAMPLE``
+^^^^^^^^^^^^^^^^^^
+
+Adds a sample action to a matched flow.
+
+The matching packets will be duplicated to a special queue or vport
+with the predefined ``ratio``, the packets sampled equals is '1/ratio'.
+All the packets continues to the target destination.
+
+When the ``ratio`` is set to 1 then the packets will be 100% mirrored.
+``actions`` represent the different set of actions for the sampled or mirrored
+packets.
+
+.. _table_rte_flow_action_sample:
+
+.. table:: SAMPLE
+
+ +--------------+---------------------------------+
+ | Field | Value |
+ +==============+=================================+
+ | ``ratio`` | 32 bits sample ratio value |
+ +--------------+---------------------------------+
+ | ``actions`` | sub-action list for sampling |
+ +--------------+---------------------------------+
+
Negative types
~~~~~~~~~~~~~~
@@ -81,6 +81,12 @@ New Features
* Added support for virtio queue statistics.
* Added support for MTU update.
+* **Added flow-based traffic sampling support.**
+
+ Added new action: ``RTE_FLOW_ACTION_TYPE_SAMPLE`` to duplicate the matching
+ packets with given ratio and redirects to vport or queue. The sampled packets
+ also can be assigned with an additional optional actions.
+
* **Updated Marvell octeontx2 ethdev PMD.**
Updated Marvell octeontx2 driver with cn98xx support.
@@ -173,6 +173,7 @@ struct rte_flow_desc_data {
MK_FLOW_ACTION(SET_IPV4_DSCP, sizeof(struct rte_flow_action_set_dscp)),
MK_FLOW_ACTION(SET_IPV6_DSCP, sizeof(struct rte_flow_action_set_dscp)),
MK_FLOW_ACTION(AGE, sizeof(struct rte_flow_action_age)),
+ MK_FLOW_ACTION(SAMPLE, sizeof(struct rte_flow_action_sample)),
};
int
@@ -2099,6 +2099,13 @@ enum rte_flow_action_type {
* see enum RTE_ETH_EVENT_FLOW_AGED
*/
RTE_FLOW_ACTION_TYPE_AGE,
+
+ /**
+ * Redirects specific ratio of packets to vport or queue.
+ *
+ * See struct rte_flow_action_sample.
+ */
+ RTE_FLOW_ACTION_TYPE_SAMPLE,
};
/**
@@ -2709,6 +2716,27 @@ struct rte_flow_action {
struct rte_flow;
/**
+ * @warning
+ * @b EXPERIMENTAL: this structure may change without prior notice
+ *
+ * RTE_FLOW_ACTION_TYPE_SAMPLE
+ *
+ * Adds a sample action to a matched flow.
+ *
+ * The matching packets will be duplicated to a special queue or vport
+ * in the predefined probabiilty, All the packets continues processing
+ * on the default flow path.
+ *
+ * When the sample ratio is set to 1 then the packets will be 100% mirrored.
+ * Additional action list be supported to add for sampled or mirrored packets.
+ */
+struct rte_flow_action_sample {
+ const uint32_t ratio; /**< packets sampled equals to '1/ratio'. */
+ const struct rte_flow_action *actions;
+ /**< sub-action list specific for the sampling hit cases. */
+};
+
+/**
* Verbose error types.
*
* Most of them provide the type of the object referenced by struct