Re: [PATCH net-next v3 04/10] net: psample: allow using rate as probability
From: Ilya Maximets
Date: Mon Jun 24 2024 - 17:40:37 EST
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@xxxxxxxxxx>
> ---
> 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.