[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <a0b35837-375b-4650-9e68-ef4883c0d0c9@ovn.org>
Date: Mon, 24 Jun 2024 23:40:19 +0200
From: Ilya Maximets <i.maximets@....org>
To: Adrian Moreno <amorenoz@...hat.com>, netdev@...r.kernel.org
Cc: i.maximets@....org, aconole@...hat.com, echaudro@...hat.com,
horms@...nel.org, dev@...nvswitch.org, Yotam Gigi <yotam.gi@...il.com>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Jamal Hadi Salim <jhs@...atatu.com>, Cong Wang <xiyou.wangcong@...il.com>,
Jiri Pirko <jiri@...nulli.us>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v3 04/10] net: psample: allow using rate as
probability
On 6/19/24 23:00, Adrian Moreno wrote:
> Although not explicitly documented in the psample module itself, the
> definition of PSAMPLE_ATTR_SAMPLE_RATE seems inherited from act_sample.
>
> Quoting tc-sample(8):
> "RATE of 100 will lead to an average of one sampled packet out of every
> 100 observed."
>
> With this semantics, the rates that we can express with an unsigned
> 32-bits number are very unevenly distributed and concentrated towards
> "sampling few packets".
> For example, we can express a probability of 2.32E-8% but we
> cannot express anything between 100% and 50%.
>
> For sampling applications that are capable of sampling a decent
> amount of packets, this sampling rate semantics is not very useful.
>
> Add a new flag to the uAPI that indicates that the sampling rate is
> expressed in scaled probability, this is:
> - 0 is 0% probability, no packets get sampled.
> - U32_MAX is 100% probability, all packets get sampled.
>
> Signed-off-by: Adrian Moreno <amorenoz@...hat.com>
> ---
> include/net/psample.h | 3 ++-
> include/uapi/linux/psample.h | 10 +++++++++-
> include/uapi/linux/tc_act/tc_sample.h | 1 +
> net/psample/psample.c | 3 +++
> 4 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/psample.h b/include/net/psample.h
> index 2ac71260a546..c52e9ebd88dd 100644
> --- a/include/net/psample.h
> +++ b/include/net/psample.h
> @@ -24,7 +24,8 @@ struct psample_metadata {
> u8 out_tc_valid:1,
> out_tc_occ_valid:1,
> latency_valid:1,
> - unused:5;
> + rate_as_probability:1,
> + unused:4;
> const u8 *user_cookie;
> u32 user_cookie_len;
> };
> diff --git a/include/uapi/linux/psample.h b/include/uapi/linux/psample.h
> index e80637e1d97b..b765f0e81f20 100644
> --- a/include/uapi/linux/psample.h
> +++ b/include/uapi/linux/psample.h
> @@ -8,7 +8,11 @@ enum {
> PSAMPLE_ATTR_ORIGSIZE,
> PSAMPLE_ATTR_SAMPLE_GROUP,
> PSAMPLE_ATTR_GROUP_SEQ,
> - PSAMPLE_ATTR_SAMPLE_RATE,
> + PSAMPLE_ATTR_SAMPLE_RATE, /* u32, ratio between observed and
> + * sampled packets or scaled probability
> + * if PSAMPLE_ATTR_SAMPLE_PROBABILITY
> + * is set.
> + */
> PSAMPLE_ATTR_DATA,
> PSAMPLE_ATTR_GROUP_REFCOUNT,
> PSAMPLE_ATTR_TUNNEL,
> @@ -20,6 +24,10 @@ enum {
> PSAMPLE_ATTR_TIMESTAMP, /* u64, nanoseconds */
> PSAMPLE_ATTR_PROTO, /* u16 */
> PSAMPLE_ATTR_USER_COOKIE, /* binary, user provided data */
> + PSAMPLE_ATTR_SAMPLE_PROBABILITY,/* no argument, interpret rate in
> + * PSAMPLE_ATTR_SAMPLE_RATE as a
> + * probability scaled 0 - U32_MAX.
> + */
>
> __PSAMPLE_ATTR_MAX
> };
> diff --git a/include/uapi/linux/tc_act/tc_sample.h b/include/uapi/linux/tc_act/tc_sample.h
> index fee1bcc20793..7ee0735e7b38 100644
> --- a/include/uapi/linux/tc_act/tc_sample.h
> +++ b/include/uapi/linux/tc_act/tc_sample.h
> @@ -18,6 +18,7 @@ enum {
> TCA_SAMPLE_TRUNC_SIZE,
> TCA_SAMPLE_PSAMPLE_GROUP,
> TCA_SAMPLE_PAD,
> + TCA_SAMPLE_PROBABILITY,
> __TCA_SAMPLE_MAX
> };
> #define TCA_SAMPLE_MAX (__TCA_SAMPLE_MAX - 1)
Not a full review yet, but this hunk seems unrelated to the set
as you're not adding rate_as_probability to act_sample.
I suppose, it was part of the plan at some point, but then
Best regards, Ilya Maximets.
Powered by blists - more mailing lists