Re: [PATCH net-next v3 04/10] net: psample: allow using rate as probability

[Ilya Maximets]

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.