On 15/07/2020 11:02, Ilya Maximets wrote:
> On 7/15/20 11:44 AM, Kevin Traynor wrote:
>> On 14/07/2020 19:24, Ilya Maximets wrote:
>>> 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:
>>>>>> @@ -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?
>>>
>>
>> Not sure if you're saying you don't want to update the stats/attrs
>> anymore past last good value in case you got a ret error from flow_get()
>> *anytime* in the past?
>
> Sorry for not being clear. I meant something like this:
>
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -557,6 +559,7 @@ struct dp_netdev_flow {
> struct dp_netdev_flow_stats stats;
>
> /* Statistics and attributes received from the netdev offload provider.
> */
> + atomic_int netdev_flow_get_result;
> struct dp_netdev_flow_stats last_stats;
> struct dp_netdev_flow_attrs last_attrs;
>
> @@ -3163,11 +3291,17 @@ dp_netdev_pmd_find_flow(const struct
> dp_netdev_pmd_thread *pmd,
> 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)
> + const struct dpif_flow_attrs *attrs,
> + int result)
> {
> 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(&netdev_flow->netdev_flow_get_result, result);
> + if (result) {
> + return;
> + }
> +
> 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);
> @@ -3175,16 +3309,23 @@ dp_netdev_flow_set_last_stats_attrs(struct
> dp_netdev_flow *netdev_flow,
>
> 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 dpif_flow_attrs *attrs,
> + int *result)
> {
> 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(&netdev_flow->netdev_flow_get_result, result);
> + if (*result) {
> + return;
> + }
> +
> 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);
> @@ -3232,13 +3373,13 @@ dpif_netdev_get_flow_offload_status(const struct
> dp_netdev *dp,
> 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. */
> - dp_netdev_flow_set_last_stats_attrs(netdev_flow, stats, attrs);
> + /* Storing statistics and attributes from the last request for
> + * later use on mutex contention. */
> + dp_netdev_flow_set_last_stats_attrs(netdev_flow, stats, attrs, ret);
> ovs_mutex_unlock(&dp->port_mutex);
> } else {
> - dp_netdev_flow_get_last_stats_attrs(netdev_flow, stats, attrs);
> - if (!attrs->dp_layer) {
> + dp_netdev_flow_get_last_stats_attrs(netdev_flow, stats, attrs, &ret);
> + if (!ret && !attrs->dp_layer) {
> /* Flow was never reported as 'offloaded' so it's harmless
> * to continue to think so. */
> ret = EAGAIN;
> @@ -3512,6 +3653,7 @@ 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);
> + atomic_init(&flow->netdev_flow_get_result, 0);
> memset(&flow->last_stats, 0, sizeof flow->last_stats);
> memset(&flow->last_attrs, 0, sizeof flow->last_attrs);
> flow->dead = false;
> ---
>
> This should mimic the last call regardless of its success.
>
got it, that looks good, thanks.
> Best regards, Ilya Maximets.
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
