On 20 March 2018 at 06:12, Eelco Chaudron <[email protected]> wrote:
> Hi Joe,
>
> I'm investigating an issue where I've seen "handler_duplicate_upcall"
> incrementing once, and where "upcall_ukey_replace" is happening quite often.
Without other evidence of undesirable behaviour, I wouldn't be overly
concerned about "handler_duplicate_upcall" happening once. All it
takes is for two packets of the same flow to be sent to userspace at
once for handling, perhaps because userspace is a little slow to
handle the first upcall, or because the sender transmits a couple of
packets in quick succession so they arrive and are handled by the
datapath at close timing, and therefore are queued to userspace at the
same time.
"upcall_ukey_replace" recurrence could be a bit more interesting, but
I'd have to think further about it. Simplest possibility that comes to
mind is that revalidators are deleting flows a bit too aggressively,
so a flow that's actively (or just about to be) receiving traffic is
flushed from the datapath, then handlers receive upcalls for the flows
soon after. If you've got a lot of flows in the datapath, I could see
this happening. A less likely option would be some weird interaction
with a datapath that violates the guarantee that the datapath dumps
flows exactly the same way that they were inserted.
"ovs-appctl upcall/show" might help a bit with digging into this.
> This all relates to the try_ukey_replace() function:
>
> +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) {
> + /* 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);
> + }
> +
> + if (replaced) {
> + COVERAGE_INC(upcall_ukey_replace);
> + } else {
> + COVERAGE_INC(handler_duplicate_upcall);
> + }
> + return replaced;
> +}
> +
>
>
>
> Currently, there is no debugging in place to get an idea if the lock failed
> or the wrong state it was in. I'll instrument the code if we can get it to
> fail again/reliably.
>
> In the meantime, I was wondering why we do an ovs_mutex_trylock(), and not
> just a lock()?
The trylock() is there because other threads may handle the exact same
flow at the exact same time, and only one of them needs to actually
handle (eg, delete / attribute stats for) it. IIRC, this typically
does not happen concurrently in handler threads because an upcall for
a particular flow is likely hashed to the same netlink socket (so only
one handler thread would attempt to handle an upcall for this flow at
the same time, though it could receive two upcalls for the same flow
in succession). However for revalidator threads it is quite possible
for the same flow to be dumped twice when a flow is being deleted from
the kernel, as the kernel flow dump API doesn't provide a guarantee
that the entire table will be dumped exactly as it is, with one
instance of each flow.
Cheers,
Joe
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
