[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <91906B2A-FE02-4757-99A8-12F57F2AFCAD@redhat.com>
Date: Thu, 15 Oct 2020 19:06:21 +0200
From: "Eelco Chaudron" <echaudro@...hat.com>
To: "Sebastian Andrzej Siewior" <bigeasy@...utronix.de>
Cc: netdev@...r.kernel.org, davem@...emloft.net, dev@...nvswitch.org,
kuba@...nel.org, pabeni@...hat.com, pshelar@....org,
jlelli@...hat.com, "Peter Zijlstra" <peterz@...radead.org>,
tglx@...utronix.de
Subject: Re: [PATCH net v2] net: openvswitch: fix to make sure flow_lookup()
is not preempted
On 15 Oct 2020, at 14:34, Sebastian Andrzej Siewior wrote:
> On 2020-10-15 11:46:53 [+0200], Eelco Chaudron wrote:
>> The flow_lookup() function uses per CPU variables, which must not be
>> preempted. However, this is fine in the general napi use case where
>> the local BH is disabled. But, it's also called in the netlink
>> context, which is preemptible. The below patch makes sure that even
>> in the netlink path, preemption is disabled.
>
> I would suggest to rephrase it: the term preemption usually means
> preempt_disable(). A preempt disabled section can be preempted /
> interrupted by hardirq and softirq. The later is mentioned and I think
> is confusing.
>
>> In addition, the u64_stats_update_begin() sync point was not
>> protected,
>> making the sync point part of the per CPU variable fixed this.
>
> I would rephrase it and mention the key details:
> u64_stats_update_begin() requires a lock to ensure one writer which is
> not ensured here. Making it per-CPU and disabling NAPI (softirq)
> ensures
> that there is always only one writer.
>
> Regarding the annotation which were mentioned here in the thread.
> Basically the this_cpu_ptr() warning worked as expected and got us
> here.
> I don't think it is wise to add annotation distinguished from the
> actual
> problem like assert_the_softirq_is_switched_off() in flow_lookup().
> The
> assert may become obsolete once the reason is removed and gets
> overseen
> and remains in the code. The commits
>
> c60c32a577561 ("posix-cpu-timers: Remove
> lockdep_assert_irqs_disabled()")
> f9dae5554aed4 ("dpaa2-eth: Remove preempt_disable() from
> seed_pool()")
>
> are just two examples which came to mind while writing this.
>
> Instead I would prefer lockdep annotation in u64_stats_update_begin()
> which is around also in 64bit kernels and complains if it is seen
> without disabled BH if observed in-serving-softirq.
> PeterZ, wasn't this mentioned before?
>
>> --- a/net/openvswitch/flow_table.c
>> +++ b/net/openvswitch/flow_table.c
>> @@ -851,9 +852,17 @@ struct sw_flow *ovs_flow_tbl_lookup(struct
>> flow_table *tbl,
>> struct mask_array *ma = rcu_dereference_ovsl(tbl->mask_array);
>> u32 __always_unused n_mask_hit;
>> u32 __always_unused n_cache_hit;
>> + struct sw_flow *flow;
>> u32 index = 0;
>>
>> - return flow_lookup(tbl, ti, ma, key, &n_mask_hit, &n_cache_hit,
>> &index);
>> + /* This function gets called trough the netlink interface and
>> therefore
>> + * is preemptible. However, flow_lookup() function needs to be
>> called
>> + * with preemption disabled due to CPU specific variables.
>
> preemption vs BH.
>
>> + */
>> + local_bh_disable();
>> + flow = flow_lookup(tbl, ti, ma, key, &n_mask_hit, &n_cache_hit,
>> &index);
>> + local_bh_enable();
>> + return flow;
>> }
>>
>> struct sw_flow *ovs_flow_tbl_lookup_exact(struct flow_table *tbl,
>
> Otherwise it looks good.
>
Thanks for your review! Made the modifications you suggested and will
send out a v3 soon.
//Eelco
Powered by blists - more mailing lists