On 7/14/20 7:52 PM, Ilya Maximets wrote:
> On 7/14/20 5:33 PM, Kevin Traynor wrote:
>> On 09/07/2020 16:19, Ilya Maximets wrote:
>>> Main thread will try to pause/stop all revalidators during datapath
>>> reconfiguration via datapath purge callback (dp_purge_cb) while
>>> holding 'dp->port_mutex'. And deadlock happens in case any of
>>> revalidator threads is already waiting on 'dp->port_mutex' while
>>> dumping offloaded flows:
>>>
>>> main thread revalidator
>>> --------------------------------- ----------------------------------
>>>
>>> ovs_mutex_lock(&dp->port_mutex)
>>>
>>> dpif_netdev_flow_dump_next()
>>> -> dp_netdev_flow_to_dpif_flow
>>> -> get_dpif_flow_status
>>> -> dpif_netdev_get_flow_offload_status()
>>> -> ovs_mutex_lock(&dp->port_mutex)
>>> <waiting for mutex here>
>>>
>>> reconfigure_datapath()
>>> -> reconfigure_pmd_threads()
>>> -> dp_netdev_del_pmd()
>>> -> dp_purge_cb()
>>> -> udpif_pause_revalidators()
>>> -> ovs_barrier_block(&udpif->pause_barrier)
>>> <waiting for revalidators to reach barrier>
>>>
>>> <DEADLOCK>
>>>
>>> We're not allowed to call offloading API without holding global
>>> port mutex from the userspace datapath due to thread safety
>>> restrictions on netdev-ofload-dpdk module. And it's also not easy
>>
>> typo s/ofload/offload/
>
> Thanks.
>
>>
>>> to rework datapath reconfiguration process in order to move actual
>>> PMD removal and datapath purge out of the port mutex.
>>>
>>> So, for now, not sleeping on a mutex if it's not immediately available
>>> seem like an easiest workaround. This will have impact on flow
>>> statistics update rate and on ability to get the latest statistics
>>> before removing the flow (latest stats will be lost in case we were
>>> not able to take the mutex). However, this will allow us to operate
>>> normally avoiding the deadlock.
>>>
>>> The last bit is that to avoid flapping of flow attributes and
>>> statistics we're not failing the operation, but returning last
>>> statistics and attributes returned by offload provider. Since those
>>> might be updated in different threads, stores and reads are atomic.
>>>
>>
>> Nice explanation, thanks
>>
>>> Reported-by: Frank Wang (王培辉) <wangpei...@inspur.com>
>>> Reported-at:
>>> https://mail.openvswitch.org/pipermail/ovs-dev/2020-June/371753.html
>>> Fixes: a309e4f52660 ("dpif-netdev: Update offloaded flows statistics.")
>>> Signed-off-by: Ilya Maximets <i.maxim...@ovn.org>
>>> ---
>>>
>>> Tested with unit tests only. Needs checking with the real setup.
>>> Especially stats/attrs update parts.
>>>
>>> AUTHORS.rst | 1 +
>>> lib/dpif-netdev.c | 78 +++++++++++++++++++++++++++++++++++++++++++----
>>> 2 files changed, 73 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/AUTHORS.rst b/AUTHORS.rst
>>> index 8e6a0769f..eb36a01d0 100644
>>> --- a/AUTHORS.rst
>>> +++ b/AUTHORS.rst
>>> @@ -504,6 +504,7 @@ Edwin Chiu ec...@vmware.com
>>> Eivind Bulie Haanaes
>>> Enas Ahmad enas.ah...@kaust.edu.sa
>>> Eric Lopez
>>> +Frank Wang (王培辉) wangpei...@inspur.com
>>> Frido Roose fr.ro...@gmail.com
>>> Gaetano Catalli gaetano.cata...@gmail.com
>>> Gavin Remaley gavin_rema...@selinc.com
>>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>>> index 629a0cb53..2eb7f32ff 100644
>>> --- a/lib/dpif-netdev.c
>>> +++ b/lib/dpif-netdev.c
>>> @@ -490,6 +490,12 @@ struct dp_netdev_flow_stats {
>>> atomic_uint16_t tcp_flags; /* Bitwise-OR of seen tcp_flags values.
>>> */
>>> };
>>>
>>> +/* Contained by struct dp_netdev_flow's 'last_attrs' member. */
>>> +struct dp_netdev_flow_attrs {
>>> + atomic_bool offloaded; /* True if flow is offloaded to HW. */
>>> + ATOMIC(const char *) dp_layer; /* DP layer the flow is handled in. */
>>> +};
>>> +
>>> /* A flow in 'dp_netdev_pmd_thread's 'flow_table'.
>>> *
>>> *
>>> @@ -550,6 +556,10 @@ struct dp_netdev_flow {
>>> /* Statistics. */
>>> struct dp_netdev_flow_stats stats;
>>>
>>> + /* Statistics and attributes received from the netdev offload
>>> provider. */
>>> + struct dp_netdev_flow_stats last_stats;
>>> + struct dp_netdev_flow_attrs last_attrs;
>>> +
>>> /* Actions. */
>>> OVSRCU_TYPE(struct dp_netdev_actions *) actions;
>>>
>>> @@ -3150,9 +3160,43 @@ dp_netdev_pmd_find_flow(const struct
>>> dp_netdev_pmd_thread *pmd,
>>> return NULL;
>>> }
>>>
>>> +static void
>>> +dp_netdev_flow_set_last_stats_attrs(struct dp_netdev_flow *netdev_flow,
>>> + const struct dpif_flow_stats *stats,
>>> + const struct dpif_flow_attrs *attrs)
>>> +{
>>> + struct dp_netdev_flow_stats *last_stats = &netdev_flow->last_stats;
>>> + struct dp_netdev_flow_attrs *last_attrs = &netdev_flow->last_attrs;
>>> +
>>> + atomic_store_relaxed(&last_stats->used, stats->used);
>>> + atomic_store_relaxed(&last_stats->packet_count, stats->n_packets);
>>> + atomic_store_relaxed(&last_stats->byte_count, stats->n_bytes);
>>> + atomic_store_relaxed(&last_stats->tcp_flags, stats->tcp_flags);
>>> +
>>> + atomic_store_relaxed(&last_attrs->offloaded, attrs->offloaded);
>>> + atomic_store_relaxed(&last_attrs->dp_layer, attrs->dp_layer);
>>> +}
>>> +
>>> +static void
>>> +dp_netdev_flow_get_last_stats_attrs(struct dp_netdev_flow *netdev_flow,
>>> + struct dpif_flow_stats *stats,
>>> + struct dpif_flow_attrs *attrs)
>>> +{
>>> + struct dp_netdev_flow_stats *last_stats = &netdev_flow->last_stats;
>>> + struct dp_netdev_flow_attrs *last_attrs = &netdev_flow->last_attrs;
>>> +
>>> + atomic_read_relaxed(&last_stats->used, &stats->used);
>>> + atomic_read_relaxed(&last_stats->packet_count, &stats->n_packets);
>>> + atomic_read_relaxed(&last_stats->byte_count, &stats->n_bytes);
>>> + atomic_read_relaxed(&last_stats->tcp_flags, &stats->tcp_flags);
>>> +
>>> + atomic_read_relaxed(&last_attrs->offloaded, &attrs->offloaded);
>>> + atomic_read_relaxed(&last_attrs->dp_layer, &attrs->dp_layer);
>>> +}
>>> +
>>> static bool
>>> dpif_netdev_get_flow_offload_status(const struct dp_netdev *dp,
>>> - const struct dp_netdev_flow
>>> *netdev_flow,
>>> + struct dp_netdev_flow *netdev_flow,
>>> struct dpif_flow_stats *stats,
>>> struct dpif_flow_attrs *attrs)
>>> {
>>> @@ -3175,11 +3219,31 @@ dpif_netdev_get_flow_offload_status(const struct
>>> dp_netdev *dp,
>>> }
>>> ofpbuf_use_stack(&buf, &act_buf, sizeof act_buf);
>>> /* Taking a global 'port_mutex' to fulfill thread safety
>>> - * restrictions for the netdev-offload-dpdk module. */
>>> - ovs_mutex_lock(&dp->port_mutex);
>>> - ret = netdev_flow_get(netdev, &match, &actions,
>>> &netdev_flow->mega_ufid,
>>> - stats, attrs, &buf);
>>> - ovs_mutex_unlock(&dp->port_mutex);
>>> + * restrictions for the netdev-offload-dpdk module.
>>> + *
>>> + * XXX: Main thread will try to pause/stop all revalidators during
>>> datapath
>>> + * reconfiguration via datapath purge callback (dp_purge_cb) while
>>> + * holding 'dp->port_mutex'. So we're not waiting for mutex here.
>>> + * Otherwise, deadlock is possible, bcause revalidators might
>>> sleep
>>> + * waiting for the main thread to release the lock and main thread
>>> + * will wait for them to stop processing.
>>> + * This workaround might make statistics less accurate. Especially
>>> + * for flow deletion case, since there will be no other attempt.
>>> */
>>> + if (!ovs_mutex_trylock(&dp->port_mutex)) {
>>> + ret = netdev_flow_get(netdev, &match, &actions,
>>> + &netdev_flow->mega_ufid, stats, attrs, &buf);
>>> + /* Storing attributes from the last request for later use on mutex
>>> + * contention. */
>>
>> Shouldn't you check 'ret' here before the set
>
> Yes. That makes sense. I'll move below call under 'if (!ret)'.
Another option is to actually store 'ret' along with last_{stats,attrs}.
And return last return value that we actually had. That will protect
us from flapping in case the flow dissapeared from the HW (for example
with linux tc this might happen if someone removed flows from qdisk
externally). In case we're storing only on success, this will result
in returniing error if we took the mutex and sucess if we didn't but have
good attributes and statistics from one of the previous calls.
What do you think?
>
>>
>>> + dp_netdev_flow_set_last_stats_attrs(netdev_flow, stats, attrs);
>>> + ovs_mutex_unlock(&dp->port_mutex);
>>> + } else {
>>> + dp_netdev_flow_get_last_stats_attrs(netdev_flow, stats, attrs);
>>> + if (!attrs->dp_layer) {
>>> + /* Flow was never reported as 'offloaded' so it's harmless
>>> + * to continue to think so. */
>>> + ret = EAGAIN;
>>> + }
>>> + }
>>> netdev_close(netdev);
>>> if (ret) {
>>> return false;
>>> @@ -3448,6 +3512,8 @@ dp_netdev_flow_add(struct dp_netdev_pmd_thread *pmd,
>>> /* Do not allocate extra space. */
>>> flow = xmalloc(sizeof *flow - sizeof flow->cr.flow.mf + mask.len);
>>> memset(&flow->stats, 0, sizeof flow->stats);
>>> + memset(&flow->last_stats, 0, sizeof flow->last_stats);
>>> + memset(&flow->last_attrs, 0, sizeof flow->last_attrs);
>>> flow->dead = false;
>>> flow->batch = NULL;
>>> flow->mark = INVALID_FLOW_MARK;
>>>
>>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
