On 31 August 2016 at 13:18, Jarno Rajahalme <ja...@ovn.org> wrote:
> With a minor question below,
>
> Acked-by: Jarno Rajahalme <ja...@ovn.org>
>
>> On Aug 31, 2016, at 11:06 AM, Joe Stringer <j...@ovn.org> wrote:
>>
>> If a revalidator dumps/revalidates a flow during the 'dump' phase,
>> resulting in the deletion of the flow, then ukey->flow_exists is set to
>> false, and the ukey is kept around until the 'sweep' phase. The ukey is
>> kept around to ensure that cases like duplicated dumps from the
>> datapaths do not result in multiple attribution of the same stats.
>>
>> However, if an upcall for this flow comes for a handler between the
>> revalidator 'dump' and 'sweep' phases, the handler will lookup the ukey
>> and find that the ukey exists, then skip installing a new flow entirely.
>> As a result, for this period all traffic for the flow is slowpathed.
>> If there is a lot of traffic hitting this flow, then it will all be
>> handled in userspace until the 'sweep' phase. Eventually the
>> revalidators will reach the sweep phase and delete the ukey, and
>> subsequently the handlers should install a new flow.
>>
>> To reduce the slowpathing of this traffic during flow table transitions,
>> allow the handler to identify this case during miss upcall handling and
>> replace the existing ukey with a new ukey. The handler will then be able
>> to install a flow for this traffic, allowing the traffic flow to return
>> to the fastpath.
>>
>> Signed-off-by: Joe Stringer <j...@ovn.org>
>> ---
>> v2: Rebase against ukey lifecycle patch.
>> ---
>> ofproto/ofproto-dpif-upcall.c | 32 ++++++++++++++++++++++++++++++++
>> 1 file changed, 32 insertions(+)
>>
>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>> index ce5a392caf78..04873cc42fff 100644
>> --- a/ofproto/ofproto-dpif-upcall.c
>> +++ b/ofproto/ofproto-dpif-upcall.c
>> @@ -50,6 +50,7 @@ COVERAGE_DEFINE(dumped_duplicate_flow);
>> COVERAGE_DEFINE(dumped_new_flow);
>> COVERAGE_DEFINE(handler_duplicate_upcall);
>> COVERAGE_DEFINE(upcall_ukey_contention);
>> +COVERAGE_DEFINE(upcall_ukey_replace);
>> COVERAGE_DEFINE(revalidate_missed_dp_flow);
>>
>> /* A thread that reads upcalls from dpif, forwards each upcall's packet,
>> @@ -1568,6 +1569,36 @@ ukey_create_from_dpif_flow(const struct udpif *udpif,
>> return 0;
>> }
>>
>> +static bool
>> +try_ukey_replace(struct umap *umap, struct udpif_key *old_ukey,
>> + struct udpif_key *new_ukey)
>> + OVS_REQUIRES(umap->mutex)
>> + OVS_TRY_LOCK(true, new_ukey->mutex)
>> +{
>> + bool replaced = false;
>> +
>> + if (!ovs_mutex_trylock(&old_ukey->mutex)) {
>> + if (old_ukey->state == UKEY_EVICTED) {
>> + COVERAGE_INC(upcall_ukey_replace);
>> +
>> + /* The flow was deleted during the current revalidator dump,
>> + * but its ukey won't be fully cleaned up until the sweep phase.
>> + * In the mean time, we are receiving upcalls for this traffic.
>> + * Expedite the (new) flow install by replacing the ukey. */
>> + ovs_mutex_lock(&new_ukey->mutex);
>> + cmap_replace(&umap->cmap, &old_ukey->cmap_node,
>> + &new_ukey->cmap_node, new_ukey->hash);
>> + ovsrcu_postpone(ukey_delete__, old_ukey);
>> + transition_ukey(old_ukey, UKEY_DELETED);
>> + transition_ukey(new_ukey, UKEY_VISIBLE);
>> + replaced = true;
>> + }
>> + ovs_mutex_unlock(&old_ukey->mutex);
>> + }
>> +
>> + return replaced;
>> +}
>> +
>> /* Attempts to insert a ukey into the shared ukey maps.
>> *
>> * On success, returns true, installs the ukey and returns it in a locked
>> @@ -1590,6 +1621,7 @@ ukey_install__(struct udpif *udpif, struct udpif_key
>> *new_ukey)
>> if (old_ukey->key_len == new_ukey->key_len
>> && !memcmp(old_ukey->key, new_ukey->key, new_ukey->key_len)) {
>> COVERAGE_INC(handler_duplicate_upcall);
>> + locked = try_ukey_replace(umap, old_ukey, new_ukey);
>
> Should we do the coverage only if the try_ukey_replace() returns false?
Can do, might be a smidgen clearer.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
