From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 8B1BCA00C5; Mon, 6 Jul 2020 01:55:10 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 5EBE51D72A; Mon, 6 Jul 2020 01:55:09 +0200 (CEST) Received: from mail-oi1-f194.google.com (mail-oi1-f194.google.com [209.85.167.194]) by dpdk.org (Postfix) with ESMTP id D63F51D5C5 for ; Mon, 6 Jul 2020 01:55:07 +0200 (CEST) Received: by mail-oi1-f194.google.com with SMTP id t198so18195700oie.7 for ; Sun, 05 Jul 2020 16:55:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=broadcom.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=XRvDVzxSjmLkHSfpvIswDcZMPmf6mpJdzmQsUFgcOeE=; b=ekZGstFhkc0M5hh8In8LKdfDSEBIVBDCbfH4bRa/KHhqckyd6e6qUL0nNjXq34WoPQ GtB0tj5nJgBHP5Q2dN0AWE638MZeyIiPVd1vtiJUKw+RCMBDdBP3KhAIR0beYL3qMcTt /Ztmr9x5xiUMzKYfIsyjkF3zQzARBfxpIDGWE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=XRvDVzxSjmLkHSfpvIswDcZMPmf6mpJdzmQsUFgcOeE=; b=HiVq3ryLT27WJWhQTeSngPn541tTSlWBHcOG9RS9mrmeEosnKgCcJyzvwdG36Ta49u q+WdgxzZKpTmpSKbhoyoZ6ypDpXJFdZCjSM9FRJaygRKFa060A3lqvfoUePMMNqhLK0U ayDD0+J3GQ/L2V45Ey4uU3fOr+AeLRHZ/joIwThrnWiheP6YPyqoupmTV3lJT2yhWjKD C55W/9GH/y3VZ+fbUd2fncc9kBg9F1msLDz0b/R8gZ5Mxk0W1tXE7EJotisdgV78fnNN 67EpNGT9obr6SPSeHMfSgwbdTmJ9L38ruafAeX3SF14RX6eJOvtgomIz8WZfipV4rvME kLsg== X-Gm-Message-State: AOAM530ctDM04xL5IG0ryk1ktAIArMtsaPNRWp7MoHhQ81FqodY9Ft6m qOyKRqWh+o4BWidRayCaO+z3MnSCLoJ2wt9R1RGGMw== X-Google-Smtp-Source: ABdhPJwGPZFSltyIYXLVjyfq9WkMasam1l6y/lheyUkHSG8nrhd8LhrnniLZvFtDWEhJKjLg9MTo2B3BCyXiF25ydhM= X-Received: by 2002:aca:e1d6:: with SMTP id y205mr14860225oig.179.1593993306832; Sun, 05 Jul 2020 16:55:06 -0700 (PDT) MIME-Version: 1.0 References: <1593102379-400132-1-git-send-email-jiaweiw@mellanox.com> <1593715390-83047-1-git-send-email-jiaweiw@mellanox.com> <1593715390-83047-2-git-send-email-jiaweiw@mellanox.com> In-Reply-To: From: Ajit Khaparde Date: Sun, 5 Jul 2020 16:54:50 -0700 Message-ID: To: Ori Kam Cc: Andrew Rybchenko , "Jiawei(Jonny) Wang" , Slava Ovsiienko , Matan Azrad , "dev@dpdk.org" , Thomas Monjalon , Raslan Darawsheh , "ian.stokes@intel.com" , "fbl@redhat.com" Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH v2 1/7] ethdev: introduce sample action for rte flow X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Sun, Jul 5, 2020 at 3:19 AM Ori Kam wrote: > Hi Andrew, > > I replied to some of your comments. > Best, > Ori > > -----Original Message----- > > From: dev 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 > > > Acked-by: Ori Kam > > > --- > > > 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 > > > > >