diff mbox series

[RFC,net-next,v2,4/5] net:sched:act_sample: add action cookie to sample

Message ID 20240408125753.470419-5-amorenoz@redhat.com (mailing list archive)
State RFC
Delegated to: Netdev Maintainers
Headers show
Series net: openvswitch: Add sample multicasting. | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 942 this patch: 942
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 4 maintainers not CCed: jhs@mojatatu.com pabeni@redhat.com kuba@kernel.org edumazet@google.com
netdev/build_clang success Errors and warnings before: 953 this patch: 953
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 953 this patch: 953
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 27 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Adrián Moreno April 8, 2024, 12:57 p.m. UTC
If the action has a user_cookie, pass it along to the sample so it can
be easily identified.

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 net/sched/act_sample.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Ilya Maximets April 8, 2024, 1:20 p.m. UTC | #1
[copying my previous reply since this version actually has netdev@ in Cc]

On 4/8/24 14:57, Adrian Moreno wrote:
> If the action has a user_cookie, pass it along to the sample so it can
> be easily identified.
> 
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
>  net/sched/act_sample.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)

Nit: some spaces in the subject would be nice.

> 
> diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
> index a69b53d54039..5c3f86ec964a 100644
> --- a/net/sched/act_sample.c
> +++ b/net/sched/act_sample.c
> @@ -165,9 +165,11 @@ TC_INDIRECT_SCOPE int tcf_sample_act(struct sk_buff *skb,
>  				     const struct tc_action *a,
>  				     struct tcf_result *res)
>  {
> +	u8 cookie_data[TC_COOKIE_MAX_SIZE] = {};
>  	struct tcf_sample *s = to_sample(a);
>  	struct psample_group *psample_group;
>  	struct psample_metadata md = {};
> +	struct tc_cookie *user_cookie;
>  	int retval;
>  
>  	tcf_lastuse_update(&s->tcf_tm);
> @@ -189,6 +191,16 @@ TC_INDIRECT_SCOPE int tcf_sample_act(struct sk_buff *skb,
>  		if (skb_at_tc_ingress(skb) && tcf_sample_dev_ok_push(skb->dev))
>  			skb_push(skb, skb->mac_len);
>  
> +		rcu_read_lock();
> +		user_cookie = rcu_dereference(a->user_cookie);
> +		if (user_cookie) {
> +			memcpy(cookie_data, user_cookie->data,
> +			       user_cookie->len);
> +			md.user_cookie = cookie_data;
> +			md.user_cookie_len = user_cookie->len;
> +		}
> +		rcu_read_unlock();

Not sure what is better - extending rcu critical section
beyond psample_sample_packet() or copying the cookie...
What do you think?

> +
>  		md.trunc_size = s->truncate ? s->trunc_size : skb->len;
>  		psample_sample_packet(psample_group, skb, s->rate, &md);
>
Adrián Moreno April 11, 2024, 8:40 a.m. UTC | #2
On 4/8/24 15:20, Ilya Maximets wrote:
> [copying my previous reply since this version actually has netdev@ in Cc]
> 
> On 4/8/24 14:57, Adrian Moreno wrote:
>> If the action has a user_cookie, pass it along to the sample so it can
>> be easily identified.
>>
>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>> ---
>>   net/sched/act_sample.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
> 
> Nit: some spaces in the subject would be nice.
> 
>>
>> diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
>> index a69b53d54039..5c3f86ec964a 100644
>> --- a/net/sched/act_sample.c
>> +++ b/net/sched/act_sample.c
>> @@ -165,9 +165,11 @@ TC_INDIRECT_SCOPE int tcf_sample_act(struct sk_buff *skb,
>>   				     const struct tc_action *a,
>>   				     struct tcf_result *res)
>>   {
>> +	u8 cookie_data[TC_COOKIE_MAX_SIZE] = {};
>>   	struct tcf_sample *s = to_sample(a);
>>   	struct psample_group *psample_group;
>>   	struct psample_metadata md = {};
>> +	struct tc_cookie *user_cookie;
>>   	int retval;
>>   
>>   	tcf_lastuse_update(&s->tcf_tm);
>> @@ -189,6 +191,16 @@ TC_INDIRECT_SCOPE int tcf_sample_act(struct sk_buff *skb,
>>   		if (skb_at_tc_ingress(skb) && tcf_sample_dev_ok_push(skb->dev))
>>   			skb_push(skb, skb->mac_len);
>>   
>> +		rcu_read_lock();
>> +		user_cookie = rcu_dereference(a->user_cookie);
>> +		if (user_cookie) {
>> +			memcpy(cookie_data, user_cookie->data,
>> +			       user_cookie->len);
>> +			md.user_cookie = cookie_data;
>> +			md.user_cookie_len = user_cookie->len;
>> +		}
>> +		rcu_read_unlock();
> 
> Not sure what is better - extending rcu critical section
> beyond psample_sample_packet() or copying the cookie...
> What do you think?

Ha! I also scratched my head on this one.

I tried to follow all the possible paths and, as far as I could tell, there is 
no explicit sleeping involved. However, there is a call to yield() towards the 
end of netlink_broadcast_filtered() that made me think it's safer to copy the 
cookie which, in this particular case, is fairly limited in size.

Have I missed something?

> 
>> +
>>   		md.trunc_size = s->truncate ? s->trunc_size : skb->len;
>>   		psample_sample_packet(psample_group, skb, s->rate, &md);
>>   
>
diff mbox series

Patch

diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
index a69b53d54039..5c3f86ec964a 100644
--- a/net/sched/act_sample.c
+++ b/net/sched/act_sample.c
@@ -165,9 +165,11 @@  TC_INDIRECT_SCOPE int tcf_sample_act(struct sk_buff *skb,
 				     const struct tc_action *a,
 				     struct tcf_result *res)
 {
+	u8 cookie_data[TC_COOKIE_MAX_SIZE] = {};
 	struct tcf_sample *s = to_sample(a);
 	struct psample_group *psample_group;
 	struct psample_metadata md = {};
+	struct tc_cookie *user_cookie;
 	int retval;
 
 	tcf_lastuse_update(&s->tcf_tm);
@@ -189,6 +191,16 @@  TC_INDIRECT_SCOPE int tcf_sample_act(struct sk_buff *skb,
 		if (skb_at_tc_ingress(skb) && tcf_sample_dev_ok_push(skb->dev))
 			skb_push(skb, skb->mac_len);
 
+		rcu_read_lock();
+		user_cookie = rcu_dereference(a->user_cookie);
+		if (user_cookie) {
+			memcpy(cookie_data, user_cookie->data,
+			       user_cookie->len);
+			md.user_cookie = cookie_data;
+			md.user_cookie_len = user_cookie->len;
+		}
+		rcu_read_unlock();
+
 		md.trunc_size = s->truncate ? s->trunc_size : skb->len;
 		psample_sample_packet(psample_group, skb, s->rate, &md);