On 2025-03-10 15:17:17 [+0100], Ilya Maximets wrote:
> > --- a/net/openvswitch/actions.c
> > +++ b/net/openvswitch/actions.c
> > @@ -82,6 +82,8 @@ struct ovs_action {
> > struct action_fifo action_fifos;
> > struct action_flow_keys flow_keys;
> > int exec_level;
> > + struct task_struct *owner;
> > + local_lock_t bh_lock;
> > };
> >
> > static DEFINE_PER_CPU(struct ovs_action, ovs_actions);
> > @@ -1690,8 +1692,14 @@ int ovs_execute_actions(struct datapath *dp, struct
> > sk_buff *skb,
> > const struct sw_flow_actions *acts,
> > struct sw_flow_key *key)
> > {
> > + struct ovs_action *ovs_act = this_cpu_ptr(&ovs_actions);
> > int err, level;
> >
> > + if (ovs_act->owner != current) {
> > + local_lock_nested_bh(&ovs_actions.bh_lock);
>
> Wouldn't this cause a warning when we're in a syscall/process context?
My understanding is that is only invoked in softirq context. Did I
misunderstood it? Otherwise that this_cpu_ptr() above should complain
that preemption is not disabled and if preemption is indeed not disabled
how do you ensure that you don't get preempted after the
__this_cpu_inc_return() in several tasks (at the same time) leading to
exceeding the OVS_RECURSION_LIMIT?
> We will also be taking a spinlock in a general case here, which doesn't
> sound particularly great, since we can potentially be holding it for a
> long time and it's also not free to take/release on this hot path.
> Is there a version of this lock that's a no-op on non-RT?
local_lock_nested_bh() does not acquire any lock on !PREEMPT_RT. It only
verifies that in_softirq() is true.
> > + ovs_act->owner = current;
> > + }
> > +
> > level = __this_cpu_inc_return(ovs_actions.exec_level);
> > if (unlikely(level > OVS_RECURSION_LIMIT)) {
> > net_crit_ratelimited("ovs: recursion limit reached on datapath
> > %s, probable configuration error\n",
> > @@ -1710,5 +1718,10 @@ int ovs_execute_actions(struct datapath *dp, struct
> > sk_buff *skb,
> >
> > out:
> > __this_cpu_dec(ovs_actions.exec_level);
> > +
> > + if (level == 1) {
> > + ovs_act->owner = NULL;
> > + local_unlock_nested_bh(&ovs_actions.bh_lock);
> > + }
>
> Seems dangerous to lock every time the owner changes but unlock only
> once on level 1. Even if this works fine, it seems unnecessarily
> complicated. Maybe it's better to just lock once before calling
> ovs_execute_actions() instead?
My understanding is this can be invoked recursively. That means on first
invocation owner == NULL and then you acquire the lock at which point
exec_level goes 0->1. On the recursive invocation owner == current and
you skip the lock but exec_level goes 1 -> 2.
On your return path once level becomes 1, then it means that dec made it
go 1 -> 0, you unlock the lock.
The locking part happens only on PREEMPT_RT because !PREEMPT_RT has
softirqs disabled which guarantee that there will be no preemption.
tools/testing/selftests/net/openvswitch should cover this?
> Also, the name of the struct ovs_action doesn't make a lot of sense,
> I'd suggest to call it pcpu_storage or something like that instead.
> I.e. have a more generic name as the fields inside are not directly
> related to each other.
Understood. ovs_pcpu_storage maybe?
> Best regards, Ilya Maximets.
Sebastian
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
